Issue with Signature Verification When 'Transforms' Tag is Absent in 'Reference' Element about xml-crypto HOT 5 CLOSED

Disclaimer: I'm not too familiar with XML signature specification. This comment is just a result
of trying few things (starting with trying to come up with test material hinted by @DiegoMajluf ) and writing down some findings. It is up to someone else to decide what to do with these findings.

First of all there seems to be implementation difference between 3.2.0 and 4.1.0 in loadReference function.

It seems that quite a lot of code is at 3.2.0 version outside of this if statement:

i.e. these lines are NOT inside that if statement block at version 3.2.0:

4.1.0 version contains all of those inside aforementioned if statement block

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.

1. /tmp/xml-crypto-issue-378/signed_document.xml
   • does not have Transforms element
2. /tmp/xml-crypto-issue-378/tampered_signed_document.xml
   • is a result for (1) after changing with sed one string in order to have tampered signed content

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 xmlsec1 (aka "template"):

<?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 xmlsec1:

<?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 (some text is replaced with sed to some tampered text):

<?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 3.2.0 and 4.1.0.

Case 3.2.0:

// 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:

Test signed document signature validation.
Signature was valid
Test tampered document signature validation ( IT MUST return validation error)
[
  'invalid signature: for uri #bookid calculated digest is WyqD++sYNPkb4d9XUrGqn1c8xqo= but the xml to validate supplies digest LsMoqo1d6Sqh8DKLp00MK0fSBDA='
]
signature validation failed....as it should have...

Case 4.1.0

// 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:

Test signed document signature validation.
Signature was valid
Test tampered document signature validation ( IT MUST return validation error)
Signature was valid...it should not have been valid!!!!!

This issue looks as if current version ( 4.x ) of xml-crypto might have signature validation bypass vulnerability (NOTE: I do not know how validation must work if Transforms is missing).

ping @LoneRifle @cjbarth @djaqua

from xml-crypto.

Code in this comment is written against xml-crypto version 2e32d502c99a08936c73885405c8b5a4e217dfce which is content of the master branch at the time of writing this.

Here are two test cases with documents with Signature without Transforms element.

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:

• fails invalid signature without transforms element fails
• verifies valid signature without transforms element passes, BUT...

...xml-crypto considers both cases valid and why it does so is because:

1. This line is never executed
   

   xml-crypto/src/signed-xml.ts

   Line 622 in 2e32d50

   this.addReference({

2. and because there are no refenrences this code block return always true (i.e. it doesn't iterate anything):
   

   xml-crypto/src/signed-xml.ts

   Lines 473 to 480 in 2e32d50

   	validateReferences(doc: Document) {
   	for (const ref of this.references) {
   	if (!this.validateReference(ref, doc)) {
   	return false;
   	}
   	}
   	return true;
   	}

Next step is to try to re-introduce < 4.0.0 versions control flow to loadReference function (dunno if this was too naive fix. I did not read code too carefully):

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:

• fails invalid signature without transforms element passes
• verifies valid signature without transforms element fails

Actually both fails due same reason (i.e. reason is not what one would expect) and reason is that this function

<book ID="bookid"><name>some text</name></book>

but getCanonXml returns:

<book ID="bookid"><name></name></book>

and digest is calculated from incorrect content here:

(fails invalid signature without transforms element ends up to failure due same reason...i.e. getCanonXml function returning XML without text node and not because text node would have had text some tampered text)

FWIW,
xmlsec1 (which was used to produce signed document) calculated sha1 LsMoqo1d6Sqh8DKLp00MK0fSBDA= for canonicalized document (<book ID="bookid"><name>some text</name></book>) and one gets same hash for that from xml-crypto's sha1 implementation. I.e. either xmlsec1 is applying canonicalization incorrectly or xml-crypto's c14n implementation has a bug (but since xml-crypto 3.2.0 seems to handle this correctly I would say that there is a bug at 4.1.0 version. NOTE: I haven't (yet) step debugged 3.2.0).

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
From current master:

From 3.2.0:

i.e. at least current master version's c14n-canonicalization has incorrect if statement. Dunno if any of those would handle CDATA correctly.

Side note:

Is this program logic correct?

AFAIK it applies potentially multiple transforms so that input is always original (canonXml) instead of output of previous transform operation (transformedXml).

From XML Signature Syntax and Processing Version 1.1 https://www.w3.org/TR/2013/REC-xmldsig-core1-20130411/#sec-Transforms

... The optional Transforms element contains an ordered list of Transform elements; these describe how the signer obtained the data object that was digested. The output of each Transform serves as input to the next Transform. The input to the first Transform is the result of dereferencing the URI attribute of the Reference element. The output from the last Transform is the input for the DigestMethod algorithm. ...

I do not know if that version of spec is correct source to look things up. There is also XML Signature Syntax and Processing Version 2.0 which has following statements https://www.w3.org/TR/xmldsig-core2/#sec-Transforms and https://www.w3.org/TR/xmldsig-core2/#sec-TransformsProcessingModel

Same program block from 3.2.0 version:

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.

from xml-crypto.

Is this program logic correct?

Some preliminary testing I've done indicates that it isn't correct, but I don't have a test case for it yet.

As for the other item reported here, I've put up #379 to address it. Thanks for your thorough analysis @srd90.

from xml-crypto.

@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 [ 4.0.0 , 4.1.0 ]. Additional finding (incorrect C14N implementation) was introduced after 4.1.0 to master. What I'm trying to say is that if/when you bumped into this issue in production or in some E2E test env it means that there is good chance that signature bypass issue has ended up to production at some (other) place and maybe there should be CVE for this.

from xml-crypto.

@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

from xml-crypto.