[BUG?]: duplicate reference in signature

[tenjaa]

Hi, we ran into a similar problem as described in https://github.com/node-saml/xml-crypto/issues/165 But on our side, we reused the SignedXml object and called for each request addReference. This produced the same problem as described in the other issue.

What is the advise here? Do not reuse the SignedXml object? Is there a clearReferences method missing? Is this a bug?

[cjbarth]

Since #165 was resolved, we'll see to start over with the effort to narrow down that problem. Can you please tell us the version of xml-crypto that you're using, give some sample code, and provide a sample XML document?

We make sure that the head of master always builds, so, you can always try to see if your code will run with latest, installed from a GitHub commit. In newer builds we've removed some of the default that were causing problems for people and added in more exceptions to call out more clearly when there is a condition that won't work.

[srd90]

IMHO author of the issue already answered to his/her own question: "Do not reuse the SignedXml object?"

I.e. answer is: Do not reuse the SignedXml object.

---

Based on these hints that issue author provided (quotes from issue report):

... ...we reused the SignedXml object and called for each request addReference. ... Do not reuse the SignedXml object? Is there a clearReferences method missing? ...

it is quite obvious that she/he initialized SignedXml once and reused it multiple times. Quick look to xml-crypto API doesn't support this sort of usage ("pooling of initialized SignedXml object(s)").

I.e. issue author might have done something like this (in pseudo code):

for each request {
  signedXml.addReference(...)
  signedXml.computeSignature(....)
}

and here is proof that aforementioned approach shall produce multiple references:

// npm init
// npm install --save-exact [email protected]

var SignedXml = require("xml-crypto").SignedXml,
  fs = require("fs");

var xml = `
<library>
  <book>
    <name>FOO</name>
  </book>
</library>
`;

// "pool" initialized SignedXml object
var sig = new SignedXml();
// https://github.com/node-saml/xml-crypto/blob/v3.2.0/test/static/client.pem
sig.signingKey = `
-----BEGIN PRIVATE KEY-----
MIICdwIBADANBgkqhkiG9w0BAQEFAASCAmEwggJdAgEAAoGBAL4vpoH3H3byehjj
7RAGxefGRATiq4mXtzc9Q91W7uT0DTaFEbjzVch9aGsNjmLs4QHsoZbuoUmi0st4
x5z9SQpTAKC/dW8muzacT3E7dJJYh03MAO6RiH4LG34VRTq1SQN6qDt2rCK85eG4
5NHI4jceptZNu6Zot1zyO5/PYuFpAgMBAAECgYAhspeyF3M/xB7WIixy1oBiXMLY
isESFAumgfhwU2LotkVRD6rgNl1QtMe3kCNWa9pCWQcYkxeI0IzA+JmFu2shVvoR
oL7eV4VCe1Af33z24E46+cY5grxNhHt/LyCnZKcitvCcrzXExUc5n6KngX0mMKgk
W7skZDwsnKzhyUV8wQJBAN2bQMeASQVOqdfqBdFgC/NPnKY2cuDi6h659QN1l+kg
X3ywdZ7KKftJo1G9l45SN9YpkyEd9zEO6PMFaufJvZUCQQDbtAWxk0i8BT3UTNWC
T/9bUQROPcGZagwwnRFByX7gpmfkf1ImIvbWVXSpX68/IjbjSkTw1nj/Yj1NwFZ0
nxeFAkEAzPhRpXVBlPgaXkvlz7AfvY+wW4hXHyyi0YK8XdPBi25XA5SPZiylQfjt
Z6iN6qSfYqYXoPT/c0/QJR+orvVJNQJBANhRPNXljVTK2GDCseoXd/ZiI5ohxg+W
UaA/1fDvQsRQM7TQA4NXI7BO/YmSk4rW1jIeOxjiIspY4MFAIh+7UL0CQFL6zTg6
wfeMlEZzvgqwCGoLuvTnqtvyg45z7pfcrg2cHdgCXIy9kErcjwGiu6BOevEA1qTW
Rk+bv0tknWvcz/s=
-----END PRIVATE KEY-----
`;

