XML signature still fails to verify

[componentspace]

Description

An XML signature in a SAML response fails to verify using System.Security.Cryptography.Xml.SignedXml.

I believe the XML signature should verify as it does when using a Java application.

The attached ZIP includes:

samlresponse-fails.xml - signed XML that fails to verify
samlresponse-verifies.xml - signed XML that does verify, for comparison

Program.cs - .NET console application demonstrating the problem
VerifySignature.java - Java application that can successfully verify the signatures for both files

The samlresponse-fails.xml includes a SAML response that's signed and a SAML assertion that's also signed. It's the SAML response signature that fails to verify in the .NET console application but does verify in the Java application.

The samlresponse-verifies.xml includes a SAML response that's signed and this verifies in both the .NET console application and the Java application.

The .NET console application was built using .NET 8.0 and System.Security.Cryptography.Xml v8.0.0.

This is exactly the same issue reported in #31117 back in 2019. That issue was incorrectly identified as a duplicate of a different issue.

I believe the issue is related to the use of carriage return entity references in the XML.

For example:





Open the XML files included in the attached ZIP using Notepad etc to see the entity references.

xml-signature-issue.zip

Reproduction Steps

Build and run the Program.cs in the ZIP using .NET 8 and System.Security.Cryptography.Xml v8.0.0.

Expected behavior

The SAML response XML signature in samlresponse-fails.xml should verify.

Actual behavior

The SAML response XML signature in samlresponse-fails.xml fails to verify. The same signature verifies using Java.

Regression?

No

Known Workarounds

None

Configuration

.NET SDK:
Version: 8.0.202
Commit: 25674bb2f4
Workload version: 8.0.200-manifests.8cf8de6d

Runtime Environment:
OS Name: Windows
OS Version: 10.0.22631
OS Platform: Windows
RID: win-x64
Base Path: C:\Program Files\dotnet\sdk\8.0.202\

Other information

I suspect the canonicalization of XML that includes carriage return entity references isn't handled properly.

[dotnet-policy-service]

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

[artur-reaboi]

It is important to note that standard eIDAS Node implementation (which is in Java), inserts those encoded characters in base64 encoded digests and certificates. This results in us being unable to validate SAML messages signed by widely used eIDAS Nodes.

[mpbraithwaite]

Would love to see this addressed. We have had to support customers that have had \r\n characters in their ADFS field values and using SAML authentication.

[AlexCozlovschi]

This would be very helpful, we have are receiving messages from different users, and they are all have their own implementation on Java, PHP, Python, we are using C# .Net Core for confirmations and management, but this problem makes our work much harder, my colleagues are asking to switch our stack to JAVA, because it has better compatibility. I still insist on keeping C#, and this modification could be the main argument.

[peterilis]

We would love a fix here as well, it is affecting customers setting up SAML integrations against us.

[VikingRedleg]

I also would appreciate a fix to this issue.

[neilpendlington]

Massively frustrating issue that caused an expensive amount of investigation in our organisation. For us, the problem surfaced whilst using .NET framework 4.8 proving that the issue is long-standing in the .NET ecosystem.
Or potentially if the issue lies in the Windows Cryptography subsystem, it's even more important there's a fix made available.

[bartonjs]

I don't have the sources in front of me to quote, but the problem here is really one of layering.

• When XmlReader reads the XML, it turns 
 or 
 into a literal CR
• Somewhere else along the way, CRLF gets normalized into LF
• By the time it makes it to SignedXml's canonicalization, the CR is gone.

At every step of the way something reasonable is happening, but in the end it differs from Java. The best answer, of course, is "don't use carriage returns", but that's on the document creator, not the document verifier.

I also have to say at this point that my personal opinion is that xmldsig is the worst possible choice for any kind of document signing. It is a fundamentally insecure specification.

That said, there are a few notes I can give to help someone make progress.

1. GetIdElement

In the demo program, GetIdElement is overridden to return a fixed value. Since SAML uses Capital-I, Capital-D ID, a SAML version would be

public override XmlElement GetIdElement(XmlDocument document, string idValue)
{
    if (document == null)
    {
	return null;
    }

    try
    {
	XmlConvert.VerifyNCName(idValue);
    }
    catch (XmlException)
    {
	return null;
    }

    return (XmlElement)document.SelectSingleNode($"//*[@ID=\"{idValue}\"]");
}

