-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce new .signedReferences property of signature to help prevent signature wrapping attacks. #489
base: 6.x
Are you sure you want to change the base?
Introduce new .signedReferences property of signature to help prevent signature wrapping attacks. #489
Changes from all commits
f9b3682
c2a7d7a
b901de8
ea1ad24
d2c9f8c
b0192ab
e0c1ac1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
export { SignedXml } from "./signed-xml"; | ||
export { C14nCanonicalization, C14nCanonicalizationWithComments } from "./c14n-canonicalization"; | ||
export { | ||
ExclusiveCanonicalization, | ||
ExclusiveCanonicalizationWithComments, | ||
} from "./exclusive-canonicalization"; | ||
export * from "./utils"; | ||
export * from "./types"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import * as signatureAlgorithms from "./signature-algorithms"; | |
import * as crypto from "crypto"; | ||
import * as isDomNode from "@xmldom/is-dom-node"; | ||
|
||
|
||
export class SignedXml { | ||
idMode?: "wssecurity"; | ||
idAttributes: string[]; | ||
|
@@ -87,6 +88,8 @@ export class SignedXml { | |
"http://www.w3.org/2000/09/xmldsig#enveloped-signature": envelopedSignatures.EnvelopedSignature, | ||
}; | ||
|
||
// TODO: In V7.x we may consider deprecating sha1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm all for putting in a deprecation warning now and removing it later on. Either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only temporary for some SAML libraries to use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I was thinking there is no reason not to throw a deprecation notice now if someone uses sha1, no need to wait for 7.x to throw the notice. |
||
|
||
/** | ||
* To add a new hash algorithm create a new class that implements the {@link HashAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} | ||
*/ | ||
|
@@ -96,6 +99,8 @@ export class SignedXml { | |
"http://www.w3.org/2001/04/xmlenc#sha512": hashAlgorithms.Sha512, | ||
}; | ||
|
||
// TODO: In V7.x we may consider deprecating sha1 | ||
|
||
/** | ||
* To add a new signature algorithm create a new class that implements the {@link SignatureAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} | ||
*/ | ||
|
@@ -112,6 +117,7 @@ export class SignedXml { | |
}; | ||
|
||
static noop = () => null; | ||
public signedReferences: string[] = []; | ||
|
||
/** | ||
* The SignedXml constructor provides an abstraction for sign and verify xml documents. The object is constructed using | ||
|
@@ -154,6 +160,9 @@ export class SignedXml { | |
this.CanonicalizationAlgorithms; | ||
this.HashAlgorithms; | ||
this.SignatureAlgorithms; | ||
// this populates only after verifying the signature | ||
// array of bytes that are cryptographically authenticated | ||
this.signedReferences = []; // TODO: should we rename this to something better. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That seems like a descriptive name to me. What other thoughts do you have? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea, only problem is that people might confuse it to be a array of Reference objects, when it's an array of bytes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chatty suggested options like |
||
} | ||
|
||
/** | ||
|
@@ -252,6 +261,7 @@ export class SignedXml { | |
this.signedXml = xml; | ||
|
||
const doc = new xmldom.DOMParser().parseFromString(xml); | ||
|
||
// Reset the references as only references from our re-parsed signedInfo node can be trusted | ||
this.references = []; | ||
|
||
|
@@ -307,25 +317,45 @@ export class SignedXml { | |
return false; | ||
} | ||
|
||
// (Stage B authentication step, show that the signedInfoCanon is signed) | ||
|
||
// first find the key & signature algorithm, this should match | ||
// Stage B: Take the signature algorithm and key and verify the SignatureValue against the canonicalized SignedInfo | ||
const signer = this.findSignatureAlgorithm(this.signatureAlgorithm); | ||
const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey; | ||
if (key == null) { | ||
throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); | ||
} | ||
|
||
if (callback) { | ||
signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue, callback); | ||
|
||
// let's clear the callback up a little bit, so we can access it's results, | ||
// and decide whether to reset signature value or not | ||
const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); | ||
// true case | ||
if (sigRes === true) { | ||
if (callback) { | ||
callback(null, true); | ||
} else { | ||
return true; | ||
} | ||
} else { | ||
const verified = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); | ||
// false case | ||
// reset the signedReferences back to empty array | ||
// I would have preferred to start by verifying the signedInfoCanon first, if that's OK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like our tests would catch if this causes breaking changes. Did one of the existing tests fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all test cases passed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If all tests past when verifying |
||
// but that may cause some breaking changes? | ||
this.signedReferences = []; | ||
|
||
|
||
if (verified === false) { | ||
if (callback) { | ||
callback(new Error( | ||
`invalid signature: the signature value ${this.signatureValue} is incorrect`, | ||
)); | ||
return; // return early | ||
} else { | ||
throw new Error( | ||
`invalid signature: the signature value ${this.signatureValue} is incorrect`, | ||
); | ||
} | ||
|
||
return true; | ||
} | ||
} | ||
|
||
|
@@ -525,6 +555,11 @@ export class SignedXml { | |
|
||
return false; | ||
} | ||
// This step can only be done after we have verified the signedInfo | ||
// we verified that they have same hash | ||
// so, the canonXml and only the canonXml can be trusted | ||
// append this to signedReferences | ||
this.signedReferences.push(canonXml); | ||
|
||
return true; | ||
} | ||
|
@@ -575,6 +610,7 @@ export class SignedXml { | |
|
||
const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo"); | ||
if (!utils.isArrayHasLength(signedInfoNodes)) { | ||
|
||
throw new Error("no signed info node found"); | ||
} | ||
if (signedInfoNodes.length > 1) { | ||
|
@@ -655,6 +691,7 @@ export class SignedXml { | |
if (nodes.length === 0) { | ||
throw new Error(`could not find DigestValue node in reference ${refNode.toString()}`); | ||
} | ||
|
||
if (nodes.length > 1) { | ||
throw new Error( | ||
`could not load reference for a node that contains multiple DigestValue nodes: ${refNode.toString()}`, | ||
|
@@ -713,7 +750,7 @@ export class SignedXml { | |
) { | ||
transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315"); | ||
} | ||
const refUri = isDomNode.isElementNode(refNode) | ||
const refUri = isDomNode.isElementNode(refNode) | ||
? refNode.getAttribute("URI") || undefined | ||
: undefined; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog is automatically generated from the title of the PR. Any additional notes should go in the README.