Skip to content
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

[BUG] Signature calculation is inconsistent for different line endings #511

Closed
mhassan1 opened this issue Dec 11, 2020 · 0 comments · Fixed by #512
Closed

[BUG] Signature calculation is inconsistent for different line endings #511

mhassan1 opened this issue Dec 11, 2020 · 0 comments · Fixed by #512
Labels

Comments

@mhassan1
Copy link
Contributor

In #498, we added newline normalization before signature validation in validateSignatureForCert. The suggestion for this fix came from #431 (comment); however, I don't believe this fix goes deep enough into newline normalization.

While this fix normalizes newlines right before the signature validation, it doesn't normalize newlines in the XML payload that is passed to sig.loadSignature. As a result, the sig.getCanonSignedInfoXml that is part of sig.checkSignature encodes carriage returns into 
, and the signature computed from that canonicalized SignedInfo XML is incorrect.

** Spec-driven development **

This is related to the XML spec for newlines mentioned here: #431 (comment)

** Community development model **

PR to come

To Reproduce

  1. Add a newline to response.root-signed.assertion-signed.xml inside the Signature block.
  2. Re-run test-signatures.js and see that R1A - both signed => valid fails, as expected, since the signature should be different now that the XML is different.
  3. Compute the correct digests and signatures for this modified file and update them in DigestValue and SignatureValue
  4. Re-run test-signatures.js and see that all tests pass except for CRLF line endings and CR line endings.

Expected behavior
XML payloads with non-LF line endings should have the same digest and signature values as the same XML payload with LF line endings, and it is up to the XML processor to normalize line endings to ensure that (#431 (comment))

Environment

  • Node.js version: 14.15.1
  • passport-saml version: 2.0.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant