SignedXml: throw InvalidOperationException on instance reuse

[krwq]

PR is AI generated locally

A SignedXml instance is intended for a single signing or verification operation. Some methods (notably GetPublicKey, called from CheckSignature) advance internal state as they walk the KeyInfo clauses and X509 certificate enumerators, so calling ComputeSignature or CheckSignature more than once on the same instance produces incorrect results.

Add an "already used" flag on SignedXml and throw InvalidOperationException when ComputeSignature or CheckSignature (any overload) is called on an instance that has already been used. The flag is set at method entry, so any call marks the instance used regardless of outcome.

Extracted the CheckSignature bodies into private CheckSignatureCore / CheckSignatureReturningKeyCore helpers so the CheckSignatureReturningKey and CheckSignature(X509Certificate2, bool) internal call chains do not trip the guard on themselves.

Added SignedXml_InstanceReuseTests covering ComputeSignature twice, CheckSignature twice, cross-operation reuse (compute then check, check then compute) and both KeyedHashAlgorithm overloads. Guarded with ConditionalFact(PlatformDetection.IsNotNetFramework) because the guard only ships in the modern .NET target.

Updated three existing verify-with-good-and-bad-key tests (VerifyHMAC_MD5, VerifyHMAC_SHA256/384/512) and CheckSignatureEmptySafe and the SignHMAC helper to create a fresh SignedXml for each verification instead of reusing one.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR enforces a single-operation lifecycle for System.Security.Cryptography.Xml.SignedXml by tracking whether an instance has already performed a sign or verify operation and throwing InvalidOperationException on reuse. It also updates/extends tests to validate the new behavior and to avoid reusing SignedXml instances across multiple verifications.

Changes:

• Add an _alreadyUsed guard in SignedXml and throw InvalidOperationException when reuse is detected across signing/verification APIs (and related mutating APIs after use).
• Add a new resource string (Cryptography_Xml_InstanceAlreadyUsed) for the exception message.
• Add new tests and update existing HMAC verification tests to use fresh SignedXml instances per verification.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography.Xml/tests/System.Security.Cryptography.Xml.Tests.csproj	Includes the new instance-reuse test file in the test project.
src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlTest.cs	Updates tests/helpers to avoid reusing a SignedXml instance across multiple verifications.
src/libraries/System.Security.Cryptography.Xml/tests/SignedXml_InstanceReuseTests.cs	New coverage for reuse scenarios across ComputeSignature/CheckSignature (and related APIs).
src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXml.cs	Implements the _alreadyUsed tracking and throws on attempted reuse.
src/libraries/System.Security.Cryptography.Xml/src/Resources/Strings.resx	Adds the localized exception string for instance reuse.

Suppressed comments (2)

src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXml.cs:355

• CheckSignature(X509Certificate2 certificate, bool verifySignatureOnly) marks the instance as used before validating certificate. If certificate is null, the method will throw later (via certificate.Extensions), but the instance will already be poisoned. Add an explicit ArgumentNullException.ThrowIfNull(certificate) before transitioning to the used state.

        public bool CheckSignature(X509Certificate2 certificate, bool verifySignatureOnly)
        {
            ThrowIfAlreadyUsed();
            _alreadyUsed = true;

src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXml.cs:329

• CheckSignature(KeyedHashAlgorithm macAlg) transitions the instance to the used state before macAlg is validated (null is checked inside CheckSignedInfo(macAlg)). This can poison the instance on ArgumentNullException, preventing a retry with a valid algorithm. Validate macAlg before setting _alreadyUsed = true.

        public bool CheckSignature(KeyedHashAlgorithm macAlg)
        {
            ThrowIfAlreadyUsed();
            _alreadyUsed = true;

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[bartonjs]

This change feels below your normal quality. Review it (and iterate on it) to your own standards of authorship, ideally before opening the PR.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXml.cs:498

• In ComputeSignature(KeyedHashAlgorithm), the instance-reuse guard currently runs after computing signatureMethod. If hash.HashName is unsupported, a reused instance can still throw CryptographicException instead of the intended InvalidOperationException for reuse. Consider moving ThrowIfAlreadyUsed() ahead of the signatureMethod switch so reuse is detected before any hash-name validation work.

            ThrowIfAlreadyUsed();
            _alreadyUsed = true;

            BuildDigestedReferences();
            SignedInfo!.SignatureMethod = signatureMethod;

[Copilot]

🔵 Needs a closer look

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/libraries/System.Security.Cryptography.Xml/tests/SignedXmlInstanceReuseTests.cs:233

• The PR description says the reuse flag is set at method entry so any ComputeSignature/CheckSignature call marks the instance used regardless of outcome. But the implementation (and tests like ComputeSignature_MissingSigningKey_DoesNotPoisonInstance) intentionally allow retry after certain failures (e.g., missing SigningKey / non-HMAC keyed hash) because _alreadyUsed is only set after some validation. Please reconcile this by either updating the PR description to match the implemented semantics, or adjusting the guard/tests to match the stated behavior.

        public void ComputeSignature_MissingSigningKey_DoesNotPoisonInstance()
        {
            using (RSA key = RSA.Create())
            {
                XmlDocument doc = new XmlDocument { PreserveWhitespace = true };
                doc.LoadXml(ExampleXml);

                SignedXml signedXml = new SignedXml(doc);
                Reference reference = new Reference { Uri = "" };
                reference.AddTransform(new XmlDsigEnvelopedSignatureTransform());
                signedXml.AddReference(reference);

                Assert.Throws<CryptographicException>(() => signedXml.ComputeSignature());

                signedXml.SigningKey = key;
                signedXml.ComputeSignature();
                Assert.NotNull(signedXml.SignatureValue);
            }

src/libraries/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/SignedXml.cs:405

• Utils.GetAnyPublicKey(certificate) can return null (e.g., certificate has neither RSA/ECDSA/DSA public key). This method now calls CheckSignatureCore(publicKey!), which bypasses the public overload’s null validation and can throw (or behave unpredictably) instead of cleanly failing verification. Handle the null public-key case explicitly (likely return false) before calling into CheckSignatureCore.

            using (AsymmetricAlgorithm? publicKey = Utils.GetAnyPublicKey(certificate))
            {
                if (!CheckSignatureCore(publicKey!))
                {
                    return false;

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite