From 81e1bc93aa0bcd3f0224897c44112eec8fdfcf6c Mon Sep 17 00:00:00 2001 From: Lukas Kohl Date: Fri, 26 Jul 2024 15:10:48 +0200 Subject: [PATCH 1/7] Add conditional check for PDB location File.OpenRead can throw if pdb.PdbLocation is a directory. This can happen if (for some reason) the pdb file is not found. In which case PdbLocation will return the working directory, causing File.OpenRead to throw. --- ReleaseHistory.md | 1 + src/BinaryParsers/PEBinary/PortableExecutable/PE.cs | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ReleaseHistory.md b/ReleaseHistory.md index fb7b47bb..ccbcec2f 100644 --- a/ReleaseHistory.md +++ b/ReleaseHistory.md @@ -16,6 +16,7 @@ - NEW => new feature ## UNRELEASED +* BUG: Fix `TryGetPortablePdbMetadataReader` unexpectedly causes `UnauthorizedAccessException` error when the PDB file is missing. [1001](https://github.com/microsoft/binskim/pull/1001). ## **v4.3.0** * DEP: Update `msdia140.dll` from 14.36.32532.0 to 14.40.33810.0. This update fixes the `System.AccessViolationException: Attempted to read or write protected memory` exception that occurs when reading certain PDB files. [996](https://github.com/microsoft/binskim/pull/996) diff --git a/src/BinaryParsers/PEBinary/PortableExecutable/PE.cs b/src/BinaryParsers/PEBinary/PortableExecutable/PE.cs index d02e7d78..a1aa5d68 100644 --- a/src/BinaryParsers/PEBinary/PortableExecutable/PE.cs +++ b/src/BinaryParsers/PEBinary/PortableExecutable/PE.cs @@ -868,7 +868,11 @@ private bool TryGetPortablePdbMetadataReader(Pdb pdb, out MetadataReader pdbMeta out MetadataReaderProvider pdbProvider, out _)) { - pdbProvider = MetadataReaderProvider.FromPortablePdbStream(File.OpenRead(pdb.PdbLocation)); + if (File.Exists(pdb.PdbLocation)) + { + pdbProvider = MetadataReaderProvider.FromPortablePdbStream(File.OpenRead(pdb.PdbLocation)); + } + if (pdbProvider == null) { pdbMetadataReader = null; From 3f42e99d8e3695c1a19db6a36446a8b433795dea Mon Sep 17 00:00:00 2001 From: Marek Aldorf Date: Mon, 29 Jul 2024 17:30:46 +0200 Subject: [PATCH 2/7] Adding ConsoleLogger to a compiler data logger outputs --- ReleaseHistory.md | 1 + src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ReleaseHistory.md b/ReleaseHistory.md index fb7b47bb..3f6122aa 100644 --- a/ReleaseHistory.md +++ b/ReleaseHistory.md @@ -16,6 +16,7 @@ - NEW => new feature ## UNRELEASED +* BUG: Fork telemetry to log always to Console and AppInsights in the same time when Error occur. [1002](https://github.com/microsoft/binskim/pull/1002) ## **v4.3.0** * DEP: Update `msdia140.dll` from 14.36.32532.0 to 14.40.33810.0. This update fixes the `System.AccessViolationException: Attempted to read or write protected memory` exception that occurs when reading certain PDB files. [996](https://github.com/microsoft/binskim/pull/996) diff --git a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs index 725bfeb9..e460f9ec 100644 --- a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs +++ b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs @@ -48,6 +48,8 @@ private bool IsValidScanTarget(string file) public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(AnalyzeOptions options, ref BinaryAnalyzerContext context) { + base.InitializeGlobalContextFromOptions(options, ref context); + if (this.Telemetry?.TelemetryClient != null) { var aggregatingLogger = new AggregatingLogger(); @@ -55,6 +57,7 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze var ruleTelemetryLogger = new RuleTelemetryLogger(this.Telemetry.TelemetryClient); ruleTelemetryLogger.AnalysisStarted(); + aggregatingLogger.Loggers.Add(context.Logger); aggregatingLogger.Loggers.Add(ruleTelemetryLogger); context.Logger = aggregatingLogger; } @@ -65,7 +68,7 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze ? options.MaxFileSizeInKilobytes.Value : long.MaxValue; - base.InitializeGlobalContextFromOptions(options, ref context); + // Update context object based on command-line parameters. context.SymbolPath = options.SymbolsPath ?? context.SymbolPath; From d8bc8cd48369220d9b66940935862d2d938f0654 Mon Sep 17 00:00:00 2001 From: Marek Aldorf Date: Tue, 6 Aug 2024 12:36:29 +0200 Subject: [PATCH 3/7] Add comment to clarify logger aggregation --- src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs index e460f9ec..6b429185 100644 --- a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs +++ b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs @@ -57,6 +57,7 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze var ruleTelemetryLogger = new RuleTelemetryLogger(this.Telemetry.TelemetryClient); ruleTelemetryLogger.AnalysisStarted(); + // Combine rule telemetry with any other loggers that may be present. aggregatingLogger.Loggers.Add(context.Logger); aggregatingLogger.Loggers.Add(ruleTelemetryLogger); context.Logger = aggregatingLogger; From c2be8e83956aa0f65b6d8cb728541669e2bae0e3 Mon Sep 17 00:00:00 2001 From: Lukas Kohl Date: Tue, 6 Aug 2024 16:46:06 +0200 Subject: [PATCH 4/7] Add test library for missing PDB for PEs Adds a sample library that has been compiled as a standalone portable executable. The library doesn't have an accompanying pdb file, where prior to commit `81e1bc93` would make binskim throw on the working thread. --- ...ve_x86_VS2022_PdbRandomlyMissing.dll.sarif | 328 ++++++++++++++++++ .../Native_x86_VS2022_PdbRandomlyMissing.dll | Bin 0 -> 4096 bytes 2 files changed, 328 insertions(+) create mode 100644 src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif create mode 100644 src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll diff --git a/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif b/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif new file mode 100644 index 00000000..f040db6a --- /dev/null +++ b/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif @@ -0,0 +1,328 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.6.json", + "version": "2.1.0", + "runs": [ + { + "results": [ + { + "ruleId": "BA2004", + "ruleIndex": 0, + "message": { + "id": "Error_Managed", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll", + "Unknown" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + }, + { + "ruleId": "BA2005", + "ruleIndex": 1, + "kind": "pass", + "level": "none", + "message": { + "id": "Pass", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + }, + { + "ruleId": "BA2009", + "ruleIndex": 2, + "kind": "pass", + "level": "none", + "message": { + "id": "Pass", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + }, + { + "ruleId": "BA2019", + "ruleIndex": 3, + "kind": "pass", + "level": "none", + "message": { + "id": "Pass", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + }, + { + "ruleId": "BA2021", + "ruleIndex": 4, + "kind": "pass", + "level": "none", + "message": { + "id": "Pass", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + }, + { + "ruleId": "BA2027", + "ruleIndex": 5, + "message": { + "id": "Warning", + "arguments": [ + "Native_x86_VS2022_PdbRandomlyMissing.dll" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + } + } + } + ] + } + ], + "tool": { + "driver": { + "name": "testhost", + "version": "15.0.0.0", + "rules": [ + { + "id": "BA2004", + "name": "EnableSecureSourceCodeHashing", + "fullDescription": { + "text": "Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code.\r\nThis information is typically used to resolve source file when debugging but it can also be used to verify that a specific body of source code is, in fact, the code that was used to produce a specific set of binaries and PDBs.\r\nThis validation is helpful in verifying supply chain integrity. Due to this security focus, it is important that the hashing algorithm used to produce checksums is secure.\r\nLegacy hashing algorithms, such as MD5 and SHA-1, have been demonstrated to be broken by modern hardware (that is, it is computationally feasible to force hash collisions, in which a common hash is generated from distinct files).\r\nUsing a secure hashing algorithm, such as SHA-256, prevents the possibility of collision attacks, in which the checksum of a malicious file is used to produce a hash that satisfies the system that it is, in fact, the original file processed by the compiler.\r\nFor managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the '' project property with 'SHA256' to enable secure source code hashing.\r\nFor native code - use to MSVC 17.0 (14.30.*) or later if possible. For VC projects use PlatformToolset property with 'v143' or later value.\r\nWhen using older MSVC versions add /ZH:SHA_256 on cl.exe command line." + }, + "help": { + "text": "Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code.\r\nThis information is typically used to resolve source file when debugging but it can also be used to verify that a specific body of source code is, in fact, the code that was used to produce a specific set of binaries and PDBs.\r\nThis validation is helpful in verifying supply chain integrity. Due to this security focus, it is important that the hashing algorithm used to produce checksums is secure.\r\nLegacy hashing algorithms, such as MD5 and SHA-1, have been demonstrated to be broken by modern hardware (that is, it is computationally feasible to force hash collisions, in which a common hash is generated from distinct files).\r\nUsing a secure hashing algorithm, such as SHA-256, prevents the possibility of collision attacks, in which the checksum of a malicious file is used to produce a hash that satisfies the system that it is, in fact, the original file processed by the compiler.\r\nFor managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the '' project property with 'SHA256' to enable secure source code hashing.\r\nFor native code - use to MSVC 17.0 (14.30.*) or later if possible. For VC projects use PlatformToolset property with 'v143' or later value.\r\nWhen using older MSVC versions add /ZH:SHA_256 on cl.exe command line." + }, + "messageStrings": { + "Pass": { + "text": "'{0}' is a {1} binary which was compiled with a secure (SHA-256) source code hashing algorithm." + }, + "Warning_NativeWithInsecureStaticLibraryCompilands": { + "text": "'{0}' is a native binary that links one or more static libraries that include object files which were hashed using an insecure checksum algorithm.\r\nInsecure checksum algorithms are subject to collision attacks and its use can compromise supply chain integrity.\r\nTo resolve this issue, use newer versions of libraries that are compiled with /ZH:SHA_256. MSVC: 17.0 (14.30.*) or later. Windows SDK: 10.0.18362.0 or later.\r\nThe following modules are out of policy:\r\n{1}" + }, + "Error_Managed": { + "text": "'{0}' is a managed binary compiled with an insecure ({1}) source code hashing algorithm. {1} is subject to collision attacks and its use can compromise supply chain integrity. Pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the project property with 'SHA256' to enable secure source code hashing." + }, + "Error_NativeWithInsecureDirectCompilands": { + "text": "'{0}' is a native binary that directly compiles and links one or more object files which were hashed using an insecure checksum algorithm.\r\nInsecure checksum algorithms are subject to collision attacks and its use can compromise supply chain integrity.\r\nUse MSVC 17.0 (14.30.*) or later if possible.\r\nWhen using older MSVC versions, pass '/ZH:SHA_256' on the cl.exe command-line to enable secure source code hashing.\r\nThe following modules are out of policy:\r\n{1}" + }, + "NotApplicable_InvalidMetadata": { + "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2004EnableSecureSourceCodeHashing" + }, + { + "id": "BA2005", + "name": "DoNotShipVulnerableBinaries", + "fullDescription": { + "text": "Do not ship obsolete libraries for which there are known security vulnerabilities." + }, + "help": { + "text": "Do not ship obsolete libraries for which there are known security vulnerabilities." + }, + "messageStrings": { + "Pass": { + "text": "'{0}' is not known to be an obsolete binary that is vulnerable to one or more security problems." + }, + "Error": { + "text": "'{0}' appears to be an obsolete library (version {1}) for which there are known security vulnerabilities. \r\nTo resolve this issue, obtain a version of {0} that is newer than version {2}. If this binary is not in fact {0}, ignore this warning." + }, + "Error_CouldNotParseVersion": { + "text": "Version information for '{0}' could not be parsed. The binary therefore could not be verified not to be an obsolete binary that is known to be vulnerable to one or more security problems." + }, + "NotApplicable_InvalidMetadata": { + "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2005DoNotShipVulnerableBinaries", + "properties": { + "equivalentBinScopeRuleReadableName": "BinaryVersionsCheck" + } + }, + { + "id": "BA2009", + "name": "EnableAddressSpaceLayoutRandomization", + "fullDescription": { + "text": "Binaries should linked as DYNAMICBASE to be eligible for relocation by Address Space Layout Randomization (ASLR). ASLR is an important mitigation that makes it more difficult for an attacker to exploit memory corruption vulnerabilities. Configure your tools to build with this feature enabled. For C and C++ binaries, add /DYNAMICBASE to your linker command line. For .NET applications, use a compiler shipping with Visual Studio 2008 or later." + }, + "help": { + "text": "Binaries should linked as DYNAMICBASE to be eligible for relocation by Address Space Layout Randomization (ASLR). ASLR is an important mitigation that makes it more difficult for an attacker to exploit memory corruption vulnerabilities. Configure your tools to build with this feature enabled. For C and C++ binaries, add /DYNAMICBASE to your linker command line. For .NET applications, use a compiler shipping with Visual Studio 2008 or later." + }, + "messageStrings": { + "Pass": { + "text": "'{0}' is properly compiled to enable Address Space Layout Randomization, reducing an attacker's ability to exploit code in well-known locations." + }, + "Error_NotDynamicBase": { + "text": "'{0}' is not marked as DYNAMICBASE. This means that the binary is not eligible for relocation by Address Space Layout Randomization (ASLR). ASLR is an important mitigation that makes it more difficult for an attacker to exploit memory corruption vulnerabilities.\r\nTo resolve this issue, configure your tools to build with this feature enabled. For C and C++ binaries, add /DYNAMICBASE to your linker command line.\r\nFor VC projects use ItemDefinitionGroup - Link - RandomizedBaseAddress property with 'true' value.\r\nFor .NET applications, use a compiler shipping with Visual Studio 2008 or later." + }, + "Error_RelocsStripped": { + "text": "'{0}' is marked as DYNAMICBASE but relocation data has been stripped from the image, preventing address space layout randomization. " + }, + "Error_WinCENoRelocationSection": { + "text": "'{0}' is a Windows CE image but does not contain any relocation data, preventing Address Space Layout Randomization." + }, + "NotApplicable_InvalidMetadata": { + "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2009EnableAddressSpaceLayoutRandomization", + "properties": { + "equivalentBinScopeRuleReadableName": "DBCheck" + } + }, + { + "id": "BA2019", + "name": "DoNotMarkWritableSectionsAsShared", + "fullDescription": { + "text": "Code or data sections should not be marked as both shared and writable. Because these sections are shared across processes, this condition might permit a process with low privilege to alter memory in a higher privilege process.\r\nIf you do not actually require that a section be both writable and shared, remove one or both of these attributes (by modifying your .DEF file, the appropriate linker /section switch arguments, etc.).\r\nIf you must share common data across processes (for inter-process communication (IPC) or other purposes) use CreateFileMapping with proper security attributes or an actual IPC mechanism instead (COM, named pipes, LPC, etc.)." + }, + "help": { + "text": "Code or data sections should not be marked as both shared and writable. Because these sections are shared across processes, this condition might permit a process with low privilege to alter memory in a higher privilege process.\r\nIf you do not actually require that a section be both writable and shared, remove one or both of these attributes (by modifying your .DEF file, the appropriate linker /section switch arguments, etc.).\r\nIf you must share common data across processes (for inter-process communication (IPC) or other purposes) use CreateFileMapping with proper security attributes or an actual IPC mechanism instead (COM, named pipes, LPC, etc.)." + }, + "messageStrings": { + "Pass": { + "text": "'{0}' contains no data or code sections marked as both shared and writable, helping to prevent the exploitation of code vulnerabilities." + }, + "Error": { + "text": "'{0}' contains one or more code or data sections ({1}) which are marked as both shared and writable. Because these sections are shared across processes, this condition might permit a process with low privilege to alter memory in a higher privilege process.\r\nIf you do not actually require that a section be both writable and shared, remove one or both of these attributes (by modifying your .DEF file, the appropriate linker /section switch arguments, etc.).\r\nIf you must share common data across processes (for inter-process communication (IPC) or other purposes) use CreateFileMapping with proper security attributes or an actual IPC mechanism instead (COM, named pipes, LPC, etc.)." + }, + "NotApplicable_InvalidMetadata": { + "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2019DoNotMarkWritableSectionsAsShared", + "properties": { + "equivalentBinScopeRuleReadableName": "SharedSectionCheck" + } + }, + { + "id": "BA2021", + "name": "DoNotMarkWritableSectionsAsExecutable", + "fullDescription": { + "text": "PE sections should not be marked as both writable and executable. This condition makes it easier for an attacker to exploit memory corruption vulnerabilities, as it may provide an attacker executable location(s) to inject shellcode.\r\nTo resolve this issue, configure your tools to not emit memory sections that are writable and executable. For example, look for uses of /SECTION on the linker command line for C and C++ programs, or #pragma section in C and C++ source code, which mark a section with both attributes.\r\nBe sure to disable incremental linking in release builds, as this feature creates a writable and executable section named '.textbss' in order to function." + }, + "help": { + "text": "PE sections should not be marked as both writable and executable. This condition makes it easier for an attacker to exploit memory corruption vulnerabilities, as it may provide an attacker executable location(s) to inject shellcode.\r\nTo resolve this issue, configure your tools to not emit memory sections that are writable and executable. For example, look for uses of /SECTION on the linker command line for C and C++ programs, or #pragma section in C and C++ source code, which mark a section with both attributes.\r\nBe sure to disable incremental linking in release builds, as this feature creates a writable and executable section named '.textbss' in order to function." + }, + "messageStrings": { + "Pass": { + "text": "'{0}' contains no data or code sections marked as both shared and executable, helping to prevent the exploitation of code vulnerabilities." + }, + "Error": { + "text": "'{0}' contains PE section(s) ({1}) that are both writable and executable. Writable and executable memory segments make it easier for an attacker to exploit memory corruption vulnerabilities, because it may provide an attacker executable location(s) to inject shellcode.\r\nTo resolve this issue, configure your tools to not emit memory sections that are writable and executable. For example, look for uses of /SECTION on the linker command line for C and C++ programs, or #pragma section in C and C++ source code, which mark a section with both attributes.\r\nEnabling incremental linking via the /INCREMENTAL argument (the default for Microsoft Visual Studio debug build) can also result in a writable and executable section named 'textbss'. For this case, disable incremental linking (or analyze an alternate build configuration that disables this feature) to resolve the problem.\r\nFor VC projects use ItemDefinitionGroup - Link - LinkIncremental property with 'false' value." + }, + "Error_UnexpectedSectionAligment": { + "text": "'{0}' has a section alignment ({1}) that is smaller than its page size ({2})." + }, + "NotApplicable_InvalidMetadata": { + "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2021DoNotMarkWritableSectionsAsExecutable", + "properties": { + "equivalentBinScopeRuleReadableName": "WXCheck" + } + }, + { + "id": "BA2027", + "name": "EnableSourceLink", + "fullDescription": { + "text": "SourceLink information should be present in the PDB. This applies to binaries built with the C# and MSVC compilers. When enabled, SourceLink information is added to the PDB. That information includes the repository URLs and commit IDs for all source files fed to the compiler.\r\nThe PDB should also be uploaded to a symbol server so that it can be discovered by a debugger such as Visual Studio. Developers can then step into the matching source code. Frictionless source-driven debugging provides a good user experience for consumers and also accelerates security response in the event of supply-chain compromise.\r\nSee https://aka.ms/sourcelink for more information." + }, + "help": { + "text": "SourceLink information should be present in the PDB. This applies to binaries built with the C# and MSVC compilers. When enabled, SourceLink information is added to the PDB. That information includes the repository URLs and commit IDs for all source files fed to the compiler.\r\nThe PDB should also be uploaded to a symbol server so that it can be discovered by a debugger such as Visual Studio. Developers can then step into the matching source code. Frictionless source-driven debugging provides a good user experience for consumers and also accelerates security response in the event of supply-chain compromise.\r\nSee https://aka.ms/sourcelink for more information." + }, + "messageStrings": { + "Pass": { + "text": "The PDB for '{0}' contains SourceLink information, maximizing engineering and security response efficiency when source code is required for debugging and other critical analysis." + }, + "Warning": { + "text": "The PDB for '{0}' does not contain SourceLink information, compromising frictionless source-driven debugging and increasing latency of security response.\r\nEnable SourceLink by configuring necessary project properties and adding a package reference for your source control provider.\r\nSee https://aka.ms/sourcelink for more information." + } + }, + "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2027EnableSourceLink" + } + ], + "properties": { + "comments": "A security and correctness analyzer for portable executable and MSIL formats." + } + } + }, + "invocations": [ + { + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", + "index": 0 + }, + "hashes": { + "md5": "68A19DE304E6450EB2D2B4B65C7C47E3", + "sha-1": "13F06A8BCA79D5385AB59F1A5816211C96A52FDB", + "sha-256": "E349466D4205C2E9D0E94A2A0FCEE000DC082137BF6075F4E484870CDBFADF7D" + } + } + ], + "columnKind": "utf16CodeUnits" + } + ] +} \ No newline at end of file diff --git a/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll b/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll new file mode 100644 index 0000000000000000000000000000000000000000..4bb6d72391a96e00659c0149cbde458d6c8c549e GIT binary patch literal 4096 zcmeHJPiz!b82@Hzu`IL{Ap}FfVG05Y(=9EC&_YZ9&?<%6T_EBr%+9`UhtAF{^JYso zG1{1*L=%D+PkJy04T*_hFcJ-TA;E(ril_%UdN4)<2cwAx@%P@$bf-`h4<;UbllRT{ zeee7K{tg~ENwbKk2L0A8qVxER)+ym%gGmfGJos?~ottxI#d&4dl@*y$i>oEiEqF#z zH4VpceKp5auk5InqxS4ct3@}@^!obb%Fy&sFH!!b1g*RJ*>91zo21d)M2e^moY@f< ze^ua}0t9#>h+B{%e%THIf-l-EI&^GGzEiRQ8UsX)=qXN>A+S!wRXd#(r_QY+%ETG} z6jzCcVmi9dCVbFmMZOr5xrcdxl1(K#qmLCM%Ufe85|daQL$B^_hl1?>kvLA7*9 zMLx(^0gWE7`du_Bn~{(kjtB(Bo}-N^ecIwmc?AYsm(D1JX^; zBW2{SBRq=1yo2D#qF?&rr_YU;nZ-f3p&^v55hrei0PwreqG)EA> zE3LHnlJq`57?XHCa1jgK3VF0sioiwM%M%xrB&FzrGMjeL4Zx*}fJF(Pq3g<0nh&Mf zbevuVKcid%{GJ4SOTvp1Zk7Ba5}yTKVO_=quYzc#5Amy&)Q~DhUjx1beF1Hzh4cV@ zqb#QtfU9Y(#9II})CFkLfP_y<_>_c(gqDOyB&-0grqwh?*GZ?Pv<|R|HUd6E2LM}X z6mToO1K3R;0X{*O0Mqm(;M0Ih$2^U7WLG&aPxEKhDk=UV-_2IG@;2*BiT2z7Ib3F^^lW zbLZfIGvazh!^fau-&G2+k648=f}Cb#n2n^35$0Ehkw32*4-%8Y!^}1&Bo-TwG8ytv zIn%EyMgzit;Z&*v!)3>}icIo-D`(l39~%t|9}4MV8rX1@Vcdt8!_4&cyloS3agv8~ z5={;AupxomNH=}gi}*d3QE*)DTPBZKk&kHZ(##vTOvb|*gHNmmkb82%XWYMBHB0-6 z;e@cs#p+dQKmH!5JobfX=h~V+zqKyWGx?q7zIpoU%T)K=PshH!_-^Gy-I?tNUs&*U za!U;7I1bxcv4PmtQ~LHCG?|_0jiE2^-y8vU@qUL)I>j8GOtg zwX@tUdnU_ztmN`+ER$)tn>(Bhlgc{GN8Z_^ma~3hQ(M-yoH3S%}AAI%8&ows>pIEroIV~gpAN)HNNXT`e5=}0F1H4Zrr~3)+uFW&>h1_kz-~{rc zcMOS@g*OZyo+mqrt_6Eb@~$P?@ZX19=pbM(4FemXJ+vG20AL?*(Js{7_#?QH0$b7T zv|Ww{@4;v!WRZ9?D6nea6zL-iCkca1hemL&h_;gOpunrngM>pI;|BSVb3p~|OpSP- zD2U02lm&ia21|`nCWTi-@U>A67~x)@2i45_hoePS#+R}VVzgtC&g@6W zxh&3VqBS@y71{;eg47flm#88mF5uN6a#GRzrJp?^UsO~0iSiUwP?^8;7t}SVTAs?X zYWGst6kc&kSnWvf_Ucq?Bj!)ku?w~1$gPN3wUL#I6*ax0RUJ`RBEOsH$Jk=L@yVy{ z+b4>)Iv%_;Iy4;en#vs0&Erb!(Dr8f)^FBS?i)_tuw93BXcfk_?c3^;^~rW4eA%cl z;qVTv>^WPwIm(I#UthFL&*kojzut6ZLK+;8 z9VsJDmKU!=Jj4o&-=o#m$f}A~wbMHRAL2US#m1Sf+5&cH1|M+7-7)5AYT4>C@gPAN lM+}>@Fez!Id4_1B(3_{l-rhV_1PHV@M>Dhylm8!Ye*(1S+L-_V literal 0 HcmV?d00001 From da3bd0992b7d3f7c4f0a05cb9e7f30ffb4405790 Mon Sep 17 00:00:00 2001 From: Marek Aldorf Date: Wed, 7 Aug 2024 13:28:54 +0200 Subject: [PATCH 5/7] Adding type check to avoid empty IAnalysisLogger in the first plase of context.Logger.Loggers --- src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs index 6b429185..99edb664 100644 --- a/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs +++ b/src/BinSkim.Driver/MultithreadedAnalyzeCommand.cs @@ -8,6 +8,8 @@ using System.Linq; using System.Reflection; +using CommandLine; + using Microsoft.CodeAnalysis.BinaryParsers; using Microsoft.CodeAnalysis.IL.Rules; using Microsoft.CodeAnalysis.IL.Sdk; @@ -52,13 +54,21 @@ public override BinaryAnalyzerContext InitializeGlobalContextFromOptions(Analyze if (this.Telemetry?.TelemetryClient != null) { + // Create an aggregating logger that will combine all loggers into a single logger. var aggregatingLogger = new AggregatingLogger(); + if (context.Logger is AggregatingLogger) + { + aggregatingLogger = context.Logger as AggregatingLogger; + } + else + { + aggregatingLogger.Loggers.Add(context.Logger); + } var ruleTelemetryLogger = new RuleTelemetryLogger(this.Telemetry.TelemetryClient); ruleTelemetryLogger.AnalysisStarted(); // Combine rule telemetry with any other loggers that may be present. - aggregatingLogger.Loggers.Add(context.Logger); aggregatingLogger.Loggers.Add(ruleTelemetryLogger); context.Logger = aggregatingLogger; } From 3756410c57b794016b0f61f7d352c2519c35209d Mon Sep 17 00:00:00 2001 From: Lukas Kohl Date: Wed, 7 Aug 2024 16:17:24 +0200 Subject: [PATCH 6/7] Updated expected sarif file for PdbRandomlyMissing library --- ...ve_x86_VS2022_PdbRandomlyMissing.dll.sarif | 96 +------------------ 1 file changed, 4 insertions(+), 92 deletions(-) diff --git a/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif b/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif index f040db6a..3da61220 100644 --- a/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif +++ b/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Expected/Native_x86_VS2022_PdbRandomlyMissing.dll.sarif @@ -4,30 +4,9 @@ "runs": [ { "results": [ - { - "ruleId": "BA2004", - "ruleIndex": 0, - "message": { - "id": "Error_Managed", - "arguments": [ - "Native_x86_VS2022_PdbRandomlyMissing.dll", - "Unknown" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", - "index": 0 - } - } - } - ] - }, { "ruleId": "BA2005", - "ruleIndex": 1, + "ruleIndex": 0, "kind": "pass", "level": "none", "message": { @@ -49,7 +28,7 @@ }, { "ruleId": "BA2009", - "ruleIndex": 2, + "ruleIndex": 1, "kind": "pass", "level": "none", "message": { @@ -71,7 +50,7 @@ }, { "ruleId": "BA2019", - "ruleIndex": 3, + "ruleIndex": 2, "kind": "pass", "level": "none", "message": { @@ -93,7 +72,7 @@ }, { "ruleId": "BA2021", - "ruleIndex": 4, + "ruleIndex": 3, "kind": "pass", "level": "none", "message": { @@ -112,26 +91,6 @@ } } ] - }, - { - "ruleId": "BA2027", - "ruleIndex": 5, - "message": { - "id": "Warning", - "arguments": [ - "Native_x86_VS2022_PdbRandomlyMissing.dll" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///Z:/src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/Native_x86_VS2022_PdbRandomlyMissing.dll", - "index": 0 - } - } - } - ] } ], "tool": { @@ -139,34 +98,6 @@ "name": "testhost", "version": "15.0.0.0", "rules": [ - { - "id": "BA2004", - "name": "EnableSecureSourceCodeHashing", - "fullDescription": { - "text": "Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code.\r\nThis information is typically used to resolve source file when debugging but it can also be used to verify that a specific body of source code is, in fact, the code that was used to produce a specific set of binaries and PDBs.\r\nThis validation is helpful in verifying supply chain integrity. Due to this security focus, it is important that the hashing algorithm used to produce checksums is secure.\r\nLegacy hashing algorithms, such as MD5 and SHA-1, have been demonstrated to be broken by modern hardware (that is, it is computationally feasible to force hash collisions, in which a common hash is generated from distinct files).\r\nUsing a secure hashing algorithm, such as SHA-256, prevents the possibility of collision attacks, in which the checksum of a malicious file is used to produce a hash that satisfies the system that it is, in fact, the original file processed by the compiler.\r\nFor managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the '' project property with 'SHA256' to enable secure source code hashing.\r\nFor native code - use to MSVC 17.0 (14.30.*) or later if possible. For VC projects use PlatformToolset property with 'v143' or later value.\r\nWhen using older MSVC versions add /ZH:SHA_256 on cl.exe command line." - }, - "help": { - "text": "Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code.\r\nThis information is typically used to resolve source file when debugging but it can also be used to verify that a specific body of source code is, in fact, the code that was used to produce a specific set of binaries and PDBs.\r\nThis validation is helpful in verifying supply chain integrity. Due to this security focus, it is important that the hashing algorithm used to produce checksums is secure.\r\nLegacy hashing algorithms, such as MD5 and SHA-1, have been demonstrated to be broken by modern hardware (that is, it is computationally feasible to force hash collisions, in which a common hash is generated from distinct files).\r\nUsing a secure hashing algorithm, such as SHA-256, prevents the possibility of collision attacks, in which the checksum of a malicious file is used to produce a hash that satisfies the system that it is, in fact, the original file processed by the compiler.\r\nFor managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the '' project property with 'SHA256' to enable secure source code hashing.\r\nFor native code - use to MSVC 17.0 (14.30.*) or later if possible. For VC projects use PlatformToolset property with 'v143' or later value.\r\nWhen using older MSVC versions add /ZH:SHA_256 on cl.exe command line." - }, - "messageStrings": { - "Pass": { - "text": "'{0}' is a {1} binary which was compiled with a secure (SHA-256) source code hashing algorithm." - }, - "Warning_NativeWithInsecureStaticLibraryCompilands": { - "text": "'{0}' is a native binary that links one or more static libraries that include object files which were hashed using an insecure checksum algorithm.\r\nInsecure checksum algorithms are subject to collision attacks and its use can compromise supply chain integrity.\r\nTo resolve this issue, use newer versions of libraries that are compiled with /ZH:SHA_256. MSVC: 17.0 (14.30.*) or later. Windows SDK: 10.0.18362.0 or later.\r\nThe following modules are out of policy:\r\n{1}" - }, - "Error_Managed": { - "text": "'{0}' is a managed binary compiled with an insecure ({1}) source code hashing algorithm. {1} is subject to collision attacks and its use can compromise supply chain integrity. Pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the project property with 'SHA256' to enable secure source code hashing." - }, - "Error_NativeWithInsecureDirectCompilands": { - "text": "'{0}' is a native binary that directly compiles and links one or more object files which were hashed using an insecure checksum algorithm.\r\nInsecure checksum algorithms are subject to collision attacks and its use can compromise supply chain integrity.\r\nUse MSVC 17.0 (14.30.*) or later if possible.\r\nWhen using older MSVC versions, pass '/ZH:SHA_256' on the cl.exe command-line to enable secure source code hashing.\r\nThe following modules are out of policy:\r\n{1}" - }, - "NotApplicable_InvalidMetadata": { - "text": "'{0}' was not evaluated for check '{1}' as the analysis is not relevant based on observed metadata: {2}." - } - }, - "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2004EnableSecureSourceCodeHashing" - }, { "id": "BA2005", "name": "DoNotShipVulnerableBinaries", @@ -278,25 +209,6 @@ "properties": { "equivalentBinScopeRuleReadableName": "WXCheck" } - }, - { - "id": "BA2027", - "name": "EnableSourceLink", - "fullDescription": { - "text": "SourceLink information should be present in the PDB. This applies to binaries built with the C# and MSVC compilers. When enabled, SourceLink information is added to the PDB. That information includes the repository URLs and commit IDs for all source files fed to the compiler.\r\nThe PDB should also be uploaded to a symbol server so that it can be discovered by a debugger such as Visual Studio. Developers can then step into the matching source code. Frictionless source-driven debugging provides a good user experience for consumers and also accelerates security response in the event of supply-chain compromise.\r\nSee https://aka.ms/sourcelink for more information." - }, - "help": { - "text": "SourceLink information should be present in the PDB. This applies to binaries built with the C# and MSVC compilers. When enabled, SourceLink information is added to the PDB. That information includes the repository URLs and commit IDs for all source files fed to the compiler.\r\nThe PDB should also be uploaded to a symbol server so that it can be discovered by a debugger such as Visual Studio. Developers can then step into the matching source code. Frictionless source-driven debugging provides a good user experience for consumers and also accelerates security response in the event of supply-chain compromise.\r\nSee https://aka.ms/sourcelink for more information." - }, - "messageStrings": { - "Pass": { - "text": "The PDB for '{0}' contains SourceLink information, maximizing engineering and security response efficiency when source code is required for debugging and other critical analysis." - }, - "Warning": { - "text": "The PDB for '{0}' does not contain SourceLink information, compromising frictionless source-driven debugging and increasing latency of security response.\r\nEnable SourceLink by configuring necessary project properties and adding a package reference for your source control provider.\r\nSee https://aka.ms/sourcelink for more information." - } - }, - "helpUri": "https://github.com/microsoft/binskim/blob/main/docs/BinSkimRules.md#rule-BA2027EnableSourceLink" } ], "properties": { From a8a4d830c8fa759b844a0b1ac8faf86a0053323d Mon Sep 17 00:00:00 2001 From: Marek Aldorf Date: Fri, 9 Aug 2024 16:28:59 +0200 Subject: [PATCH 7/7] Add telemetry-related unit tests and using directives 1. Verify ConsoleLogger when telemetry is disabled. 2. Verify ConsoleLogger and RuleTelemetryLogger when telemetry is enabled. 3. Verify RuleTelemetryLogger when quiet option and telemetry are enabled. 4. Verify no loggers when both quiet option and telemetry are disabled. --- .../MultithreadedAnalyzeCommandTests.cs | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs b/src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs index 46cb35b7..2fecd28b 100644 --- a/src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs +++ b/src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs @@ -2,11 +2,16 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Generic; using System.Text; using FluentAssertions; +using Microsoft.ApplicationInsights; +using Microsoft.ApplicationInsights.Extensibility; using Microsoft.CodeAnalysis.IL; +using Microsoft.CodeAnalysis.IL.Sdk; +using Microsoft.CodeAnalysis.Sarif.Writers; using Xunit; @@ -150,5 +155,89 @@ private static string[] Shuffle(string[] data) return data; } + + [Fact] + public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_TelemetryNotSpecifiedHasOneConsoleLogger() + { + var options = new AnalyzeOptions(); + options.TargetFileSpecifiers = new List { "test.dll" }; + options.DisableTelemetry = true; + options.OutputFilePath = "test/path/"; + var context = new BinaryAnalyzerContext(); + + var command = new MultithreadedAnalyzeCommand(); + command.InitializeGlobalContextFromOptions(options, ref context); + + Assert.IsType(context.Logger); + + var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger; + Assert.Contains(aggregatingLogger.Loggers, l => l is ConsoleLogger); + Assert.Equal(1, aggregatingLogger.Loggers.Count); + } + + [Fact] + public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_TelemetrySpecifiedHasTwoLoggers() + { + var telemetryConfiguration = TelemetryConfiguration.CreateDefault(); + var telemetryClient = new TelemetryClient(telemetryConfiguration); + var telemetry = new Telemetry(telemetryConfiguration); + + var options = new AnalyzeOptions(); + options.TargetFileSpecifiers = new List { "test.dll" }; + options.OutputFilePath = "test/path/"; + var context = new BinaryAnalyzerContext(); + + var command = new MultithreadedAnalyzeCommand(telemetry); + command.InitializeGlobalContextFromOptions(options, ref context); + + Assert.IsType(context.Logger); + + var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger; + Assert.Contains(aggregatingLogger.Loggers, l => l is ConsoleLogger); + Assert.Contains(aggregatingLogger.Loggers, l => l is RuleTelemetryLogger); + Assert.Equal(2, aggregatingLogger.Loggers.Count); + } + + [Fact] + public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_QuietOptionSetHasOneRuleTelemetryLogger() + { + var telemetryConfiguration = TelemetryConfiguration.CreateDefault(); + var telemetryClient = new TelemetryClient(telemetryConfiguration); + var telemetry = new Telemetry(telemetryConfiguration); + + var options = new AnalyzeOptions(); + options.TargetFileSpecifiers = new List { "test.dll" }; + options.OutputFilePath = "test/path/"; + options.Quiet = true; + var context = new BinaryAnalyzerContext(); + + var command = new MultithreadedAnalyzeCommand(telemetry); + command.InitializeGlobalContextFromOptions(options, ref context); + + Assert.IsType(context.Logger); + + var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger; + Assert.Contains(aggregatingLogger.Loggers, l => l is RuleTelemetryLogger); + Assert.Equal(1, aggregatingLogger.Loggers.Count); + } + + [Fact] + public void MultithreadedAnalyzeCommand_InitializeGlobalContextFromOptions_QuietOptionSetAndNoTelemetryHasOneConsoleLogger() + { + var options = new AnalyzeOptions(); + options.TargetFileSpecifiers = new List { "test.dll" }; + options.DisableTelemetry = true; + options.Quiet = true; + options.OutputFilePath = "test/path/"; + var context = new BinaryAnalyzerContext(); + + var command = new MultithreadedAnalyzeCommand(); + command.InitializeGlobalContextFromOptions(options, ref context); + + Assert.IsType(context.Logger); + + var aggregatingLogger = (Sarif.Driver.AggregatingLogger)context.Logger; + Assert.Equal(0, aggregatingLogger.Loggers.Count); + } } }