Enable analysis for managed binary #335

eddynaka · 2021-01-18T10:59:08Z

No description provided.

michaelcfanning · 2021-01-18T19:26:06Z

src/BinaryParsers/PEBinary/PEBinary.cs

@@ -43,7 +43,7 @@ private Pdb LoadPdb()
            // We should never be required to load a PDB for a managed assembly that does
            // not incorporate native code, as no managed-relevant rule currently crawls
            // PDBs for its analysis.


delete all this eventually. :) #Closed

michaelcfanning · 2021-01-18T19:29:33Z

    public string PdbLocation => this.session.globalScope.symbolsFileName;

consider updating this. #Closed

Refers to: src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs:399 in 2fb9917. [](commit_id = 2fb9917, deletion_comment = False)

eddynaka · 2021-01-18T21:37:18Z

    public string PdbLocation => this.session.globalScope.symbolsFileName;

updated the logic...

In reply to: 762434522 [](ancestors = 762434522)

Refers to: src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs:399 in 2fb9917. [](commit_id = 2fb9917, deletion_comment = False)

michaelcfanning · 2021-01-23T23:37:45Z

src/BinaryParsers/BinaryParsers.csproj

@@ -5,6 +5,12 @@
    <RootNamespace>Microsoft.CodeAnalysis.BinaryParsers</RootNamespace>
    <TargetFramework>$(NetStandardVersion)</TargetFramework>
  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


Can collapse this into a single property group if you remove the condition attribute #Closed

michaelcfanning · 2021-01-23T23:40:58Z

src/BinaryParsers/PEBinary/PortableExecutable/PE.cs

+                }
+            }
+
+            return false;


return false [](start = 12, length = 12)

Always prefer early exit if possible. So, change the code to say if (!TryOpen) then return false immediately. #Closed

michaelcfanning · 2021-01-23T23:41:29Z

src/BinaryParsers/PEBinary/PortableExecutable/PE.cs

