Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

* FPS: BA2004.EnableSecureSourceCodeHashing now will no longer generate false positives for UWP App regarding dummy.obj. #987

Merged
merged 6 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ReleaseHistory.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
* BUG: Fix `ERR998.ExceptionInAnalyze`: `InvalidOperationException: Unrecognized crypto HRESULT: 0x80096011` for check `BA2022.SignSecurely` when the signature is malformed, by adding missing error code to error description mappings. [969](https://github.com/microsoft/binskim/pull/969)
* NEW: `BA4002.ReportElfOrMachoCompilerData`, which collects telemetry data for Elf and Macho files, is now enabled by default.
* NEW: Add `--disable-telemetry` argument to disable telemetry collection.
* FPS: `BA2004.EnableSecureSourceCodeHashing` will now no longer generate false positives for Universal Windows Platform (UWP) app regarding `dummy.obj`. [#976](https://github.com/microsoft/binskim/pull/976)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: FPS should come after BUG in the standard order.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stacywray I don't think that list is ordered. Alphabetical ordering makes sense, but why should BUG be prioritized over FPS otherwise?


## **v4.2.1**
* FPS: `BA2004.EnableSecureSourceCodeHashing` now will no longer generate false positives on precompiled headers, they are always without hash. [#965](https://github.com/microsoft/binskim/pull/965)
Expand Down
10 changes: 5 additions & 5 deletions docs/BinSkimRules.md
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ Binaries should not take dependencies on code with known security vulnerabilitie

### Description

Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code. This 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. This 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. Legacy 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). Using 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. For managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the `<ChecksumAlgorithm>` project property with 'SHA256' to enable secure source code hashing. For native binaries, pass '/ZH:SHA_256' on the cl.exe command-line to enable secure source code hashing.
Compilers can generate and store checksums of source files in order to provide linkage between binaries, their PDBs, and associated source code. This 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. This 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. Legacy 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). Using 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. For managed binaries, pass '-checksumalgorithm:SHA256' on the csc.exe command-line or populate the '<ChecksumAlgorithm>' project property with 'SHA256' to enable secure source code hashing. For native binaries, pass '/ZH:SHA_256' on the cl.exe command-line to enable secure source code hashing.

### Messages

Expand All @@ -359,7 +359,7 @@ Compilers can generate and store checksums of source files in order to provide l

#### `Managed`: Error

'{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 `<ChecksumAlgorithm>` property with 'SHA256' to enable secure source code hashing.
'{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 <ChecksumAlgorithm> property with 'SHA256' to enable secure source code hashing.

#### `NativeWithInsecureDirectCompilands`: Error

Expand Down Expand Up @@ -808,18 +808,18 @@ Images should be correctly signed by trusted publishers using cryptographically

### Description

Application code should be compiled with the Spectre mitigations switch (/Qspectre cl.exe command-line argument or `<SpectreMitigation>Spectre</SpectreMitigation>` build property). Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve this issue, provide the /Qspectre switch on the compiler command-line (or specify `<SpectreMitigation>Spectre</SpectreMitigation>` in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request. You may need to install the 'C++ spectre-mitigated libs' component from the Visual Studio installer if you observe violations against C runtime libraries such as libcmt.lib, libvcruntime.lib, etc.
Application code should be compiled with the Spectre mitigations switch (/Qspectre cl.exe command-line argument or <SpectreMitigation>Spectre</SpectreMitigation> build property). Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve this issue, provide the /Qspectre switch on the compiler command-line (or specify <SpectreMitigation>Spectre</SpectreMitigation> in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request. You may need to install the 'C++ spectre-mitigated libs' component from the Visual Studio installer if you observe violations against C runtime libraries such as libcmt.lib, libvcruntime.lib, etc.

### Messages

#### `Warning`: Warning

'{0}' was compiled with one or more modules that do not enable code generation mitigations for speculative execution side-channel attack (Spectre) vulnerabilities. Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve the issue, provide the /Qspectre switch on the compiler command-line (or specify `<SpectreMitigation>Spectre</SpectreMitigation>` in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request.
'{0}' was compiled with one or more modules that do not enable code generation mitigations for speculative execution side-channel attack (Spectre) vulnerabilities. Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve the issue, provide the /Qspectre switch on the compiler command-line (or specify <SpectreMitigation>Spectre</SpectreMitigation> in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request.
{1}

#### `WarningMissingCommandLine`: Warning

{0}' was compiled with one or more modules with a toolset that supports /Qspectre but a compiland `RawCommandLine` value is missing and the rule is therefore not able to determine if `/Qspectre` is specified. The likely cause is that the code was linked to a static library with no debug information. It is not known whether code generation mitigations for speculative execution side-channel attack (Spectre) vulnerabilities was enabled. Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve the issue, ensure that the compiler command line is present (provide the /Z7 switch) and provide the /Qspectre switch on the compiler command-line (or specify `<SpectreMitigation>Spectre</SpectreMitigation>` in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request.
{0}' was compiled with one or more modules with a toolset that supports /Qspectre but a compiland `RawCommandLine` value is missing and the rule is therefore not able to determine if `/Qspectre` is specified. The likely cause is that the code was linked to a static library with no debug information. It is not known whether code generation mitigations for speculative execution side-channel attack (Spectre) vulnerabilities was enabled. Spectre attacks can compromise hardware-based isolation, allowing non-privileged users to retrieve potentially sensitive data from the CPU cache. To resolve the issue, ensure that the compiler command line is present (provide the /Z7 switch) and provide the /Qspectre switch on the compiler command-line (or specify <SpectreMitigation>Spectre</SpectreMitigation> in build properties), or pass /d2guardspecload in cases where your compiler supports this switch and it is not possible to update to a toolset that supports /Qspectre. This warning should be addressed for code that operates on data that crosses a trust boundary and that can affect execution, such as parsing untrusted file inputs or processing query strings of a web request.

#### `SpectreMitigationUnknownNoCommandLine`: Warning

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ public void AnalyzeNativeBinaryAndPdb(BinaryAnalyzerContext context)
continue;
}

if (IsLikelyUwpDummyObj(omDetails.Language, omDetails.Library, omDetails.Name))
{
continue;
}

if (omDetails.Name.EndsWith(MSVCStandardApplicationFrameworkFileName) ||
omDetails.Name.EndsWith(AssemblyAttributesObjFileName) ||
omDetails.Name.EndsWith(AssemblyInfoObjFileName))
Expand Down Expand Up @@ -310,5 +315,11 @@ public IEnumerable<IOption> GetOptions()
// RequiredCompilerWarnings,
}.ToImmutableArray();
}

internal static bool IsLikelyUwpDummyObj(Language language, string library, string name) =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is so much more readable with the improved wrapping.

If you want to change the method name to IsUwpDummyObj --well, I think it's more than 'likely' given all these checks. :) (Or 'Presumed' if you want to be super cautious.) Up to you though. If you decide to make that change let me know and I'll restamp quickly.

language == Language.MASM &&
library != null &&
library.Equals(name, StringComparison.Ordinal) &&
library.Equals(@"c:\dummy.obj", StringComparison.Ordinal);
}
}
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using FluentAssertions;

using Microsoft.CodeAnalysis.BinaryParsers.ProgramDatabase;

using Xunit;

namespace Microsoft.CodeAnalysis.IL.Rules
{
public class EnableSecureSourceCodeHashingTests
{
[Fact]
public void IsLikelyUwpDummyObjTests()
{
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"c:\dummy.obj", @"c:\dummy.obj").Should().BeTrue();
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"d:\dummy.obj", @"d:\dummy.obj").Should().BeFalse();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaopeng-gh To make sure I understand this correctly, d:\dummy.obj should result in false? The drive letter must be c?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I believe the official one should have the path fixed as c:\dummy.obj

EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.C, @"c:\dummy.obj", @"c:\dummy.obj").Should().BeFalse();
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, @"c:\Dummy.obj", @"c:\Dummy.obj").Should().BeFalse();
EnableSecureSourceCodeHashing.IsLikelyUwpDummyObj(Language.MASM, "AnyLib", @"c:\dummy.obj").Should().BeFalse();
}
}
}
Loading