PPT-2536: fix SAML authentication bypass (unsigned assertions accepted) - #14
Merged
Conversation
Real clients use SAML, so forged-assertion handling cannot rest on assumption. Adds spec/spec_helpers/saml_fixtures.cr — a factory that mints SAML 2.0 Responses with CURRENT timestamps and signs them via SAML::XMLSecurity.sign_document, plus two throwaway self-signed keypairs in spec/fixtures/saml/ (a legitimate IdP and a different "attacker" one). Generated rather than canned because crystal-saml's shipped responses expired years ago; its own specs only use them with allowed_clock_drift of 10 years, which auth.cr does not set. Loosening production validation to make a test pass would defeat the point. Why this is auth.cr's problem and not the shard's: crystal-saml's validate_signature does `return true` when the document has no <ds:Signature> at all, and the crypto check only runs when want_signature_validated is set. auth.cr gates BOTH flags on the strat carrying a cert/fingerprint: want_assertions_signed: strat.idp_cert.presence || ... ? true : false want_signature_validated: strat.idp_cert.presence || ... ? true : false so whether a forgery is refused depends on our configuration, not the library. Three cases against a cert-configured strat: * correctly signed by the IdP cert -> accepted * signed by a different valid keypair -> rejected, no user linked * entirely unsigned -> rejected, no user linked Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first cut asserted that a forged assertion redirects to /auth/failure. That is only one valid way to reject: a 4xx is equally correct, and the assertion produced `nil` (no Location) for the attacker case and `false` (some other Location) for the unsigned case — so the failures said nothing about whether a forgery actually succeeded. Assert the load-bearing invariant instead: no UserAuthLookup is created and the response is not a success. On failure, report status, Location, whether a lookup appeared, and a body excerpt, so CI distinguishes "my test was wrong" from "a forged assertion was accepted" without guesswork. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes an authentication bypass. A SAML Response with NO signature was
accepted: status 303, redirect to /, and a UserAuthLookup provisioned — i.e.
anyone able to POST to the ACS could log in as an identity of their choosing.
Nothing in the stack prevented it:
* crystal-saml's validate_signature does `return true` when the document
contains no <ds:Signature> at all
* want_assertions_signed is only used to advertise WantAssertionsSigned in
our SP metadata; no code reads it during validation
* want_signature_validated gates the crypto check, but that is only reached
once a signature exists, and we derive it from cert/fingerprint presence —
so a strat with neither disabled verification completely
This is a REGRESSION against the service being replaced: ruby-saml's
validate_signed_elements rejects zero signature nodes unconditionally
(`!signed_elements.empty?`), regardless of want_assertions_signed, and
separately requires the Assertion to be signed when the SP asks for it.
Two guards before the assertion reaches the engine:
1. fail closed when the strat has no idp_cert and no idp_cert_fingerprint —
with no trust anchor, "accept" means "trust anyone who reaches the ACS".
BOTH dev-server SAML strats are currently configured this way.
2. require a signature to be present, capped at two nodes, and covering the
Response or the Assertion — mirroring ruby-saml.
Four specs cover it: correctly signed accepted; attacker-signed rejected;
unsigned rejected; no-trust-anchor rejected even when signed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Security fix
A SAML Response with no signature at all was accepted:
303→/, with aUserAuthLookupprovisioned. Anyone able to POST to the ACS could log in as an identity of their choosing.Nothing in the stack prevented it:
crystal-saml'svalidate_signaturedoesreturn truewhen the document contains no<ds:Signature>want_assertions_signedis only used to advertiseWantAssertionsSignedin our SP metadata — no code reads it during validationwant_signature_validatedgates the crypto check, but that is only reached once a signature exists, and we derive it from cert/fingerprint presence — so a strat with neither disabled verification entirelyThis was a regression against the service being replaced.
ruby-saml 1.18.1rejects on two independent grounds:validate_signed_elementsrefuses zero signature nodes unconditionally (!signed_elements.empty?), andvalidate_signaturebails whenget_fingerprintis nil (no cert and no fingerprint configured).The fix
Two guards before the assertion reaches the engine:
idp_certnoridp_cert_fingerprintrefuses to authenticate. With nothing to verify against, "accept" means "trust anyone who reaches the ACS".ResponseorAssertion, mirroringruby-saml'svalidate_signed_elements.Equal to ruby-saml on every axis; stricter on one (ruby-saml will proceed to attempt validation with no anchor, we refuse outright).
Coverage
New
spec/spec_helpers/saml_fixtures.crmints SAML 2.0 Responses with current timestamps and signs them viaSAML::XMLSecurity.sign_document, with two throwaway keypairs (legitimate IdP + attacker).Generated rather than canned because
crystal-saml's fixtures expired years ago and its own specs only use them with a 10-yearallowed_clock_driftthat auth.cr does not set.172 examples, 0 failures. CI and Build both green.Operational note
Both dev-server SAML strats have
idp_cert=ABSENTandidp_cert_fingerprint=ABSENT. They are already non-functional under Ruby (which rejects for the same reason), so this changes nothing operationally — it stops auth.cr succeeding insecurely where Ruby correctly fails.Worth reporting upstream:
crystal-samltreating an unsigned document as validly signed affects anything built on it.🤖 Generated with Claude Code