-
Notifications
You must be signed in to change notification settings - Fork 189
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.
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
Merged
Changes from all 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 9899fcd
wip
cjbarth fd36c19
Merge branch 'master' into 6.1.0-changes
cjbarth c5657f6
More proper deprecation
cjbarth 156fde7
Improve typing and getReferences object
cjbarth 04bb79e
Recommended future changes
cjbarth 9ac1547
fix spelling
cjbarth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
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
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.
If the entire reference object is contaminated then your suggested code over at
node-samlis contaminated.This is a super small change that literally takes the exact same data and puts in on the
referenceobject. 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.Uh oh!
There was an error while loading. Please reload this page.
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.
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 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:
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
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.
I don't want to ship a
node-samllibrary that throws deprecation notices. If it is deprecated, we shouldn't be using it innode-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()
That is now a deprecated function due this this PR. So, users, like you did in your change in
node-saml, can keep usinggetReferences()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.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.
Agreed.
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.
@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:
This isolates the signed data into one place.
new API:
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.
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.
@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 tonode-saml? We still use it innode-samlin 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.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.
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.
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.
IMHO that change "rolls back" filled
signedReferencepointers correctly in case of reference list or signature validation error situationsThere 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.
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.
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().
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).