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

fix: 🔒 replace exe.exe to avoid security scan findings #551

Closed

Conversation

erikpaasonen
Copy link

fixes #550

I was unsure what the source code for the existing binary was, so I blindly tried with a simple Go package with this as main.go:

package main

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

Happy to recompile on Go 1.22 if specific source code is needed for this binary.

@erikpaasonen
Copy link
Author

For what it may be worth, here is the GH Actions run link where this binary file passed your unit tests in my fork: https://github.com/erikpaasonen/mimetype/actions/runs/9781337916

@erikpaasonen
Copy link
Author

Apologies for the force push on this branch. I screwed up merge from my fork's master branch containing changes I had to put in to get my fork to run independently of this repo (which I do not intend to include in this PR to upstream).

@gabriel-vasile
Copy link
Owner

gabriel-vasile commented Jul 8, 2024

Thank you!

The updated exe file is smaller, but it's still 1.34MB.
When cloning the repo, both old and updated exe will be downloaded. So this change will increase git history by 1.34MB, albeit compressed.

To avoid this increase in repo size, we can use a C Hello world program, which is only a few KB instead of 1MB. C uses dynamic linking, go uses static, that's where the difference comes from.
So, the 2 things I'll ask for:

  1. compile a C hello world with something like gcc -O3 -o exe.exe main.c or use this
    helloworld.zip
  2. confirm that the exe file does not trigger JFrog Xray

Edit: don't worry about the force push. It's actually good, because it does not pollute git with many commits.
Edit2: these antivirus programs are really annoying. They mark anything .exe as virus. This is the VirusTotal scan for the file I linked.

@erikpaasonen
Copy link
Author

Great catch! I recompiled, however now the unit tests fail in my fork's branch:

--- FAIL: TestDetect (1.01s)
mimetype_test.go:251: File: exe.exe; Expected: application/vnd.microsoft.portable-executable != Detected: application/x-mach-binary; err:
mimetype_test.go:259: File: exe.exe; Expected: application/vnd.microsoft.portable-executable != Detected: application/x-mach-binary; err:
mimetype_test.go:264: File: exe.exe; Expected: application/vnd.microsoft.portable-executable != Detected: application/x-mach-binary; err:
--- FAIL: TestDetectReader (0.63s)
mimetype_test.go:343: File: exe.exe; Mime: application/vnd.microsoft.portable-executable != DetectedMime: application/x-mach-binary; err:

I figure this is your area of expertise so I pushed the commit anyway to get feedback. Unsure whether the unit tests can be adjusted to allow for detecting this type or whether I really do need to wrestle the binary into application/vnd.microsoft.portable-executable (since that's what is actually registered with IANA).

Good news though: the VirusTotal scan comes back clean, and can confirm that this file passes the Xray scan as well.

@gabriel-vasile
Copy link
Owner

gabriel-vasile commented Jul 9, 2024

For executable files Windows uses portable executable (PE).
MacOS, however, uses mach.

When compiling a program, by default, the compiler will generate the executable for current OS.
Because you are using MacOS, it generates mach.
But this PR needs an exe PE file, for Windows.

So the options to get a PE executable for Windows are:

  1. from your MacOS machine do cross-compilation (cross-compilation means compiling for a different OS).
    This option requires more effort. Cross-compiling is not easy. It's probably easier to start a Windows virtual machine and compile there.
  2. use the hellow.exe from the zip that I provided (only if Xray says it is clean)
  3. use some exe file from https://github.com/pts/pts-tinype (if Xray says they are clean)

I say, let's try the easy options first (2 and 3) and see if Xray is ok with them.

@erikpaasonen
Copy link
Author

Yes, I wasn't sure whether this was a hard-and-fast requirement for this project:

But this PR needs an exe PE file, for Windows.

Perhaps I should have directly said cross-compiling rather than abstract it by the phrase "wrestling the binary into [format]". 👍

I'm uncomfortable proposing an executable which is being flagged as malicious (even if without merit) since the nature of this PR is to placate non-nuanced security tooling in the first place. If we trade one legit exception for another we've just shuffled the problem I'm trying to solve sideways. The file you provided has more VirusTotal detections than I'd like in my git commit history, so I'd prefer to cross-compile rather than use that one. Unfortunately every single executable in pts-tinype has at least one detection.

That said, one of them had only a single detection, and that one is merely an "unsafe" AI score marking. Can't eliminate all outliers, but any reasonable/real-world security tooling would agree the prepondernace of evidence is that this file is safe enough.

So I'm least-uncomfortable with consuming hh3wa.exe as exe.exe here to save the hassle of cross compiling for Windows. (I don't personally have ready access to a Windows machine... would have to spin up a VM in a cloud provider or something.)

Jfrog Xray scan is also clean on this file. 👍

@erikpaasonen erikpaasonen changed the title fix: 🔒 replace binary w/Hello World compiled on Go 1.22 fix: 🔒 replace exe.exe to avoid security scan findings Jul 9, 2024
@chkp-erezca
Copy link

Hi,
I'm having the same issue with JFrog Xray. I'm using Go in Alpine Linux. Is there a new version with this fix? Currently the latest version I have is 1.4.4 and I get this error about Go 1.12.

gabriel-vasile added a commit that referenced this pull request 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 pull request 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

@chkp-erezca, please update to https://github.com/gabriel-vasile/mimetype/releases/tag/v1.4.5
@erikpaasonen, I will close this PR in favour of #561. Changing the file is a temporary fix (sooner or later, some AV will report again). Thank you for raising the issue.

@erikpaasonen erikpaasonen deleted the replace-exe-exe branch July 26, 2024 18:50
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 this pull request may close these issues.

File exe.exe compiled with Go 1.12 contains inherent security vulnerabilities
3 participants