Issue with Signature Verification When 'Transforms' Tag is Absent in 'Reference' Element #378
Comments
|
Disclaimer: I'm not too familiar with XML signature specification. This comment is just a result First of all there seems to be implementation difference between It seems that quite a lot of code is at Lines 642 to 665 in 777e157 i.e. these lines are NOT inside that Lines 667 to 699 in 777e157
Lines 543 to 598 in 06cab68 Looks something that might not have been planned change of behaviour. About this actual GH issue. Since @DiegoMajluf did not provide concrete examples here is script that produces two documents.
actual script to generate that material locally is: #!/bin/bash
# prepare test material to investigate issue
# https://github.com/node-saml/xml-crypto/issues/378
# client.pem == https://github.com/node-saml/xml-crypto/blob/v3.2.0/test/static/client.pem
CLIENT_PEM=/tmp/xml-crypto-issue-378/client.pem
# client_public.pem == https://github.com/node-saml/xml-crypto/blob/v3.2.0/test/static/client_public.pem
CLIENT_PUBLIC_PEM=/tmp/xml-crypto-issue-378/client_public.pem
KEYSTORE=/tmp/xml-crypto-issue-378/keystore.p12
SIGNED_DOCUMENT_TEMPLATE=/tmp/xml-crypto-issue-378/signed_document_template.xml
SIGNED_DOCUMENT=/tmp/xml-crypto-issue-378/signed_document.xml
TAMPERED_SIGNED_DOCUMENT=/tmp/xml-crypto-issue-378/tampered_signed_document.xml
# crete p12 keystore to be used with xmlsec1 to create signed
# document without Transforms element
openssl \
pkcs12 \
-nodes \
-export \
-out $KEYSTORE \
-inkey $CLIENT_PEM \
-in $CLIENT_PUBLIC_PEM \
-passout pass:password
echo -n '<library><book ID="bookid"><name>some text</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="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue></DigestValue></Reference></SignedInfo><SignatureValue/></Signature></library>' \
> $SIGNED_DOCUMENT_TEMPLATE
echo -e "\n\n----------------------------------"
echo "Created template to be used to generate signed document"
echo "to $SIGNED_DOCUMENT_TEMPLATE -file"
echo "NOTE: it does not containt Transformations element"
echo "Pretty printed version looks like this:"
cat $SIGNED_DOCUMENT_TEMPLATE | xmllint --format -
echo "----------------------------------"
echo -e "\n\nSign aforementioned template"
xmlsec1 \
--sign \
--id-attr:ID book \
--output $SIGNED_DOCUMENT \
--pkcs12 $KEYSTORE \
--pwd password \
$SIGNED_DOCUMENT_TEMPLATE
echo "Signed document:"
cat $SIGNED_DOCUMENT
echo -e "\nPretty printed version of signed document ( $SIGNED_DOCUMENT ):"
cat $SIGNED_DOCUMENT | xmllint --format -
echo "----------------------------------"
echo "Create tampered signed document:"
cat $SIGNED_DOCUMENT | sed -e 's/some text/some tampered text/' > $TAMPERED_SIGNED_DOCUMENT
cat $TAMPERED_SIGNED_DOCUMENT
echo -e "\nPretty printed version of tampered signed document ( $TAMPERED_SIGNED_DOCUMENT )"
cat $TAMPERED_SIGNED_DOCUMENT | xmllint --format -
echo "----------------------------------"
echo "use these files when testing xml-crypto 3.2.0 and 4.1.0 validation functionality"
echo " signed document file: $SIGNED_DOCUMENT"
echo "tampered signed document file: $TAMPERED_SIGNED_DOCUMENT"
echo "it is important that you use aforementioned files as is from the disk instead of"
echo "copy pasting those in order to prevent any additional formatting being made by"
echo "echo IDEs etc."For the sake of readability of this comment here are pretty printed versions of key bits of that script. DO NOT use pretty printed versions for actual issue debugging. Input for <?xml version="1.0"?>
<library>
<book ID="bookid">
<name>some text</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="#bookid">
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
<DigestValue/>
</Reference>
</SignedInfo>
<SignatureValue/>
</Signature>
</library>Signed document produced by <?xml version="1.0"?>
<library>
<book ID="bookid">
<name>some text</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="#bookid">
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
<DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue>
</Reference>
</SignedInfo>
<SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue>
</Signature>
</library>Signed document which content has been tampered ( <?xml version="1.0"?>
<library>
<book ID="bookid">
<name>some tampered text</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="#bookid">
<DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
<DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue>
</Reference>
</SignedInfo>
<SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue>
</Signature>
</library>Now that we have some test material which might be close enought something that issue reported had lets proceed to test what happens when these are validated with Case // https://github.com/node-saml/xml-crypto/issues/378
//
// test with xml-crypto 3.2.0
// npm init
// npm insall [email protected]
const select = require("xml-crypto").xpath,
dom = require("@xmldom/xmldom").DOMParser,
SignedXml = require("xml-crypto").SignedXml,
FileKeyInfo = require("xml-crypto").FileKeyInfo,
fs = require("fs");
// https://github.com/node-saml/xml-crypto/blob/v3.2.0/test/static/client_public.pem
const client_public_pem = "/tmp/xml-crypto-issue-378/client_public.pem";
// these files are produced by separaed shell script
const signed_xml = fs.readFileSync("/tmp/xml-crypto-issue-378/signed_document.xml").toString();
const tampered_signed_xml = fs.readFileSync("/tmp/xml-crypto-issue-378/tampered_signed_document.xml").toString();
function validateXml(xml, key) {
const doc = new dom().parseFromString(xml);
const signature = select(
doc,
"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']"
)[0];
const sig = new SignedXml();
sig.keyInfoProvider = new FileKeyInfo(client_public_pem);
sig.loadSignature(signature.toString());
const res = sig.checkSignature(xml);
if (!res) console.log(sig.validationErrors);
return res;
}
console.log("Test signed document signature validation.");
if (validateXml(signed_xml)) {
console.log("Signature was valid")
} else {
console.log("signature validation failed.")
}
console.log("Test tampered document signature validation ( IT MUST return validation error)");
if (validateXml(tampered_signed_xml)) {
console.log("Signature was valid...it should not have been valid!!!!!")
} else {
console.log("signature validation failed....as it should have...")
}Output: Case // https://github.com/node-saml/xml-crypto/issues/378
//
// test with xml-crypto 4.1.0
// npm init
// npm insall [email protected]
const xpath = require("xpath"),
dom = require("@xmldom/xmldom").DOMParser,
SignedXml = require("xml-crypto").SignedXml,
FileKeyInfo = require("xml-crypto").FileKeyInfo,
fs = require("fs");
// https://github.com/node-saml/xml-crypto/blob/v3.2.0/test/static/client_public.pem
const client_public_pem = fs.readFileSync("/tmp/xml-crypto-issue-378/client_public.pem").toString();
const signed_xml = fs.readFileSync("/tmp/xml-crypto-issue-378/signed_document.xml").toString();
const tampered_signed_xml = fs.readFileSync("/tmp/xml-crypto-issue-378/tampered_signed_document.xml").toString();
function validateXml(xml, key) {
const doc = new dom().parseFromString(xml);
const signature = xpath.select(
"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
doc
)[0];
const sig = new SignedXml();
sig.publicCert = client_public_pem;
sig.loadSignature(signature.toString());
const res = sig.checkSignature(xml);
if (!res) console.log(sig.validationErrors);
return res;
}
console.log("Test signed document signature validation.");
if (validateXml(signed_xml)) {
console.log("Signature was valid")
} else {
console.log("signature validation failed.")
}
console.log("Test tampered document signature validation ( IT MUST return validation error)");
if (validateXml(tampered_signed_xml)) {
console.log("Signature was valid...it should not have been valid!!!!!")
} else {
console.log("signature validation failed....as it should have...")
}Output: This issue looks as if current version ( ping @LoneRifle @cjbarth @djaqua |
|
Code in this comment is written against Here are two test cases with documents with diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts
index f1cbe3f..8a185f8 100644
--- a/test/signature-unit-tests.spec.ts
+++ b/test/signature-unit-tests.spec.ts
@@ -877,6 +877,10 @@ describe("Signature unit tests", function () {
passValidSignature("./test/static/valid_signature_with_unused_prefixes.xml");
});
+ it("verifies valid signature without transforms element", function () {
+ passValidSignature("./test/static/valid_signature_without_transforms_element.xml");
+ });
+
it("fails invalid signature - signature value", function () {
failInvalidSignature("./test/static/invalid_signature - signature value.xml");
});
@@ -918,6 +922,10 @@ describe("Signature unit tests", function () {
);
});
+ it("fails invalid signature without transforms element", function () {
+ failInvalidSignature("./test/static/invalid_signature_without_transforms_element.xml");
+ });
+
it("allow empty reference uri when signing", function () {
const xml = "<root><x /></root>";
const sig = new SignedXml();
diff --git a/test/static/invalid_signature_without_transforms_element.xml b/test/static/invalid_signature_without_transforms_element.xml
new file mode 100644
index 0000000..9c48372
--- /dev/null
+++ b/test/static/invalid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<library><book ID="bookid"><name>some tampered text</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="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>
diff --git a/test/static/valid_signature_without_transforms_element.xml b/test/static/valid_signature_without_transforms_element.xml
new file mode 100644
index 0000000..5d22a29
--- /dev/null
+++ b/test/static/valid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<library><book ID="bookid"><name>some text</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="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>Result:
...
Next step is to try to re-introduce diff --git a/src/signed-xml.ts b/src/signed-xml.ts
index 5511769..629f577 100644
--- a/src/signed-xml.ts
+++ b/src/signed-xml.ts
@@ -596,38 +596,40 @@ export class SignedXml {
.filter((value) => value.length > 0);
}
- if (utils.isArrayHasLength(this.implicitTransforms)) {
- this.implicitTransforms.forEach(function (t) {
- transforms.push(t);
- });
- }
-
- /**
- * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
- * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
- * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
- * @see:
- * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
- * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
- * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
- */
- if (
- transforms.length === 0 ||
- transforms[transforms.length - 1] ===
- "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
- ) {
- transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
- }
+ }
- this.addReference({
- transforms,
- digestAlgorithm: digestAlgo,
- uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
- digestValue,
- inclusiveNamespacesPrefixList,
- isEmptyUri: false,
+ if (utils.isArrayHasLength(this.implicitTransforms)) {
+ this.implicitTransforms.forEach(function (t) {
+ transforms.push(t);
});
}
+
+ /**
+ * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
+ * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
+ * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
+ * @see:
+ * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
+ * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
+ * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
+ */
+ if (
+ transforms.length === 0 ||
+ transforms[transforms.length - 1] ===
+ "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
+ ) {
+ transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
+ }
+
+ this.addReference({
+ transforms,
+ digestAlgorithm: digestAlgo,
+ uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
+ digestValue,
+ inclusiveNamespacesPrefixList,
+ isEmptyUri: false,
+ });
+
}
/**Result:
Actually both fails due same reason (i.e. reason is not what one would expect) and reason is that this function Lines 942 to 966 in 2e32d50 ends up returning incorrectly(?) canonicalized xml. In case of verifies valid signature without transforms element XML should look like this:
<book ID="bookid"><name>some text</name></book>but <book ID="bookid"><name></name></book>and digest is calculated from incorrect content here: Lines 457 to 459 in 2e32d50 ( FWIW, console.log(
require('crypto')
.createHash("sha1")
.update('<book ID="bookid"><name>some text</name></book>', "utf8")
.digest("base64")
);
// output: LsMoqo1d6Sqh8DKLp00MK0fSBDA=Reason for missing text node might be this/these xml-crypto/src/c14n-canonicalization.ts Lines 171 to 176 in 2e32d50 xml-crypto/src/exclusive-canonicalization.ts Lines 177 to 182 in 2e32d50 From xml-crypto/lib/c14n-canonicalization.js Lines 183 to 188 in 777e157 xml-crypto/lib/exclusive-canonicalization.js Lines 185 to 190 in 777e157 i.e. at least current Side note: Is this program logic correct? Lines 950 to 965 in 2e32d50 AFAIK it applies potentially multiple transforms so that input is always original ( From
I do not know if that version of spec is correct source to look things up. There is also Same program block from Lines 994 to 1009 in 777e157 Disclaimer: purpose of this comment was to share some additional findings after I looked around out of curiosity. I do not know how valid these findings are. |
|
@DiegoMajluf you bumped into and reported this issue (which escalated little bit) in the first place. Consider reviewing / testing PR's ( #379 and #380 ) that @cjbarth introduced with material you use. Original issue is present in published versions |
|
@srd90, thanks for your superb work on this issue. The issue with the skip of digest calculation seems to be rectified. However, at first glance, I'm still experiencing some problems with the canonicalization. So, allow me to verify a little more thoroughly with my testing documents |
Good Day
I have this signture on a XML Document that it content has been deliberately modified to fail the verification
As the Reference tag does not contain a Transforms tag, when I apply the checkSignature function to that document, the result is true. However, this is due to the function performing verification solely on the signature's digest, without recalculating the digest itself.
To reproduce the problem, take any verified document that doesn't has the Transforms tag, and then make any modification to the content that would cause the digest calculation to fail to match the original. You'll observe that the signature is still verified.
Let me know if I need to provide more details
The text was updated successfully, but these errors were encountered: