Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve and simplify validation logic #373

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Improve and simplify validation logic #373

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77ba25e
Improve and simplify validation logic
cjbarth Aug 3, 2023
225a596
lint
cjbarth Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 76 additions & 78 deletions src/signed-xml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,32 +253,31 @@
const doc = new xmldom.DOMParser().parseFromString(xml);

if (!this.validateReferences(doc)) {
if (!callback) {
return false;
} else {
if (callback) {
callback(new Error("Could not validate references"));
return;
}

return false;
}

if (!callback) {
// Synchronous flow
if (!this.validateSignatureValue(doc)) {
return false;
}
return true;
const signedInfoCanon = this.getCanonSignedInfoXml(doc);
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");

Check warning on line 268 in src/signed-xml.ts

View check run for this annotation

Codecov / codecov/patch

src/signed-xml.ts#L268 

Added line #L268 was not covered by tests
}
if (callback) {
signer.verifySignature(signedInfoCanon, key, this.signatureValue, callback);

Check warning on line 271 in src/signed-xml.ts

View check run for this annotation

Codecov / codecov/patch

src/signed-xml.ts#L271 

Added line #L271 was not covered by tests
} else {
// Asynchronous flow
this.validateSignatureValue(doc, (err: Error | null, isValidSignature?: boolean) => {
if (err) {
this.validationErrors.push(
`invalid signature: the signature value ${this.signatureValue} is incorrect`,
);
callback(err);
} else {
callback(null, isValidSignature);
}
});
const res = signer.verifySignature(signedInfoCanon, key, this.signatureValue);
if (res === false) {
this.validationErrors.push(
`invalid signature: the signature value ${this.signatureValue} is incorrect`,
);
}

return res;
}
}

Expand Down Expand Up @@ -331,25 +330,16 @@
return this.getCanonXml(ref.transforms, node, c14nOptions);
}

/** @deprecated */
validateSignatureValue(doc: Document): boolean;
/** @deprecated */
validateSignatureValue(doc: Document, callback: ErrorFirstCallback<boolean>): void;
/** @deprecated */
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;
if (key == null) {
throw new Error("KeyInfo or publicCert or privateKey is required to validate signature");
}
if (typeof callback === "function") {
signer.verifySignature(signedInfoCanon, key, this.signatureValue, callback);
if (callback) {
this.checkSignature(doc.toString(), callback);

Check warning on line 340 in src/signed-xml.ts

View check run for this annotation

Codecov / codecov/patch

src/signed-xml.ts#L340 

Added line #L340 was not covered by tests
} else {
const res = signer.verifySignature(signedInfoCanon, key, this.signatureValue);
if (res === false) {
this.validationErrors.push(
`invalid signature: the signature value ${this.signatureValue} is incorrect`,
);
}
return res;
return this.checkSignature(doc.toString());

Check warning on line 342 in src/signed-xml.ts

View check run for this annotation

Codecov / codecov/patch

src/signed-xml.ts#L342 

Added line #L342 was not covered by tests
}
}

Expand Down Expand Up @@ -407,7 +397,7 @@
}
}

// @ts-expect-error This is a problem with the types on `xpath`
// @ts-expect-error FIXME: xpath types are wrong
if (!xpath.isNodeLike(targetElem)) {
continue;
}
Expand All @@ -424,57 +414,65 @@
throw new Error("No references passed validation");
}

validateReferences(doc) {
for (const ref of this.references) {
let elemXpath;
const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri;
let elem: xpath.SelectReturnType = [];
validateReference(ref: Reference, doc: Document) {
const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri;
let elem: xpath.SelectSingleReturnType;

if (uri === "") {
elem = xpath.select("//*", doc);
} else if (uri?.indexOf("'") !== -1) {
// xpath injection
throw new Error("Cannot validate a uri with quotes inside it");
} else {
let num_elements_for_id = 0;
for (const attr of this.idAttributes) {
const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`;
const tmp_elem = xpath.select(tmp_elemXpath, doc);
if (utils.isArrayHasLength(tmp_elem)) {
num_elements_for_id += tmp_elem.length;
elem = tmp_elem;
elemXpath = tmp_elemXpath;
if (uri === "") {
elem = xpath.select1("//*", doc);
} else if (uri?.indexOf("'") !== -1) {
// xpath injection
throw new Error("Cannot validate a uri with quotes inside it");
} else {
let num_elements_for_id = 0;
for (const attr of this.idAttributes) {
const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`;
const tmp_elem = xpath.select(tmp_elemXpath, doc);
if (utils.isArrayHasLength(tmp_elem)) {
num_elements_for_id += tmp_elem.length;

if (num_elements_for_id > 1) {
throw new Error(
"Cannot validate a document which contains multiple elements with the " +
"same value for the ID / Id / Id attributes, in order to prevent " +
"signature wrapping attack.",
);
}
}
if (num_elements_for_id > 1) {
throw new Error(
"Cannot validate a document which contains multiple elements with the " +
"same value for the ID / Id / Id attributes, in order to prevent " +
"signature wrapping attack.",
);
}

ref.xpath = elemXpath;
elem = tmp_elem[0];
ref.xpath = tmp_elemXpath;
}
}
}

// Note, we are using the last found element from the loop above
if (!utils.isArrayHasLength(elem)) {
this.validationErrors.push(
`invalid signature: the signature references an element with uri ${ref.uri} but could not find such element in the xml`,
);
return false;
}
// @ts-expect-error FIXME: xpath types are wrong
if (!xpath.isNodeLike(elem)) {
const validationError = new Error(
`invalid signature: the signature references an element with uri ${ref.uri} but could not find such element in the xml`,
);
this.validationErrors.push(validationError.message);
return false;
}

const canonXml = this.getCanonReferenceXml(doc, ref, elem[0]);
const canonXml = this.getCanonReferenceXml(doc, ref, elem);
const hash = this.findHashAlgorithm(ref.digestAlgorithm);
const digest = hash.getHash(canonXml);

const hash = this.findHashAlgorithm(ref.digestAlgorithm);
const digest = hash.getHash(canonXml);
if (!utils.validateDigestValue(digest, ref.digestValue)) {
const validationError = new Error(
`invalid signature: for uri ${ref.uri} calculated digest is ${digest} but the xml to validate supplies digest ${ref.digestValue}`,
);
this.validationErrors.push(validationError.message);

if (!utils.validateDigestValue(digest, ref.digestValue)) {
this.validationErrors.push(
`invalid signature: for uri ${ref.uri} calculated digest is ${digest} but the xml to validate supplies digest ${ref.digestValue}`,
);
return false;
}

return true;
}

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