Skip to content

Adjust deprecation to better reflect real-world usage #498

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

Merged
cjbarth merged 7 commits into from
Apr 21, 2025
Merged

Adjust deprecation to better reflect real-world usage #498

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e0dc85
Introduce new .getSignedReferences() function of signature to help pr…
cjbarth Apr 9, 2025
9899fcd
wip
cjbarth Apr 20, 2025
fd36c19
Merge branch 'master' into 6.1.0-changes
cjbarth Apr 20, 2025
c5657f6
More proper deprecation
cjbarth Apr 20, 2025
156fde7
Improve typing and getReferences object
cjbarth Apr 20, 2025
04bb79e
Recommended future changes
cjbarth Apr 20, 2025
9ac1547
fix spelling
cjbarth Apr 21, 2025
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
22 changes: 15 additions & 7 deletions src/signed-xml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export class SignedXml {
valid(ated). Put simply: if one fails, they are all not trustworthy.
*/
this.signedReferences = [];
// TODO: add this break change here later on for even more security: `this.references = [];`
if (callback) {
callback(new Error("Could not validate all references"), false);
return;
Expand Down Expand Up @@ -357,6 +358,7 @@ export class SignedXml {
// but that may cause some breaking changes, so we'll handle that in v7.x.
// If we were validating `signedInfoCanon` first, we wouldn't have to reset this array.
this.signedReferences = [];
// TODO: add this break change here later on for even more security: `this.references = [];`

if (callback) {
callback(
Expand Down Expand Up @@ -539,14 +541,14 @@ export class SignedXml {
}
}

ref.getValidatedNode = (xpathSelector?: string) => {
ref.getValidatedNode = deprecate((xpathSelector?: string) => {
xpathSelector = xpathSelector || ref.xpath;
if (typeof xpathSelector !== "string" || ref.validationError != null) {
return null;
}
const selectedValue = xpath.select1(xpathSelector, doc);
return isDomNode.isNodeLike(selectedValue) ? selectedValue : null;
};
}, "`ref.getValidatedNode()` is deprecated and insecure. Use `ref.signedReference` or `this.getSignedReferences()` instead.");

if (!isDomNode.isNodeLike(elem)) {
const validationError = new Error(
Expand All @@ -573,6 +575,7 @@ export class SignedXml {
// thus the `canonXml` and _only_ the `canonXml` can be trusted.
// Append this to `signedReferences`.
this.signedReferences.push(canonXml);
ref.signedReference = canonXml;
Copy link

@ahacker1-securesaml ahacker1-securesaml Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one will even bother using the API here. Better to clearly warn the user that the entire reference object is contaminated and shift to using the new API which I already designed and spent efforts into making it secure.
We are really duplicating a lot of work here

Copy link
Contributor Author

@cjbarth cjbarth Apr 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the entire reference object is contaminated then your suggested code over at node-saml is contaminated.

This is a super small change that literally takes the exact same data and puts in on the reference object. So, really no additional effort, but it certainly makes things a lot easier for a lot of migration cases; a user no longer needs to figure out which signed reference to use, they can get the one they want the same way as before.

Copy link

@ahacker1-securesaml ahacker1-securesaml Apr 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the entire reference object is contaminated then your suggested code over at node-saml is contaminated.

I don't use what is inside the getReferences() Data for processing, I only use it for some spurious validation checks. You can see in my change, only the data from getSignedReferences() is processed for SAML assertions.

This is a super small change that literally takes the exact same data and puts in on the reference object. So, really no additional effort, but it certainly makes things a lot easier for a lot of migration cases; a user no longer needs to figure out which signed reference to use, they can get the one they want the same way as before.

This is a change that doesn't benefit the users of xml-crypto at all. it takes time to review, could possible contain security defects.

Your change doesn't warn users against how they current verify XML Signatures:
https://github.com/node-saml/xml-crypto/tree/v6.0.0#:~:text=const%20uri%20%3D%20sig.getReferences()%5B0%5D.uri%3B%20//%20might%20not%20be%200%3B%20it%20depends%20on%20the%20document
Which is fundamentally insecure.
Users can still do something like:

const uri = sig.getReferences()[0].uri; // might not be 0; it depends on the document
const id = uri[0] === "#" ? uri.substring(1) : uri;

And they will not get a deprecation warning of their insecure behavior.

What we should be focusing on is getting the SAML Libraries to start using the new APIs. The current code that I wrote is secure, future changes will (even small), are just an annoyance and take time to review.

Remember, we can always leave discussions in V7. Now is not the time

Copy link
Contributor Author

@cjbarth cjbarth Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to ship a node-saml library that throws deprecation notices. If it is deprecated, we shouldn't be using it in node-saml.

Your referenced example about checking the ID will still cause a deprecation warning because just a few lines below that is this: https://github.com/node-saml/xml-crypto/tree/v6.0.0#:~:text=const%20elem%20%3D%20sig.references%5B0%5D.getvalidatedelement()

const elem = sig.references[0].getValidatedElement()

That is now a deprecated function due this this PR. So, users, like you did in your change in node-saml, can keep using getReferences() to look at an ID or other properties, which is totally harmless, even if it has been attacked, but they can not get to the potentially insecure reference without getting a deprecation warning.

Copy link
Contributor

@markstos markstos Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to ship a node-saml library that throws deprecation notices. If it is deprecated, we shouldn't be using it in node-saml.

Agreed.

Copy link

@ahacker1-securesaml ahacker1-securesaml Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjbarth

I think we should just keep all the signedReferences in 1 place, instead of maintaining 2 separate places.

  • It makes it easier to maintain, i.e. there are security vulnerabilities @srd90 pointed out with how it is derived.

  • It also ensures that there is 1 right way to do things.

Now previous API:

getSignedReferences()

This isolates the signed data into one place.

new API:

getReferences()[idx].signedReference

Insecure because the getReferences() each Reference object is garbage. We are mixing signed and unsigned data. it's a very difficult security boundary to enforce.

Copy link
Contributor Author

@cjbarth cjbarth Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahacker1-securesaml , I'm very confused by your back and forth on this. If getReferences() is garbage, why are you proposing it for the upgrades to node-saml? We still use it in node-saml in other places too like for validating that a signature reference exists.

I take @srd90 's point about validation and signing. @srd90 has been a valued, cooperative, helpful, long-time watchful eye over these projects and has helped us all immeasurably to keep this code functioning as best we can and to help us to understand many things related to security along the way. No one maintintaing this project gets paid to do this work; there is no corporate sponsor.

I don't understand how looking at an ID is insecure in any way. Please elaborate on how the ID is insecure @ahacker1-securesaml .

I also don't understand how to find the reference that I'm after without some additional metadata, which is what getReferences() provides. Thus, the thinking was to provide the validated XML along with the metadata. If I did so incorrectly, which it appears I have, I'll pull forward the changes the I marked at TODO for v7 and call it a security fix in a 6.2 release. If it is unsigned, that is one thing; it can stay. If it fails a signature check, then it should be removed along with all other data because something corrupting has happened and we don't want to trust anything.

Copy link
Contributor Author

@cjbarth cjbarth Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there exist >1 references and any of the 1+n ( n > 0 ) reference validation fails then it seems based on quick code browsing that at least ref numer 1's signedReference points to canonical representation of ref XML and it is "labelled" as "signed" even though actual signature validation has not occured at all. At this stage only reference digest is validated but attacker might have recalculated digest after altering content at the end of reference "pointer". I.e. this looks similar case that there was with signedReferences array which was not resetted if any of the 1+n references validation failed.

Thank you @srd90 . You can see from my comment I thought something might need to be done here, but I stopped short of a secure solution. Please have a look at #500 to see if that addresses the concern.

Copy link

@srd90 srd90 Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at #500 to see if that addresses the concern.

IMHO that change "rolls back" filled signedReference pointers correctly in case of reference list or signature validation error situations

Copy link

@ahacker1-securesaml ahacker1-securesaml Apr 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahacker1-securesaml , I'm very confused by your back and forth on this. If getReferences() is garbage, why are you proposing it for the upgrades to node-saml? We still use it in node-saml in other places too like for validating that a signature reference exists.

I never propose to use the API. I only keep using the getReferences() API to throw error messages, i.e. never do any security checks based off of it. I.e. because some test cases and some integrators will depend on the error messages given. To make those error messages I have to get use the Reference ID.

But stricly speaking, look at the PR contents, I only use the contents from getSignedReferences(). Try to follow the logic behind the changed. Remove our usage of getReferences() and my program is still secure.

validating that a signature reference exists.

Can't you just do .getSignedReferences() exist as well. The ID is not really relavant for anything.

I also don't understand how to find the reference that I'm after without some additional metadata, which is what getReferences() provides. Thus, the thinking was to provide the validated XML along with the metadata. If I did so incorrectly, which it appears I have, I'll pull forward the changes the I marked at TODO for v7 and call it a security fix in a 6.2 release. If it is unsigned, that is one thing; it can stay. If it fails a signature check, then it should be removed along with all other data because something corrupting has happened and we don't want to trust anything.

getReferences() currently provides an URI or ID attribute (i.e. additional metadata). The ID attribute is a random string which is irrelavant, and shouldn't be used for security decisions.

I think this is a fundamental misconception. You should never go back into the original document, because the original document is attacker controlled. If you use the ID to drive security, then you would have to use the original document, thus your system would be using untrusted data.

For example, you might do something like originalDocument.getElementByID("referenceID"). This is useless, since the orignal document is attacker controlled. I.e. since only parts of it is signed.
Furthermore, you don't really need the original Document, you MUST be using only the signed portions.

Instead what users MUST do is use the contents from getSignedReferences() API, which is isolated from the original document, and contains only signed data.

Please give a counterexample, where you cannot use getSignedReferences() but can only use getReferences().

I take @srd90 's point about validation and signing. @srd90 has been a valued, cooperative, helpful, long-time watchful eye over these projects and has helped us all immeasurably to keep this code functioning as best we can and to help us to understand many things related to security along the way. No one maintintaing this project gets paid to do this work; there is no corporate sponsor.

You should ask for more funding. Your package handles security for the largest websites. It's immoral and unethical to be maintaining a package without funding - it makes supply chain attacks easier and increases code software vulnerabilities (i.e. no code auditing).


return true;
}
Expand Down Expand Up @@ -821,13 +824,18 @@ export class SignedXml {
}

/**
* @deprecated Use `.getSignedReferences()` instead.
* Returns the list of references.
*/
getReferences = deprecate(
() => this.references,
"getReferences() is deprecated. Use `.getSignedReferences()` instead.",
);
getReferences() {
// TODO: Refactor once `getValidatedNode` is removed
/* Once we completely remove the deprecated `getValidatedNode()` method,
we can change this to return a clone to prevent accidental mutations,
e.g.:
return [...this.references];
*/

return this.references;
}

getSignedReferences() {
return [...this.signedReferences];
Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ export interface Reference {
validationError?: Error;

getValidatedNode(xpathSelector?: string): Node | null;

signedReference?: string;
}

/** Implement this to create a new CanonicalizationOrTransformationAlgorithm */
Expand Down