Skip to content

Commit 1d6b22e

Browse files
JeromyStJstatiaCopilot
authored
Fix thread-safety issue in IndirectSignatureFactory hash computation (#192)
* Fix thread-safety issue in IndirectSignatureFactory hash computation InternalHashAlgorithm is a shared instance cached in the factory. HashAlgorithm.ComputeHash() is not thread-safe and throws CryptographicException under concurrent usage. The fix creates a fresh HashAlgorithm instance per ComputeHash call via a CreateHashAlgorithm() helper method, ensuring thread safety without contention. Added regression tests that exercise the factory from multiple threads concurrently to validate both sync and async code paths. Fixes #191 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix thread-safety issue in CoseX509Thumprint.Match hash computation CoseX509Thumprint cached a shared HashAlgorithm instance (Hasher) and reused it in Match(), which is not thread-safe under concurrent calls. Replaced with per-call CreateHashAlgorithm() that returns fresh disposable instances, matching the same pattern used for the IndirectSignatureFactory fix. Added concurrent Match and construction regression tests. Related to #191 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: remove cached InternalHashAlgorithm, return fresh instance from property Per review feedback, removed the cached InternalHashAlgorithm field entirely. The public HashAlgorithm property now returns a fresh instance per access via CreateHashAlgorithm(), preserving the public contract while eliminating the thread-unsafe shared state. HashLength and InternalCoseHashAlgorithm are now derived from HashAlgorithmName via static helpers. Dispose is now a no-op since there are no cached resources to release. CoseHashV path (obsoleted) is already safe — its constructors create their own using HashAlgorithm instances internally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix missing dispose in CoseHashV.HashValue setter The HashValue property setter created a HashAlgorithm via GetHashAlgorithmFromCoseHashAlgorithm for length validation but never disposed it. Added using to ensure proper cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jeromy Statia (from Dev Box) <jstatia@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 40908bb commit 1d6b22e

File tree

7 files changed

+234
-50
lines changed

7 files changed

+234
-50
lines changed

‎CoseHandler.Tests/CoseX509ThumbprintTests.cs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,67 @@ public void ConstructThumbprintWithUnsupportedAlgo()
5050
{
5151
_ = new CoseX509Thumprint(SelfSignedCert1, HashAlgorithmName.SHA3_512);
5252
}
53+
54+
/// <summary>
55+
/// Validates that a single <see cref="CoseX509Thumprint"/> instance can safely call
56+
/// <see cref="CoseX509Thumprint.Match"/> from multiple threads concurrently without
57+
/// throwing <see cref="CryptographicException"/>.
58+
/// Regression test for https://github.com/microsoft/CoseSignTool/issues/191.
59+
/// </summary>
60+
[TestMethod]
61+
public void ConcurrentMatchShouldNotThrow()
62+
{
63+
// Arrange — one shared thumbprint, many threads calling Match
64+
CoseX509Thumprint thumbprint = new(SelfSignedCert1);
65+
int degreeOfParallelism = Environment.ProcessorCount * 2;
66+
int iterationsPerThread = 50;
67+
68+
// Act & Assert — hammer Match() from many threads at once
69+
Action concurrentAction = () =>
70+
{
71+
Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i =>
72+
{
73+
// Alternate between matching and non-matching certs
74+
if (i % 2 == 0)
75+
{
76+
thumbprint.Match(SelfSignedCert1).Should().BeTrue();
77+
}
78+
else
79+
{
80+
thumbprint.Match(SelfSignedCert2).Should().BeFalse();
81+
}
82+
});
83+
};
84+
85+
concurrentAction.Should().NotThrow<CryptographicException>();
86+
}
87+
88+
/// <summary>
89+
/// Validates that concurrent construction of <see cref="CoseX509Thumprint"/> instances
90+
/// with different hash algorithms is thread-safe.
91+
/// Regression test for https://github.com/microsoft/CoseSignTool/issues/191.
92+
/// </summary>
93+
[TestMethod]
94+
public void ConcurrentConstructionAndMatchShouldNotThrow()
95+
{
96+
// Arrange
97+
HashAlgorithmName[] algorithms = new[] { HashAlgorithmName.SHA256, HashAlgorithmName.SHA384, HashAlgorithmName.SHA512 };
98+
int degreeOfParallelism = Environment.ProcessorCount * 2;
99+
int iterationsPerThread = 30;
100+
101+
// Act & Assert — create thumbprints and match from many threads
102+
Action concurrentAction = () =>
103+
{
104+
Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i =>
105+
{
106+
HashAlgorithmName algo = algorithms[i % algorithms.Length];
107+
CoseX509Thumprint thumbprint = new(SelfSignedCert1, algo);
108+
109+
thumbprint.Match(SelfSignedCert1).Should().BeTrue();
110+
thumbprint.Match(SelfSignedCert2).Should().BeFalse();
111+
});
112+
};
113+
114+
concurrentAction.Should().NotThrow<CryptographicException>();
115+
}
53116
}