Or if you want to just be flexible, call base and append the search for an ID version

public override XmlElement GetIdElement(XmlDocument document, string idValue)
{
    if (document == null)
    {
	return null;
    }

    try
    {
	XmlConvert.VerifyNCName(idValue);
    }
    catch (XmlException)
    {
	return null;
    }

    return
        base.GetIdElement(document, idValue) ??
        (XmlElement)document.SelectSingleNode($"//*[@ID=\"{idValue}\"]");
}

2. It's maybe possible to restore the CR

I have some old notes on this problem, which looked promising... but they don't work. (Worse, they invalidate the signature on the one that passes).

    internal class XmlCRElement : XmlElement
    {
        protected internal XmlCRElement(string prefix, string localName, string namespaceURI, XmlDocument doc)
            : base(prefix, localName, namespaceURI, doc)
        {
        }

        public override string OuterXml => base.OuterXml.Replace("\r", "
");
    }

    internal class CRSignedXml : SignedXml
    {
        public CRSignedXml(XmlDocument doc) : base(doc)
        {
        }

        public override XmlElement GetIdElement(XmlDocument document, string idValue)
        {
            XmlElement elem = base.GetIdElement(document, idValue);

            if (elem != null && !(elem is XmlCRElement))
            {
                XmlElement crElem = new XmlCRElement(
                    elem.Prefix,
                    elem.LocalName,
                    elem.NamespaceURI,
                    elem.OwnerDocument);

                crElem.InnerXml = elem.InnerXml;

                for (int i = 0; i < elem.Attributes.Count; i++)
                {
                    crElem.Attributes.Append(elem.Attributes[i]);
                }

                elem.ParentNode?.ReplaceChild(crElem, elem);

                return crElem;
            }

            return elem;
        }
    }

That is attempting to de-normalize CR back to 
 so that it gets processed during canonicalization. It isn't working, though (my guess is that something is wrong with attribute copying, since what I had in my notes was a foreach that throws an exception on this particular input).

3. We will probably never fix this bug.

To quote from the README.md on this library:

We only consider fixes that unblock critical issues

There are circumstances where this library and other implementations of xmldsig will produce incompatible answers (signatures produced by one cannot be verified by another). These issues generally cannot be fixed, as they would invalidate signatures produced by previous versions of the library in those same circumstances.

Because of these incompatibilities, and design concerns with the specification itself, we do not recommend the use of this library by any application unless it needs to interoperate with existing files or services that are already based on these formats.

There are newer editions/versions of both the xmlenc and xmldsig specifications, but this implementation will not be updated to incorporate their changes.

Given our stance on the library, I can't imagine anyone from the .NET product taking the time to develop a compatible solution, write all the tests for it, and drive it to completion.

If anyone can get my notes into a functional state, great! Please share the answer (here, StackOverflow, both, wherever) so others can benefit.

4. If someone develops the fix, and it is "surgical", we'll probably accept it.

The key on (3) is "we" won't fix it. Someone could. If the fix requires rewriting all of System.Xml or our C14N implementation(s) then we won't take it. If there's something small that can be done and an option to enable it (or some other gesture, like using a derived type (as in my notes)), and we have tests proving that people not opting in to the new CR handling get the same answers as before, then we could accept the fix.

5. .NET Framework will never fix this bug.

If the fix requires putting a new member on SignedXml (e.g. bool PreserveCarriageReturn { get; set; }), .NET Framework will never get it. This kind of problem, while frustrating, is outside of the level of changes that we still make to .NET Framework.

If the fix involves a new type that extends SignedXml and doesn't touch any of the SignedXml implementation (like my notes are trying to accomplish), then it would be possible to expose that via the NuGet package we build out of this repository and make it available for .NET Framework. That's fixing it "for" .NET Framework, but neither "in" nor "by" .NET Framework. The distinction may be subtle, but is logistically important.

[componentspace]

It's not just that it differs from Java. I've confirmed the Java implementation is correct. Therefore, this looks to me like a bug in the .NET implementation.

You may consider XML signature the worst possible choice but this is a requirement of the SAML specification which is used by a very large number of organizations.

The goal of the GetIdElement method in the demo program was brevity. It's not intended to be production level code.

If my understanding is correct, this is a bug that isn't considered important enough for Microsoft to fix.

Is that a fair assessment?