yaronn
V6 minor
Since the effort to move to 6.1 or a full, breaking, 7.x would be the same, why not release this as the 7.x and actually make the breaking changes so that there is no doubt that the consumer got it right?
@ahacker1-securesaml , if the changes I've just pushed look good, I'll land this and release.
I've tried to put up some edits to the README, but it appears that you've restricted my ability to push to this branch. Was that intentional @ahacker1-securesaml ?
@ahacker1-securesaml , I've pushed up some changes. I'd like to get at least one test for this. I've been trying to inspect the other tests and I haven't actually found a case where .signedReferences is populated. Do you happen to have a test? That would speed this up. Even just the rough structure or sample code you used for local testing. I can make it fit with this project.
@cjbarth i really appreciate the efforts spent reviewing the changes.
However, I have deadlines to meet. The code currently just works with other SAML libraries. We want to ship it ASAP.
If there are other feedback, please leave that to v7. There we have more time to discuss
However, I have deadlines to meet. The code currently just works with other SAML libraries. We want to ship it ASAP.
Appreciate the collaboration here, but the bottom line priority here is security of node-saml, unconstrained by some organization's internal deadlines.
Yea, but the new changes are secure. AND doesn't break anyone I have spent time perfecting both of those cases, and I really want to get it done.
Now, we are discussing about some cosmetic changes like README.md, test cases, ... I just don't feel that these discussions are relevant, as much, given this is a security fix.
Furthermore, if we want to discuss later cosmetics, discussin v7
I don't have the time to keep on arguing on these small cosmetic changes, (note each response takes like 1 day to resolve), we'll be sidelined by months if we keep on arguing on the non-security parts.
Furthermore, I have been stuck in a loop trying to argue for this addition for months and months. I really feels like no progress is being made.
Summary of the security relevant parts:
getReferences() returns an Array of Reference objects. The Reference objects cannot be used for security decisions:
- First, because it is loaded from an unauthenticated XML Signature
- The Reference Object also exposes some APIs attributes that are unsuitable for security decisions.
I am not going to explain the intracacies of the problems with using the Refernece object. Please trust the expert here, when he says so.
Hence we are deprecating the getReferences() API.
-
A new backwards compatible API has been added: .signedReferences without breaking the original getReferences():
-
It returns an array of bytes that were authenticated. If you use only the .signedReferences, there is nothing that can go wrong. (Compared to using the Reference Object).
-
You can review the source code that the .signedReferences API only contains authenticated data, instead of unauthenticated data.
FAQ: Why not create a new major release to remove the .getReferences() API completely (and not push a minor release):
-
We want dependents to be able to use the new API ASAP, since it is a security feature.
-
It takes time to upgrade to v7 for the reasons:
One has to write the migration guide, write the underlying code, ensure it doesn't break things ... in V7. No guarentees here.
Currently the minor release is just + 5 lines of code, and done!
-
It is breaking change that will prevent dependents from upgrading
-
Some users might rely on this .getReferences() API for signing an XML signature not verifying an XML Signature
-
Users might complain, about upgrading, hence they will not upgrade to v7 at all. Best to just use a minor release so everyone can upgrade to optionally use the secure feature.
-
if we can push breaking changes, I would rather redesign the API as a whole.
@ahacker1-securesaml and @srd90 , have a look at the latest code I pushed. It is linted and tested and has updated comments to reflect our discussion here. I'm content to land this and release it as 6.1 if you both agree.
@ahacker1-securesaml , just one more thought. We have getReferences() as a function that simply returns an array to make sure that no consumer of the library can write to that array. Would a similar pattern be appropriate here to make sure that a consumer can't accidentally or intentionally modify .signedReferences themselves? We could make it an internal array and then expose getSignedReferences(). Such a refactor would only take a moment on my end. To make doubly sure, I'd probably return [...this.signedReferences] to get a copy, which is even better than what we do now.
@ahacker1-securesaml or @srd90 , just so I make sure I'm not missing anything, can you comment on the subtle differences between the new .getSignedReferences() API and the previously recommended technique of using .validateElementAgainstReferences() API. It seems the former is a list of the validated references and the latter is a function to take any xpath, look it up in the document, and see if that specific element passes validation.
I'm not saying that we shouldn't have the .getSignedReferences() API, but I'm wondering if we should also deprecate .validateElementAgainstReference() API. I'm sure that API was written specifically to help people validate references, though, admittedly, in a not very elegant way.
I'm trying to clean up all the internal uses of getReferences() so that we don't have any internal deprecation warnings, and I want to make sure I get this 100% correct.
When you use: validateElementAgainstReferences(myElement, doc).
This ends up crytographically authenticating: https://github.com/node-saml/xml-crypto/blob/cc24755d3b170ba6991a573c8091b96a341405c7/src/signed-xml.ts#L499C13-L499C21
canonXml. See the code on how canonXml is generated.
However, the client will ultimately processing myElement, not canonXml. These two are different, and there have been canonicalization attacks exploiting this. I.e. note one of the many differences an attacker can exploit is that the canonXml stripes away comments from myElement.
When you use:
myData = getSignedReferences(), it returns canonXml, which is the same utf-8 encoded string we authenticated previously. parse(myData)
You then process solely the same result, so there is no differential an attacker can exploit. I.e. you use the exact same thing that is verified. Now the only possible vulnerability is inside how the canonXml is verified. For example actual vulnerabilities within the crypto, hash collision attack. That is outside our domain.
Deprecation
Yes, we should deprecate the validateElementAgainstReferences. It would be best to remove it, because there's going to be lots of vulnerabilities stemming from this.
I'm trying to clean up all the internal uses of getReferences() so that we don't have any internal deprecation warnings, and I want to make sure I get this 100% correct.
I'm not against using getReferences() as a sanity check, i.e. to check that the uri supposedly matches. The problem is that people are using it as their sole security measure, which would be insecure.
The secure way is documented in README.md. I'm against cleaning up the calls of getReferences() for now. They're still suitable for the sanity checks. We should focus solely on using the getSignedReferences() API. It's a bigger priority to do the secure thing than to clean up the outdated sanity checks
I'll turn my attention back to node-saml then.
So then, getReferences() shouldn't have been deprecated?
There needs to be some way of getting the user to use the new API to verify XML Signatures. Technically, users can still use the old API, but they must use the new .getSignedReferences API to be secure. If they can't use the old getReferences() API, then they are forced to use the new API. Hence, the deprecation serves as a warning message.
I'm not sure I fully understand. If the .reference contains non-canonical data that is potentially dangerous, why would we say they can use it at all? Are you referring to the other properties that are returned? If so, we can add a test to make sure there is a 1-1 correspondence between getReferences() and getSignedReferences() and then deprecate only the property of getReferences() that returns the non-canonical reference. In fact, we can actually just make the v7 simply change the value of the property to be the same as is in getSignedReferences().
Or, I could see a very strong argument for doing that in a minor release because what we're returning now (the non-canonical data) is dangerous and wrong, but the canonical data would be a minor, and typically non-consequential change for most users.
Even better would be to deprecate the references[].getValidatedNode() and add a references[].getSignedReference() property that would just return the canonicalized data, the same data that would be found in getSignedReferences(). That would be a very simple change that wouldn't break anyone and would reduce the instances of dealing with deprecations that some users are facing.
What am I missing here?
.reference contains non-canonical data that is potentially dangerous, why would we say they can use it at all? Are you referring to the other properties that are returned?
They might use it as a sanity check i.e. the other properties. There's a misunderstanding on your part, the getReferences() API doesn't return any interesting data that can be used for authentication. I.e. it returns a Reference object, you can't use that anyways.
If so, we can add a test to make sure there is a 1-1 correspondence between getReferences() and getSignedReferences() and then deprecate only the property of getReferences() that returns the non-canonical reference.
Again the way that getReferences() is derived is fundamentally incorrect, i.e. it starts by loading untrusted data. There's no point in trying to fix this API. Just don't use it for security.
Or, I could see a very strong argument for doing that in a minor release because what we're returning now (the non-canonical data) is dangerous and wrong, but the canonical data would be a minor, and typically non-consequential change for most users.
We are not returning the non-canonical data. To be accurate the getReferences() API returns garbage data i.e. the Reference class, which has nothing to do with the canonXml. We want to force the user to only use the canonXml.
Even better would be to deprecate the references[].getValidatedNode() and add a references[].getSignedReference() property that would just return the canonicalized data, the same data that would be found in getSignedReferences(). That would be a very simple change that wouldn't break anyone and would reduce the instances of dealing with deprecations that some users are facing.
No point. The whole point of this was to start getting users to use canonXml and not untrusted data. It's going to take work into refactoring the logic, and the deprecation serves as a warning.
You proposed adding an obscure API that no one will even see. Vast majority of users will continue on their insecure methods i.e. using the getReferences() API, and there won't be any security benefit.
To be frank the current API design in xml-crypto you inherited from @yaronn is insecure. It tries to mix signing and verifying and does nothing correct. Now, it's quite difficult to secure, without breaking anyone.
Overall, can you trust my judgement on the issue. I am trying to reduce the friction, so we can release it ASAP. I have deadlines too and other research to finish. If you want to argue, then we can do it after the security fixes land
Anyone complaining about deprecation warnings should just be using NaCl. It's designed and maintained by actual cryptographic experts. Just lead the complainers there.
We should be placing a huge warning on top of the library to switch to NaCl box, and start deprecating the library. It's actively endangering newcomers that this is a "secure" library. The only reason users should be using the library is for SAML. And nothing else.
Ok, and the only reason I care about this library is for SAML applications, like node-saml. I don't want to ship node-saml where during its normal, expected, runtime operations, it throws deprecation warnings. That is the standard that should be expected. It can't be deprecated if there is no alternative.
So, for the sake of anything related to this discussion, we can put a note at the top of the README directing people to use libsodium, but we have to figure out a way to not have deprecation warnings with normally running secure-as-we-can-make-it-right-now code.