Skip to content

Harden AuthXMLRequest.setPrincipal against arbitrary class instantiation (same pattern as GHSA-wg5r-wc3x-39vc) - #1088

Open
BarakSrour wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
BarakSrour:harden/authxmlrequest-setprincipal-reflection
Open

Harden AuthXMLRequest.setPrincipal against arbitrary class instantiation (same pattern as GHSA-wg5r-wc3x-39vc)#1088
BarakSrour wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
BarakSrour:harden/authxmlrequest-setprincipal-reflection

Conversation

@BarakSrour

@BarakSrour BarakSrour commented Jul 29, 2026

Copy link
Copy Markdown

What

AuthXMLRequest.setPrincipal(String, String) resolves and instantiates a class named by its className argument:

public void setPrincipal(String className, String principalValue) {
    try {
        Class clName = Class.forName(className);
        principal = (Principal) clName.newInstance();
    } catch (ClassNotFoundException ce) {
    } catch (IllegalAccessException ia) {
    } catch (InstantiationException ie) {
    } catch (Exception e) {
    }
}

This is the same shape AuthXMLUtils.createCustomCallback had before edcf968 (GHSA-wg5r-wc3x-39vc / CVE-2026-62379). This PR applies the same hardening you used there: resolve with initialize=false, and confirm the class is a Principal before newInstance().

Why the existing cast is not a control

The (Principal) cast is evaluated only after newInstance() 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-argument Class.forName also defaults to initialize=true, so a static initializer runs at resolution time even when no instance is ever created. Checked in an isolated JVM:

Class.forName(name) then (Iface) c.newInstance()       ->  [static initializer RAN]
                                                          [constructor RAN]
                                                          ClassCastException
Class.forName(name, false, loader) + isAssignableFrom  ->  rejected, neither ran

Two details specific to this method:

  • Every exception is swallowed by empty catch blocks, including the ClassCastException. Side effects from loading a caller-named class would leave no trace in the logs at all — unlike createCustomCallback, which at least reached debug.error.
  • principalValue is 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:

  • AuthXMLRequest is referenced by five files, all in openam-core — itself, AuthXMLRequestParser, AuthXMLResponse, AuthXMLHandler and AuthUtils — and none calls setPrincipal. I checked the whole monorepo: the other setPrincipal matches are com.iplanet.ums.* setPrincipal(Principal) plus unrelated single-argument methods in openam-federation, openam-sts and openam-auth-windowsdesktopsso.
  • With no reachable path there is no attacker model, so no CVE and no advisory are warranted, and I am not requesting either. This is defence in depth on a public method that is one caller away from being live, in a class that reached Class.forName(caller_string).newInstance() under CVE-2026-62379.
  • Limits of my own claim: reachability was assessed statically against this repository and the published openam-core 16.1.1 / 16.1.2 artifacts. I have no visibility into downstream integrations, and setPrincipal is public on a public class, 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 public method on a public class 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 the AuthXMLUtilsSecurityTest added in edcf968 (TestNG + AssertJ, probe class with static flags). It asserts a non-Principal class is neither initialized nor instantiated, and that a legitimate Principal still 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 forName does. 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-argument Class.forName shape 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.

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.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants