Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File exe.exe compiled with Go 1.12 contains inherent security vulnerabilities #550

Closed
erikpaasonen opened this issue Jul 3, 2024 · 1 comment

Comments

@erikpaasonen
Copy link

Jfrog Xray finds inherent vulnerabilities in Go binaries compiled with Go 1.12, such as testdata/exe.exe in this repo. (full report.) I found that replacing the executable with a binary compiled with Go 1.21 or above causes Xray to find zero of these vulnerabilities. (There is one medium finding when compiled on 1.20.14.) I'd like to suggest to go ahead and replace the exe.exe file, so that there are as few barriers as possible to consuming this very useful Go package.

This issue has been alluded to in the past, but no action was taken because security tooling in general ought to ignore test data. While I agree, I can also see how managing per-repo test data conventions and exceptions becomes exponentially compounded when inspecting Go package dependencies. So where possible, it would be more convenient overall if the vulnerabilities simply didn't exist in the first place.

From what I can tell, the exe.exe file is only used to check whether a file is a legit binary executable. (It may not even ever be executed directly?) It seems that an arbitrary binary would work here. I compiled a Hello World app in Go 1.22 with the following source code:

package main

func main() {
	println("Hello World!")
	return
}

And I tested this replacement exe.exe file in my fork with success -- both with Jfrog Xray scan of my fork (before and after proving that the vulns go away) as well as validating that all the unit tests in go.yml GitHub Action workflow still pass (as long as the binary was compiled for the Windows platform, interestingly enough).

I do plan to submit a PR here with the binary that I tested once I obtain organizational permission for this OSS contribution.

gabriel-vasile added a commit that referenced this issue Jul 26, 2024
After considering #550 and #551, there seems it's hard to create a
windows executable that satisfies these 2 conditions:
- is small in size
- does not trigger antivirus alerts; seems like many antiviruses just
  don't care what's inside an exe. If it's .exe then it's a virus.

Looking back on it, adding fixture files is not perfect: seems nice to
have the library tests work with real files, but:
- it does not count towards test-coverage
- real files increase repo size

Going forward I think I will remove more and more of the files in
testdata folder, and add more unit tests.
gabriel-vasile added a commit that referenced this issue Jul 26, 2024
After considering #550 and #551, there seems it's hard to create a
windows executable that satisfies these 2 conditions:
- is small in size
- does not trigger antivirus alerts; seems like many antiviruses just
  don't care what's inside an exe. If it's .exe then it's a virus.

Looking back on it, adding fixture files is not perfect: seems nice to
have the library tests work with real files, but:
- it does not count towards test-coverage
- real files increase repo size

Going forward I think I will remove more and more of the files in
testdata folder, and add more unit tests.
@gabriel-vasile
Copy link
Owner

gabriel-vasile added a commit that referenced this issue Oct 8, 2024
https://github.com/file/file/blob/7c62d696b06e53fc5be015c41a57513278ac6c54/magic/Magdir/msooxml
The algorithms is not 100% percent reliable. For example, a
zero compression zip containing a docx will still sometimes be detected
as docx instead of zip (it depends on how many files and the order of
files in the zip)

Second thing in this PR is removing some test data fixtures.
From now, I'll try as much as possible to write regular unit tests
without relying on test file fixtures. #575 (comment)
related #550 #575
gabriel-vasile added a commit that referenced this issue Oct 8, 2024
https://github.com/file/file/blob/7c62d696b06e53fc5be015c41a57513278ac6c54/magic/Magdir/msooxml
The algorithms is not 100% percent reliable. For example, a
zero compression zip containing a docx will still sometimes be detected
as docx instead of zip (it depends on how many files and the order of
files in the zip)

Second thing in this PR is removing some test data fixtures.
From now, I'll try as much as possible to write regular unit tests
without relying on test file fixtures. #575 (comment)
related #550 #575
closes #400
gabriel-vasile added a commit that referenced this issue Oct 8, 2024
* Make mso detection work similar to what file/file does

https://github.com/file/file/blob/7c62d696b06e53fc5be015c41a57513278ac6c54/magic/Magdir/msooxml
The algorithms is not 100% percent reliable. For example, a
zero compression zip containing a docx will still sometimes be detected
as docx instead of zip (it depends on how many files and the order of
files in the zip)

Second thing in this PR is removing some test data fixtures.
From now, I'll try as much as possible to write regular unit tests
without relying on test file fixtures. #575 (comment)
related #550 #575
closes #400

* zipContains: remove unnecessary zip sig check

The check is already done in parent function.
gabriel-vasile added a commit that referenced this issue Oct 10, 2024
https://github.com/file/file/blob/7c62d696b06e53fc5be015c41a57513278ac6c54/magic/Magdir/msooxml
The algorithms is not 100% percent reliable. For example, a
zero compression zip containing a docx will still sometimes be detected
as docx instead of zip (it depends on how many files and the order of
files in the zip)

Second thing in this PR is removing some test data fixtures.
From now, I'll try as much as possible to write regular unit tests
without relying on test file fixtures. #575 (comment)
related #550 #575
closes #400
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants