Repository navigation
[BUG?]: duplicate reference in signature #409
Description
Activity
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-cryptothat you're using, give some sample code, and provide a sample XML document?We make sure that the head of
masteralways builds, so, you can always try to see if your code will run withlatest, 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.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 requestaddReference.
...
Do not reuse the SignedXml object?
Is there aclearReferencesmethod missing?
...it is quite obvious that she/he initialized
SignedXmlonce 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 xml-crypto@3.2.0 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>
FWIW it sure looks like #165 was caused by same issue (pooling of SignedXml object instances). See how author of the issue #165 has named SignedXml object used at the
signAssertionfunction. "signer" would implicate a singleton/service/shared object which is managed outside ofsignAssertionfunction / 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).
@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 :)
We'd welcome a PR to address this.
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
Line 281 in 777e157
which is filled by computeSignature
Line 868 in 777e157
and read by getSignedXml
Line 1133 in 777e157
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.0version.This should be resolved with a doc update.
- Signing multiple documents
- you can reuse the same
SignedXmlinstance to sign multiple documents with the same options
- Verifying multiple documents
- you can reuse the same
SignedXmlinstance to verify multiple documents with the same options
- Mixing sign and verify
- you must NEVER use the same
SignedXMLinstance for both sign and verify operations (algorithm pollution, reference pollution, key confusion)
Or if you want to keep it simple:
Reusing the same SignedXml instance
While technically possible in some cases, the recommended usage pattern is to create a new
SignedXmlinstance each time you need to sign or verify a document.to reuse an instance to sign multiple documents but use different references, you must reset the references between calls using
signedXml.getReferences().length = 0you must NEVER use the same
SignedXMLinstance for both sign and verify operations (algorithm pollution, reference pollution, key confusion)Reacted by Torben
Hi,
we ran into a similar problem as described in #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
clearReferencesmethod missing?Is this a bug?