diff --git a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java index 3e626e664d..4d552d5948 100644 --- a/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java +++ b/core/src/main/java/org/apache/struts2/ognl/SecurityMemberAccess.java @@ -393,8 +393,13 @@ public static String toPackageName(Class clazz) { // call, whereas getPackageName() is computed once and cached on the Class. getPackage() // returns null for exactly arrays, primitives and void, so the guard reproduces the // previous result for every input. Note that void.class.isPrimitive() is true. - // Arrays deliberately keep the empty package here: getPackageName() would resolve them - // to the element type's package, which would loosen the allowlist. See WW-5674. + // Arrays deliberately keep the empty package. WW-5676 weighed resolving them to the element + // type's package and decided against it: that would tighten nothing, because the package + // check is unreachable for array targets -- every reflectively reachable member of an array + // class declares in java.lang.Object, which is permanently excluded -- while it would loosen + // the allowlist, implicitly allowlisting com.app.Thing[] for any application configuring + // struts.allowlist.packageNames=com.app. SecurityMemberAccessArrayTargetTest pins that + // reasoning; reopen WW-5676 if it ever stops holding. if (clazz.isArray() || clazz.isPrimitive()) { return ""; } diff --git a/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java new file mode 100644 index 0000000000..e229145e8e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/ognl/SecurityMemberAccessArrayTargetTest.java @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.ognl; + +import ognl.OgnlContext; +import org.apache.struts2.util.StrutsProxyService; +import org.junit.Before; +import org.junit.Test; + +import java.io.File; +import java.lang.reflect.Method; +import java.util.HashSet; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Pins the reasoning behind WW-5676, which decided that {@link SecurityMemberAccess#toPackageName} + * must keep resolving arrays to the empty package rather than to the element type's package. + *

+ * The concern WW-5676 was raised to investigate was that an array of an excluded-package type, say + * {@code java.io.File[]}, resolves to the empty package and so slips past + * {@code struts.excludedPackageNames} even though {@code java.io} is excluded by default. That + * reads like a defensive gap, but the package check is unreachable for array targets, because of + * the two facts pinned below: + *

    + *
  1. every member reflectively reachable on an array class declares in {@code java.lang.Object} + * — array {@code clone()} is a JVM-internal method absent from the reflection view; and
  2. + *
  3. {@code java.lang.Object} is permanently excluded — it is the built-in default of + * {@code excludedClasses} and the setters only ever accumulate onto that default.
  4. + *
+ * {@code checkExclusionList} tests the declaring class before the package, so it always denies at + * the first check and never reaches the package comparison. + *

+ * Resolving arrays to the element package would therefore tighten nothing, while genuinely + * loosening the allowlist: {@code struts.allowlist.packageNames=com.app} would begin to allowlist + * {@code com.app.Thing[]} implicitly, which today requires an explicit + * {@code struts.allowlist.classes} entry. These tests fail loudly if either fact stops holding, + * because that is what would turn the decision around. + */ +public class SecurityMemberAccessArrayTargetTest { + + private static final List> ARRAY_SHAPES = List.of( + String[].class, + File[].class, + int[].class, + Object[][].class, + SecurityMemberAccess[].class); + + private OgnlContext context; + private SecurityMemberAccess sma; + + @Before + public void setUp() { + context = ognl.Ognl.createDefaultContext(null); + ProviderAllowlist providerAllowlist = mock(ProviderAllowlist.class); + ThreadAllowlist threadAllowlist = mock(ThreadAllowlist.class); + when(providerAllowlist.getProviderAllowlist()).thenReturn(new HashSet<>()); + when(threadAllowlist.getAllowlist()).thenReturn(new HashSet<>()); + sma = new SecurityMemberAccess(providerAllowlist, threadAllowlist); + sma.setProxyService(new StrutsProxyService(new StrutsProxyCacheFactory<>("1000", "basic"))); + } + + /** + * Fact one. If a future JDK exposes further members on array classes, the unreachability + * argument breaks and WW-5676 has to be reopened. + */ + @Test + public void everyReflectiveMemberOfAnArrayClassDeclaresInObject() { + for (Class arrayClass : ARRAY_SHAPES) { + assertThat(arrayClass.getMethods()) + .as("public methods of %s", arrayClass.getName()) + .isNotEmpty() + .allSatisfy(method -> assertThat(method.getDeclaringClass()).isEqualTo(Object.class)); + assertThat(arrayClass.getDeclaredMethods()) + .as("declared methods of %s", arrayClass.getName()) + .isEmpty(); + assertThat(arrayClass.getFields()) + .as("public fields of %s, including the synthetic length", arrayClass.getName()) + .isEmpty(); + } + } + + /** + * Fact one, continued. Array {@code clone()} is a JVM-internal method: the JLS gives array + * types a public {@code clone()}, but it is not reflectively discoverable, so it can never + * reach {@code checkExclusionList} with the array type as its declaring class. + */ + @Test + public void arrayCloneIsNotReflectivelyReachable() { + for (Class arrayClass : ARRAY_SHAPES) { + assertThatExceptionOfType(NoSuchMethodException.class) + .as("clone() of %s", arrayClass.getName()) + .isThrownBy(() -> arrayClass.getMethod("clone")); + } + } + + /** + * Fact two. {@code useExcludedClasses} folds into the existing set rather than replacing it, + * so no configuration can drop the built-in {@code java.lang.Object} entry. + */ + @Test + public void objectStaysExcludedWhateverIsConfigured() { + assertThat(sma.isClassExcluded(Object.class)) + .as("java.lang.Object is excluded by default") + .isTrue(); + + sma.useExcludedClasses("java.lang.Class,java.lang.Runtime"); + + assertThat(sma.isClassExcluded(Object.class)) + .as("java.lang.Object stays excluded after excludedClasses is configured without it") + .isTrue(); + } + + /** + * The payoff: no member of an array target is accessible, so the empty package name that + * {@code toPackageName} returns for arrays is never compared against + * {@code struts.excludedPackageNames} in the first place. + */ + @Test + public void noMemberOfAnArrayTargetIsAccessible() { + for (boolean allowlistEnabled : new boolean[]{true, false}) { + sma.useEnforceAllowlistEnabled(String.valueOf(allowlistEnabled)); + File[] target = {new File("/tmp")}; + for (Method member : target.getClass().getMethods()) { + assertThat(sma.isAccessible(context, target, member, member.getName())) + .as("allowlistEnabled=%s member=%s", allowlistEnabled, member) + .isFalse(); + } + } + } +}