[
https://issues.apache.org/jira/browse/WW-5676?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107149#comment-18107149
]
Lukasz Lenart edited comment on WW-5676 at 8/23/26 4:07 PM:
------------------------------------------------------------
Investigated. The answer is *no change* — and the premise this ticket was filed
on turns out to be wrong.
h3. Array {{clone()}} is not reflectively reachable
The ticket identified {{clone()}} as the exception whose declaring class is the
array type itself. It is not. Array {{clone()}} is a JVM-internal method: the
JLS gives array types a public {{clone()}}, but it is absent from the
reflection view. Verified on Temurin 17.0.14, 21.0.7 and 25.0.1, for
{{String[]}}, {{java.io.File[]}}, {{int[]}} and {{Object[][]}}:
||probe||result||
|{{getMethods()}}|exactly the 7 public {{java.lang.Object}} methods|
|{{getDeclaredMethods()}}|empty|
|{{getMethod("clone")}}|{{NoSuchMethodException}}|
|{{getFields()}} / {{getField("length")}}|empty / {{NoSuchFieldException}}|
So every member reflectively reachable on an array class declares in
{{java.lang.Object}}.
h3. The package check is therefore unreachable for array targets
{{java.lang.Object}} cannot be configured out of the exclusion list. It is the
built-in default of {{SecurityMemberAccess.excludedClasses}}, and
{{useExcludedClasses}} routes through {{ConfigParseUtil.toNewClassesSet}},
which _accumulates_ onto the existing set instead of replacing it.
{{checkExclusionList}} tests the declaring class before the package, so an
array target is denied at the first check every time. Confirmed end to end
against a real {{SecurityMemberAccess}} configured with the production
{{struts-excluded-classes.xml}} values and a {{java.io.File}} array as the
target: all 9 reachable members denied, with the allowlist both enabled and
disabled.
The "{{java.io.File[]}} escapes the {{java.io}} exclusion" concern is accurate
as a statement about {{toPackageName}} in isolation, but no caller can reach
it. It is not a security gap, so this stays public — nothing to report
privately.
h3. Answers to the four questions
# Is the exclusion-path gap exploitable? No — unreachable, as above.
# Is the allowlist loosening acceptable? Moot, and it would be pure cost. Since
the exclusion path gains nothing, resolving arrays to the element package
tightens nothing while genuinely loosening the allowlist:
{{struts.allowlist.packageNames=com.app}} would start implicitly allowlisting
{{com.app.Thing[]}}, which today needs an explicit {{struts.allowlist.classes}}
entry.
# Should {{void}} and primitives be treated separately? Moot —
{{target.getClass()}} is never primitive and {{member.getDeclaringClass()}} is
never primitive, so neither can appear at either call site.
# Migration-guide note? Not needed. No behaviour change.
Keeping the empty package also keeps {{toPackageName}} in agreement with
{{checkDefaultPackageAccess}}'s raw {{getPackage() == null}} reading, which is
what WW-5677 wants to unify.
h3. What was done instead
PR [#1858|https://github.com/apache/struts/pull/1858] records the reasoning
where it can rot loudly rather than quietly:
* new {{SecurityMemberAccessArrayTargetTest}} (4 tests) pinning both facts the
decision rests on — the two behavioural ones were mutation-checked by
defaulting {{excludedClasses}} to the empty set, which fails both;
* the comment on {{toPackageName}} no longer states the false premise and
points at that test.
No production behaviour change, no configuration change. The existing
{{arraysAndPrimitivesResolveToTheEmptyPackage}} and
{{toPackageNameMatchesLegacyAcrossClassShapes}} in
{{SecurityMemberAccessPackageMatchingTest}} still pin the behaviour itself and
are untouched.
was (Author: lukaszlenart):
Investigated. The answer is _no change_ — and the premise this ticket was filed
on turns out to be wrong.
h3. Array {{clone()}} is not reflectively reachable
The ticket identified {{clone()}} as the exception whose declaring class is the
array type itself. It is not. Array {{clone()}} is a JVM-internal method: the
JLS gives array types a public {{clone()}}, but it is absent from the
reflection view. Verified on Temurin 17.0.14, 21.0.7 and 25.0.1, for
{{String[]}}, {{java.io.File[]}}, {{int[]}} and {{Object[][]}}:
|| probe || result ||
| {{getMethods()}} | exactly the 7 public {{java.lang.Object}} methods |
| {{getDeclaredMethods()}} | empty |
| {{getMethod("clone")}} | {{NoSuchMethodException}} |
| {{getFields()}} / {{getField("length")}} | empty / {{NoSuchFieldException}} |
So every member reflectively reachable on an array class declares in
{{java.lang.Object}}.
h3. The package check is therefore unreachable for array targets
{{java.lang.Object}} cannot be configured out of the exclusion list. It is the
built-in default of {{SecurityMemberAccess.excludedClasses}}, and
{{useExcludedClasses}} routes through {{ConfigParseUtil.toNewClassesSet}},
which _accumulates_ onto the existing set instead of replacing it.
{{checkExclusionList}} tests the declaring class before the package, so an
array target is denied at the first check every time. Confirmed end to end
against a real {{SecurityMemberAccess}} configured with the production
{{struts-excluded-classes.xml}} values and {{target = new
java.io.File[]\{...\}}}: all 9 reachable members denied, with the allowlist
both enabled and disabled.
The "{{java.io.File[]}} escapes the {{java.io}} exclusion" concern is accurate
as a statement about {{toPackageName}} in isolation, but no caller can reach
it. It is not a security gap, so this stays public — nothing to report
privately.
h3. Answers to the four questions
* *Is the exclusion-path gap exploitable?* No — unreachable, as above.
* *Is the allowlist loosening acceptable?* Moot, and it would be pure cost.
Since the exclusion path gains nothing, resolving arrays to the element package
tightens nothing while genuinely loosening the allowlist:
{{struts.allowlist.packageNames=com.app}} would start implicitly allowlisting
{{com.app.Thing[]}}, which today needs an explicit {{struts.allowlist.classes}}
entry.
* *Should {{void}} and primitives be treated separately?* Moot —
{{target.getClass()}} is never primitive and {{member.getDeclaringClass()}} is
never primitive, so neither can appear at either call site.
* *Migration-guide note?* Not needed. No behaviour change.
Keeping the empty package also keeps {{toPackageName}} in agreement with
{{checkDefaultPackageAccess}}'s raw {{getPackage() == null}} reading, which is
what WW-5677 wants to unify.
h3. What was done instead
PR [#1858|https://github.com/apache/struts/pull/1858] records the reasoning
where it can rot loudly rather than quietly:
* new {{SecurityMemberAccessArrayTargetTest}} (4 tests) pinning both facts the
decision rests on — the two behavioural ones were mutation-checked by
defaulting {{excludedClasses}} to the empty set, which fails both;
* the comment on {{toPackageName}} no longer states the false premise and
points at that test.
No production behaviour change, no configuration change. The existing
{{arraysAndPrimitivesResolveToTheEmptyPackage}} and
{{toPackageNameMatchesLegacyAcrossClassShapes}} in
{{SecurityMemberAccessPackageMatchingTest}} still pin the behaviour itself and
are untouched.
> Decide whether array and primitive types should resolve to their
> element/wrapper package in OGNL security checks
> ----------------------------------------------------------------------------------------------------------------
>
> Key: WW-5676
> URL: https://issues.apache.org/jira/browse/WW-5676
> Project: Struts 2
> Issue Type: Improvement
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Split out of [WW-5674|https://issues.apache.org/jira/browse/WW-5674], where
> the question was deliberately deferred so that ticket could remain a pure
> no-behaviour-change optimisation.
> h2. Current behaviour
> {{SecurityMemberAccess.toPackageName(Class)}} returns the empty string for
> arrays, primitives and {{void}}:
> {code:java}
> public static String toPackageName(Class clazz) {
> if (clazz.isArray() || clazz.isPrimitive()) {
> return "";
> }
> return clazz.getPackageName();
> }
> {code}
> (The real signature is generic; generics are omitted here because the issue
> tracker mangles them.)
> The {{isArray() || isPrimitive()}} guard exists specifically to preserve
> this. It reproduces the pre-WW-5674 implementation, which used
> {{clazz.getPackage()}} — and {{getPackage()}} returns null for exactly those
> three categories.
> {{Class.getPackageName()}} resolves them differently: arrays resolve to the
> element type's package and primitives to {{java.lang}}. Verified on Temurin
> 17:
> || class || current {{toPackageName}} || {{getPackageName()}} ||
> | {{String[]}} | {{""}} | {{java.lang}} |
> | {{java.io.File[]}} | {{""}} | {{java.io}} |
> | {{int}}, {{int[]}}, {{void}} | {{""}} | {{java.lang}} |
> | {{com.app.MyThing[]}} | {{""}} | {{com.app}} |
> h2. Why this is not a simple hardening fix
> {{toPackageName}} feeds two checks that the change would push in _opposite_
> directions.
> _The exclusion path would tighten._ {{isExcludedPackageNames}} would start
> matching arrays. A {{java.io.File[]}} currently has package {{""}} and
> escapes {{struts.excludedPackageNames}} entirely, even though {{java.io}} is
> excluded by default. That looks like a genuine defensive gap.
> _The allowlist path would loosen._ {{isClassAllowlisted}} would start
> matching arrays too. An application that sets
> {{struts.allowlist.packageNames=com.app.actions}} does _not_ today thereby
> allowlist {{com.app.actions.MyThing[]}}, because the array's package is
> {{""}}; arrays must be listed explicitly in {{struts.allowlist.classes}}.
> After the change they would be allowlisted implicitly.
> The allowlist is the primary OGNL defence in Struts 7.x and is enabled by
> default ({{struts.allowlist.enable=true}}), so making it more permissive
> needs its own security reasoning rather than riding along with a performance
> change.
> h2. What needs deciding
> * Whether the exclusion-path gap (arrays of excluded-package types escaping
> exclusion) is exploitable in practice. Note that for an array target,
> {{member.getDeclaringClass()}} is usually {{java.lang.Object}}, which is
> already in {{struts.excludedClasses}}; {{clone()}} is the notable exception,
> as its declaring class is the array type itself.
> * Whether the allowlist loosening is acceptable, or whether the two paths
> should use different package semantics.
> * Whether {{void}} and primitives should be treated separately from arrays —
> they can never be an OGNL target in the same way.
> * Whether any change here needs a migration-guide note, since it can break
> applications that rely on the current behaviour in either direction.
> h2. Constraints
> Any change must keep {{checkDefaultPackageAccess}} consistent — it
> independently inspects {{getPackage()}} and would otherwise disagree with
> {{toPackageName}} about what "the default package" means. See WW-5677, which
> proposes routing that method through {{toPackageName}}.
> Requires equivalence tests in {{SecurityMemberAccessPackageMatchingTest}},
> which already pins the current array/primitive behaviour in
> {{arraysAndPrimitivesResolveToTheEmptyPackage}} and
> {{toPackageNameMatchesLegacyAcrossClassShapes}}. Those tests will need
> updating deliberately, not incidentally.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
