SignedXml fails to validate second signature in same document

[AndersAbel]

The EnvelopedSignature transform should filter out the signature that contains the current Transform element. This is handled by Reference.LoadXml in that it injects a number into the XmlDsigEnvelopedSignatureTransform.SignaturePosition property indicating that the nth signature node should be ignored.

First step is to get a list of signatures: https://github.com/dotnet/corefx/blob/926a859dc49beb9dcd3b590deae609b7a87e70c1/src/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/Reference.cs#L277

Then the current signature's position in that list is calculated.

The problem is that //ds:Signature will find all signatures in the contained document, even the signature signs a child node. Consider the following example:

<xml>
  <a ID=\"a\">
    <ds:Signature>...<Reference URI="#a"/>...</ds:Signature>
  </a>
  <b ID=\"b\">
    <c>
      <ds:Signature>...<Reference URI="#b"/>...</ds:Signature>
    </c>
  </b>
</xml>

When the second signature is verified, the signatureList will contain both signatures. But when evaluating the reference, the traversal starts at the b element, so in the token stream only one signature is present.

This bug is present both in the .NET Core framework and the full .NET Framework. I reported it on the connect site, long ago, then suggesting a fix of using .//ds:Signature as the xpath expression instead. It would solve the problem if the second signature was directly beneath b (as is the case with nested signatures in SAML2P), but not in this extended case.

The correct fix would be to not use a global XPath search, but resolve the reference and then create the signatureList based on the nodes beneath the referenced node.

A runnable proof-of-concept is available at https://github.com/AndersAbel/DualXmlDsigBug. It produces the following output when run:

Xml: <xml><a ID="a"/><b ID="b"><c/></b></xml>
Signing b...
Check signature B: True
Signing a...
Check signature A: True
Check signature B: False
Check signature B, in own document: True

The final "in own document" check is the same signature validation run on b.OuterXml, to show that the signature is indeed valid when validated in its own context.

When/if fixing this, please consider how it affects code that targets .Net Standard. Best would of course be if the fix could be back-ported to full .NET Framework as well.

[AndersAbel]

Thinking more about this, I don't think a full reference resolution is required. It would be enough to just traverse from the Signature node and upwards until a parent with an ID matching the reference is found. Then count the number of references signatures below that node. This would work, because the enveloped signature transform is only relevant if the signature node is a child of the node being signed.

@karelz If you think that is a viable solution, I can submit a PR. Including incorporating my test program as unit tests. Should I go ahead with that? Would love to have this bug fixed for the next release if possible.

[karelz]

Area owners should comment - @bartonjs @GrabYourPitchforks cc @krwq

[bartonjs]

It would be enough to just traverse from the Signature node and upwards until a parent with an ID matching the reference is found.

Just because the enveloped transform was applied doesn't mean it was actually enveloped.

<xml>
  <a ID="a">
  </a>
  <b ID="b">
    <c>
    </c>
  </b>
  <ds:Signature>...<Reference URI="#a"/>...</ds:Signature>
  <ds:Signature>...<Reference URI="#b"/>...</ds:Signature>
</xml>

is also a legal representation, even though the enveloped transform was not required.

---

If I had to take a guess, I'd say that the problem is the position tagger uses the indices from //ds:Signature, but the position evaluator uses ancestor-or-self::dsig:Signature[1] (from... some context).

If the fix is as simple as making the "is this me" check use the same logic as the "this is you", we'd likely take it. If it's more complex, I don't know that we would (at least, not without understanding what standard-on-top-of-xmldsig is impacted by this being broken... we discourage use of signing anything other than the whole document (Reference URI=""), so multiple signatures isn't something we expect a lot).

[AndersAbel]

Just because the enveloped transform was applied doesn't mean it was actually enveloped.

Nope. And that is already covered later in the algorithm. What it does is:

1. Find all signature nodes in the signed document (should be signed node, that is what the bug is about)
2. Loop through those signature nodes and check if any of them is the same as the current reference's parent signature node.
   a. In that case, set XmlDsigEnvelopedSignatureTransform.SignaturePosition to the index (1-based index!) of the match.
3. When applying the transform, remove the signature at SignaturePosition, if SignaturePosition != 0

The check in step 3 is at https://github.com/dotnet/corefx/blob/926a859dc49beb9dcd3b590deae609b7a87e70c1/src/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/XmlDsigEnvelopedSignatureTransform.cs#L132

