Harden SAML validation and add opt-in security defenses#9
Merged
Conversation
Fix two correctness bugs in encrypted-assertion handling and add several backward-compatible, opt-in security controls. All changes are additive: existing public signatures, default values, and behavior are unchanged, so integrators see no difference unless they explicitly opt in. Bug fixes: - SamlResponse.decryptAssertion: correct a wrong-variable guard that let a failed decryption dereference null and throw an uncaught NullPointerException out of the constructor; it now raises a typed ValidationException. - SamlResponse.validateTimestamps: read the decrypted document so an encrypted assertion's Conditions (NotBefore/NotOnOrAfter) are actually validated instead of being silently skipped. New opt-in controls (defaults preserve current behavior): - Metadata signature validation: wire the existing Util.validateMetadataSign into new Saml2Settings.validateMetadata(...) and IdPMetadataParser.parse*(...) overloads that accept a trusted signing certificate (must be provisioned out-of-band; documented in Javadoc). - Replay protection: new ReplayCache SPI plus InMemoryReplayCache; enable via Saml2Settings/Auth.setReplayCache(...). Detects replayed assertion and logout-message IDs. - RSA-OAEP key transport for outbound NameID encryption and an inbound decrypt key-transport algorithm allow-list, both configurable; defaults preserve RSA-1.5 behavior. Algorithm rejection reuses the existing generic decryption-failure path to avoid introducing a padding-oracle side channel. - Saml2Settings.getSecurityWarnings() surfaces weak-configuration warnings and is logged at settings build time (checkSettings() is unchanged). Docs: correct the security-defaults note in CLAUDE.md and harden the RelayState redirect example in README and the JSP sample. Adds tests for every change; full suite passes (core 415, toolkit 92).
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
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.
Summary
Fixes two correctness bugs in encrypted-assertion handling and adds several
backward-compatible, opt-in security controls surfaced by a security review
of the SP validation paths.
All changes are additive: no existing public method signature, default
value, or runtime behavior changes. Integrators see no difference unless they
explicitly opt in. Full test suite passes (core 415, toolkit 92).
Bug fixes
SamlResponse.decryptAssertion— a guard checked the wrong node list(
encryptedDataNodes, already proven non-empty) instead of thepost-decryption
AssertionDataNodes. BecauseUtil.decryptElementswallowsdecryption failures, a failed decryption left no
saml:Assertion, and thecode dereferenced
null, throwing an uncaughtNullPointerExceptionout ofthe
SamlResponseconstructor (whichAuth.processResponsedoes not wrap).It now raises a typed
ValidationException.SamlResponse.validateTimestamps— it read the raw (pre-decryption)document, so for an encrypted assertion the
Conditions(
NotBefore/NotOnOrAfter) live inside the ciphertext,getElementsByTagNameNSmatched nothing, and the temporal-validity window was silently never
checked. It now reads the decrypted document like every other accessor.
New opt-in security controls (defaults preserve current behavior)
unused)
Util.validateMetadataSigninto new overloads:Saml2Settings.validateMetadata(xml, cert[, fingerprint, alg, rejectDeprecatedAlg])and
IdPMetadataParser.parseXML/parseFileXML/parseRemoteXML(..., X509Certificate).Signature validation runs only when a trust anchor is supplied.
ReplayCacheSPI + thread-safeInMemoryReplayCache(TTL, no background threads). Enable withauth.setReplayCache(new InMemoryReplayCache())orsettings.setReplayCache(...). Detects replayed assertion IDs (SSO) andlogout-message IDs (SLO); IDs are recorded only after a message is otherwise
fully valid, so invalid messages cannot poison the cache. Default
null=disabled (unchanged behavior).
encryption algorithm is now selectable via
onelogin.saml2.security.nameid_encryption_algorithm(defaultrsa-1_5,unchanged); an optional inbound allow-list
onelogin.saml2.security.allowed_key_transport_algorithms(default empty =permissive) lets deployments reject legacy PKCS#1 v1.5 key transport. A
rejected algorithm reuses the existing generic decryption-failure path, so it
is indistinguishable from a wrong-key failure (no new padding-oracle side
channel).
Saml2Settings.getSecurityWarnings()reportsweak-but-valid settings (strict off, weak fingerprint algorithm, deprecated
algorithms not rejected, assertions/messages not required signed) and is
logged at settings build time.
checkSettings()is left unchanged.Docs
CLAUDE.md(deprecated algorithms arenot rejected by default; fingerprint default is
sha1) and add productionrecommendations.
RelayStateredirect example inREADME.mdand the JSP sample(fix a reference-equality guard and add an allow-list warning to prevent an
open redirect in copied integration code).
Backward compatibility
fields / new methods; existing callers and the ~27 tests that validate the
same fixture multiple times pass unmodified.
inbound, no replay cache, no metadata signature check).
Testing
reproductions, metadata signature validation,
getSecurityWarnings(),RSA-OAEP output + allow-list rejection,
InMemoryReplayCache(including aconcurrency smoke test), and SSO/SLO replay detection.
mvn -B test: core 415, toolkit 92, 0 failures/errors.