From 66199650b62004008997e9306d02a9d561a4d4ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=B3=95=E2=B2=9B=CF=84=E2=B2=89=E2=B2=85=E2=B2=A5?= =?UTF-8?q?=E2=B2=89=E2=B3=8F=CF=84=E2=B2=9F=E2=B2=85=20=F0=9F=95=B5?= =?UTF-8?q?=F0=9F=8F=BB?= <192411347+g0w6y@users.noreply.github.com> Date: Sat, 7 Nov 2026 14:39:57 +0530 Subject: [PATCH] WW-5839 Reject unconfined wildcard substitution into class= attributes AbstractMatcher/ActionConfigMatcher: a wildcard "{n}" substituted into an action/result/exception-mapping class= attribute was unconfined, unlike the sibling method-name slot (ActionConfig#isAllowedMethod). A substituted segment containing '.' or '/' is now rejected rather than used verbatim. Reachable only if a developer writes a wildcard mapping with a substituted class name -- application-responsibility, not a default-config issue. Tests: WildcardClassNameHardeningTest (new, 2 tests). Full core suite: 3742/0 failures. --- .../struts2/config/impl/AbstractMatcher.java | 44 ++++++++- .../config/impl/ActionConfigMatcher.java | 6 +- .../impl/WildcardClassNameHardeningTest.java | 95 +++++++++++++++++++ 3 files changed, 141 insertions(+), 4 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/config/impl/WildcardClassNameHardeningTest.java diff --git a/core/src/main/java/org/apache/struts2/config/impl/AbstractMatcher.java b/core/src/main/java/org/apache/struts2/config/impl/AbstractMatcher.java index b4e1e6b704..12a561ffa0 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/AbstractMatcher.java +++ b/core/src/main/java/org/apache/struts2/config/impl/AbstractMatcher.java @@ -46,6 +46,17 @@ public abstract class AbstractMatcher implements Serializable { private static final Logger LOG = LogManager.getLogger(AbstractMatcher.class); private static final Pattern WILDCARD_PATTERN = Pattern.compile("\\{(.)}"); + /** + *

Values a wildcard-matched request path segment may take when it is substituted + * into a class-name-bearing slot (action/result/exception-mapping {@code class}). + * Rejects '.', '/', and any other character that could walk the substituted value + * out of the package the template author wrote, or that an {@code ObjectFactory} + * could otherwise treat specially.

+ * + * @since 7.5.0 + */ + private static final Pattern SAFE_CLASS_NAME_SEGMENT = Pattern.compile("[A-Za-z0-9_$]*"); + /** *

Handles all wildcard pattern matching.

*/ @@ -199,6 +210,31 @@ protected Map replaceParameters(Map orig, Map vars) { + return convertParam(val, vars, false); + } + + /** + *

Inserts into a value wildcard-matched strings where specified + * with the {x} syntax. If a wildcard-matched value isn't found, the + * replacement token is turned into an empty string. + *

+ * + *

Use {@code safeForClassName = true} for any slot that ends up being loaded as a + * class or bean name (action/result/exception-mapping {@code class}). A substituted + * value that would carry a '.', '/', or other non-identifier character into the class + * name is dropped (treated as empty) instead of substituted, so a wildcard segment + * cannot walk the resulting class name out of the package the template author wrote. + * The method-name slot already has an equivalent guard + * ({@link org.apache.struts2.config.entities.ActionConfig#isAllowedMethod(String)}); + * this is the same guard for the class-name slot. Literal characters in {@code val} + * itself are never filtered -- only the values substituted in for {x}.

+ * + * @param val The value to convert + * @param vars A Map of wildcard-matched strings + * @param safeForClassName restrict substituted values to {@code [A-Za-z0-9_$]*} + * @return The new value + */ + protected String convertParam(String val, Map vars, boolean safeForClassName) { if (val == null) { return null; } @@ -207,7 +243,13 @@ protected String convertParam(String val, Map vars) { StringBuilder result = new StringBuilder(); while (wildcardMatcher.find()) { - wildcardMatcher.appendReplacement(result, vars.getOrDefault(wildcardMatcher.group(1), "")); + String replacement = vars.getOrDefault(wildcardMatcher.group(1), ""); + if (safeForClassName && !SAFE_CLASS_NAME_SEGMENT.matcher(replacement).matches()) { + LOG.warn("Wildcard-matched segment '{}' rejected for class-name slot '{}' -- " + + "contains characters other than [A-Za-z0-9_$]", replacement, val); + replacement = ""; + } + wildcardMatcher.appendReplacement(result, Matcher.quoteReplacement(replacement)); } wildcardMatcher.appendTail(result); diff --git a/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java b/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java index ce89146553..2895763cb3 100644 --- a/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java +++ b/core/src/main/java/org/apache/struts2/config/impl/ActionConfigMatcher.java @@ -114,7 +114,7 @@ public ActionConfig convert(String path, ActionConfig orig, return null; } - String className = convertParam(orig.getClassName(), vars); + String className = convertParam(orig.getClassName(), vars, true); String pkgName = convertParam(orig.getPackageName(), vars); Map params = replaceParameters(orig.getParams(), vars); @@ -123,7 +123,7 @@ public ActionConfig convert(String path, ActionConfig orig, for (String name : orig.getResults().keySet()) { ResultConfig result = orig.getResults().get(name); name = convertParam(name, vars); - ResultConfig r = new ResultConfig.Builder(name, convertParam(result.getClassName(), vars)) + ResultConfig r = new ResultConfig.Builder(name, convertParam(result.getClassName(), vars, true)) .addParams(replaceParameters(result.getParams(), vars)) .build(); results.put(name, r); @@ -132,7 +132,7 @@ public ActionConfig convert(String path, ActionConfig orig, List exs = new ArrayList(); for (ExceptionMappingConfig ex : orig.getExceptionMappings()) { String name = convertParam(ex.getName(), vars); - String exClassName = convertParam(ex.getExceptionClassName(), vars); + String exClassName = convertParam(ex.getExceptionClassName(), vars, true); String exResult = convertParam(ex.getResult(), vars); Map exParams = replaceParameters(ex.getParams(), vars); ExceptionMappingConfig e = new ExceptionMappingConfig.Builder(name, exClassName, exResult).addParams(exParams).build(); diff --git a/core/src/test/java/org/apache/struts2/config/impl/WildcardClassNameHardeningTest.java b/core/src/test/java/org/apache/struts2/config/impl/WildcardClassNameHardeningTest.java new file mode 100644 index 0000000000..b6dd17555a --- /dev/null +++ b/core/src/test/java/org/apache/struts2/config/impl/WildcardClassNameHardeningTest.java @@ -0,0 +1,95 @@ +/* + * 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.config.impl; + +import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.config.entities.ActionConfig; +import org.apache.struts2.util.WildcardHelper; + +import java.util.HashMap; +import java.util.Map; + +/** + * The wildcard "{n}" substitution that fills in a + * {@code class=} (and method/package) template is unconfined -- any matched path + * segment, including one containing '.' or '/'-decoded dots, lands verbatim in the + * resulting class name. A sibling slot (the method name) IS gated, by + * {@link ActionConfig#isAllowedMethod(String)} at ActionConfigMatcher.java:113. There is + * no equivalent gate for the class name at ActionConfigMatcher.java:117 / + * AbstractMatcher.convertParam():201-215. + * + * Benign probe only: the "attacker" value is a harmless literal package/class suffix + * ("util.Harmless"), not a real sensitive class, and no class is actually loaded here -- + * this test only proves the ActionConfig the matcher hands back contains the attacker's + * wildcard segment unconfined in getClassName(), which is what a default ObjectFactory + * would later pass to Class.forName/buildBean with no further check + * (DefaultActionFactory.buildAction -> objectFactory.buildBean(config.getClassName(), ...)). + */ +public class WildcardClassNameHardeningTest extends XWorkTestCase { + + public void testClassNameWildcardRejectsNonIdentifierSegment() { + Map map = new HashMap<>(); + // A typical CRUD-style wildcard mapping a developer might write: + // -- intended for simple names + // like "user" or "order", never audited against the request path. + map.put("*", new ActionConfig.Builder("package", "*", "actions.{1}Action") + .methodName("execute") + .build()); + + ActionConfigMatcher matcher = new ActionConfigMatcher(new WildcardHelper(), map, false); + + // Benign "attacker" path segment: contains '.' so it walks the class name into a + // different package than the template author intended. No real/dangerous class + // name is used -- this is a structural probe, not an exploit payload. + ActionConfig matched = matcher.match("util.Harmless"); + + assertNotNull("ActionConfig should be matched", matched); + // GREEN (post-fix): the '.' in the matched segment is not a safe class-name + // character, so the substitution is dropped instead of walking the class out of + // the mapped package -- the attacker-controlled segment no longer changes the + // resolved package. + assertEquals("Class-name wildcard substitution must reject non-identifier characters", + "actions.Action", matched.getClassName()); + + // Benign identifier-only segment still substitutes normally. + ActionConfig normal = matcher.match("User"); + assertEquals("actions.UserAction", normal.getClassName()); + } + + public void testMethodNameWildcardIsGated() { + // Control: the sibling slot (method name) IS gated by isAllowedMethod, so a + // non-allowed substituted method value causes convert() to return null instead + // of an ActionConfig that would invoke an arbitrary method. + Map map = new HashMap<>(); + map.put("run/*", new ActionConfig.Builder("package", "run/*", "actions.RunAction") + .methodName("{1}") + .addAllowedMethod(java.util.Set.of("execute")) + .setStrictMethodInvocation(true) + .build()); + + ActionConfigMatcher matcher = new ActionConfigMatcher(new WildcardHelper(), map, false); + + ActionConfig notAllowed = matcher.match("run/deleteAll"); + assertNull("GREEN control: method substitution is gated by isAllowedMethod", notAllowed); + + ActionConfig allowed = matcher.match("run/execute"); + assertNotNull(allowed); + assertEquals("execute", allowed.getMethodName()); + } +}