From 5f0f636cc6600240f918ba0b670df5bfe0b59e9e Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 7 Apr 2026 12:46:40 -0700 Subject: [PATCH 1/4] 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 microsoft/CoseSignTool#191 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../IndirectSignatureFactoryTests.cs | 77 +++++++++++++++++++ ...directSignatureFactory.CoseHashEnvelope.cs | 10 ++- .../IndirectSignatureFactory.Direct.cs | 10 ++- .../IndirectSignatureFactory.cs | 14 ++++ 4 files changed, 103 insertions(+), 8 deletions(-) diff --git a/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs b/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs index 53b4498b..5e17fce4 100644 --- a/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs +++ b/CoseIndirectSignature.Tests/IndirectSignatureFactoryTests.cs @@ -502,4 +502,81 @@ public async Task TestCreateIndirectSignatureAsyncWithoutPayloadLocation() indirectSignature.TryGetPayloadHashAlgorithm(out CoseHashAlgorithm? algo).Should().BeTrue(); algo!.Should().Be(CoseHashAlgorithm.SHA256); } + + /// + /// Validates that a single instance can safely produce + /// indirect signatures from multiple threads concurrently without throwing + /// + /// ("Concurrent operations from multiple threads on this type are not supported."). + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [Test] + public void ConcurrentHashComputationShouldNotThrow() + { + // Arrange — one shared factory, many threads + ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider(); + using IndirectSignatureFactory factory = new(); + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 20; + + // Act — hammer the factory from many threads at once + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + byte[] payload = new byte[128]; + Random.Shared.NextBytes(payload); + + CoseSign1Message result = factory.CreateIndirectSignature( + payload, + coseSigningKeyProvider, + "application/test.concurrent"); + + // Verify each result is valid + result.IsIndirectSignature().Should().BeTrue(); + result.SignatureMatches(payload).Should().BeTrue(); + }); + }; + + // Assert — no CryptographicException from concurrent ComputeHash + concurrentAction.Should().NotThrow(); + } + + /// + /// Validates that the async indirect signature creation path is also safe under + /// concurrent usage from multiple threads. + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [Test] + public async Task ConcurrentAsyncHashComputationShouldNotThrow() + { + // Arrange — one shared factory, many concurrent tasks + ICoseSigningKeyProvider coseSigningKeyProvider = TestUtils.SetupMockSigningKeyProvider(); + using IndirectSignatureFactory factory = new(); + int concurrentTasks = Environment.ProcessorCount * 4; + + // Act — launch many concurrent async operations + Task[] tasks = new Task[concurrentTasks]; + for (int i = 0; i < concurrentTasks; i++) + { + tasks[i] = Task.Run(async () => + { + byte[] payload = new byte[128]; + Random.Shared.NextBytes(payload); + + CoseSign1Message result = await factory.CreateIndirectSignatureAsync( + payload, + coseSigningKeyProvider, + "application/test.concurrent.async"); + + // Verify each result is valid + result.IsIndirectSignature().Should().BeTrue(); + result.SignatureMatches(payload).Should().BeTrue(); + }); + } + + // Assert — all tasks complete without CryptographicException + Func awaitAll = async () => await Task.WhenAll(tasks); + await awaitAll.Should().NotThrowAsync(); + } } diff --git a/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs b/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs index f3428969..73f75b4a 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.CoseHashEnvelope.cs @@ -39,9 +39,10 @@ private object CreateIndirectSignatureWithChecksInternalCoseHashEnvelopeFormat( if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); } else { @@ -100,9 +101,10 @@ private async Task CreateIndirectSignatureWithChecksInternalCoseHashEnve // Note: HashAlgorithm.ComputeHashAsync is not available in netstandard2.0 // For better async support in the future, consider targeting net6.0+ where // ComputeHashAsync is available on specific hash algorithm implementations + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); } else { diff --git a/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs b/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs index 202756a9..35041101 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.Direct.cs @@ -35,9 +35,10 @@ private object CreateIndirectSignatureWithChecksInternalDirectFormat( string extendedContentType; if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName); } else @@ -118,9 +119,10 @@ private async Task CreateIndirectSignatureWithChecksInternalDirectFormat string extendedContentType; if (!payloadHashed) { + using HashAlgorithm hasher = this.CreateHashAlgorithm(); hash = streamPayload != null - ? InternalHashAlgorithm.ComputeHash(streamPayload) - : InternalHashAlgorithm.ComputeHash(bytePayload!.Value.ToArray()); + ? hasher.ComputeHash(streamPayload) + : hasher.ComputeHash(bytePayload!.Value.ToArray()); extendedContentType = ExtendContentTypeDirect(contentType, HashAlgorithmName); } else diff --git a/CoseIndirectSignature/IndirectSignatureFactory.cs b/CoseIndirectSignature/IndirectSignatureFactory.cs index 220a390e..5c090f0b 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.cs @@ -45,6 +45,10 @@ public enum IndirectSignatureVersion /// /// The HashAlgorithm this factory is using. /// + /// + /// WARNING: This instance is NOT thread-safe for ComputeHash operations. + /// Use to create a thread-safe instance for hashing. + /// public HashAlgorithm HashAlgorithm => InternalHashAlgorithm; /// @@ -52,6 +56,16 @@ public enum IndirectSignatureVersion /// public HashAlgorithmName HashAlgorithmName => InternalHashAlgorithmName; + /// + /// Creates a new instance that is safe for use on the calling thread. + /// + /// A new instance. Callers are responsible for disposing this instance. + private HashAlgorithm CreateHashAlgorithm() + { + return CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(InternalHashAlgorithmName) + ?? throw new InvalidOperationException($"Failed to create HashAlgorithm for {InternalHashAlgorithmName}"); + } + /// /// The CoseSign1 Message Factory this factory is using. /// From b5bd4b186ca472a5de0e620f030c4d6a3bc62901 Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 7 Apr 2026 13:41:31 -0700 Subject: [PATCH 2/4] 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 microsoft/CoseSignTool#191 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CoseHandler.Tests/CoseX509ThumbprintTests.cs | 63 ++++++++++++++++++++ CoseSign1.Certificates/CoseX509Thumbprint.cs | 44 ++++++++------ 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/CoseHandler.Tests/CoseX509ThumbprintTests.cs b/CoseHandler.Tests/CoseX509ThumbprintTests.cs index a466e231..76d51113 100644 --- a/CoseHandler.Tests/CoseX509ThumbprintTests.cs +++ b/CoseHandler.Tests/CoseX509ThumbprintTests.cs @@ -50,4 +50,67 @@ public void ConstructThumbprintWithUnsupportedAlgo() { _ = new CoseX509Thumprint(SelfSignedCert1, HashAlgorithmName.SHA3_512); } + + /// + /// Validates that a single instance can safely call + /// from multiple threads concurrently without + /// throwing . + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [TestMethod] + public void ConcurrentMatchShouldNotThrow() + { + // Arrange — one shared thumbprint, many threads calling Match + CoseX509Thumprint thumbprint = new(SelfSignedCert1); + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 50; + + // Act & Assert — hammer Match() from many threads at once + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + // Alternate between matching and non-matching certs + if (i % 2 == 0) + { + thumbprint.Match(SelfSignedCert1).Should().BeTrue(); + } + else + { + thumbprint.Match(SelfSignedCert2).Should().BeFalse(); + } + }); + }; + + concurrentAction.Should().NotThrow(); + } + + /// + /// Validates that concurrent construction of instances + /// with different hash algorithms is thread-safe. + /// Regression test for https://github.com/microsoft/CoseSignTool/issues/191. + /// + [TestMethod] + public void ConcurrentConstructionAndMatchShouldNotThrow() + { + // Arrange + HashAlgorithmName[] algorithms = new[] { HashAlgorithmName.SHA256, HashAlgorithmName.SHA384, HashAlgorithmName.SHA512 }; + int degreeOfParallelism = Environment.ProcessorCount * 2; + int iterationsPerThread = 30; + + // Act & Assert — create thumbprints and match from many threads + Action concurrentAction = () => + { + Parallel.For(0, degreeOfParallelism * iterationsPerThread, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism }, i => + { + HashAlgorithmName algo = algorithms[i % algorithms.Length]; + CoseX509Thumprint thumbprint = new(SelfSignedCert1, algo); + + thumbprint.Match(SelfSignedCert1).Should().BeTrue(); + thumbprint.Match(SelfSignedCert2).Should().BeFalse(); + }); + }; + + concurrentAction.Should().NotThrow(); + } } \ No newline at end of file diff --git a/CoseSign1.Certificates/CoseX509Thumbprint.cs b/CoseSign1.Certificates/CoseX509Thumbprint.cs index 96a3c69b..84567f70 100644 --- a/CoseSign1.Certificates/CoseX509Thumbprint.cs +++ b/CoseSign1.Certificates/CoseX509Thumbprint.cs @@ -23,11 +23,6 @@ public class CoseX509Thumprint { -44, HashAlgorithmName.SHA512 } }; - /// - /// Hash algorithm instance used to compute thumbprints - /// - private HashAlgorithm? Hasher { get; set; } - /// /// Gets the HashId used in the CBOR/COSE representation of the x5t header /// @@ -48,8 +43,9 @@ private CoseX509Thumprint() { } /// The certificate to create a thumbprint for. public CoseX509Thumprint(X509Certificate2 cert) { - BuildHasher(GetHashID(HashAlgorithmName.SHA256.Name ?? "SHA256")); - Thumbprint = Hasher?.ComputeHash(cert.RawData); + SetHashId(GetHashID(HashAlgorithmName.SHA256.Name ?? "SHA256")); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + Thumbprint = hasher.ComputeHash(cert.RawData); } /// @@ -58,9 +54,10 @@ public CoseX509Thumprint(X509Certificate2 cert) /// The certificate to create a thumbprint for. public CoseX509Thumprint(X509Certificate2 cert, HashAlgorithmName hashAlgorithm) { - BuildHasher(GetHashID(hashAlgorithm.Name + SetHashId(GetHashID(hashAlgorithm.Name ?? throw new CryptographicException(nameof(hashAlgorithm), "The supplied hash algorithm name was not recognized."))); - Thumbprint = Hasher?.ComputeHash(cert.RawData); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + Thumbprint = hasher.ComputeHash(cert.RawData); } #region Public Methods @@ -75,8 +72,8 @@ public CoseX509Thumprint(X509Certificate2 cert, HashAlgorithmName hashAlgorithm) /// public bool Match(X509Certificate2 certificate) { - return Thumbprint.ToArray().SequenceEqual(Hasher?.ComputeHash(certificate.RawData) - ?? throw new InvalidOperationException($"The current {nameof(CoseX509Thumprint)} object is not yet initialized.")); + using HashAlgorithm hasher = this.CreateHashAlgorithm(); + return Thumbprint.ToArray().SequenceEqual(hasher.ComputeHash(certificate.RawData)); } /// @@ -108,7 +105,7 @@ public static CoseX509Thumprint Deserialize(CborReader reader) } int hashId = reader.ReadInt32(); - result.BuildHasher(hashId); + result.SetHashId(hashId); if (reader.PeekState() != CborReaderState.ByteString) { @@ -149,22 +146,35 @@ private static int GetHashID(string algorithmName) return data.Key; } - // Sets HashID and returns the value for Hasher. - private void BuildHasher(int coseHashAlgorithmId) + // Sets HashID and validates the algorithm is supported. + private void SetHashId(int coseHashAlgorithmId) { - if (!HashAlgorithmToCoseValues.TryGetValue(coseHashAlgorithmId, out HashAlgorithmName algName)) + if (!HashAlgorithmToCoseValues.TryGetValue(coseHashAlgorithmId, out _)) { throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {coseHashAlgorithmId}"); } HashId = coseHashAlgorithmId; + } + + /// + /// Creates a new instance based on the stored . + /// Each call returns a fresh instance that is safe for use on the calling thread. + /// Callers are responsible for disposing the returned instance. + /// + private HashAlgorithm CreateHashAlgorithm() + { + if (!HashAlgorithmToCoseValues.TryGetValue(HashId, out HashAlgorithmName algName)) + { + throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {HashId}"); + } // HashAlgorithmName values are not constants, so we can't use an actual switch here. - Hasher = + return algName == HashAlgorithmName.SHA256 ? SHA256.Create() : algName == HashAlgorithmName.SHA384 ? SHA384.Create() : algName == HashAlgorithmName.SHA512 ? SHA512.Create() : - throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {coseHashAlgorithmId}"); + throw new CoseX509FormatException($"Unsupported thumbprint hash algorithm value of {HashId}"); } #endregion From eb7aced5565725aeb25f12d4b190b96c5f1e509c Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 7 Apr 2026 13:47:40 -0700 Subject: [PATCH 3/4] Address review: remove cached InternalHashAlgorithm, return fresh instance from property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../IndirectSignatureFactory.cs | 72 +++++++++++-------- 1 file changed, 44 insertions(+), 28 deletions(-) diff --git a/CoseIndirectSignature/IndirectSignatureFactory.cs b/CoseIndirectSignature/IndirectSignatureFactory.cs index 5c090f0b..974fce8d 100644 --- a/CoseIndirectSignature/IndirectSignatureFactory.cs +++ b/CoseIndirectSignature/IndirectSignatureFactory.cs @@ -36,23 +36,19 @@ public enum IndirectSignatureVersion CoseHashEnvelope } - private readonly HashAlgorithm InternalHashAlgorithm; private readonly uint HashLength; private readonly CoseHashAlgorithm InternalCoseHashAlgorithm; private readonly HashAlgorithmName InternalHashAlgorithmName; private readonly ICoseSign1MessageFactory InternalMessageFactory; /// - /// The HashAlgorithm this factory is using. + /// Creates a new instance matching the algorithm this factory is configured with. + /// Each access returns a new instance. Callers are responsible for disposing the returned instance. /// - /// - /// WARNING: This instance is NOT thread-safe for ComputeHash operations. - /// Use to create a thread-safe instance for hashing. - /// - public HashAlgorithm HashAlgorithm => InternalHashAlgorithm; + public HashAlgorithm HashAlgorithm => CreateHashAlgorithm(); /// - /// The HashAlgorightmName this factory is using. + /// The HashAlgorithmName this factory is using. /// public HashAlgorithmName HashAlgorithmName => InternalHashAlgorithmName; @@ -95,21 +91,49 @@ public IndirectSignatureFactory() : this(HashAlgorithmName.SHA256) public IndirectSignatureFactory(HashAlgorithmName hashAlgorithmName, ICoseSign1MessageFactory coseSign1MessageFactory) { InternalHashAlgorithmName = hashAlgorithmName; - InternalHashAlgorithm = CoseSign1MessageIndirectSignatureExtensions.CreateHashAlgorithmFromName(hashAlgorithmName) ?? throw new ArgumentOutOfRangeException(nameof(hashAlgorithmName), $"hashAlgorithmName[{hashAlgorithmName}] could not be instantiated into a valid HashAlgorithm"); InternalMessageFactory = coseSign1MessageFactory; - HashLength = (uint)InternalHashAlgorithm.HashSize / 8; - InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromHashAlgorithm(InternalHashAlgorithm); + InternalCoseHashAlgorithm = GetCoseHashAlgorithmFromName(hashAlgorithmName); + HashLength = GetHashLengthFromName(hashAlgorithmName); } - private CoseHashAlgorithm GetCoseHashAlgorithmFromHashAlgorithm(HashAlgorithm algorithm) + private static CoseHashAlgorithm GetCoseHashAlgorithmFromName(HashAlgorithmName algorithmName) { - return algorithm switch + if (algorithmName == HashAlgorithmName.SHA256) { - SHA256 => CoseHashAlgorithm.SHA256, - SHA384 => CoseHashAlgorithm.SHA384, - SHA512 => CoseHashAlgorithm.SHA512, - _ => throw new ArgumentException($@"No mapping for hash algorithm {algorithm.GetType().FullName} to any {nameof(CoseHashAlgorithm)}") - }; + return CoseHashAlgorithm.SHA256; + } + + if (algorithmName == HashAlgorithmName.SHA384) + { + return CoseHashAlgorithm.SHA384; + } + + if (algorithmName == HashAlgorithmName.SHA512) + { + return CoseHashAlgorithm.SHA512; + } + + throw new ArgumentOutOfRangeException(nameof(algorithmName), $"No mapping for hash algorithm {algorithmName} to any {nameof(CoseHashAlgorithm)}"); + } + + private static uint GetHashLengthFromName(HashAlgorithmName algorithmName) + { + if (algorithmName == HashAlgorithmName.SHA256) + { + return 32; + } + + if (algorithmName == HashAlgorithmName.SHA384) + { + return 48; + } + + if (algorithmName == HashAlgorithmName.SHA512) + { + return 64; + } + + throw new ArgumentOutOfRangeException(nameof(algorithmName), $"Unsupported hash algorithm: {algorithmName}"); } /// @@ -328,21 +352,13 @@ internal static bool HashMatches(CoseHashAlgorithm hashAlgorithm, ReadOnlyMemory { 64, HashAlgorithmName.SHA512 } }); - private bool DisposedValue; /// - /// Dispose pattern implementation + /// Dispose pattern implementation. No unmanaged resources to release; + /// HashAlgorithm instances are now created and disposed per-operation. /// /// True if called from Dispose() private void Dispose(bool disposing) { - if (!DisposedValue) - { - if (disposing) - { - HashAlgorithm.Dispose(); - } - DisposedValue = true; - } } /// From 31721d10f6272c1e34c640fc3a53a80be134291f Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 7 Apr 2026 13:49:51 -0700 Subject: [PATCH 4/4] 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> --- CoseIndirectSignature/CoseHashV.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CoseIndirectSignature/CoseHashV.cs b/CoseIndirectSignature/CoseHashV.cs index 64912c35..30b137df 100644 --- a/CoseIndirectSignature/CoseHashV.cs +++ b/CoseIndirectSignature/CoseHashV.cs @@ -39,7 +39,7 @@ public byte[] HashValue } // sanity check the length of the hash against the specified algorithm to be sure we're not allowing a mismatch. - HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm); + using HashAlgorithm algo = IndirectSignatureFactory.GetHashAlgorithmFromCoseHashAlgorithm(Algorithm); if (value.Length != (algo.HashSize / 8)) { throw new ArgumentOutOfRangeException(nameof(value), @$"The hash value length of {value.Length} did not match the CoseHashAlgorithm {Algorithm} required length of {algo.HashSize / 8}");