BinSkim .NET Updates to version 9 #1024

AllDwarf · 2024-12-19T16:48:27Z

[WIP] BinSkim .NET Updates to version 9

Overview

This pull request focuses on updating the BinSkim project to incorporate the latest .NET updates. The goal is to ensure compatibility with the newest .NET features and improve overall performance and security.

Changes

Updated the project to target the latest .NET version.

Benefits

Enhanced Compatibility: Ensures that the BinSkim project is compatible with the latest .NET version, taking advantage of new features and improvements.

Testing

Verified that the project builds successfully with the latest .NET version.
Ran all existing tests to ensure no regressions were introduced.
Conducted manual testing to confirm that the functionality remains intact.
Additional Notes
If you encounter any issues or have questions about these updates, please feel free to reach out.

src/BinaryParsers/ElfBinary/Dwarf/DwarfMemoryReader.cs

+            catch (Exception ex)
+            {
+                throw new InvalidOperationException("Failed to read string from memory.", ex);
+            }


BuildAndTest.cmd

README.md

mkacmar

There are some configs under .vscode/ you want to change too.

global.json

mkacmar · 2025-01-30T23:11:18Z

src/BinaryParsers/ElfBinary/Dwarf/DwarfMemoryReader.cs

@@ -121,13 +121,18 @@ public ulong ReadLength(out bool is64bit)
        /// <summary>
        /// Reads the string from the current position in the stream.
        /// </summary>
-        [HandleProcessCorruptedStateExceptions]


I may be reading the docs wrong, but with HandleProcessCorruptedStateExceptions being removed - are you still trying to catch CSEs with the try-catch block?

well I went over the docs, and as of now the only option seems to be try catch, so I did it with the generic try catch clause. But I'm more than open to the suggestions for improvements :)

Yeah, I'm kind of wondering if try-catch here actually serves any purpose and whether we can remove it altogether to not give us a false sense of "security" of exception being caught (from docs):

By default, the common language runtime (CLR) does not deliver these exceptions to managed code, and the try/catch blocks (and other exception-handling clauses) are not invoked for them.

.NET Core only: Even though this attribute exists in .NET Core, since the recovery from corrupted process state exceptions is not supported, this attribute is ignored. The CLR doesn't deliver corrupted process state exceptions to the managed code.

It doesn't seem like we have any automated tests for this right?

src/Test.UnitTests.BinaryParsers/PEBinary/PEBinaryTests.cs

src/Test.UnitTests.BinSkim.Driver/MultithreadedAnalyzeCommandTests.cs

src/Test.FunctionalTests.BinSkim.Driver/UpdateBaselines.ps1

src/Test.ConcurrencyTests/rewriteUnitTests.ps1

mkacmar · 2025-01-31T00:10:42Z

src/Directory.Packages.props

Nitpick: It seems there's a mix of major, minor and patch updates - I assume not all of them are strictly .NET update related (I'd be surprised if some of these patch level updates would be). I think it's fine to update dependencies in general, but too many changes here make it a bit tricky to review these. Separate PR in a regular dependency update cycle would be great in the future.

README.md

Enhance memory reading and COM object management with error handling and platform checks Add static keyword to lambdas in AnalyzeCommandTests.cs Modified lambda expressions in AnalyzeCommandTests.cs to be static. This change ensures that the lambdas do not capture variables from the enclosing scope, potentially improving performance and clarity. Fixing version constant file Refactor project to target .NET 9.0 and remove obsolete configurations Update ADO build configuration to target .NET 9.0 and add .vscode to .gitignore Update GitHub workflows to target .NET 9.0 and improve formatting steps Update .gitignore and improve Build scripts for .NET 9.0 compatibility Directory.Packages.props fixes Fix formatting in VersionConstants.cs by removing unnecessary whitespace Update ADO build configuration to allow all branches for PRs Remove platform-specific exceptions in PDB handling for cross-platform support Refactor platform-specific code and resource management Removed `using System.Runtime.Versioning;` and `[SupportedOSPlatform("windows")]` from `MSDiaComWrapper.cs` and `Pdb.cs`. Replaced `Marshal.GetObjectForIUnknown` with `ResourceReleaser.GetObjectForIUnknown` in `MSDiaComWrapper.cs`. Removed `OperatingSystem.IsWindows()` checks in `Pdb.cs` and ensured `PlatformSpecificHelpers.ThrowIfNotOnWindows()` still enforces Windows-only execution in `Init` methods. Simplified codebase by removing redundant platform checks.

mkacmar

I've left a few comments, overall LGTM!

mkacmar · 2025-02-12T14:08:53Z

.github/workflows/dotnet-format.yml

@@ -16,4 +16,4 @@ jobs:
      run: dotnet tool install -g dotnet-format

    - name: dotnet format
-      run: dotnet-format --folder --check
+      run: dotnet-format --folder --check


Nitpick: is this intentional?

mkacmar · 2025-02-12T14:15:46Z

src/BinaryParsers/ElfBinary/Dwarf/DwarfMemoryReader.cs

