Skip to content

Commit

Permalink
CodeQL suppressions (#10524)
Browse files Browse the repository at this point in the history
* Add CodeQL supporessions

* Apply suggestions from code review

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

* Make the BinaryFormatter opt-in implications more explicit

* Accept PR suggestion

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

---------

Co-authored-by: Rainer Sigwald <raines@microsoft.com>
  • Loading branch information
JanKrivanek and rainersigwald authored Aug 19, 2024
1 parent f422d8d commit 1a51dd8
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 2 deletions.
2 changes: 1 addition & 1 deletion documentation/wiki/ChangeWaves.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
- [Emit eval props if requested by any sink](https://github.com/dotnet/msbuild/pull/10243)

### 17.10
- [AppDomain configuration is serialized without using BinFmt](https://github.com/dotnet/msbuild/pull/9320) - feature can be opted out only if [BinaryFormatter](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter) is allowed at runtime by editing `MSBuild.runtimeconfig.json`
- [AppDomain configuration is serialized without using BinFmt](https://github.com/dotnet/msbuild/pull/9320) - feature can be opted out only if [BinaryFormatter](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.serialization.formatters.binary.binaryformatter) is allowed at runtime by editing `MSBuild.runtimeconfig.json`. **Please note that [any usage of BinaryFormatter is insecure](https://learn.microsoft.com/dotnet/standard/serialization/binaryformatter-security-guide).**
- [Warning on serialization custom events by default in .NET framework](https://github.com/dotnet/msbuild/pull/9318)
- [Cache SDK resolver data process-wide](https://github.com/dotnet/msbuild/pull/9335)
- [Target parameters will be unquoted](https://github.com/dotnet/msbuild/pull/9452), meaning the ';' symbol in the parameter target name will always be treated as separator
Expand Down
6 changes: 5 additions & 1 deletion src/Framework/BinaryTranslator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1197,11 +1197,15 @@ public void TranslateEnum<T>(ref T value, int numericValue)
/// <param name="value">The value to be translated.</param>
public void TranslateDotNet<T>(ref T value)
{
if (!TranslateNullable(value))
// All the calling paths are already guarded by ChangeWaves.Wave17_10 - so it's a no-op adding it here as well.
// But let's have it here explicitly - so it's clearer for the CodeQL reviewers.
if (!TranslateNullable(value) || !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10))
{
return;
}

// codeql[cs/dangerous-binary-deserialization] This code needs explicit opt-in to be used (ChangeWaves.Wave17_10). This exists as a temporary compat opt-in for old 3rd party loggers, before they are migrated based on documented guidance.
// The opt-in documentation: https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves.md#1710
BinaryFormatter formatter = new BinaryFormatter();
formatter.Serialize(_packetStream, value);
}
Expand Down
1 change: 1 addition & 0 deletions src/Tasks/ManifestUtil/Util.cs
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ private static void GetFileInfoImpl(string path, string targetFrameWorkVersion,
if (string.IsNullOrEmpty(targetFrameWorkVersion) || CompareFrameworkVersions(targetFrameWorkVersion, Constants.TargetFrameworkVersion40) <= 0)
{
#pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter
// codeql[cs/weak-crypto] .NET 4.0 and earlier versions cannot parse SHA-2. Newer Frameworks use SHA256. https://devdiv.visualstudio.com/DevDiv/_workitems/edit/139025
hashAlg = SHA1.Create(
#if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES
"System.Security.Cryptography.SHA1CryptoServiceProvider"
Expand Down
2 changes: 2 additions & 0 deletions src/Tasks/ManifestUtil/mansign2.cs
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ private static byte[] ComputeHashFromManifest(XmlDocument manifestDom, bool oldF
else
{
#pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter
// codeql[cs/weak-crypto] SHA1 is retained for compatibility reasons as an option in VisualStudio signing page and consequently in the trust manager, default is SHA2. https://devdiv.visualstudio.com/DevDiv/_workitems/edit/139025
using (SHA1 sha1 = SHA1.Create(
#if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES
"System.Security.Cryptography.SHA1CryptoServiceProvider"
Expand Down Expand Up @@ -648,6 +649,7 @@ private static byte[] ComputeHashFromManifest(XmlDocument manifestDom, bool oldF
else
{
#pragma warning disable SA1111, SA1009 // Closing parenthesis should be on line of last parameter
// codeql[cs/weak-crypto] SHA1 is retained for compatibility reasons as an option in VisualStudio signing page and consequently in the trust manager, default is SHA2. https://devdiv.visualstudio.com/DevDiv/_workitems/edit/139025
using (SHA1 sha1 = SHA1.Create(
#if FEATURE_CRYPTOGRAPHIC_FACTORY_ALGORITHM_NAMES
"System.Security.Cryptography.SHA1CryptoServiceProvider"
Expand Down

0 comments on commit 1a51dd8

Please sign in to comment.