microsoft / binskim

A binary static analysis tool that provides security and correctness results for Windows Portable Executable and *nix ELF binary formats
Other
779 stars 157 forks source link

warning BA2026 SDL check for Rust #582

Open cataggar opened 2 years ago

cataggar commented 2 years ago

Is warning BA2026 that makes sure a PE is compiled with SDL checks a false positive for Rust compiled binaries?

PS C:\Users\cataggar\tmp> binskim version
BinSkim PE/MSIL Analysis Driver 1.9.3.0

PS C:\Users\cataggar\tmp> binskim analyze myapp.exe
Analyzing...
C:\Users\cataggar\tmp\myapp.exe: warning BA2026: 'myapp.exe' is a Windows PE that wasn't compiled with recommended Security Development Lifecycle (SDL) checks. As a result some critical compile-time and runtime checks may be disabled, increasing the possibility of an exploitable runtime issue. To resolve this problem, pass '/sdl' on the cl.exe command-line, set the 'SDL checks' property in the 'C/C++ -> General' Configuration property page, or explicitly set the 'SDLCheck' property in the project file (nested within a 'CLCompile' element) to 'true'.

Done. 1 files scanned.
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). Pass --verbose on the command-line for more information.
shaopeng-gh commented 5 months ago

More input from other users

I dug into this a bit more, and I'm not sure how we were ever passing this before: 
LLVM *never* sets the SDL bit in it's object files, therefore it is never set for Rust either.

this seems like a bug in BinSkim - it [supposed to handle compilers without /sdl support]   
(https://github.com/microsoft/binskim/blob/65ce486ae4c2c0902e36ddbd3b3c491ad67c9c53/src/BinSkim.Rules/PERules/BA2026.EnableMicrosoftCompilerSdlSwitch.cs#L132-L137)   
but it seems that code assumes the sdl bit is neither 0 nor 1   
(as best I can tell, this means that the binary doesn't have a "feature" section in its debug directory),   
which isn't true for LLVM generated binaries.  
The current implementation is broken and suffers from both false negatives and false positives: 
it's checking if the /sdl count in the debug directory feature list is non-zero: 
so if at least one .obj was built /sdl enabled, then the entire binary passes.

I recommend changing the implementation to how BinSkim checks for spectre mitigations:   
walk each obj, filter by language and check the compiler flags.   
We can then make sure that each obj with C++ has the /sdl switch enabled.   
This also has the added benefits of being able to indicate *which* obj/lib doesn't have the flag set,   
which would help debug cases like this.  
this check in BinSkim doesn’t appear to make sense with my understanding  
of the value (a count of modules with the /sdl feature that were linked into the binary).