fix!: validate signatures against live message to prevent XSW#6
Merged
Conversation
Signature validation flattened the message signature into a value/algorithm/data DTO and verified that. For HTTP-POST (enveloped) signatures this reconstructed a detached SignatureStringReader and checked only that the signature covered SignedInfo - dropping LightSAML's XML Signature Wrapping defense. An attacker could wrap a genuinely signed Response so the valid signature covered a hidden copy while a forged assertion was consumed, and it validated. The flaw was reachable both via handle*(validate: true) and the public validateSignature() method. Validate the live SamlMessage instead, delegating to the signature reader's own validate(), which runs the wrapping check that a flattened DTO cannot express. extractSignature() no longer flattens enveloped XML signatures, so validateSignature() no longer reports a wrapped POST message as valid. Resolves litesaml#5
Validation was split in two: validateMessageSignature(SamlMessage) for the live message (used by handle*(validate: true)) and validateSignature(Message) for after-the-fact validation from the flattened DTO. The DTO-based path only worked for detached Redirect signatures — for POST it could never run the XML Signature Wrapping check, which needs the live document. Keep only the live-message method, renamed to validateSignature(SamlMessage). Drop the public DTO-based validateSignature(), extractSignature(), the Signature DTO, and the now-unused signature field on message DTOs. Signatures are verified inline during handle*(validate: true); the message DTOs are pure parsed data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
william-suppo
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #5
POST-binding signature validation (added in 2.1.0, #4) is vulnerable to XML Signature Wrapping (XSW): an attacker holding one genuinely signed Response can wrap it so the valid signature covers a hidden copy while the application consumes a forged assertion, and
handle*(validate: true)reports it as valid. I added a test for this, which fails on v2.1.0 but is fixed here.The vulnerability
2.1.0 validates by flattening the message signature into a
value/algorithm/dataDTO and verifying that. For an enveloped POST signature this reconstructs a detachedSignatureStringReaderand checks only that the signature coversSignedInfo— it never runs LightSAML's XSW defense (SignatureXmlReader::validate()→assertNoXmlSignatureWrapping()), because a flattened DTO has no way to express the document structure that check inspects (reference targets, duplicate IDs). The hole is reachable both viahandle*(validate: true)and the publicvalidateSignature().The fix
Validate the live LightSAML message instead of a flattened DTO, delegating to the signature reader's own
validate()—SignatureStringReaderfor Redirect,SignatureXmlReaderfor POST.SignatureXmlReader::validate()runs the wrapping check that a flattened representation cannot. As a result, validation now needs the live message (not the DTO).Breaking changes
validateSignature()now takes the liveSamlMessage, not a message DTO.extractSignature().handle*(validate: true, issuer: ...), so the trusted issuer must be supplied when handling the message rather than read from the returned DTO and validated afterward.Signatureclass and thesignatureproperty on message DTOs are gone. Message DTOs are now pure parsed data.