bumped xml-crypto from 1.5.3 to 2.0.0

[keiferc]

Patches signature validation bypass using injected HMAC-SHA1 signature. Patch is rolled out in xml-crypto 2.0.0

[cjbarth]

I note that this is a semver major change. Are there any API or behavioral changes that we depend on that were changed? What is more, since our default is HMAC-SHA1, should we change our default, or even deprecate SHA1 (which would be a semver major for us)?

[keiferc]

The xml-crypto semver major change is due to how the HMAC and public key digital signature methods use the same variable to provide the key. Since HMAC-SHA1 is less commonly used (due to folks moving away from SHA1), the xml-crypto folks decided to disable using HMAC-SHA1 as the default, hence the breaking change.

Deprecating SHA1 would be ideal, simply because SHA256 is more collision resistant. Therefore, for the long-term, I would recommend using RSA-SHA256 as the signature algorithm, as it is more likely than HMAC-SHA1 to stay supported in the near future.

For the short-term, it is possible to upgrade xml-crypto to 2.0.0 with minimal changes. The API remains the same. The only difference is that HMAC-SHA1 is disabled by default and its enablement would disable xml-crypto's digital signature algorithms (note: having both enabled is what opens a vulnerability). From what I can tell, passport-saml doesn't use xml-crypto's digital signature algorithms, so explicitly enabling HMAC-SHA1 will not break anything. If passport-saml uses both xml-crypto's HMAC-SHA1 and digital signature algorithms, then passport-saml is subject to a severe vulnerability and needs to switch to using one of the two.

To enable HMAC-SHA1 with xml-crypto 2.0.0, you can just add a SignedXml.enableHMAC() e.g. (require('xml-crypto').SignedXml.enableHMAC();) and thus safely upgrade xml-crypto to 2.0.0.

[damien-git]

Shouldn't yarn.lock and package-lock.json be updated at the same time, as with the dependabot PR #471 ?

[cjbarth]

Yes @damien-git , package-lock.json should be updated, but we should delete yarn.lock as we don't support that in this project (it hasn't been kept up to date).

[cjbarth]

• Need to explicitly enable support for SHA1
• Need to update package-lock.json
• Need to delete yarn.lock

[markstos]

I'm eager to get this security update out as well, so I'm starting on the changes recommended by @cjbarth now.

[markstos]

I've pushed package-lock.json / yarn.lock updates.

Testing found that enableHMAC() was wrong for this project, causing 44 tests to fail. It will not be enabled by default.

I'm going to get some lunch and will release within a couple of hours.

[keiferc]

@cjbarth I may be missing something, but I have not found any explicit setting of HMAC-SHA1 as the default signing algorithm in passport-saml. From what I can tell, passport-saml uses the xml-crypto default of RSA-SHA1 and only enables RSA signing (as seen in algorithms.js and in the README). Could you point me to where I can find the HMAC setting?

[cjbarth]

@keiferc It was my mistake. You are correct.