Harden AuthXMLRequest.setPrincipal against arbitrary class instantiation (same pattern as GHSA-wg5r-wc3x-39vc) - #1088
Open
BarakSrour wants to merge 2 commits into
Conversation
Resolve the caller-named class with initialize=false and confirm it is a java.security.Principal before instantiating it. The (Principal) cast is evaluated only after newInstance() has already run the class's static initializer and constructor, so it is not a control. Same hardening as AuthXMLUtils.createCustomCallback in edcf968 (GHSA-wg5r-wc3x-39vc / CVE-2026-62379). setPrincipal has no callers in the tree, so this is defence in depth on a public method, not a fix for a reachable vulnerability.
Asserts that a non-Principal class is neither initialized nor instantiated, and that a legitimate Principal still resolves. The probe's flags deliberately live in a separate holder class: reading or writing a static field triggers that class's initialization, so flags held on the probe itself would be set by the test's own reset and the was-it-initialized assertion would pass even against the unhardened code. Verified that both assertions fail against the 1-arg Class.forName shape.
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.
What
AuthXMLRequest.setPrincipal(String, String)resolves and instantiates a class named by itsclassNameargument:This is the same shape
AuthXMLUtils.createCustomCallbackhad before edcf968 (GHSA-wg5r-wc3x-39vc / CVE-2026-62379). This PR applies the same hardening you used there: resolve withinitialize=false, and confirm the class is aPrincipalbeforenewInstance().Why the existing cast is not a control
The
(Principal)cast is evaluated only afternewInstance()returns, so the named class's static initializer and no-arg constructor have both already run by the time the cast can reject it. The one-argumentClass.forNamealso defaults toinitialize=true, so a static initializer runs at resolution time even when no instance is ever created. Checked in an isolated JVM:Two details specific to this method:
catchblocks, including theClassCastException. Side effects from loading a caller-named class would leave no trace in the logs at all — unlikecreateCustomCallback, which at least reacheddebug.error.principalValueis never used. It appears in the signature and the Javadoc, and nowhere in the body.This is not a vulnerability report and needs no advisory
Stating this plainly so it is not routed as a security disclosure:
AuthXMLRequestis referenced by five files, all inopenam-core— itself,AuthXMLRequestParser,AuthXMLResponse,AuthXMLHandlerandAuthUtils— and none callssetPrincipal. I checked the whole monorepo: the othersetPrincipalmatches arecom.iplanet.ums.*setPrincipal(Principal)plus unrelated single-argument methods inopenam-federation,openam-stsandopenam-auth-windowsdesktopsso.publicmethod that is one caller away from being live, in a class that reachedClass.forName(caller_string).newInstance()under CVE-2026-62379.openam-core16.1.1 / 16.1.2 artifacts. I have no visibility into downstream integrations, andsetPrincipalispublicon apublicclass, so an external caller may exist.You may prefer to delete it instead
The method is unreachable in-tree and ignores its second parameter, so removing it outright is arguably the better change and I am happy to switch this PR to a deletion. I proposed hardening because deleting a
publicmethod on apublicclass is a breaking change for any downstream caller, and that trade-off is yours. Say which you would prefer.Testing
Adds
AuthXMLRequestSecurityTest, following the structure of theAuthXMLUtilsSecurityTestadded in edcf968 (TestNG + AssertJ, probe class with static flags). It asserts a non-Principalclass is neither initialized nor instantiated, and that a legitimatePrincipalstill resolves.One design note worth flagging, because the obvious version of this test silently passes against the unhardened code: the probe's flags live in a separate holder class. Reading or writing a static field triggers that class's initialization, so flags stored on the probe itself get set by the test's own reset, and the "was it initialized" assertion then passes no matter what
forNamedoes. With the flags held elsewhere the probe is only ever named by a class literal, which does not trigger initialization. I verified both assertions fail against the 1-argumentClass.forNameshape and pass with this change.I have not been able to run the module's full test suite locally (the build needs JDK 17 and this host has 16), so please treat CI as the authority on that.