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.

Sign up for GitHub

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

FIX: [CodeQL: SM02196] Weak cryptography in TrackingConfigHashAlgorithm.cs #5065

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

FIX: [CodeQL: SM02196] Weak cryptography in TrackingConfigHashAlgorithm.cs #5065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8e2f83
Updating the hash to SHA256 and updating unit tests
MantavyaDh Dec 18, 2024
12d2efd
Adding the feature knob
MantavyaDh Dec 19, 2024
b9f7f27
Fixing the Unit Tests
MantavyaDh Dec 20, 2024
92d726f
Correcting the execution context
MantavyaDh Dec 20, 2024
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
7 changes: 7 additions & 0 deletions src/Agent.Sdk/Knob/AgentKnobs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,5 +779,12 @@ public class AgentKnobs
"If true, agent will use sparse checkout in checkout task.",
new RuntimeKnobSource("AGENT_USE_SPARSE_CHECKOUT_IN_CHECKOUT_TASK"),
new BuiltInDefaultKnobSource("false"));

public static readonly Knob UseSha256InComputeHash = new Knob(
nameof(UseSha256InComputeHash),
"If true, agent will use SHA256 algorithm in ComputeHash.",
new RuntimeKnobSource("AGENT_USE_SHA256_IN_COMPUTE_HASH"),
new EnvironmentKnobSource("AGENT_USE_SHA256_IN_COMPUTE_HASH"),
new BuiltInDefaultKnobSource("true"));
}
}
3 changes: 2 additions & 1 deletion src/Agent.Worker/Build/TrackingConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.ComponentModel;
using System.Globalization;
using System.IO;
using Agent.Sdk.Knob;

namespace Microsoft.VisualStudio.Services.Agent.Worker.Build
{
Expand Down Expand Up @@ -98,7 +99,7 @@ public TrackingConfig(
}

// Now that we have all the repositories set up, we can compute the config hash
HashKey = TrackingConfigHashAlgorithm.ComputeHash(CollectionId, DefinitionId, RepositoryTrackingInfo);
HashKey = TrackingConfigHashAlgorithm.ComputeHash(CollectionId, DefinitionId, RepositoryTrackingInfo, AgentKnobs.UseSha256InComputeHash.GetValue(executionContext).AsBoolean());
}

[JsonIgnore]
Expand Down
37 changes: 27 additions & 10 deletions src/Agent.Worker/Build/TrackingConfigHashAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class TrackingConfigHashAlgorithm
/// <summary>
/// This method returns the hash key that combines repository hash keys.
/// </summary>
public static string ComputeHash(string collectionId, string definitionId, IList<RepositoryTrackingInfo> repositories)
public static string ComputeHash(string collectionId, string definitionId, IList<RepositoryTrackingInfo> repositories, bool UseSha256InComputeHash)
{
// Validate parameters.
ArgUtil.NotNull(collectionId, nameof(collectionId));
Expand Down Expand Up @@ -50,22 +50,39 @@ public static string ComputeHash(string collectionId, string definitionId, IList
definitionId,
string.Join(';', repositories.OrderBy(x => x.Identifier).Select(x => $"{x.Identifier}:{x.RepositoryUrl}")));
}
return CreateHash(hashInput);
return CreateHash(hashInput, UseSha256InComputeHash);
}

private static string CreateHash(string hashInput)
private static string CreateHash(string hashInput, bool UseSha256InComputeHash)
{
using (SHA1 sha1Hash = SHA1.Create())
if(UseSha256InComputeHash)
{
byte[] data = sha1Hash.ComputeHash(Encoding.UTF8.GetBytes(hashInput));
StringBuilder hexString = new StringBuilder();
for (int i = 0; i < data.Length; i++)
using (SHA256 sha256Hash = SHA256.Create())
{
hexString.Append(data[i].ToString("x2"));
}
byte[] data = sha256Hash.ComputeHash(Encoding.UTF8.GetBytes(hashInput));
StringBuilder hexString = new StringBuilder();
for (int i = 0; i < data.Length; i++)
{
hexString.Append(data[i].ToString("x2"));
}

return hexString.ToString();
return hexString.ToString();
}
}
else
{
using (SHA1 SHA1 = SHA1.Create())
{
byte[] data = SHA1.ComputeHash(Encoding.UTF8.GetBytes(hashInput));
StringBuilder hexString = new StringBuilder();
for (int i = 0; i < data.Length; i++)
{
hexString.Append(data[i].ToString("x2"));
}

return hexString.ToString();
}
}
}
}
}
5 changes: 3 additions & 2 deletions src/Agent.Worker/ExecutionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,8 +718,9 @@ private string GetWorkspaceIdentifier(Pipelines.AgentJobRequestMessage message)
Variables.TryGetValue(Constants.Variables.System.CollectionId, out string collectionId);
Variables.TryGetValue(Constants.Variables.System.DefinitionId, out string definitionId);
var repoTrackingInfos = message.Resources.Repositories.Select(repo => new Build.RepositoryTrackingInfo(repo, "/")).ToList();
var workspaceIdentifier = Build.TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, repoTrackingInfos);