‎CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -502,4 +502,81 @@ public async Task TestCreateIndirectSignatureAsyncWithoutPayloadLocation()
502502
indirectSignature.TryGetPayloadHashAlgorithm(out CoseHashAlgorithm? algo).Should().BeTrue();
503503
algo!.Should().Be(CoseHashAlgorithm.SHA256);
504504
}
505+
506+
/// <summary>
507+
/// Validates that a single <see cref="IndirectSignatureFactory"/> instance can safely produce
508+
/// indirect signatures from multiple threads concurrently without throwing
509+
/// <see cref="System.Security.Cryptography.CryptographicException"/>
510+
/// ("Concurrent operations from multiple threads on this type are not supported.").
511+
/// Regression test for https://github.com/microsoft/CoseSignTool/issues/191.
512+
/// </summary>
513+
[Test]
514+
public void ConcurrentHashComputationShouldNotThrow()
515+
{
516+
// Arrange — one shared factory, many threads
517+
ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider();
518+
using IndirectSignatureFactory factory = new();
519+
int degreeOfParallelism = Environment.ProcessorCount * 2;
520+
int iterationsPerThread = 20;
521+
522+
// Act — hammer the factory from many threads at once
523+
Action concurrentAction = () =>
524+
{
525+
Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i =>
526+
{
527+
byte[] payload = new byte[128];
528+
Random.Shared.NextBytes(payload);
529+
530+
CoseSign1Message result = factory.CreateIndirectSignature(
531+
payload,
532+
coseSigningKeyProvider,
533+
"application/test.concurrent");
534+
535+
// Verify each result is valid
536+
result.IsIndirectSignature().Should().BeTrue();
537+
result.SignatureMatches(payload).Should().BeTrue();
538+
});
539+
};
540+
541+
// Assert — no CryptographicException from concurrent ComputeHash
542+
concurrentAction.Should().NotThrow<CryptographicException>();
543+
}
544+
545+
/// <summary>
546+
/// Validates that the async indirect signature creation path is also safe under
547+
/// concurrent usage from multiple threads.
548+
/// Regression test for https://github.com/microsoft/CoseSignTool/issues/191.
549+
/// </summary>
550+
[Test]
551+
public async Task ConcurrentAsyncHashComputationShouldNotThrow()
552+
{
553+
// Arrange — one shared factory, many concurrent tasks
554+
ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider();
555+
using IndirectSignatureFactory factory = new();
556+
int concurrentTasks = Environment.ProcessorCount * 4;
557+
558+
// Act — launch many concurrent async operations
559+
Task[] tasks = new Task[concurrentTasks];
560+
for (int i = 0; i < concurrentTasks; i++)
561+
{
562+
tasks[i] = Task.Run(async () =>
563+
{
564+
byte[] payload = new byte[128];
565+
Random.Shared.NextBytes(payload);
566+
567+
CoseSign1Message result = await factory.CreateIndirectSignatureAsync(
568+
payload,
569+
coseSigningKeyProvider,
570+
"application/test.concurrent.async");
571+
572+
// Verify each result is valid
573+
result.IsIndirectSignature().Should().BeTrue();
574+
result.SignatureMatches(payload).Should().BeTrue();
575+
});
576+
}
577+
578+
// Assert — all tasks complete without CryptographicException
579+
Func<Task> awaitAll = async () => await Task.WhenAll(tasks);
580+
await awaitAll.Should().NotThrowAsync<CryptographicException>();
581+
}
505582
}

‎CoseIndirectSignature/CoseHashV.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ public byte[] HashValue
3939
}
4040

