Skip to content

Commit 28f9221

Browse files
mattgdahacker1-securesamlblairworkos
authored
Merge commit from fork
* - Draft: see what is signed * Fix bugs * Fix bugs * Revert "Fix bugs" This reverts commit cd4b7d6fcb21cd1a40e57fd214c950c47c8667d5. * Revert "Fix bugs" This reverts commit 0947a89a04e16bedffc3cc5bf3152c6dedbe3a7d. * Revert "- Draft: see what is signed" This reverts commit 2a9c405f09f7843b994820f6933408d522a6d849. * Patch for version 3 * Only parse references from canonicalized signed info * Space * Fixing spaces * remove space * Remove fix that does not apply * Formatting * Add warning about references. * Remove formatting changes. * Backport references, SignedInfo, and DigestValue patches. * Only allow exclusive canon. * Apply changes from v6 review * Revert package-lock.json * Apply suggested changes --------- Co-authored-by: SecureSAML <[email protected]> Co-authored-by: Blair Weber <[email protected]>
1 parent 777e157 commit 28f9221

6 files changed

+189
-10
lines changed

lib/signed-xml.js

Lines changed: 98 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,46 @@ SignedXml.prototype.checkSignature = function (xml, callback) {
362362

363363
var doc = new Dom().parseFromString(xml);
364364

365+
// Reset the references as only references from our re-parsed signedInfo node can be trusted
366+
this.references = [];
367+
368+
const unverifiedSignedInfoCanon = this.getCanonSignedInfoXml(doc);
369+
if (!unverifiedSignedInfoCanon) {
370+
if (callback) {
371+
callback(new Error("Canonical signed info cannot be empty"));
372+
return;
373+
}
374+
375+
throw new Error("Canonical signed info cannot be empty");
376+
}
377+
378+
// unsigned, verify later to keep with consistent callback behavior
379+
const parsedUnverifiedSignedInfo = new Dom().parseFromString(unverifiedSignedInfoCanon, "text/xml");
380+
381+
const unverifiedSignedInfoDoc = parsedUnverifiedSignedInfo.documentElement;
382+
if (!unverifiedSignedInfoDoc) {
383+
if (callback) {
384+
callback(new Error("Could not parse signedInfoCanon into a document"));
385+
return;
386+
}
387+
388+
throw new Error("Could not parse signedInfoCanon into a document");
389+
}
390+
391+
const references = utils.findChilds(unverifiedSignedInfoDoc, "Reference");
392+
if (references.length === 0) {
393+
if (callback) {
394+
callback(new Error("could not find any Reference elements"));
395+
return;
396+
}
397+
398+
throw new Error("could not find any Reference elements");
399+
}
400+
401+
for (const reference of references) {
402+
this.loadReference(reference);
403+
}
404+
365405
if (!this.validateReferences(doc)) {
366406
if (!callback) {
367407
return false;
@@ -371,6 +411,7 @@ SignedXml.prototype.checkSignature = function (xml, callback) {
371411
}
372412
}
373413

414+
// Stage B: Take the signature algorithm and key and verify the SignatureValue against the canonicalized SignedInfo
374415
if (!callback) {
375416
// Synchronous flow
376417
if (!this.validateSignatureValue(doc)) {
@@ -394,7 +435,14 @@ SignedXml.prototype.checkSignature = function (xml, callback) {
394435

395436
SignedXml.prototype.getCanonSignedInfoXml = function (doc) {
396437
var signedInfo = utils.findChilds(this.signatureNode, "SignedInfo");
397-
if (signedInfo.length == 0) throw new Error("could not find SignedInfo element in the message");
438+
if (signedInfo.length == 0) {
439+
throw new Error("could not find SignedInfo element in the message");
440+
}
441+
if (signedInfo.length > 1) {
442+
throw new Error(
443+
"could not get canonicalized signed info for a signature that contains multiple SignedInfo nodes"
444+
);
445+
}
398446

399447
if (
400448
this.canonicalizationAlgorithm === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" ||
@@ -477,7 +525,7 @@ SignedXml.prototype.validateReferences = function (doc) {
477525

478526
var ref = this.references[r];
479527

480-
var uri = ref.uri[0] == "#" ? ref.uri.substring(1) : ref.uri;
528+
var uri = ref.uri ? (ref.uri[0]=="#" ? ref.uri.substring(1) : ref.uri) : "";
481529
var elem = [];
482530

483531
if (uri == "") {
@@ -594,11 +642,43 @@ SignedXml.prototype.loadSignature = function (signatureNode) {
594642
".//*[local-name(.)='SignatureMethod']/@Algorithm"
595643
).value;
596644

597-
this.references = [];
598-
var references = xpath.select(
599-
".//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference']",
600-
signatureNode
645+
const signedInfoNodes = utils.findChilds(this.signatureNode, "SignedInfo");
646+
if (signedInfoNodes.length == 0) {
647+
throw new Error("no signed info node found");
648+
}
649+
if (signedInfoNodes.length > 1) {
650+
throw new Error("could not load signature that contains multiple SignedInfo nodes");
651+
}
652+
653+
// Try to operate on the c14n version of signedInfo. This forces the initial getReferences()
654+
// API call to always return references that are loaded under the canonical SignedInfo
655+
// in the case that the client access the .references **before** signature verification.
656+
657+
// Ensure canonicalization algorithm is exclusive, otherwise we'd need the entire document
658+
let canonicalizationAlgorithmForSignedInfo = this.canonicalizationAlgorithm;
659+
if (
660+
!canonicalizationAlgorithmForSignedInfo ||
661+
canonicalizationAlgorithmForSignedInfo ===
662+
"http://www.w3.org/TR/2001/REC-xml-c14n-20010315" ||
663+
canonicalizationAlgorithmForSignedInfo ===
664+
"http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments"
665+
) {
666+
canonicalizationAlgorithmForSignedInfo = "http://www.w3.org/2001/10/xml-exc-c14n#";
667+
}
668+
669+
const temporaryCanonSignedInfo = this.getCanonXml(
670+
[this.canonicalizationAlgorithm || "http://www.w3.org/2001/10/xml-exc-c14n#"],
671+
signedInfoNodes[0]
601672
);
673+
const temporaryCanonSignedInfoXml = new Dom().parseFromString(
674+
temporaryCanonSignedInfo,
675+
"text/xml"
676+
);
677+
const signedInfoDoc = temporaryCanonSignedInfoXml.documentElement;
678+
679+
this.references = [];
680+
681+
const references = utils.findChilds(signedInfoDoc, "Reference");
602682
if (references.length == 0) throw new Error("could not find any Reference elements");
603683

604684
for (var i in references) {
@@ -632,10 +712,17 @@ SignedXml.prototype.loadReference = function (ref) {
632712
nodes = utils.findChilds(ref, "DigestValue");
633713
if (nodes.length == 0)
634714
throw new Error("could not find DigestValue node in reference " + ref.toString());
635-
if (nodes[0].childNodes.length == 0 || !nodes[0].firstChild.data) {
636-
throw new Error("could not find the value of DigestValue in " + nodes[0].toString());
715+
716+
if (nodes.length > 1) {
717+
throw new Error(
718+
`could not load reference for a node that contains multiple DigestValue nodes: ${ref.toString()}`
719+
);
720+
}
721+
722+
const digestValue = nodes[0].textContent;
723+
if (!digestValue) {
724+
throw new Error(`could not find the value of DigestValue in ${ref.toString()}`);
637725
}
638-
var digestValue = nodes[0].firstChild.data;
639726

640727
var transforms = [];
641728
var inclusiveNamespacesPrefixList;
@@ -688,11 +775,12 @@ SignedXml.prototype.loadReference = function (ref) {
688775
transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
689776
}
690777

778+
const refUri = ref.getAttribute("URI") || undefined;
691779
this.addReference(
692780
null,
693781
transforms,
694782
digestAlgo,
695-
utils.findAttr(ref, "URI").value,
783+
refUri,
696784
digestValue,
697785
inclusiveNamespacesPrefixList,
698786
false

test/saml-response-test.js

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,76 @@ exports["test validating SAML response WithComments"] = function (test) {
9393
test.equal(result, false);
9494
test.done();
9595
};
96+
97+
exports["test validating SAML response with digest comment"] = function (test) {
98+
var xml = fs.readFileSync("./test/static/valid_saml_with_digest_comment.xml", "utf-8");
99+
var doc = new xmldom.DOMParser().parseFromString(xml);
100+
const assertion = xpath.select1("//*[local-name(.)='Assertion']", doc);
101+
const signature = xpath.select1(
102+
"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
103+
assertion,
104+
);
105+
var sig = new crypto.SignedXml();
106+
sig.keyInfoProvider = new crypto.FileKeyInfo("./test/static/feide_public.pem");
107+
sig.loadSignature(signature);
108+
var result = sig.checkSignature(xml);
109+
test.equal(sig.references[0].digestValue, "RnNjoyUguwze5w2R+cboyTHlkQk=");
110+
test.equal(result, false);
111+
test.done();
112+
};
113+
114+
exports["test signature contains a `SignedInfo` node"] = function (test) {
115+
var xml = fs.readFileSync("./test/static/invalid_saml_no_signed_info.xml", "utf-8");
116+
var doc = new xmldom.DOMParser().parseFromString(xml);
117+
const node = xpath.select1(
118+
"/*/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
119+
doc,
120+
);
121+
var sig = new crypto.SignedXml();
122+
sig.keyInfoProvider = new crypto.FileKeyInfo("./test/static/feide_public.pem");
123+
test.throws(
124+
function () {
125+
sig.loadSignature(node);
126+
},
127+
Error,
128+
"no signed info node found"
129+
);
130+
test.done();
131+
};
132+
133+
exports["test validation ignores an additional wrapped `SignedInfo` node"] = function (test) {
134+
var xml = fs.readFileSync("./test/static/saml_wrapped_signed_info_node.xml", "utf-8");
135+
var doc = new xmldom.DOMParser().parseFromString(xml);
136+
var assertion = xpath.select("//*[local-name(.)='Assertion']", doc)[0];
137+
var signature = xpath.select(
138+
"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
139+
assertion
140+
)[0];
141+
var sig = new crypto.SignedXml();
142+
sig.keyInfoProvider = new crypto.FileKeyInfo("./test/static/saml_external_ns.pem");
143+
sig.loadSignature(signature);
144+
test.equal(sig.references.length, 1);
145+
var result = sig.checkSignature(xml);
146+
test.equal(result, true);
147+
test.done();
148+
};
149+
150+
exports["test signature does not contain multiple `SignedInfo` nodes"] = function (test) {
151+
var xml = fs.readFileSync("./test/static/saml_multiple_signed_info_nodes.xml", "utf-8");
152+
var doc = new xmldom.DOMParser().parseFromString(xml);
153+
var assertion = xpath.select("//*[local-name(.)='Assertion']", doc)[0];
154+
var signature = xpath.select(
155+
"//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
156+
assertion
157+
)[0];
158+
var sig = new crypto.SignedXml();
159+
sig.keyInfoProvider = new crypto.FileKeyInfo("./test/static/saml_external_ns.pem");
160+
test.throws(
161+
function () {
162+
sig.loadSignature(signature);
163+
},
164+
Error,
165+
"could not load signature that contains multiple SignedInfo nodes"
166+
);
167+
test.done();
168+
};
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" ID="pfx94e4a319-b6f7-4a40-25d1-01fcb642e4c5" Version="2.0" IssueInstant="2012-07-03T11:32:20Z" Destination="http://localhost:3000/login/callback" InResponseTo="_d766d16611ac0d14121b"><saml:Issuer>https://openidp.feide.no</saml:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
2+
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
3+
<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
4+
<ds:SignatureValue>dkONrkxW+LSuDvnNMG/mWYFa47d2WGyapLhXSTYqrlT9Td+tT7ciojNJ55WTaPaCMt7IrGtIxxskPAZIjdIn5pRyDxHr0joWxzZ7oZHCOI1CnQV5HjOq+rzzmEN2LctCZ6S4hbL7SQ1qJ3vp2BCXAygy4tmJOURQdnk0KLwwRS8=</ds:SignatureValue>
5+
<ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIICizCCAfQCCQCY8tKaMc0BMjANBgkqhkiG9w0BAQUFADCBiTELMAkGA1UEBhMCTk8xEjAQBgNVBAgTCVRyb25kaGVpbTEQMA4GA1UEChMHVU5JTkVUVDEOMAwGA1UECxMFRmVpZGUxGTAXBgNVBAMTEG9wZW5pZHAuZmVpZGUubm8xKTAnBgkqhkiG9w0BCQEWGmFuZHJlYXMuc29sYmVyZ0B1bmluZXR0Lm5vMB4XDTA4MDUwODA5MjI0OFoXDTM1MDkyMzA5MjI0OFowgYkxCzAJBgNVBAYTAk5PMRIwEAYDVQQIEwlUcm9uZGhlaW0xEDAOBgNVBAoTB1VOSU5FVFQxDjAMBgNVBAsTBUZlaWRlMRkwFwYDVQQDExBvcGVuaWRwLmZlaWRlLm5vMSkwJwYJKoZIhvcNAQkBFhphbmRyZWFzLnNvbGJlcmdAdW5pbmV0dC5ubzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAt8jLoqI1VTlxAZ2axiDIThWcAOXdu8KkVUWaN/SooO9O0QQ7KRUjSGKN9JK65AFRDXQkWPAu4HlnO4noYlFSLnYyDxI66LCr71x4lgFJjqLeAvB/GqBqFfIZ3YK/NrhnUqFwZu63nLrZjcUZxNaPjOOSRSDaXpv1kb5k3jOiSGECAwEAATANBgkqhkiG9w0BAQUFAAOBgQBQYj4cAafWaYfjBU2zi1ElwStIaJ5nyp/s/8B8SAPK2T79McMyccP3wSW13LHkmM1jwKe3ACFXBvqGQN0IbcH49hu0FKhYFM/GPDJcIHFBsiyMBXChpye9vBaTNEBCtU3KjjyG0hRT2mAQ9h+bkPmOvlEo/aH0xR68Z9hw4PF13w==</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/></samlp:Status><saml:Assertion xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xs="http://www.w3.org/2001/XMLSchema" ID="pfx66496e6c-3c29-230d-6d47-b245434b872d" Version="2.0" IssueInstant="2012-07-03T11:32:20Z"><saml:Issuer>https://openidp.feide.no</saml:Issuer><ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
6+
<ds:SignedInfo><ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
7+
<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
8+
<ds:Reference URI="#pfx66496e6c-3c29-230d-6d47-b245434b872d"><ds:Transforms><ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/><ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/></ds:Transforms><ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><ds:DigestValue>RnNjoyUguwze5w2R+cboyTHlkQk=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>aw5711jKP7xragunjRRCAD4mT4xKHc37iohBpQDbdSomD3ksOSB96UZQp0MtaC3xlVSkMtYw85Om96T2q2xrxLLYVA50eFJEMMF7SCVPStWTVjBlaCuOPEQxIaHyJs9Sy3MCEfbBh4Pqn9IJBd1kzwdlCrWWjAmksbFFg5wHQJA=</ds:SignatureValue>
9+
<ds:KeyInfo><ds:X509Data><ds:X509Certificate>MIICizCCAfQCCQCY8tKaMc0BMjANBgkqhkiG9w0BAQUFADCBiTELMAkGA1UEBhMCTk8xEjAQBgNVBAgTCVRyb25kaGVpbTEQMA4GA1UEChMHVU5JTkVUVDEOMAwGA1UECxMFRmVpZGUxGTAXBgNVBAMTEG9wZW5pZHAuZmVpZGUubm8xKTAnBgkqhkiG9w0BCQEWGmFuZHJlYXMuc29sYmVyZ0B1bmluZXR0Lm5vMB4XDTA4MDUwODA5MjI0OFoXDTM1MDkyMzA5MjI0OFowgYkxCzAJBgNVBAYTAk5PMRIwEAYDVQQIEwlUcm9uZGhlaW0xEDAOBgNVBAoTB1VOSU5FVFQxDjAMBgNVBAsTBUZlaWRlMRkwFwYDVQQDExBvcGVuaWRwLmZlaWRlLm5vMSkwJwYJKoZIhvcNAQkBFhphbmRyZWFzLnNvbGJlcmdAdW5pbmV0dC5ubzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAt8jLoqI1VTlxAZ2axiDIThWcAOXdu8KkVUWaN/SooO9O0QQ7KRUjSGKN9JK65AFRDXQkWPAu4HlnO4noYlFSLnYyDxI66LCr71x4lgFJjqLeAvB/GqBqFfIZ3YK/NrhnUqFwZu63nLrZjcUZxNaPjOOSRSDaXpv1kb5k3jOiSGECAwEAATANBgkqhkiG9w0BAQUFAAOBgQBQYj4cAafWaYfjBU2zi1ElwStIaJ5nyp/s/8B8SAPK2T79McMyccP3wSW13LHkmM1jwKe3ACFXBvqGQN0IbcH49hu0FKhYFM/GPDJcIHFBsiyMBXChpye9vBaTNEBCtU3KjjyG0hRT2mAQ9h+bkPmOvlEo/aH0xR68Z9hw4PF13w==</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID SPNameQualifier="passport-saml" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">_6c5dcaa3053321ff4d63785fbc3f67c59a129cde82</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2012-07-03T11:37:20Z" Recipient="http://localhost:3000/login/callback" InResponseTo="_d766d16611ac0d14121b"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2012-07-03T11:31:50Z" NotOnOrAfter="2012-07-03T11:37:20Z"><saml:AudienceRestriction><saml:Audience>passport-saml</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2012-07-03T11:32:20Z" SessionNotOnOrAfter="2012-07-03T19:32:20Z" SessionIndex="_c8e6823fe38ddbce125f9be6e5118b8c352d04bcae"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:Password</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement><saml:AttributeStatement><saml:Attribute Name="uid" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">bergie</saml:AttributeValue></saml:Attribute><saml:Attribute Name="givenName" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Henri</saml:AttributeValue></saml:Attribute><saml:Attribute Name="sn" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Bergius</saml:AttributeValue></saml:Attribute><saml:Attribute Name="cn" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Henri Bergius</saml:AttributeValue></saml:Attribute><saml:Attribute Name="mail" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">[email protected]</saml:AttributeValue></saml:Attribute><saml:Attribute Name="eduPersonPrincipalName" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">[email protected]</saml:AttributeValue></saml:Attribute><saml:Attribute Name="eduPersonTargetedID" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">8216c78fe244502efa13f62e6615c94acb7bdf3e</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:0.9.2342.19200300.100.1.1" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">bergie</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:2.5.4.42" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Henri</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:2.5.4.4" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Bergius</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:2.5.4.3" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">Henri Bergius</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:0.9.2342.19200300.100.1.3" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">[email protected]</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.6" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">[email protected]</saml:AttributeValue></saml:Attribute><saml:Attribute Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.10" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"><saml:AttributeValue xsi:type="xs:string">8216c78fe244502efa13f62e6615c94acb7bdf3e</saml:AttributeValue></saml:Attribute></saml:AttributeStatement></saml:Assertion></samlp:Response>

0 commit comments

Comments
 (0)