Adjust deprecation to better reflect real-world usage #498
Conversation
…event signature wrapping attacks
# Conflicts: # src/signed-xml.ts
|
@ahacker1-securesaml, I've reviewed your changes to We can, and should include a breaking change in v7 that would reset the Please have a look (@srd90 you're eyes are appreciated too). |
|
Firstly, the whole idea of returning teh Reference object is not sound. Including irrelavent details such as .uri will confuse users into using it for security decisions. The best way to process is to only return the signed data and NOTHING else. Returning the getReferences() would include a bunch of data such as the URI attribute. In v7, I already proposed refactoring the checkSignature method away from the main SignedXML class. The code is too difficult to read at this point, and my v7 PR significantly simplifies the logic |
|
I don't understand the point of doing active maintenance changes (i.e. linting, cleaning up code) in xml-crypto, if they don't give security benefits. It incurs a security risk for a technology that should already be obselete. You should instead start to encourage users to use libraries like libsodium. |
| @@ -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; | |||
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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).
If linting incurs a security risk, the entire ecosystem of software is in deep trouble.
Sure, and as you suggested I checked Google and ChatGPT and read up on it and found that it is a good library that doesn't cover any of the cases of XML signatures, so it would basically require someone re-implement this library if they need to use XML signatures. Now, if they just wanted to sign an entire document, sure, it is easier and safer, but otherwise it is potentially more dangerous because they'd have to rediscover all the same learnings that the xml-crypto project has learned. I'm not saying this project is the be-all-to-end-all. I'm not even saying it is totally secure. I'm not even saying that the XML signature spec is without problems. I am saying that I haven't found a better solution for XML signatures. Signing string, yes, there are better options, but not XML signatures. If there is a better option, we should drop this library in |
Most secure repos is a one that is frozen (immutable). Yes, the entire software ecosystem is in trouble. It is trivial to conduct supply chain attacks against repos. Someone could just make an excuse of "linting", and hide malicious code changes. Easiest way is to not make any unesscary changes, especially for a library that doesn't require code changes. I believe someone else caused: GHSA-2xp3-57p7-qf4v
My point is that if someone has to verify an XML Signature, they are already in deep trouble. They should convince the signer to use something like libsodium instead of xml-crypto. What I strongly believe in is not encouraging new users to use this library. There are very few legitimate use cases i.e. some outdated technologies like SAML. |
|
With regards to linting, I believe projects use something like: https://eslint.org/docs/latest/integrate/nodejs-api |
| @@ -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; | |||
There was a problem hiding this comment.
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.
|
@ahacker1-securesaml , I'm not encouraging users to use SAML. Some are. Many companies I work with only use SAML and SOAP, so there is no option. We can't let the perfect be the enemy of the good. BTW, thank you for the insight on the on the linting. The reality is that we lint as changes come in. We review the changes before they get merged. The linters aren't automatic, nor are they part of the production dependencies. Linting code makes it easier to review and catches mistakes. |
|
As this PR is right now, I see no changes only related to linting. It seems to address the problem with deprecating more than in absolutely necessary, especially given the proposed changes to |
Reason why Node-SAML had deprecation warnings was because it didn't verify XML Signatures securely and use the new APIs. If clients are ensuring that they are verifying XML Sigantures correctly as described in README.md, or are actually signing XML Signatures, then they can just use eslint and disable the warning
Yes, but you are running code when linting on your dev machine. Do you know who wrote the code for linting, maybe it's infected with malware and trying to steal your NPM Credentials. Supply chain attacks are very scary. They love to target underfunded, less maintained, but critical utility libraries for the world. See xz-utils for an example. |
Currently the entire
getReferences()API is deprecated, but in real-world using, there is no problem with much of the data coming from this API. Only the actually XML itself is problematic. Thus, make sure to deprecate only the property that is problematic.