SignedXml rejects signature that is accepted by online validators

[madelson]

Description

We are trying to use the SignedXml class to validate the XML signatures on SAML tokens. While this has worked well for us for many years, recently we've received some SAML tokens where SignedXml rejects the signature as invalid. When those same responses are run through online validators such as https://www.samltool.com/validate_response.php and https://www.rtr.at/TKP/was_wir_tun/vertrauensdienste/Signatur/signaturpruefung/Pruefung.en.html, the signature is accepted.

Here is the snippet of code showing the core signature validation logic which reproduces the issue for us (see SignedXmlIssueRepro.zip for working repro solution including a sample SAML response created using https://www.samltool.com/sign_response.php and signed with a fake cert):

			var cert = new X509Certificate2(
				...,
				password: string.Empty,
				keyStorageFlags: X509KeyStorageFlags.MachineKeySet | X509KeyStorageFlags.Exportable
			);
			var saml = File.ReadAllText(...);

			// signature validation based on https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.xml.signedxml?view=dotnet-plat-ext-5.0

			var samlDocument = new XmlDocument { PreserveWhitespace = true };
			samlDocument.LoadXml(saml);
			
			var signatureNodes = samlDocument.DocumentElement.GetElementsByTagName("ds:Signature");
			foreach (XmlNode signatureNode in signatureNodes)
			{
				Console.WriteLine($"Found signature on {signatureNode.ParentNode.Name}");
				
				var signedXml = new SignedXml(samlDocument);
				signedXml.LoadXml((XmlElement)signatureNode);
				Console.WriteLine($"Signature valid? {signedXml.CheckSignature(cert, verifySignatureOnly: true)}");
			}

When this code runs, it tries to verify 2 signatures in the document: a top level signature on the response and an assertion-level signature on the SAML token embedded within the response document. The top-level signature is accepted but the nested assertion-level signature is rejected.

Configuration

• .NET Version: .NET 5 (VS 16.8.5), also reproduced on .NET Framework 4.8
• OS: Windows 10 x64

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

We are trying to use the SignedXml class to validate the XML signatures on SAML tokens. While this has worked well for us for many years, recently we've received some SAML tokens where SignedXml rejects the signature as invalid. When those same responses are run through online validators such as https://www.samltool.com/validate_response.php and https://www.rtr.at/TKP/was_wir_tun/vertrauensdienste/Signatur/signaturpruefung/Pruefung.en.html, the signature is accepted.

Here is the snippet of code showing the core signature validation logic which reproduces the issue for us (see attached for working repro solution including a sample SAML response created using https://www.samltool.com/sign_response.php and signed with a fake cert)
SignedXmlIssueRepro.zip
:

			var cert = new X509Certificate2(
				...,
				password: string.Empty,
				keyStorageFlags: X509KeyStorageFlags.MachineKeySet | X509KeyStorageFlags.Exportable
			);
			var saml = File.ReadAllText(...);

			// signature validation based on https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.xml.signedxml?view=dotnet-plat-ext-5.0

			var samlDocument = new XmlDocument { PreserveWhitespace = true };
			samlDocument.LoadXml(saml);
			
			var signatureNodes = samlDocument.DocumentElement.GetElementsByTagName("ds:Signature");
			foreach (XmlNode signatureNode in signatureNodes)
			{
				Console.WriteLine($"Found signature on {signatureNode.ParentNode.Name}");
				
				var signedXml = new SignedXml(samlDocument);
				signedXml.LoadXml((XmlElement)signatureNode);
				Console.WriteLine($"Signature valid? {signedXml.CheckSignature(cert, verifySignatureOnly: true)}");
			}

When this code runs, it tries to verify 2 signatures in the document: a top level signature on the response and an assertion-level signature on the SAML token embedded within the response document. The top-level signature is accepted but the nested assertion-level signature is rejected.

Configuration

• .NET Version: .NET 5 (VS 16.8.5), also reproduced on .NET Framework 4.8
• OS: Windows 10 x64

Author:	madelson
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[vcsjones]

This seems to have the same underlying cause as #27500.

[madelson]

@vcsjones that issue has been open since 2018; any idea when it will be fixed?

[vcsjones]

I'd have to defer to @bartonjs for that.

[madelson]

@vcsjones I was able to amend my repro to verify that validating on a child document does appear to fix the issue:

				var childDocument = new XmlDocument { PreserveWhitespace = true };
				childDocument.LoadXml(signatureNode.ParentNode.OuterXml);
				signedXml = new SignedXml(childDocument);
				signedXml.LoadXml(childDocument.DocumentElement.ChildNodes.OfType<XmlElement>().Single(n => n.Name == "ds:Signature"));
				Console.WriteLine($"(child doc) Signature valid? {signedXml.CheckSignature(cert, verifySignatureOnly: true)}");

[bartonjs]

any idea when it will be fixed?

Essentially, the only bugs we actively fix in SignedXml are where .NET Framework gives one (desired) answer and .NET (nee Core) gives another (less desired).

If someone can come up with a small/scoped fix for this behavior we'd consider it. If it requires a big change we won't. Apparently the other issue didn't have an up for grabs label (though it had essentially these same words), so I've gone ahead and applied it.

[madelson]

@bartonjs is that because SignedXml is considered deprecated? Does .NET provide an alternative fully-supported API for XML signature creation/validation?

[bartonjs]

There may be other SignedXml-like classes for .NET, but we only have the one.

In my personal opinion, the API was trying to be too friendly, and that made it really easy to use wrong. There are a lot of notes that we put in the documentation starting at https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.xml.signedxml?view=dotnet-plat-ext-5.0#choosing-a-canonicalization-method.

Still in my personal opinion, a lot of the API errors came from spec errors. To my recollection, the initial version of the spec doesn't talk about key acceptability, which was somewhere between naive (believing that the people implementing the spec would consider it obvious) and negligent (because it's a security spec and they didn't think of it). Combined with all of the other "excuse me?!"s and gotchas in the spec, I believe that the spec/concept should be considered deprecated. (I haven't read the v2 version, maybe it patched things up a bit, but if it's still compatible with v1 then it'll be forced to leave in a lot of the problems.)

These, and other, concerns are why we left SignedXml and EncryptedXml out of .NET Core 1.0. While it came back in .NET Core 2.0 (along with a lot of other things that had been "happily removed"), we've been pretty consistent in our position that it only exists to help applications transition from .NET Framework to .NET (nee Core). No feature work, small bugfixes are OK (though they'll never get ported back to .NET Framework, which makes the "here for compatibility" types have a hard time fulfilling their mission), no major refactoring.

