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..7690865f47 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 @@ -19,6 +19,7 @@ package org.apache.struts2.config.impl; import org.apache.struts2.util.PatternMatcher; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.math.NumberUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -46,6 +47,13 @@ public abstract class AbstractMatcher implements Serializable { private static final Logger LOG = LogManager.getLogger(AbstractMatcher.class); private static final Pattern WILDCARD_PATTERN = Pattern.compile("\\{(.)}"); + /** + * Characters a wildcard-matched segment may contain when it is substituted into a class name: + * Unicode letters and digits, '_' and '$'. Excludes '.', '/' and everything else that could move + * the resulting class name out of the package the mapping's author wrote. + */ + private static final Pattern SAFE_CLASS_NAME_SEGMENT = Pattern.compile("[\\p{L}\\p{Nd}_$]*"); + /** *

Handles all wildcard pattern matching.

*/ @@ -207,13 +215,40 @@ protected String convertParam(String val, Map vars) { StringBuilder result = new StringBuilder(); while (wildcardMatcher.find()) { - wildcardMatcher.appendReplacement(result, vars.getOrDefault(wildcardMatcher.group(1), "")); + wildcardMatcher.appendReplacement(result, Matcher.quoteReplacement(vars.getOrDefault(wildcardMatcher.group(1), ""))); } wildcardMatcher.appendTail(result); return result.toString(); } + /** + *

Checks that every wildcard-matched value that {@link #convertParam(String, Map)} would + * substitute into {@code val} is safe to use as part of a class name. Callers reject the + * whole match when this returns {@code false}, as they do for a disallowed method name.

+ * + * @param val The class name template, may be {@code null} + * @param vars A Map of wildcard-matched strings + * @return {@code false} if a substituted value contains a character other than a letter, + * digit, '_' or '$' + */ + protected boolean isSafeForClassName(String val, Map vars) { + if (val == null) { + return true; + } + + Matcher wildcardMatcher = WILDCARD_PATTERN.matcher(val); + while (wildcardMatcher.find()) { + String value = vars.getOrDefault(wildcardMatcher.group(1), ""); + if (!SAFE_CLASS_NAME_SEGMENT.matcher(value).matches()) { + LOG.warn("Wildcard-matched value [{}] is not allowed in class name [{}]; match rejected", + StringUtils.normalizeSpace(value), val); + return false; + } + } + return true; + } + /** *

Stores a compiled wildcard pattern and the object it came * from.

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..77fd4e52ad 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,6 +114,10 @@ public ActionConfig convert(String path, ActionConfig orig, return null; } + if (!hasSafeClassNames(orig, vars)) { + return null; + } + String className = convertParam(orig.getClassName(), vars); String pkgName = convertParam(orig.getPackageName(), vars); @@ -152,4 +156,21 @@ public ActionConfig convert(String path, ActionConfig orig, .location(orig.getLocation()) .build(); } + + private boolean hasSafeClassNames(ActionConfig orig, Map vars) { + if (!isSafeForClassName(orig.getClassName(), vars)) { + return false; + } + for (ResultConfig result : orig.getResults().values()) { + if (!isSafeForClassName(result.getClassName(), vars)) { + return false; + } + } + for (ExceptionMappingConfig ex : orig.getExceptionMappings()) { + if (!isSafeForClassName(ex.getExceptionClassName(), vars)) { + return false; + } + } + return true; + } } 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..e75427871d --- /dev/null +++ b/core/src/test/java/org/apache/struts2/config/impl/WildcardClassNameHardeningTest.java @@ -0,0 +1,93 @@ +/* + * 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.config.entities.ExceptionMappingConfig; +import org.apache.struts2.config.entities.ResultConfig; +import org.apache.struts2.util.WildcardHelper; + +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +public class WildcardClassNameHardeningTest extends XWorkTestCase { + + private ActionConfigMatcher matcherFor(ActionConfig config) { + Map map = new HashMap<>(); + map.put(config.getName(), config); + return new ActionConfigMatcher(new WildcardHelper(), map, false); + } + + public void testActionClassNameRejectsSegmentWithDot() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "*", "actions.{1}Action") + .methodName("execute") + .build()); + + assertNull(matcher.match("util.Harmless")); + assertEquals("actions.UserAction", matcher.match("User").getClassName()); + } + + public void testResultClassNameRejectsSegmentWithDot() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "*", "actions.FixedAction") + .addResultConfig(new ResultConfig.Builder("success", "results.{1}Result").build()) + .build()); + + assertNull(matcher.match("util.Harmless")); + assertEquals("results.UserResult", matcher.match("User").getResults().get("success").getClassName()); + } + + public void testExceptionMappingClassNameRejectsSegmentWithDot() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "*", "actions.FixedAction") + .addExceptionMapping(new ExceptionMappingConfig.Builder("ex", "errors.{1}Exception", "error").build()) + .build()); + + assertNull(matcher.match("util.Harmless")); + assertEquals("errors.UserException", matcher.match("User").getExceptionMappings().get(0).getExceptionClassName()); + } + + public void testClassNameAcceptsNonAsciiIdentifierAndDollar() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "*", "actions.{1}Action") + .build()); + + assertEquals("actions.ÜserAction", matcher.match("Üser").getClassName()); + assertEquals("actions.Inner$1Action", matcher.match("Inner$1").getClassName()); + } + + public void testDollarAndBackslashAreSubstitutedLiterally() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "*", "actions.FixedAction") + .addParam("value", "{1}") + .build()); + + assertEquals("a$1b", matcher.match("a$1b").getParams().get("value")); + assertEquals("a\\b", matcher.match("a\\b").getParams().get("value")); + } + + public void testMethodNameWildcardIsGated() { + ActionConfigMatcher matcher = matcherFor(new ActionConfig.Builder("package", "run/*", "actions.RunAction") + .methodName("{1}") + .addAllowedMethod(Set.of("execute")) + .setStrictMethodInvocation(true) + .build()); + + assertNull(matcher.match("run/deleteAll")); + assertEquals("execute", matcher.match("run/execute").getMethodName()); + } +}