4141
// sanity check the length of the hash against the specified algorithm to be sure we're not allowing a mismatch.
42-
HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm);
42+
using HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm);
4343
if (value.Length != (algo.HashSize / 8))
4444
{
4545
throw new ArgumentOutOfRangeException(nameof(value), @$"The hash value length of {value.Length} did not match the CoseHashAlgorithm {Algorithm} required length of {algo.HashSize / 8}");

‎CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,10 @@ private object CreateIndirectSignatureWithChecksInternalCoseHashEnvelopeFormat(
3939

4040
if (!payloadHashed)
4141
{
42+
using HashAlgorithm hasher = this.CreateHashAlgorithm();
4243
hash = streamPayload != null
43-
? InternalHashAlgorithm.ComputeHash(streamPayload)
44-
: InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray());
44+
? hasher.ComputeHash(streamPayload)
45+
: hasher.ComputeHash(bytePayload!.Value.ToArray());
4546
}
4647
else
4748
{
@@ -100,9 +101,10 @@ private async Task<object> CreateIndirectSignatureWithChecksInternalCoseHashEnve
100101
// Note: HashAlgorithm.ComputeHashAsync is not available in netstandard2.0
101102
// For better async support in the future, consider targeting net6.0+ where
102103
// ComputeHashAsync is available on specific hash algorithm implementations
104+
using HashAlgorithm hasher = this.CreateHashAlgorithm();
103105
hash = streamPayload != null
104-
? InternalHashAlgorithm.ComputeHash(streamPayload)
105-
: InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray());
106+
? hasher.ComputeHash(streamPayload)
107+
: hasher.ComputeHash(bytePayload!.Value.ToArray());
106108
}
107109
else
108110
{

‎CoseIndirectSignature/IndirectSignatureFactory.Direct.cs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,10 @@ private object CreateIndirectSignatureWithChecksInternalDirectFormat(
3535
string extendedContentType;
3636
if (!payloadHashed)
3737
{
38+
using HashAlgorithm hasher = this.CreateHashAlgorithm();
3839
hash = streamPayload != null
39-
? InternalHashAlgorithm.ComputeHash(streamPayload)
40-
: InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray());
40+
? hasher.ComputeHash(streamPayload)
41+
: hasher.ComputeHash(bytePayload!.Value.ToArray());
4142
extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName);
4243
}
4344
else
@@ -118,9 +119,10 @@ private async Task<object> CreateIndirectSignatureWithChecksInternalDirectFormat
118119
string extendedContentType;
119120
if (!payloadHashed)
120121
{
122+
using HashAlgorithm hasher = this.CreateHashAlgorithm();
121123
hash = streamPayload != null
122-
? InternalHashAlgorithm.ComputeHash(streamPayload)
123-
: InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray());
124+
? hasher.ComputeHash(streamPayload)
125+
: hasher.ComputeHash(bytePayload!.Value.ToArray());
124126
extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName);
125127
}
126128
else

‎CoseIndirectSignature/IndirectSignatureFactory.cs‎

Lines changed: 54 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,22 +36,32 @@ public enum IndirectSignatureVersion
3636
CoseHashEnvelope
3737
}
3838

39-
private readonly HashAlgorithm InternalHashAlgorithm;
4039
private readonly uint HashLength;
4140
private readonly CoseHashAlgorithm InternalCoseHashAlgorithm;
4241
private readonly HashAlgorithmName InternalHashAlgorithmName;
4342
private readonly ICoseSign1MessageFactory InternalMessageFactory;
4443

4544
/// <summary>
46-
/// The HashAlgorithm this factory is using.
45+
/// Creates a new <see cref="System.Security.Cryptography.HashAlgorithm"/> instance matching the algorithm this factory is configured with.
46+
/// Each access returns a new instance. Callers are responsible for disposing the returned instance.
4747
/// </summary>
48-
public HashAlgorithm HashAlgorithm => InternalHashAlgorithm;
48+
public HashAlgorithm HashAlgorithm => CreateHashAlgorithm();
4949

5050
/// <summary>
51-
/// The HashAlgorightmName this factory is using.
51+
/// The HashAlgorithmName this factory is using.
5252
/// </summary>
5353
public HashAlgorithmName HashAlgorithmName => InternalHashAlgorithmName;
5454

