[WIP] Add BA2027.EnableSourceLink#657
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I see you added test binaries to the functional test folder. You'll want to run |
pharring
changed the title
|
pharring
changed the title
| } | ||
| ], | ||
| "message": { | ||
| "text": "'Binskim.linux-x64.dll' was not evaluated because its PDB could not be loaded (E_PDB_FORMAT)." |
There was a problem hiding this comment.
In general, any transition you see from 'executionSuccessful' == true to the appearance of error notifications will be a usability problem.
We need to chase down these 'PDB can't be loaded' issues.
Several high-level matters will be helpful to settle: 1) you need to make sure your PDB loading only occurs when the target is valid for your analysis, 2) you need to make sure your analysis only occurs if you observe the binary is built with a toolchain that provides source-link support.
i.e., we don't generally push for things like 'you need to update your stale compiler toolchain so you can enable sourcelink'. The pressure to upgrade is handled in the 'BuildWithSecureTools' rule. Assuming that rule is enforcing a min-version that also enables SourceLink, all we need to do is update that rule docs to mention SourceLink as yet another motivation to upgrade your compiler.
You may discover the min-version that rule enforces is too early for SourceLink support, in which case we should consider lifting it.
| "invocations": [ | ||
| { | ||
| "executionSuccessful": true | ||
| "toolConfigurationNotifications": [ |
There was a problem hiding this comment.
This notification is a concern because it suggests we used to successfully analyze this binary but now cannot, because we are (for the first time) attempting to location a PDB for it. This could indicate that your rule's CanAnalyze needs to be filtering something it is not. In any case, the new appearance of E_PDB_NO_DEBUG_INFO is probably something we shouldn't see in your baseline updates.
|
|
||
| public override AnalysisApplicability CanAnalyzePE(PEBinary target, PropertiesDictionary policy, out string reasonForNotAnalyzing) | ||
| { | ||
| reasonForNotAnalyzing = null; |
There was a problem hiding this comment.
| // not whether they can be read. | ||
| if (pdb.FileType == PdbFileType.Portable) | ||
| { | ||
| string sourceLinkDocument = target.PE.ManagedPdbGetSourceLinkDocument(pdb); |
There was a problem hiding this comment.
Since we don't actually read this document, is there a more efficient API we could create that simply notices where the reader has content? Looking at your helper, can we avoid the ReadUTF8 call and just return true because its Length is >0? That would save something wouldn't it?
return sourceLinkReader.Length == 0 ? null : sourceLinkReader.ReadUTF8(sourceLinkReader.Length);
| else | ||
| { | ||
| IEnumerable<string> sourceLinkDocuments = pdb.WindowsPdbGetSourceLinkDocuments(); | ||
| return sourceLinkDocuments != null && sourceLinkDocuments.Any(); |
There was a problem hiding this comment.
Looking at your yield iterator implementation, looks like this code path is as efficient as it could be, i.e., breaks on the first evidence we've got support.
Just asking: is there no magic value, other (reliable) heuristic we can use to make this determination? I ask merely because as a rule, we try to avoid PDB parsing (and COM interop) wherever possible. Sniffing a magic file in a binary is admirably low-cost. :)
My sense is there's no opportunity here and we've got a reasonably efficient implementation (save the unnecessary presence of ReadUTF8 in the Portable code path, if I'm correct there's a fix we could make here).
There was a problem hiding this comment.
read form the test file i can see the linker commandline have the:
/sourcelink
/ERRORREPORT:PROMPT /OUT:V:\T\binskim\src\BuildSamples\SourceLink\x64\Release\CPlusPlus_SourceLink.exe /NOLOGO /MANIFEST "/MANIFESTUAC:level='asInvoker' uiAccess='false'" /manifest:embed /DEBUG /PDB:V:\T\binskim\src\BuildSamples\SourceLink\x64\Release\CPlusPlus_SourceLink.pdb /SUBSYSTEM:CONSOLE /OPT:REF /OPT:ICF /LTCG:incremental /LTCGOUT:x64\Release\CPlusPlus_SourceLink.iobj /TLBID:1 /DYNAMICBASE /NXCOMPAT /IMPLIB:V:\T\binskim\src\BuildSamples\SourceLink\x64\Release\CPlusPlus_SourceLink.lib /MACHINE:X64 /sourcelink:x64\Release\CPlusPlus_SourceLink.sourcelink.json
https://docs.microsoft.com/en-us/cpp/build/reference/sourcelink?view=msvc-170
pharring
changed the title
Add well-known compiler detection for clang and rust Update pass/fail messages Add test cases for rust and clang Other PR feedback
|
@pharring, this is approved. First, I've made you a write contributor to this codebase, so please feel free to simply create branches off main for future PRs. Second, we have a problem in a pipeline (that apparently can't build your fork) which is prevent us from merging your change. We are working on that. :) |
| *.meta | ||
| *.obj | ||
| *.pch | ||
| *.pdb |
There was a problem hiding this comment.
if we can use the folder ignore it would be better,
above:
# Build results bld/ bin/ obj/
this particular one *.pdb since we frequently checking in pdb files
| ## Rule `BA3030.UseCheckedFunctionsWithGcc` | ||
|
|
||
| ### Description | ||
|
|
5 participants
Fixes #656
A few notes:
The version of Dia2Lib that we use is a bit out of date. I need IDiaDataSource3 to get access to the raw stream data, so I redefined it locally for now.
I created a SourceLink.sln under src/BuildSamples to generate combinations of C#/C++ with/without SourceLink. The outputs of these builds are then checked in under FunctionalTestData. I also grabbed a couple of existing clang/gcc/macho binaries to test the "NotApplicable" code path.