Introduce new .signedReferences property of signature to help prevent signature wrapping attacks. #489
Conversation
|
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? |
markstos
changed the title
markstos
changed the title
|
|
||
| # Changelog | ||
|
|
||
| - Introduced new .signedReferences property of signature to help prevent signature wrapping attacks. |
There was a problem hiding this comment.
The changelog is automatically generated from the title of the PR. Any additional notes should go in the README.
| @@ -87,6 +88,8 @@ export class SignedXml { | |||
| "http://www.w3.org/2000/09/xmldsig#enveloped-signature": envelopedSignatures.EnvelopedSignature, | |||
| }; | |||
|
|
|||
| // TODO: In V7.x we may consider deprecating sha1 | |||
There was a problem hiding this comment.
I'm all for putting in a deprecation warning now and removing it later on. Either way.
There was a problem hiding this comment.
It's only temporary for some SAML libraries to use.
There was a problem hiding this comment.
Sure. I was thinking there is no reason not to throw a deprecation notice now if someone uses sha1, no need to wait for 7.x to throw the notice.
| @@ -154,6 +160,9 @@ export class SignedXml { | |||
| this.CanonicalizationAlgorithms; | |||
| this.HashAlgorithms; | |||
| this.SignatureAlgorithms; | |||
| // this populates only after verifying the signature | |||
| // array of bytes that are cryptographically authenticated | |||
| this.signedReferences = []; // TODO: should we rename this to something better. | |||
There was a problem hiding this comment.
That seems like a descriptive name to me. What other thoughts do you have?
There was a problem hiding this comment.
yea, only problem is that people might confuse it to be a array of Reference objects, when it's an array of bytes.
Something like protectedBytes/protectedData could be better.
There was a problem hiding this comment.
Chatty suggested options like authenticatedData and signatureVerifiedBytes. I have no problems with log descriptive names. I think, at a minimum, we should make sure the type is correct; right now it is untyped.
| const verified = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); | ||
| // false case | ||
| // reset the signedReferences back to empty array | ||
| // I would have preferred to start by verifying the signedInfoCanon first, if that's OK |
There was a problem hiding this comment.
I feel like our tests would catch if this causes breaking changes. Did one of the existing tests fail?
There was a problem hiding this comment.
all test cases passed
There was a problem hiding this comment.
If all tests past when verifying signedInfoCanon first, I'm ok if you wanted to make that change now. If you still think it is too risky, I understand, though I'd be interested in what test might give us reassurance.
No description provided.