WW-5676 Keep arrays resolving to the empty package in OGNL security checks - #1858
Merged
Merged
Conversation
WW-5676 asked whether toPackageName should resolve arrays to the element type's package instead of the empty package. It should not, and the premise the ticket was filed on turns out to be wrong. The ticket assumed clone() is reachable on an array target with the array type as its declaring class, leaving java.io.File[] free to slip past the java.io entry in struts.excludedPackageNames. Array clone() is a JVM-internal method and is absent from the reflection view: on Temurin 17, 21 and 25, getMethods() on an array class returns only java.lang.Object's methods, getDeclaredMethods() is empty and getMethod("clone") throws. java.lang.Object is in turn permanently excluded - it is the built-in default of excludedClasses, and useExcludedClasses accumulates onto that default rather than replacing it, so no configuration can drop it. Since checkExclusionList tests the declaring class before the package, an array target is always denied at the first check and the package comparison is never reached. Resolving arrays to the element package would therefore tighten nothing while genuinely loosening the allowlist, so the behaviour stays as is. This commit records the reasoning where it can rot loudly instead of quietly: SecurityMemberAccessArrayTargetTest pins both facts the decision rests on, and the comment on toPackageName no longer states the false premise. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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.



Fixes WW-5676
WW-5676 asked whether
SecurityMemberAccess.toPackageNameshould resolve arrays to the element type's package, and primitives/voidtojava.lang, instead of the empty package. The answer is no, and the premise the ticket was filed on is wrong. This PR changes no behaviour — it pins the reasoning so the question is not reopened from the same false premise.The premise that failed
The ticket argued there was a defensive gap on the exclusion path:
and identified
clone()as the way in:Array
clone()is a JVM-internal method. The JLS gives array types a publicclone(), but it is not in the reflection view. Verified on Temurin 17.0.14, 21.0.7 and 25.0.1, forString[],java.io.File[],int[]andObject[][]:getMethods()java.lang.ObjectmethodsgetDeclaredMethods()[]getMethod("clone")NoSuchMethodExceptiongetFields()/getField("length")[]/NoSuchFieldExceptionSo every member reflectively reachable on an array class declares in
java.lang.Object.Why the package check is unreachable
java.lang.Objectcannot be configured out of the exclusion list. It is the built-in default ofSecurityMemberAccess.excludedClasses, anduseExcludedClassesgoes throughConfigParseUtil.toNewClassesSet, which accumulates onto the existing set rather than replacing it.checkExclusionListtestsisClassExcluded(member.getDeclaringClass())beforeisPackageExcluded, so an array target is denied at the first check every time. An end-to-end check against a realSecurityMemberAccesswith the productionstruts-excluded-classes.xmlvalues andtarget = new java.io.File[]{...}denies all 9 reachable members, with the allowlist both enabled and disabled.Why changing it would be a net loss
struts.allowlist.packageNames=com.appwould begin implicitly allowlistingcom.app.Thing[], which today requires an explicitstruts.allowlist.classesentry. The allowlist is the primary OGNL defence in 7.x and is on by default.""also keepstoPackageNamein agreement withcheckDefaultPackageAccess's rawgetPackage() == nullreading, which is what WW-5677 wants to unify.Primitives and
voidare moot either way:target.getClass()is never primitive andmember.getDeclaringClass()is never primitive, so they cannot appear at either call site.What this PR contains
SecurityMemberAccessArrayTargetTest(new, 4 tests) pins both facts the decision rests on, so either one breaking fails loudly rather than silently invalidating the reasoning:everyReflectiveMemberOfAnArrayClassDeclaresInObjectarrayCloneIsNotReflectivelyReachableobjectStaysExcludedWhateverIsConfigurednoMemberOfAnArrayTargetIsAccessible— the payoff, over allowlist on and offSecurityMemberAccess.toPackageName: the comment no longer states the false premise, and points at the test.The two behavioural tests were mutation-checked: setting
excludedClassestoemptySet()fails both,noMemberOfAnArrayTargetIsAccessiblespecifically atallowlistEnabled=false.No production behaviour changes, no configuration changes, and no migration-guide note is needed. The existing
arraysAndPrimitivesResolveToTheEmptyPackageandtoPackageNameMatchesLegacyAcrossClassShapesinSecurityMemberAccessPackageMatchingTestcontinue to pin the behaviour itself and are untouched.Testing
mvn test -DskipAssembly -pl core— 3185 tests, 0 failures, 0 errors.🤖 Generated with Claude Code