for (i = 1001; i < 1005; i++) {
  sig.addReference("//*[local-name(.)='book']");
  sig.computeSignature(xml.replace("FOO", "id" + i));
}
fs.writeFileSync("/dev/stdout", sig.getSignedXml());

output of:

node index.js | xmllint --format -

is

<?xml version="1.0"?>
<library>
  <book Id="_3">
    <name>id1004</name>
  </book>
  <Signature xmlns="http://www.w3.org/2000/09/xmldsig#">
    <SignedInfo>
      <CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
      <SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
      <Reference URI="#_3">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
        <DigestValue>M/cKYA+AOiVzrFq478pKT7l6roc=</DigestValue>
      </Reference>
      <Reference URI="#_3">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
        <DigestValue>M/cKYA+AOiVzrFq478pKT7l6roc=</DigestValue>
      </Reference>
      <Reference URI="#_3">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
        <DigestValue>M/cKYA+AOiVzrFq478pKT7l6roc=</DigestValue>
      </Reference>
      <Reference URI="#_3">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
        <DigestValue>M/cKYA+AOiVzrFq478pKT7l6roc=</DigestValue>
      </Reference>
    </SignedInfo>
    <SignatureValue>pErAntw5DnXcf1RLEYOIisCcEsx0hwTL45WjTY1S5WQlh8uLjpMCWR+zkAbPgpi8W8/fr+Z8KvAePv5zti3i10FwPMAFayxVt6avaZr6d3af20XIbhOM848Rs0lpmPTGkADMtbfPiBdFDZHhSgtdNrhCHYFFe69u3S2ZgntY6EU=</SignatureValue>
  </Signature>
</library>

[srd90]

FWIW it sure looks like https://github.com/node-saml/xml-crypto/issues/165 was caused by same issue (pooling of SignedXml object instances). See how author of the issue https://github.com/node-saml/xml-crypto/issues/165 has named SignedXml object used at the signAssertion function. "signer" would implicate a singleton/service/shared object which is managed outside of signAssertion function / initialized once and reused multiple times.

---

Addition to previous comment. When I wrote:

Quick look to xml-crypto API doesn't support this sort of usage

I meant that SignedXml's addReference function is by definition something that adds new reference to SignedXml object's internal state and because there isn't any "clear SignedXml objects internal state" it cannot be reused (and even if there would have been such method like clear state there would have been chance to race conditions if it would have been used to handle concurrent execution flows).

[tenjaa]

@srd90 thanks, that is exactly my point. So yes, I agree that it is mainly wrong usage.

Our problem was, that we only ran into this issue after some time being live. The exact issue was, that in the end the response payload got too big because of too many Signature elements and led our API Gateway to reject it.

Throwing an error, immediately when adding the same reference a second time, would have been more explicit and would have allowed us to fail earlier. I think that would be my preferred solution because I don't see any reason why I would want to add exactly the same reference multiple times.

But in the end you are the experts on this library and I am fine with just doing nothing. Maybe just having this issue might help someone running into the same problem and saves them some time :)

[cjbarth]

We'd welcome a PR to address this.

[srd90]

Just out of curiosity: @tenjaa if you used shared SignedXml object instance over multiple requests didn't you also had a risk of leaking incorrect signed XML (access to your API GW with another subject's credentials)? I mean among other things SignedXml instance holds a reference to https://github.com/node-saml/xml-crypto/blob/777e15779b6002df0f2c65376baaea88621048f7/lib/signed-xml.js#L281 which is filled by computeSignature https://github.com/node-saml/xml-crypto/blob/777e15779b6002df0f2c65376baaea88621048f7/lib/signed-xml.js#L868 and read by getSignedXml https://github.com/node-saml/xml-crypto/blob/777e15779b6002df0f2c65376baaea88621048f7/lib/signed-xml.js#L1133

I.e. if there was some interrupt and context switch between

• signedXml.computeSignature(...)
• signedXml.getSignedXml()

then getSignedXml could return another entity's signed XML document.

Disclaimer: I do not use node or this library for anything so this might be stupid question. I have read that node is single threaded but clearly your "pooled" SignedXml object has maintained its internal state over multiple requests (because number of references kept on increasing) and there is a chance that you have done some blocking stuff between those method calls thus triggering context switch or something like that...

btw. links to codebase are pinned to 3.2.0 version.