// Determine the hash algorithm based on the knob value
var workspaceIdentifier = Build.TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, repoTrackingInfos, AgentKnobs.UseSha256InComputeHash.GetValue(this).AsBoolean());

Trace.Info($"WorkspaceIdentifier '{workspaceIdentifier}' created for repos {String.Join(',', repoTrackingInfos)}");
return workspaceIdentifier;
}
Expand Down
1 change: 1 addition & 0 deletions src/Test/L0/Worker/Build/BuildDirectoryManagerL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ private TestHostContext Setup(
_variables.Set(Constants.Variables.System.DefinitionId, DefinitionId);
_variables.Set(Constants.Variables.Build.Clean, $"{cleanOption}");
_ec.Setup(x => x.Variables).Returns(_variables);
_ec.Setup(x => x.GetScopedEnvironment()).Returns(new SystemEnvironment());

// Store the expected tracking file path.
_trackingFile = Path.Combine(
Expand Down
2 changes: 2 additions & 0 deletions src/Test/L0/Worker/Build/BuildJobExtensionL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,8 @@ private TestHostContext Setup([CallerMemberName] string name = "",
x.SetVariable(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>(), It.IsAny<bool>()))
.Callback((string varName, string varValue, bool isSecret, bool isOutput, bool isFilePath, bool isReadOnly, bool preserveCase) => { _variables.Set(varName, varValue, false); });

_ec.Setup(x => x.GetScopedEnvironment()).Returns(new SystemEnvironment());

_extensionManager.Setup(x => x.GetExtensions<ISourceProvider>())
.Returns(new List<ISourceProvider> { _sourceProvider.Object });

Expand Down
48 changes: 24 additions & 24 deletions src/Test/L0/Worker/Build/TrackingConfigHashAlgorithmL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
using Xunit;

namespace Microsoft.VisualStudio.Services.Agent.Tests.Worker.Build
{
{
public sealed class TrackingConfigHashAlgorithmL0
{
// This test is the original test case and is kept to make sure back compat still works.
Expand Down Expand Up @@ -38,10 +38,10 @@ public void ComputeHash_returns_correct_hash()
};

// Act.
string hashKey = TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repoInfo });
string hashKey = TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repoInfo }, true);

// Assert.
Assert.Equal("5c5c3d7ac33cca6604736eb3af977f23f1cf1146", hashKey);
Assert.Equal("55e3171bf43a983b419387b5d952d3ee7dcb195e262fc4c78d47a92763b6b001", hashKey);
}
}

Expand All @@ -62,12 +62,12 @@ public void ComputeHash_should_throw_when_parameters_invalid()
string collectionId = "866A5D79-7735-49E3-87DA-02E76CF8D03A";
string definitionId = "123";

Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, null, null));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, null));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { new RepositoryTrackingInfo() }));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, null, new[] { repo }));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, definitionId, new[] { repo }));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, null, new[] { repo }));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, null, null, true));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, null, true));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { new RepositoryTrackingInfo() }, true));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, null, new[] { repo }, true));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(null, definitionId, new[] { repo }, true));
Assert.Throws<ArgumentNullException>(() => TrackingConfigHashAlgorithm.ComputeHash(collectionId, null, new[] { repo }, true));
}
}

Expand Down Expand Up @@ -96,19 +96,19 @@ public void ComputeHash_with_single_repo_should_return_correct_hash()
string definitionId = "123";

// Make sure that only the coll, def, and url are used in the hash
Assert.Equal("9a89eaa7b8b603633ef1dd5c46464355c716268f", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }));
Assert.Equal("9a89eaa7b8b603633ef1dd5c46464355c716268f", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo2 }));
Assert.Equal(TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }), TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }));
Assert.Equal("a42b0f8ccd83cec8294b0c861a8d769e4f7fbc53ad3d3c96d2d1b66afdcdcca7", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }, true));
Assert.Equal("a42b0f8ccd83cec8294b0c861a8d769e4f7fbc53ad3d3c96d2d1b66afdcdcca7", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo2 }, true));
Assert.Equal(TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }, true), TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }, true));

