Open
Conversation
Previously, unsigned responses with an empty Destination were accepted without any Destination validation. This allowed an attacker to replay a captured response to a different SP, since the Destination was the only non-cryptographic binding to the intended recipient. Now Destination is always required and validated, regardless of whether the response is signed. The SAML spec says Destination MUST be present on signed responses and SHOULD be present otherwise — we upgrade the SHOULD to a MUST for defense in depth. Fixes #12
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
1 participant
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.
The
Destinationcheck inparseResponseis gated onresponseHasSignature || response.Destination != "". When the response envelope is unsigned (common - many IdPs only sign the assertion) andDestinationis absent, validation is not performed. This allows captured responses to be replayed to different SPs.For unsigned responses,
Destinationis the only non-cryptographic binding to the intended SP. Without this check, an attacker who captures a response can replay it to any SP that trusts the same IdP.Fix: Require
Destinationunconditionally. The SAML spec saysDestinationMUST be present for signed responses and SHOULD be present otherwise. This PR enforces MUST in both cases for defense-in-depth.Breaking Change: This is technically breaking - unsigned responses without
Destinationthat were previously accepted will now be rejected. However, this configuration is spec non-compliant, represents a real security gap, and most IdPs includeDestinationby default.Changes:
service_provider.go: Remove conditional, always requireDestinationservice_provider_test.go: Update tests, add unsigned-without-Destination testTesting:
Destination: acceptedDestination: acceptedDestination: now rejected (breaking)Destination)Fixes aashh#12