[
https://issues.apache.org/jira/browse/WW-5676?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5676:
------------------------------
Description:
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.
was:
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.
> 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
>
>
> 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)