If you have to use SignedXml/xmldsig because you're interacting with a system that requires it, then there's not much you can do about it. If it's for your own application, my recommendation is to use SignedCms. (That API isn't perfect, but the spec is much simpler: "I have some data. It's signed. Here's the signature". No canonicalization, no transforms, no "go fetch this file off the Internet, then you'll see it's trustworthy (wink, wink)", no "yeah, there's a signature, but it doesn't cover any of the part of the document/data you hoped was signed", and the API is better at expressing "this is the/a signer, here're their credentials" for caller verification)

[madelson]

@bartonjs thanks for the context. Unfortunately we're forced into dealing with this because of Saml2. For anyone who comes across this in the future, here's the workaround I ended up using to get the signature to validate:

XElement nestedSignedElement = // input
// we need to keep these when extracting out a sub-document to avoid namespace propagation
// changing the signed element and breaking the signature
var outerNamespaceDeclarations = Traverse.Along(xml.XElement.Parent, e => e.Parent)
    .SelectMany(e => e.Attributes())
    .Where(a => a.IsNamespaceDeclaration)
    .DistinctBy(a => a.Name);
var wrappedSubDocumentXml = new XElement(
    "NamespaceWrapper",
    outerNamespaceDeclarations,
    xml.XElement)
    .ToString(SaveOptions.DisableFormatting); // need to preserve whitespace for signature
var wrappedSubDocument = new XmlDocument { PreserveWhitespace = true };
wrappedSubDocument.LoadXml(wrappedSubDocumentXml);
// apply SignedXml to the signature in wrappedSubDocument