1450088 - Replace Binscope with Winchecksec

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[(Away)]

Binscope and BinSkim are tools that make sure our binaries follow Microsoft's security recommendations. We currently run binscope in CI via build/win32/autobinscope.py.

https://blogs.msdn.microsoft.com/secdevblog/2016/08/17/introducing-binskim/ says "Going forward, Binscope will be phased out in favor of BinSkim"

I took a quick peek at BinSkim. I didn't spend too long on it but I'm not convinced it has all of Binscope's checks. Maybe it's in early stages. Perhaps we might want to run both?

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I think we should ditch binscope and binskim use https://blog.trailofbits.com/2018/09/26/effortless-security-feature-detection-with-winchecksec/ =)

Comment 2

[Mike Hommey [:glandium]]

There are some complications getting Binscope to run on cross builds. There are multiple options to make it work, one with an uncertain outcome (building and installing wine-mono, but it may not work out), and one with a clear outcome but not clear whether it's worth the effort (moving the check to a separate task). Either way, it feels like it's some amount of work that might be better spent standing up this replacement.

Now, quickly looking around, binskim is even on github! https://github.com/microsoft/binskim and it looks like it's even possible to build it as a native Linux binary (https://github.com/microsoft/binskim/issues/261)!

winchecksec, however, might only work on Windows. At least, it requires Windows.h and other Windows headers.

Now, the question is whether all the relevant tests we currently run are supported by either. The tests we currently run are:

• ATLVulnCheck
• DBCheck
• DefaultGSCookieCheck
• ExecutableImportsCheck
• GSFriendlyInitCheck
• HighEntropyVACheck
• NXCheck
• RSA32Check
• SafeSEHCheck
• SharedSectionCheck
• WXCheck

(later). I got BinSkim running natively on Linux with no effort, but unfortunately, it disables a bunch of tests in that case. Here's the full verbose output indicating both the passing checks and the skipped ones:

BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'DoNotIncorporateVulnerableDependencies' was disabled as it cannot run on the curren
t platform 'Linux'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'DoNotShipVulnerableBinaries' was disabled as it cannot run on the current platform 
'Linux'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'BuildWithSecureTools' was disabled as it cannot run on the current platform 'Linux'
.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'EnableCriticalCompilerWarnings' was disabled as it cannot run on the current platfo
rm 'Linux'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'EnableStackProtection' was disabled as it cannot run on the current platform 'Linux
'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'InitializeStackProtection' was disabled as it cannot run on the current platform 'L
inux'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'DoNotDisableStackProtectionForFunctions' was disabled as it cannot run on the curre
nt platform 'Linux'.  It can only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'SignSecurely' was disabled as it cannot run on the current platform 'Linux'.  It ca
n only run on 'Windows'.
BINSKIM : warning WRN998.UnsupportedPlatform : Rule 'EnableSpectreMitigations' was disabled as it cannot run on the current platform 'Li
nux'.  It can only run on 'Windows'.
Analyzing 'xul.dll'...
/builds/worker/binskim/firefox/xul.dll: notapplicable BA2015: 'xul.dll' was not evaluated for check 'EnableHighEntropyVirtualAddresses' 
as the analysis is not relevant based on observed metadata: image is not an executable program.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA2016: 'xul.dll' was not evaluated for check 'MarkImageAsNXCompatible' as the ana
lysis is not relevant based on observed metadata: image is a 64-bit binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA2018: 'xul.dll' was not evaluated for check 'EnableSafeSEH' as the analysis is n
ot relevant based on observed metadata: image is not a 32-bit binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA3001: 'xul.dll' was not evaluated for check 'EnablePositionIndependentExecutable
' as the analysis is not relevant based on observed metadata: image is not an ELF binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA3002: 'xul.dll' was not evaluated for check 'DoNotMarkStackAsExecutable' as the 
analysis is not relevant based on observed metadata: image is not an ELF binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA3003: 'xul.dll' was not evaluated for check 'EnableStackProtector' as the analys
is is not relevant based on observed metadata: image is not an ELF binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA3010: 'xul.dll' was not evaluated for check 'EnableReadOnlyRelocations' as the a
nalysis is not relevant based on observed metadata: image is not an ELF binary.
/builds/worker/binskim/firefox/xul.dll: notapplicable BA3030: 'xul.dll' was not evaluated for check 'UseCheckedFunctionsWithGcc' as the 
analysis is not relevant based on observed metadata: image is not an ELF binary.
/builds/worker/binskim/firefox/xul.dll: pass BA2001: 'xul.dll' is a 64-bit image with a base address that is >= 4 gigabytes, increasing 
the effectiveness of Address Space Layout Randomization (which helps prevent attackers from executing security-sensitive code in well-kn
own locations).
/builds/worker/binskim/firefox/xul.dll: pass BA2008: 'xul.dll' enables the control flow guard mitigation. As a result, the operating sys
tem will force an application to close if an attacker is able to redirect execution in the component to an unexpected location.
/builds/worker/binskim/firefox/xul.dll: pass BA2009: 'xul.dll' is properly compiled to enable Address Space Layout Randomization, reduci
ng an attacker's ability to exploit code in well-known locations.
/builds/worker/binskim/firefox/xul.dll: pass BA2010: 'xul.dll' does not have an imports section that is marked as executable, helping to
 prevent the exploitation of code vulnerabilities.
/builds/worker/binskim/firefox/xul.dll: pass BA2012: 'xul.dll' is a C or C++ binary built with the buffer security feature that properly
 preserves the stack protecter cookie. This has the effect of enabling a significant increase in entropy provided by the operating syste
