Lukasz Lenart created WW-5676:
---------------------------------
Summary: 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
Fix For: 7.4.0
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 {{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.
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)