+        public bool IsChecksumAlgorithmSecureForFullPdb()
+        {
+            const string sha256 = "8829d00f-11b8-4213-878b-770e8597ac16";
+            var sha256guid = new Guid(sha256);


declare a private static readonly Guid that's initialized to this. #Closed

michaelcfanning · 2021-01-23T23:44:26Z

src/BinaryParsers/PEBinary/PortableExecutable/PE.cs

+            const string sha256 = "8829d00f-11b8-4213-878b-770e8597ac16";
+            var sha256guid = new Guid(sha256);
+
+            if (this.peReader.TryOpenAssociatedPortablePdb(


TryOpenAssociatedPortablePdb [](start = 30, length = 28)

eventually pdb location logic needs to understand binskim's '--local-symbol-directories' command-line arguments. let's open an issue and take this in a future change. #Closed

issue opened: #338

In reply to: 563213769 [](ancestors = 563213769)

michaelcfanning · 2021-01-23T23:46:38Z

src/BinaryParsers/PEBinary/PortableExecutable/PE.cs

@@ -789,5 +789,66 @@ public bool IsWixBinary
                return this.isWixBinary.Value;
            }
        }
+
+        public bool IsChecksumAlgorithmSecureForPortablePdb()


IsChecksumAlgorithmSecureForPortablePdb [](start = 20, length = 39)

rather than creating these helpers, you should add a helper that simply returns the checksum algorithm for managed PDBs.

ManagedPdbSourceFileChecksumAlgorithm { get; }

This property s/be on the PDB class, not in the PE class.

In reply to: 563213952 [](ancestors = 563213952)

oh...remembered why i didn't move the below logic to pdb.cs: to do th reading I need a few things: peReader and the metadatareader.

In reply to: 563214137 [](ancestors = 563214137,563213952)

Changed a little bit, let me know what do u think.

In reply to: 563297494 [](ancestors = 563297494,563214137,563213952)

michaelcfanning · 2021-01-23T23:48:43Z

src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs

@@ -134,6 +143,42 @@ private void WindowsNativeLoadPdbUsingDia(string pePath, string symbolPath, stri

        public bool IsStripped => this.GlobalScope.IsStripped;

+        public PdbFileType FileType


You should cache this property value rather than computing it every time. #Resolved

We are caching unless it's unknown

In reply to: 563214200 [](ancestors = 563214200)

michaelcfanning · 2021-01-23T23:50:59Z

src/BinaryParsers/PEBinary/ProgramDatabase/Pdb.cs

+            get
+            {
+                string path = this.session.globalScope.symbolsFileName;
+                if (File.Exists(path))


Make this if (File.Exists(path) || !Directory.Exists(path)) { return path;}

and you will simplify the remaining code. #Closed

thanks for the suggestion!

In reply to: 563214413 [](ancestors = 563214413)

michaelcfanning · 2021-01-23T23:53:14Z

src/BinSkim.Rules/PERules/BA2004.EnableSecureSourceCodeHashing.cs

-            //            if (portableExecutable.IsResourceOnly) { return result; }
+            if (target.PE.IsManaged && di == null)
+            {
+                reasonForNotAnalyzing = MetadataConditions.CouldNotLoadPdb;


reasonForNotAnalyzing = MetadataConditions.CouldNotLoadPdb [](start = 16, length = 58)

in other checks, the absence of a PDB results in an error level notification, not a 'not applicable' message. 'Not applicable' is not the right return value here, as this return value means 'the binary isn't a valid scan target'. that's not true here, managed code is a valid scan target, the problem is that we can't analyze it due to a missing pdb. you should go look and see how the native pdb reading errors handle a missing pdb.

can you validate again? pushed new changes. Thank you

In reply to: 563214549 [](ancestors = 563214549)

michaelcfanning · 2021-01-23T23:54:26Z

...Test.FunctionalTests.BinSkim.Driver/BaselineTestsData/Expected/Binskim.win-x86.RTR.dll.sarif

@@ -62,7 +62,7 @@
            "arguments": [
              "Binskim.win-x86.RTR.dll",
              "EnableSecureSourceCodeHashing",
-              "image is a managed IL library (i.e., ahead of time compiled) assembly"
+              "an exception occurred attempting to load its pdb"


"an exception occurred attempting to load its pdb" [](start = 14, length = 50)

this is wrong. a missing PDB is an error (because analysis that could have occurred did not). 'not applicable' means, 'we didn't look at this because the analysis is not applicable to this scan target'

michaelcfanning

🕐

michaelcfanning

eddynaka added 2 commits January 18, 2021 07:58

Enable analysis for managed binary

ce7ec85

updating sarif expected results

2fb9917

michaelcfanning reviewed Jan 18, 2021

View reviewed changes

eddynaka added 3 commits January 18, 2021 18:03

Merge branch 'main' into users/ednakamu/ba2004-managed-improvement

2c5fab7

updating logic to pdblocation

a11b918

removing old comment

d00f1a7

eddynaka added 4 commits January 20, 2021 10:50

updating pdb loader for checksum and tests

6dda74e

updating logic to check if we can process and tests

703b000

adding more tests

2bb08fc

Merge branch 'main' into users/ednakamu/ba2004-managed-improvement

53c5787

michaelcfanning reviewed Jan 23, 2021

View reviewed changes

michaelcfanning requested changes Jan 23, 2021

View reviewed changes

eddynaka added 2 commits January 24, 2021 11:16

addressing pr comments

5a171d9

Moving from NotApplicable to FailureLevel = Error

3c8b349

eddynaka marked this pull request as ready for review January 24, 2021 23:59

michaelcfanning approved these changes Jan 25, 2021

View reviewed changes

michaelcfanning merged commit a7816c7 into main Jan 25, 2021

michaelcfanning deleted the users/ednakamu/ba2004-managed-improvement branch January 25, 2021 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable analysis for managed binary #335

Enable analysis for managed binary #335

eddynaka commented Jan 18, 2021

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning commented Jan 18, 2021 •

edited by eddynaka

Loading

eddynaka commented Jan 18, 2021

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

eddynaka Jan 24, 2021

michaelcfanning Jan 23, 2021

michaelcfanning Jan 23, 2021

eddynaka Jan 24, 2021

eddynaka Jan 24, 2021

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

eddynaka Jan 24, 2021

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

eddynaka Jan 24, 2021

michaelcfanning Jan 23, 2021

eddynaka Jan 24, 2021

michaelcfanning Jan 23, 2021

michaelcfanning left a comment

michaelcfanning left a comment

		@@ -134,6 +143,42 @@ private void WindowsNativeLoadPdbUsingDia(string pePath, string symbolPath, stri

		public bool IsStripped => this.GlobalScope.IsStripped;

		public PdbFileType FileType

Enable analysis for managed binary #335

Enable analysis for managed binary #335

Conversation

eddynaka commented Jan 18, 2021

michaelcfanning Jan 18, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning commented Jan 18, 2021 • edited by eddynaka Loading

eddynaka commented Jan 18, 2021

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 23, 2021 • edited by eddynaka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

michaelcfanning Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning commented Jan 18, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading

michaelcfanning Jan 23, 2021 •

edited by eddynaka

Loading