diff --git a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java new file mode 100644 index 0000000000..7f077f7c9a --- /dev/null +++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java @@ -0,0 +1,37 @@ +/* + * 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 com.opensymphony.xwork2.interceptor; + +/** + * This interface is implemented by actions that want to declare acceptable parameter value. Works in conjunction with {@link + * ParametersInterceptor}. For example, actions may want to create a white list of parameter values they will accept or a + * blacklist of parameter values they will reject to prevent clients from setting other unexpected (and possibly dangerous) + * parameter values. + */ +public interface ParameterValueAware { + + /** + * Tests if the the action will accept the parameter with the given value. + * + * @param parameterValue the parameter value + * @return true if accepted, false otherwise + */ + boolean acceptableParameterValue(String parameterValue); + +} diff --git a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java index 781a8f3e54..f81ba15fd1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -37,10 +37,14 @@ import org.apache.struts2.dispatcher.HttpParameters; import java.util.Collection; +import java.util.Collections; import java.util.Comparator; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.TreeMap; import java.util.regex.Pattern; +import com.opensymphony.xwork2.util.TextParseUtil; /** * This interceptor sets all parameters on the value stack. @@ -62,6 +66,9 @@ public class ParametersInterceptor extends MethodFilterInterceptor { private ValueStackFactory valueStackFactory; private ExcludedPatternsChecker excludedPatterns; private AcceptedPatternsChecker acceptedPatterns; + private Set excludedValuePatterns = null; + private Set acceptedValuePatterns = null; + @Inject public void setValueStackFactory(ValueStackFactory valueStackFactory) { @@ -183,8 +190,10 @@ protected void setParameters(final Object action, ValueStack stack, HttpParamete for (Map.Entry entry : params.entrySet()) { String parameterName = entry.getKey(); + boolean isAcceptableParameter = isAcceptableParameter(parameterName, action); + isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action); - if (isAcceptableParameter(parameterName, action)) { + if (isAcceptableParameter) { acceptableParameters.put(parameterName, entry.getValue()); } } @@ -263,6 +272,23 @@ protected boolean isAcceptableParameter(String name, Object action) { ParameterNameAware parameterNameAware = (action instanceof ParameterNameAware) ? (ParameterNameAware) action : null; return acceptableName(name) && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name)); } + + /** + * Checks if parameter value can be accepted or thrown away + * + * @param param the parameter + * @param action current action + * @return true if parameter is accepted + */ + protected boolean isAcceptableParameterValue(Parameter param, Object action) { + ParameterValueAware parameterValueAware = (action instanceof ParameterValueAware) ? (ParameterValueAware) action : null; + boolean acceptableParmValue = (parameterValueAware == null || parameterValueAware.acceptableParameterValue(param.getValue())); + if(hasParamValuesToExclude() || hasParamValuesToAccept()) { + // Additional validations to process + acceptableParmValue &= acceptableValue(param.getName(), param.getValue()); + } + return acceptableParmValue; + } /** * Gets an instance of the comparator to use for the ordered sorting. Override this @@ -290,7 +316,16 @@ protected String getParameterLogMap(HttpParameters parameters) { return logEntry.toString(); } - + + /** + * Validates the name passed is: + * * Within the max length of a parameter name + * * Is not excluded + * * Is accepted + * + * @param name - Name to check + * @return true if accepted + */ protected boolean acceptableName(String name) { if (isIgnoredDMI(name)) { LOG.trace("DMI is enabled, ignoring DMI method: {}", name); @@ -311,6 +346,24 @@ private boolean isIgnoredDMI(String name) { } } + /** + * Validates: + * * Value is null/blank + * * Value is not excluded + * * Value is accepted + * + * @param name - Param name (for logging) + * @param value - value to check + * @return true if accepted + */ + protected boolean acceptableValue(String name, String value) { + boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value))); + if (!accepted) { + LOG.warn("Parameter [{}] was not accepted with value [{}] and will be dropped!", name, value); + } + return accepted; + } + protected boolean isWithinLengthLimit(String name) { boolean matchLength = name.length() <= paramNameMaxLength; if (!matchLength) { @@ -354,7 +407,86 @@ protected boolean isExcluded(String paramName) { } return false; } + + public void setAcceptedValuePatterns(String commaDelimitedPatterns) { + Set patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns); + if (acceptedValuePatterns == null) { + // Limit unwanted log entries (for 1st call, acceptedValuePatterns null) + LOG.debug("Sets accepted value patterns to [{}], note this may impact the safety of your application!", patterns); + } else { + LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and may impact the safety of your application!", + acceptedValuePatterns, patterns); + } + acceptedValuePatterns = new HashSet<>(patterns.size()); + try { + for (String pattern : patterns) { + acceptedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); + } + } finally { + acceptedValuePatterns = Collections.unmodifiableSet(acceptedValuePatterns); + } + } + + public void setExcludeValuePatterns(String commaDelimitedPatterns) { + Set patterns = TextParseUtil.commaDelimitedStringToSet(commaDelimitedPatterns); + if (excludedValuePatterns == null) { + // Limit unwanted log entries (for 1st call, excludedValuePatterns null) + LOG.debug("Setting excluded value patterns to [{}]", patterns); + } else { + LOG.warn("Replacing accepted patterns [{}] with [{}], be aware that this affects all instances and may impact safety of your application!", + excludedValuePatterns, patterns); + } + excludedValuePatterns = new HashSet<>(patterns.size()); + try { + for (String pattern : patterns) { + excludedValuePatterns.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); + } + } finally { + excludedValuePatterns = Collections.unmodifiableSet(excludedValuePatterns); + } + } + + protected boolean isParamValueExcluded(String value) { + if (hasParamValuesToExclude()) { + for (Pattern excludedPattern : excludedValuePatterns) { + if (value != null) { + if (excludedPattern.matcher(value).matches()) { + LOG.warn("Parameter value [{}] matches excluded pattern [{}] and will be dropped.", value, + excludedPattern); + return true; + } + } + } + } + return false; + } + + protected boolean isParamValueAccepted(String value) { + if (hasParamValuesToAccept()) { + for (Pattern excludedPattern : acceptedValuePatterns) { + if (value != null) { + if (excludedPattern.matcher(value).matches()) { + return true; + } + } + } + } else { + // acceptedValuePatterns not defined so anything is allowed + return true; + } + LOG.warn("Parameter value [{}] did not match any acceptedValuePattern pattern and will be dropped.", value); + return false; + } + + private boolean hasParamValuesToExclude() { + return excludedValuePatterns != null && excludedValuePatterns.size() > 0; + } + + private boolean hasParamValuesToAccept() { + return acceptedValuePatterns != null && acceptedValuePatterns.size() > 0; + } + /** * Whether to order the parameters or not * @@ -396,5 +528,4 @@ public void setAcceptParamNames(String commaDelim) { public void setExcludeParams(String commaDelim) { excludedPatterns.setExcludedPatterns(commaDelim); } - } diff --git a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java index 69d9b03a60..ce8fe8498c 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -778,6 +778,162 @@ public void testBeanListSingleValue() throws Exception { assertFalse(action.getBeanList().isEmpty()); } + public void testExcludedParametersValuesAreIgnored() throws Exception { + ParametersInterceptor pi = createParametersInterceptor(); + // Contains (based on pattern) + pi.setExcludeValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*"); + + assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}")); + + final Map actual = injectValueStackFactory(pi); + ValueStack stack = injectValueStack(actual); + + final Map expected = new HashMap() { + { + put("fooKey", "fooValue"); + put("fooKey2", ""); + } + }; + + Map parameters = new HashMap() { + { + put("barKey$", "${2+2}"); + put("barKey2$", "foo${2+2}"); + put("barKey3$", "foo${2+2}foo"); + put("barKey%", "%{2+2}"); + put("barKey2%", "foo%{2+2}"); + put("barKey3%", "foo%{2+2}foo"); + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + put("fooKey", "fooValue"); + put("fooKey2", ""); + } + }; + pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build()); + assertEquals(expected, actual); + } + + public void testAcceptedParametersValuesAreIgnored() throws Exception { + ParametersInterceptor pi = createParametersInterceptor(); + // Starts with (based on pattern) + pi.setAcceptedValuePatterns("^\\$\\{foo\\}.*,^%\\{bar\\}.*,^fooValue"); + + assertTrue("${foo} was allowed by isParamValueAccepted", pi.isParamValueAccepted("${foo}")); + + final Map actual = injectValueStackFactory(pi); + ValueStack stack = injectValueStack(actual); + + final Map expected = new HashMap() { + { + put("fooKey", "fooValue"); + put("fooKey2", ""); + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + } + }; + + Map parameters = new HashMap() { + { + put("barKey$", "${2+2}"); + put("barKey2$", "foo${2+2}"); + put("barKey3$", "foo${2+2}foo"); + put("barKey%", "%{2+2}"); + put("barKey2%", "foo%{2+2}"); + put("barKey3%", "foo%{2+2}foo"); + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + put("fooKey", "fooValue"); + put("fooKey2", ""); + } + }; + pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build()); + assertEquals(expected, actual); + } + + public void testAcceptedAndExcludedParametersValuesAreIgnored() throws Exception { + ParametersInterceptor pi = createParametersInterceptor(); + // Starts with (based on pattern) + pi.setAcceptedValuePatterns("^\\$\\{foo\\}.*,^%\\{bar\\}.*,^fooValue"); + pi.setExcludeValuePatterns(".*\\$\\{2.*2\\}.*,.*\\%\\{2.*2\\}.*"); + + assertTrue("${foo} was allowed by isParamValueAccepted", pi.isParamValueAccepted("${foo}")); + assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}")); + + final Map actual = injectValueStackFactory(pi); + ValueStack stack = injectValueStack(actual); + + final Map expected = new HashMap() { + { + put("fooKey", "fooValue"); + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + put("fooKey2", ""); + } + }; + + Map parameters = new HashMap() { + { + put("barKey$", "${2+2}"); + put("barKey2$", "foo${2+2}"); + put("barKey%", "%{2+2}"); + put("barKey2%", "foo%{2+2}"); + put("barKey3", "nothing"); + + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + put("fooKey", "fooValue"); + put("fooKey2", ""); + } + }; + pi.setParameters(new NoParametersAction(), stack, HttpParameters.create(parameters).build()); + assertEquals(expected, actual); + } + + public void testExcludedParametersValuesAreIgnoredWithParameterValueAware() throws Exception { + ParametersInterceptor pi = createParametersInterceptor(); + // Contains (based on pattern) + pi.setExcludeValuePatterns(".*\\$\\{.*?\\}.*,.*%\\{.*?\\}.*"); + + assertTrue("${2*2} was excluded by isParamValueExcluded", pi.isParamValueExcluded("${2*2}")); + + final Map actual = injectValueStackFactory(pi); + ValueStack stack = injectValueStack(actual); + + final Map expected = new HashMap() { + { + // acceptableParameterValue only allows fooValue even though fooKey2 and fooKey3 pass the excludeValuePatterns check + put("fooKey", "fooValue"); + } + }; + + Object a = new ParameterValueAware() { + @Override + public boolean acceptableParameterValue(String parameterValue) { + // Only fooValue will be allowed because the excludeValuePatterns will block ${2+2} + return parameterValue.equals("fooValue") || parameterValue.equals("${2+2}"); + } + }; + + Map parameters = new HashMap() { + { + put("barKey$", "${2+2}"); + put("barKey2$", "foo${2+2}"); + put("barKey3$", "foo${2+2}foo"); + put("barKey%", "%{2+2}"); + put("barKey2%", "foo%{2+2}"); + put("barKey3%", "foo%{2+2}foo"); + put("allowedKey", "${foo}"); + put("allowedKey2", "%{bar}"); + put("fooKey", "fooValue"); + put("fooKey2", "fooValue2"); + put("fooKey3", ""); + } + }; + pi.setParameters(a, stack, HttpParameters.create(parameters).build()); + assertEquals(expected, actual); + } + + private ValueStack injectValueStack(Map actual) { ValueStack stack = createStubValueStack(actual); container.inject(stack);