// Make sure that different coll creates different hash
Assert.Equal("2a41800cd3e7f5983a7643698f67104ed95101f3", TrackingConfigHashAlgorithm.ComputeHash("FFFA5D79-7735-49E3-87DA-02E76CF8D03A", definitionId, new[] { repo1 }));
Assert.Equal("55437a6c3c12ea17847e33c3d96697833a05519e06acbe90fff74a64734fca1c", TrackingConfigHashAlgorithm.ComputeHash("FFFA5D79-7735-49E3-87DA-02E76CF8D03A", definitionId, new[] { repo1 }, true));

// Make sure that different def creates different hash
Assert.Equal("84b4463d95631b4d358f4b67d8994fe7d5b0c013", TrackingConfigHashAlgorithm.ComputeHash(collectionId, "321", new[] { repo1 }));
Assert.Equal("72443dbf31971f84922a6f3a6c58052fc0c60d9f1eb17b83a35e6e099098c179", TrackingConfigHashAlgorithm.ComputeHash(collectionId, "321", new[] { repo1 }, true));

// Make sure that different url creates different hash
repo1.RepositoryUrl = "https://jpricket@codedev.ms/jpricket/MyFirstProject/_git/new_url";
Assert.Equal("6505a9272091df39b90d6fd359e3bf39a7883e9e", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }));
Assert.Equal("2b29540cb9d2b68cc068af7afd0593276fc9e0b09af4e5d7b2065cc9021070fc", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1 }, true));
}
}

Expand Down Expand Up @@ -160,29 +160,29 @@ public void ComputeHash_with_multi_repos_should_return_correct_hash()
string definitionId = "123";

// Make sure we get the same hash every time
Assert.Equal("502520817d9c9d3002a7a56526f7518709fecd6a", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }));
Assert.Equal("0a7fcd16ea54872456169a3cbf5a7d8e8efda976b755a13278b193fedaeb5784", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }, true));

// Make sure that only the coll, def, identifier, and url are used in the hash
Assert.Equal(
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }),
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2_newPath }));
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }, true),
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2_newPath }, true));

// Make sure that different coll creates different hash
Assert.Equal("ea81feec2216d9da8adc7f29005d44eafbd12626", TrackingConfigHashAlgorithm.ComputeHash("FFFA5D79-7735-49E3-87DA-02E76CF8D03A", definitionId, new[] { repo1, repo2 }));
Assert.Equal("b3956a6be8dde823bce2373fdef7358e255107bc4de986a61aeaffd11e253118", TrackingConfigHashAlgorithm.ComputeHash("FFFA5D79-7735-49E3-87DA-02E76CF8D03A", definitionId, new[] { repo1, repo2 }, true));

// Make sure that different def creates different hash
Assert.Equal("8742e9847224e2b9de3884beac15759cfd8403e0", TrackingConfigHashAlgorithm.ComputeHash(collectionId, "321", new[] { repo1, repo2 }));
Assert.Equal("dff47196d014b4373641e33627901f986cde0815de0122fa76f401abd1140701", TrackingConfigHashAlgorithm.ComputeHash(collectionId, "321", new[] { repo1, repo2 }, true));

// Make sure that different url creates different hash
Assert.Equal("279dd578a58faba3f6cd23c3d62d452448b1e8cc", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1_newUrl, repo2 }));
Assert.Equal("ce83c8cd4f9b603345d21d2a294f7126e1e37c6d13cf6225516106a69528cc95", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1_newUrl, repo2 }, true));

// Make sure that different alias creates different hash
Assert.Equal("e3553307993d00df159a011b129a7f720084ee02", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1_newAlias, repo2 }));
Assert.Equal("2ca4bc7221eb412db850596fc02dc4e5b61c2125c997ea07f11215bffe605d33", TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1_newAlias, repo2 }, true));

// Make sure order doesn't change hash
Assert.Equal(
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }),
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo2, repo1 }));
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo1, repo2 }, true),
TrackingConfigHashAlgorithm.ComputeHash(collectionId, definitionId, new[] { repo2, repo1 }, true));

}
}
Expand Down
12 changes: 7 additions & 5 deletions src/Test/L0/Worker/Build/TrackingConfigL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.VisualStudio.Services.Agent.Worker.Build;
using Moq;
using Xunit;
using Agent.Sdk;