@@ -121,13 +121,18 @@ public ulong ReadLength(out bool is64bit)
        /// <summary>
        /// Reads the string from the current position in the stream.
        /// </summary>
-        [HandleProcessCorruptedStateExceptions]


Yeah, I'm kind of wondering if try-catch here actually serves any purpose and whether we can remove it altogether to not give us a false sense of "security" of exception being caught (from docs):

By default, the common language runtime (CLR) does not deliver these exceptions to managed code, and the try/catch blocks (and other exception-handling clauses) are not invoked for them.

.NET Core only: Even though this attribute exists in .NET Core, since the recovery from corrupted process state exceptions is not supported, this attribute is ignored. The CLR doesn't deliver corrupted process state exceptions to the managed code.

It doesn't seem like we have any automated tests for this right?

mkacmar · 2025-02-12T14:27:08Z

src/Test.UnitTests.BinaryParsers/PEBinary/PEBinaryTests.cs

@@ -6,6 +6,7 @@
 using System.IO;
 using System.Linq;
 using System.Reflection;
+using System.Runtime.Versioning;


Is this still needed?

AllDwarf changed the title ~~BinSkim .NET Updates to version 9~~ [WIP] BinSkim .NET Updates to version 9 Dec 19, 2024

mkacmar assigned AllDwarf Jan 9, 2025

AllDwarf force-pushed the users/marekaldorf/BinSkim_dotnet_updates branch from 3d1696b to 990a79d Compare January 9, 2025 15:45

AllDwarf force-pushed the users/marekaldorf/BinSkim_dotnet_updates branch 2 times, most recently from 8c0ae1d to c90c64e Compare January 21, 2025 10:43

AllDwarf changed the title ~~[WIP] BinSkim .NET Updates to version 9~~ BinSkim .NET Updates to version 9 Jan 21, 2025

AllDwarf marked this pull request as ready for review January 21, 2025 10:46

AllDwarf requested a review from a team as a code owner January 21, 2025 10:46

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

src/BinaryParsers/ElfBinary/Dwarf/DwarfMemoryReader.cs Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jan 21, 2025

View reviewed changes

src/BinaryParsers/ElfBinary/Dwarf/DwarfMemoryReader.cs

Comment on lines +132 to +135

catch (Exception ex)

{

throw new InvalidOperationException("Failed to read string from memory.", ex);

}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.

AllDwarf force-pushed the users/marekaldorf/BinSkim_dotnet_updates branch from c72a5f2 to 0884878 Compare January 21, 2025 20:00

AllDwarf mentioned this pull request Jan 24, 2025

Update BinSkim to use Net8 #1037

Closed

AllDwarf added this to the Release v4.3.2 milestone Jan 24, 2025

mkacmar reviewed Jan 30, 2025

View reviewed changes

BuildAndTest.cmd Outdated Show resolved Hide resolved

mkacmar reviewed Jan 30, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

mkacmar reviewed Jan 31, 2025

View reviewed changes

Sasinkas approved these changes Feb 5, 2025

View reviewed changes

AllDwarf added 2 commits February 5, 2025 15:09

Fix formatting in BuildAndTest.cmd by removing unnecessary whitespace

000e522

AllDwarf force-pushed the users/marekaldorf/BinSkim_dotnet_updates branch from 53a1ae1 to 000e522 Compare February 5, 2025 14:13

AllDwarf added 3 commits February 5, 2025 15:41

Add basic concurrency tests using Coyote framework

58f74d1

Remove unnecessary SupportedOSPlatform attribute from PEBinaryTests

9c5a907

Fix function definition formatting in UpdateBaselines.ps1

3653775

mkacmar approved these changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BinSkim .NET Updates to version 9 #1024

BinSkim .NET Updates to version 9 #1024

AllDwarf commented Dec 19, 2024

mkacmar left a comment

mkacmar Jan 30, 2025

AllDwarf Feb 5, 2025

mkacmar Feb 12, 2025

mkacmar Jan 31, 2025

mkacmar left a comment

mkacmar Feb 12, 2025

mkacmar Feb 12, 2025

mkacmar Feb 12, 2025

BinSkim .NET Updates to version 9 #1024

Are you sure you want to change the base?

BinSkim .NET Updates to version 9 #1024

Conversation

AllDwarf commented Dec 19, 2024

[WIP] BinSkim .NET Updates to version 9

Overview

Changes

Benefits

Testing

mkacmar left a comment

Choose a reason for hiding this comment

mkacmar Jan 30, 2025

Choose a reason for hiding this comment

AllDwarf Feb 5, 2025

Choose a reason for hiding this comment

mkacmar Feb 12, 2025

Choose a reason for hiding this comment

mkacmar Jan 31, 2025

Choose a reason for hiding this comment

mkacmar left a comment

Choose a reason for hiding this comment

mkacmar Feb 12, 2025

Choose a reason for hiding this comment

mkacmar Feb 12, 2025

Choose a reason for hiding this comment

mkacmar Feb 12, 2025

Choose a reason for hiding this comment