So an enveloped transform where the signature wasn't really enveloped will become a no-op.

The ancestor-or-self::dsig:Signature[1] is perfectly fine in finding the <ds:Signature> element containing the current ds:Reference being evaluated. The problem is that the index is wrongly calculated as the nth signature in the document instead of the nth signature in the referenced node.

Discouraging signing of anything except the whole document is good advice. Unfortunately the SAML2 standard doesn't follow that advice. It requires the Reference URI to be explicitly set to the ID of the node being signed. Which is often not the whole document. And there are cases with nested signatures, such as a signed assertion inside a signed SamlResponse.

[HPurvis]

I've also encountered this same issue, again related to signed SAML2 messages. The test cases are slightly different however, so I felt it was worth mentioning as they may make for a useful pair of unit tests.

The following example document should validate both signatures correctly with the current code:

<xml>
  <a ID="a">
    <b ID="b">
        <ds:Signature>...<Reference URI="#b"/>...</ds:Signature>
    </b>
    <ds:Signature>...<Reference URI="#a"/>...</ds:Signature>
  </a>
</xml>

However, it is sensitive to the position of signature A. The following is equally valid as signed XML, but will currently fail to validate signature B:

<xml>
  <a ID="a">
    <ds:Signature>...<Reference URI="#a"/>...</ds:Signature>
    <b ID="b">
        <ds:Signature>...<Reference URI="#b"/>...</ds:Signature>
    </b>
  </a>
</xml>

In the latter case, the SignaturePosition for B is detected as 2, which is correct in the scope of the whole document, but incorrect in the scope of the signed element when the transform comes to actually remove its signature. The first case only works because the SignaturePosition is 1 for the inner signature B, avoiding the scoping issue in question.

This was highlighted as our SAML2 test data has the enveloped signatures appended as the last child of the signed element which works fine, however Active Directory Federation Services generates SAML2 messages with the signature appended as the second child of the signed element, which currently fails to validate as (due to there being more elements involved) signature A ends up before element B in the document. The behaviour of ADFS is correct here, as the SAML2 XSD schema technically specifies that the signature must appear in a specific position among the children of the signed element.

I appreciate the suggestion that nested signatures should be avoided, however when it comes to consuming signatures from another application in a schema that we do not control, there's no choice but to accept them as they're valid in the XML DSIG specification and also in the SAML2 specification.

[jhudsoncedaron]

Got to the same bug report by the same pathway. Java implementations of SAML generate XML signatures that the .NET XML library can't verify.

[Naren3891]

@ALL @jhudsoncedaron , I too facing similar issue, on java front using OpenSAML Java library to Generate XML signature and verifying the signature in .NET and it always fails to validate. The same signature was able to verify with java sample.

Did you able to Root cause the issue.

[jhudsoncedaron]

@Naren3891 : Root cause identified right down to the broken line of code. Tried to write patch; didn't have enough knowledge (nor can I make good test cases). Asked stackoverflow question: https://stackoverflow.com/questions/71261077/what-exactly-does-this-xpath-expression-do-count-and-here

[AndersAbel]

@Naren3891 for the Saml2 case you can use an altered XPath expression, that's what I did in https://github.com/Sustainsys/Saml2/blob/c466d3bbf12479944e350b649f5b5c825616007b/Sustainsys.Saml2/XmlHelpers.cs#L371. Or maybe you can just use that library instead as is? ;)

@jhudsoncedaron Since I never got a positive response on my idea of fix on Oct 2, 2018 I never created that PR. But If you want to make with the issue now marked as up-for-grabs I guess it would be accepted.

@bartonjs Would you accept a PR that is along the lines I described Oct 2, 2018? I can still create a PR, but would like to have the solution idea approved before spending time on the fix (the simple part) and test cases (the extensive work).

[bartonjs]

Would you accept a PR that is along the lines I described Oct 2, 2018?

Since I don't really work with XML, and we don't ever touch SignedXml code, I'm not able to rapidly load in the implications suggested by that change. The short position for SignedXml is "a simple fix will likely get accepted, a complex fix almost certainly will not".

[AndersAbel]

I drafted a solution and pushed to AndersAbel@bd6700f.

The build is failing for me, so I have do fix that and then add tests before submitting a PR.

[bartonjs]

That change looks nicely targeted. With a test added, and maybe changing == "" to .Length == 0, it's probably good to go.