From 105b22fb9a4a02a1719f73cb035a8e249d876554 Mon Sep 17 00:00:00 2001 From: Brian Andle Date: Sat, 4 Jun 2022 18:03:57 -0700 Subject: [PATCH 1/3] WW-5184 - Add optional parameter value check to ParametersInterceptor --- .../interceptor/ParametersInterceptor.java | 137 +++++++++++++++++- .../ParametersInterceptorTest.java | 112 ++++++++++++++ 2 files changed, 246 insertions(+), 3 deletions(-) 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..21154d5468 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()); - 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) { + if(hasParamValuesToExclude() || hasParamValuesToAccept()) { + // We have something to check. + return acceptableValue(param.getName(), param.getValue()); + } else { + // No exclude/accept defined. Return true/allowed. + return true; + } + } /** * 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.info("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.info("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.info("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..d26ceef810 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,118 @@ 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); + } + + private ValueStack injectValueStack(Map actual) { ValueStack stack = createStubValueStack(actual); container.inject(stack); From 5763476830bcb71cef69fbd602f491330fa159fe Mon Sep 17 00:00:00 2001 From: Brian Andle Date: Mon, 6 Jun 2022 11:11:20 -0700 Subject: [PATCH 2/3] WW-5184 - Change info to warn from peer review --- .../xwork2/interceptor/ParametersInterceptor.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 21154d5468..b592cfb139 100644 --- a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -359,7 +359,7 @@ private boolean isIgnoredDMI(String name) { protected boolean acceptableValue(String name, String value) { boolean accepted = (value == null || value.isEmpty() || (!isParamValueExcluded(value) && isParamValueAccepted(value))); if (!accepted) { - LOG.info("Parameter [{}] was not accepted with value [{}] and will be dropped!", name, value); + LOG.warn("Parameter [{}] was not accepted with value [{}] and will be dropped!", name, value); } return accepted; } @@ -452,7 +452,7 @@ protected boolean isParamValueExcluded(String value) { for (Pattern excludedPattern : excludedValuePatterns) { if (value != null) { if (excludedPattern.matcher(value).matches()) { - LOG.info("Parameter value [{}] matches excluded pattern [{}] and will be dropped.", value, + LOG.warn("Parameter value [{}] matches excluded pattern [{}] and will be dropped.", value, excludedPattern); return true; } @@ -475,7 +475,7 @@ protected boolean isParamValueAccepted(String value) { // acceptedValuePatterns not defined so anything is allowed return true; } - LOG.info("Parameter value [{}] did not match any acceptedValuePattern pattern and will be dropped.", value); + LOG.warn("Parameter value [{}] did not match any acceptedValuePattern pattern and will be dropped.", value); return false; } From 584634a9b5ed66eabc5655a49d704a7038bd1e27 Mon Sep 17 00:00:00 2001 From: Brian Andle Date: Tue, 7 Jun 2022 21:50:34 -0700 Subject: [PATCH 3/3] WW-5184 - Added ParameterValueAware interface and unit test --- .../interceptor/ParameterValueAware.java | 37 ++++++++++++++++ .../interceptor/ParametersInterceptor.java | 14 +++--- .../ParametersInterceptorTest.java | 44 +++++++++++++++++++ 3 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 core/src/main/java/com/opensymphony/xwork2/interceptor/ParameterValueAware.java 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 b592cfb139..f81ba15fd1 100644 --- a/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java +++ b/core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java @@ -191,7 +191,7 @@ 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()); + isAcceptableParameter &= isAcceptableParameterValue(entry.getValue(), action); if (isAcceptableParameter) { acceptableParameters.put(parameterName, entry.getValue()); @@ -280,14 +280,14 @@ protected boolean isAcceptableParameter(String name, Object action) { * @param action current action * @return true if parameter is accepted */ - protected boolean isAcceptableParameterValue(Parameter param) { + 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()) { - // We have something to check. - return acceptableValue(param.getName(), param.getValue()); - } else { - // No exclude/accept defined. Return true/allowed. - return true; + // Additional validations to process + acceptableParmValue &= acceptableValue(param.getName(), param.getValue()); } + return acceptableParmValue; } /** 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 d26ceef810..ce8fe8498c 100644 --- a/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/interceptor/ParametersInterceptorTest.java @@ -888,6 +888,50 @@ public void testAcceptedAndExcludedParametersValuesAreIgnored() throws Exception 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) {