m over that produced by the C runtime start-up code.
/builds/worker/binskim/firefox/xul.dll: pass BA2019: 'xul.dll' contains no data or code sections marked as both shared and writable, hel
ping to prevent the exploitation of code vulnerabilities.
/builds/worker/binskim/firefox/xul.dll: pass BA2021: 'xul.dll' contains no data or code sections marked as both shared and executable, h
elping to prevent the exploitation of code vulnerabilities.
Analysis completed successfully.
One or more rules was disabled for an analysis target, as it was determined not to be applicable to it (this is a common condition). Pas
s --verbose on the command-line for more information.

So it looks like whatever we do, we need to run these tests on Windows, and they are not even guaranteed to work under Wine... so I'm starting to think maybe we should run them in a separate task.

David, what do you think?

Comment 3

[Mike Hommey [:glandium]]

Though, it's worth noting winchecksec may be easy to get running under wine, because it doesn't require .NET.

Comment 4

[(Away)]

Can you clarify, why is it that winchecksec's use of Windows headers makes it require wine?

Comment 5

[Mike Hommey [:glandium]]

(In reply to :dmajor from comment #4)

Can you clarify, why is it that winchecksec's use of Windows headers makes it require wine?

Well, not directly, but if it uses Windows APIs (which the blog post says it does)...

Comment 6

[(Away)]

Oh, I missed that: "Winchecksec aims for completeness in the domain of static checks, is maintained, and uses official Windows APIs for PE parsing." Well, that last part is not true after https://github.com/trailofbits/winchecksec/commit/bc8e0f97669fd2bcf94bba594e3f240d20eec418.

From a quick skim I think the only Windows API left is WinVerifyTrust in Checksec::isAuthenticode. If that means we need wine, ok, no complaint.

Given tjr's support in comment 1, I'd be fine with using winchecksec over binskim.

Comment 7

[Mike Hommey [:glandium]]

I got winchecksec (cross)-built, although I went with the version before https://github.com/trailofbits/winchecksec/commit/bc8e0f97669fd2bcf94bba594e3f240d20eec418 because pe-parse wants ICU, and that was a bit too much to get to test this.
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/chDOZ2faRKyrF2je84-ERg/runs/0/artifacts/public/build/winchecksec.tar.bz2

Its output for our dlls and exes is:

Dynamic Base    : true
ASLR            : true
High Entropy VA : true
Force Integrity : false
Isolation       : true
NX              : true
SEH             : true
CFG             : true
RFG             : false
SafeSEH         : false
GS              : true
Authenticode    : false
.NET            : false

(it also has a json output which will be more convenient)

I'm not sure if Force Integrity and RFG are supposed to be true or false. However, the SafeSEH result is surprising, considering we do have the SafeSEHCheck enabled in binscope at the moment, and it doesn't barf about it.

Note it also crashes on maintenanceservice_installer.exe but that one is produced by nsis, and we don't run binscope on it.

Comment 8

[(Away)]

Force Integrity : false -- this comes from the /integritycheck linker flag, which we don't use, which says the binary must only be loaded if it's signed. We could consider looking into this for signed builds.

RFG : false -- this is Return Flow Guard, which looks like a relatively recent thing that lld doesn't support.

SafeSEH : false -- this should really be more of a "n/a" than a "false". SafeSEH only applies to 32-bit binaries. I tried running against a 32-bit xul.dll to verify, but it got all sorts of wrong answers because it's using undecorated Windows types (i.e. not suffixed suffixed with 32 or 64) which means they default to the bitness of winchecksec. I tried building 32-bits but their cmake disallows this. It looks like after pe-parse they can handle multiple bitness.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1450088 - Build clang with libxml2 support.

This is required for llvm-mt, which building winchecksec will require.
We do a dummy change to build-clang.sh so as to change the toolchain
index hash.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1450088 - Create a Debian 10-based docker image to build toolchains.

With a backport of a more recent version of cmake.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1450088 - Add a toolchain task building winchecksec.

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1450088 - Use Winchecksec instead of Binscope.

Comment 14

[William Woodruff]

Hi there, winchecksec maintainer here. Please let me know if there's anything I can do to ease your integration!

Comment 15

[(Away)]

Thanks! It looks like at this point we're ready to start using winchecksec in CI, just waiting on code review for the patches that enable it.

Comment 16

[William Woodruff]

(In reply to :dmajor from comment #15)

Thanks! It looks like at this point we're ready to start using winchecksec in CI, just waiting on code review for the patches that enable it.

Cool! Just as a heads up: we've fixed the 32-bit build as of https://github.com/trailofbits/winchecksec/commit/fd52cd075eefc6d23401f2b82ed68897df083b1f, and are looking into Linux builds without WINE (but with no time estimate, since that'll involve reimplementing a decent chunk of wintrust).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Tracking this for Fx76 due to bug 1625142.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f8a0c7b1abb
Build clang with libxml2 support. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/bbfb7079edcd
Create a Debian 10-based docker image to build toolchains. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/da3cbe4e4a47
Add a toolchain task building winchecksec. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9a5cd37b875c
Use Winchecksec instead of Binscope. r=dmajor

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2f8a0c7b1abb
https://hg.mozilla.org/mozilla-central/rev/bbfb7079edcd
https://hg.mozilla.org/mozilla-central/rev/da3cbe4e4a47
https://hg.mozilla.org/mozilla-central/rev/9a5cd37b875c

Comment 20

[Intermittent Failures Robot]

4 failures in 4773 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 4

Platform breakdown:

• windows2012-32: 2
• windows2012-64: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1450088&startday=2020-03-23&endday=2020-03-29&tree=all