namespace Microsoft.VisualStudio.Services.Agent.Tests.Worker.Build
{
Expand Down Expand Up @@ -44,6 +45,7 @@ public void TrackingConfig_copy_legacy_ctor_should_fill_in_fields_correctly()
using (TestHostContext tc = Setup(out Mock<IExecutionContext> mockExecutionContext))
{
// Arrange.
mockExecutionContext.Setup(x => x.GetScopedEnvironment()).Returns(new SystemEnvironment());
var legacyConfig = new LegacyTrackingConfig
{
BuildDirectory = Path.Combine("path", "_work", "123"),
Expand Down Expand Up @@ -84,12 +86,12 @@ public void TrackingConfig_ctor_should_fill_in_fields_correctly()
{
using (TestHostContext tc = Setup(out Mock<IExecutionContext> mockExecutionContext))
{
mockExecutionContext.Setup(x => x.GetScopedEnvironment()).Returns(new SystemEnvironment());
// Arrange.
var repository = new RepositoryResource() { Type = RepositoryTypes.Git, Url = new Uri(RepositoryUrl) };

// Act.
var config = new TrackingConfig(mockExecutionContext.Object, new[] { repository }, DefinitionId);

// Assert.
Assert.Equal(Path.Combine("322", "a"), config.ArtifactsDirectory);
Assert.Equal("322", config.BuildDirectory);
Expand All @@ -99,7 +101,7 @@ public void TrackingConfig_ctor_should_fill_in_fields_correctly()
Assert.Equal(DefinitionName, config.DefinitionName);
Assert.Equal(3, config.FileFormatVersion);
Assert.Equal(null, config.FileLocation);
Assert.Equal("ea7c71421cca06c927f73627b66d6b4f4c3a5f4a", config.HashKey);
Assert.Equal("9706446cf81dbb09854838b405618476576051bf54b5cbf6ce493c180c0a0a87", config.HashKey);
Assert.Equal(RepositoryTypes.Git, config.RepositoryType);
Assert.Equal(RepositoryUrl, config.RepositoryUrl);
Assert.Equal(Path.Combine("322", "s"), config.SourcesDirectory);
Expand All @@ -120,12 +122,12 @@ public void TrackingConfig_clone_should_fill_in_fields_correctly()
using (TestHostContext tc = Setup(out Mock<IExecutionContext> mockExecutionContext))
{
// Arrange.
mockExecutionContext.Setup(x => x.GetScopedEnvironment()).Returns(new SystemEnvironment());
var repository = new RepositoryResource() { Type = RepositoryTypes.Git, Url = new Uri(RepositoryUrl) };

// Act.
var config = new TrackingConfig(mockExecutionContext.Object, new[] { repository }, DefinitionId);
var clone = config.Clone();

// Assert.
// Verify the original first
Assert.Equal(Path.Combine("322", "a"), config.ArtifactsDirectory);
Expand All @@ -136,7 +138,7 @@ public void TrackingConfig_clone_should_fill_in_fields_correctly()
Assert.Equal(DefinitionName, config.DefinitionName);
Assert.Equal(3, config.FileFormatVersion);
Assert.Equal(null, config.FileLocation);
Assert.Equal("ea7c71421cca06c927f73627b66d6b4f4c3a5f4a", config.HashKey);
Assert.Equal("9706446cf81dbb09854838b405618476576051bf54b5cbf6ce493c180c0a0a87", config.HashKey);
Assert.Equal(RepositoryTypes.Git, config.RepositoryType);
Assert.Equal(RepositoryUrl, config.RepositoryUrl);
Assert.Equal(Path.Combine("322", "s"), config.SourcesDirectory);
Expand All @@ -155,7 +157,7 @@ public void TrackingConfig_clone_should_fill_in_fields_correctly()
Assert.Equal(DefinitionName, clone.DefinitionName);
Assert.Equal(3, clone.FileFormatVersion);
Assert.Equal(null, clone.FileLocation);
Assert.Equal("ea7c71421cca06c927f73627b66d6b4f4c3a5f4a", clone.HashKey);
Assert.Equal("9706446cf81dbb09854838b405618476576051bf54b5cbf6ce493c180c0a0a87", clone.HashKey);
Assert.Equal(RepositoryTypes.Git, clone.RepositoryType);
Assert.Equal(RepositoryUrl, clone.RepositoryUrl);
Assert.Equal(Path.Combine("322", "s"), clone.SourcesDirectory);
Expand Down
Loading
Loading