55+
/// <summary>
56+
/// Creates a new <see cref="System.Security.Cryptography.HashAlgorithm"/> instance that is safe for use on the calling thread.
57+
/// </summary>
58+
/// <returns>A new <see cref="System.Security.Cryptography.HashAlgorithm"/> instance. Callers are responsible for disposing this instance.</returns>
59+
private HashAlgorithm CreateHashAlgorithm()
60+
{
61+
return CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(InternalHashAlgorithmName)
62+
?? throw new InvalidOperationException($"Failed to create HashAlgorithm for {InternalHashAlgorithmName}");
63+
}
64+
5565
/// <summary>
5666
/// The CoseSign1 Message Factory this factory is using.
5767
/// </summary>
@@ -81,21 +91,49 @@ public IndirectSignatureFactory() : this(HashAlgorithmName.SHA256)
8191
public IndirectSignatureFactory(HashAlgorithmName hashAlgorithmName, ICoseSign1MessageFactory coseSign1MessageFactory)
8292
{
8393
InternalHashAlgorithmName = hashAlgorithmName;
84-
InternalHashAlgorithm = CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(hashAlgorithmName) ?? throw new ArgumentOutOfRangeException(nameof(hashAlgorithmName), $"hashAlgorithmName[{hashAlgorithmName}] could not be instantiated into a valid HashAlgorithm");
8594
InternalMessageFactory = coseSign1MessageFactory;
86-
HashLength = (uint)InternalHashAlgorithm.HashSize / 8;
87-
InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromHashAlgorithm(InternalHashAlgorithm);
95+
InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromName(hashAlgorithmName);
96+
HashLength = GetHashLengthFromName(hashAlgorithmName);
8897
}
8998

90-
private CoseHashAlgorithm GetCoseHashAlgorithmFromHashAlgorithm(HashAlgorithm algorithm)
99+
private static CoseHashAlgorithm GetCoseHashAlgorithmFromName(HashAlgorithmName algorithmName)
91100
{
92-
return algorithm switch
101+
if (algorithmName == HashAlgorithmName.SHA256)
93102
{
94-
SHA256 => CoseHashAlgorithm.SHA256,
95-
SHA384 => CoseHashAlgorithm.SHA384,
96-
SHA512 => CoseHashAlgorithm.SHA512,
97-
_ => throw new ArgumentException($@"No mapping for hash algorithm {algorithm.GetType().FullName} to any {nameof(CoseHashAlgorithm)}")
98-
};
103+
return CoseHashAlgorithm.SHA256;
104+
}
105+
106+
if (algorithmName == HashAlgorithmName.SHA384)
107+
{
108+
return CoseHashAlgorithm.SHA384;
109+
}
110+
111+
if (algorithmName == HashAlgorithmName.SHA512)
112+
{
113+
return CoseHashAlgorithm.SHA512;
114+
}
115+
116+
throw new ArgumentOutOfRangeException(nameof(algorithmName), $"No mapping for hash algorithm {algorithmName} to any {nameof(CoseHashAlgorithm)}");
117+
}
118+
119+
private static uint GetHashLengthFromName(HashAlgorithmName algorithmName)
120+
{
121+
if (algorithmName == HashAlgorithmName.SHA256)
122+
{
123+
return 32;
124+
}
125+
126+
if (algorithmName == HashAlgorithmName.SHA384)
127+
{
128+
return 48;
129+
}
130+
131+
if (algorithmName == HashAlgorithmName.SHA512)
132+
{
133+
return 64;
134+
}
135+
136+
throw new ArgumentOutOfRangeException(nameof(algorithmName), $"Unsupported hash algorithm: {algorithmName}");
99137
}
100138

101139
/// <summary>
@@ -314,21 +352,13 @@ internal static bool HashMatches(CoseHashAlgorithm hashAlgorithm, ReadOnlyMemory
314352
{ 64, HashAlgorithmName.SHA512 }
315353
});
316354

317-
private bool DisposedValue;
318355
/// <summary>
319-
/// Dispose pattern implementation
356+
/// Dispose pattern implementation. No unmanaged resources to release;
357+
/// HashAlgorithm instances are now created and disposed per-operation.
320358
/// </summary>
321359
/// <param name="disposing">True if called from Dispose()</param>
322360
private void Dispose(bool disposing)
323361
{
324-
if (!DisposedValue)
325-
{
326-
if (disposing)
327-
{
328-
HashAlgorithm.Dispose();
329-
}
330-
DisposedValue = true;
331-
}
332362
}
333363

334364
/// <inheritdoc/>

0 commit comments

Comments
 (0)