[
https://issues.apache.org/jira/browse/WW-5184?focusedWorklogId=778707&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-778707
]
ASF GitHub Bot logged work on WW-5184:
--------------------------------------
Author: ASF GitHub Bot
Created on: 06/Jun/22 17:36
Start Date: 06/Jun/22 17:36
Worklog Time Spent: 10m
Work Description: yasserzamani commented on code in PR #559:
URL: https://github.com/apache/struts/pull/559#discussion_r890375109
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -354,7 +407,86 @@ protected boolean isExcluded(String paramName) {
}
return false;
}
+
+ public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
Review Comment:
BTW another methods annotated with `@Inject` as well as some other changes
are required enabling user to configure these patterns via XML configuration.
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -354,7 +407,86 @@ protected boolean isExcluded(String paramName) {
}
return false;
}
+
+ public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
+ Set<String> 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<String> 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,
Review Comment:
same here
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -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);
Review Comment:
Sorry my failure in previous PR. Could you please log such thing with WARN
level? as well as other similar places.
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -354,7 +407,86 @@ protected boolean isExcluded(String paramName) {
}
return false;
}
+
+ public void setAcceptedValuePatterns(String commaDelimitedPatterns) {
+ Set<String> 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<String> 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);
Review Comment:
and here
Issue Time Tracking
-------------------
Worklog Id: (was: 778707)
Time Spent: 2h 10m (was: 2h)
> Add optional parameter value check to ParametersInterceptor
> -----------------------------------------------------------
>
> Key: WW-5184
> URL: https://issues.apache.org/jira/browse/WW-5184
> Project: Struts 2
> Issue Type: Improvement
> Reporter: Brian Andle
> Priority: Major
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> It is known that developers utilizing Struts/Freemarker should always ensure
> proper sanitization to prevent OGNL/Freemarker evaluation on untrusted user
> input when %{/$\{ in FTL being passed into Struts tags.
> These patterns aren't always practical to resolve/find especially in legacy
> code. This isn't a solely a legacy code of course it's just as easy to make a
> mistake in newer code as well.
> The following would end up rendering 81
> Payload:
> {code:java}
> untrustedInput=%25%7B9%2A9%7D {code}
> FTL:
>
> {code:java}
> <@s.form theme="simple" action="${untrustedInput}" id="myForm4">
> </@s.form> {code}
>
> Java:
> {code:java}
> private String untrustedInput;
> public String getUntrustedInput() {
> return untrustedInput;
> }
> public void setUntrustedInput(String untrustedInput) {
> this.untrustedInput = untrustedInput;
> } {code}
>
> This ticket is to add an optional `params.excludeValuePatterns` so that
> ParametersInterceptor can drop incoming parameter itself if the value matches
> a pattern to be excluded.
>
> {code:java}
> <param
> name="params.excludeValuePatterns">.*\$\{.*?\}.*,.*%\{.*?\}.*</param>{code}
>
> Since this is a pattern and would be executed against the values themselves
> there is the potential of a performance impact however I since it's optional
> we shouldn't see any measurable impact when not enabled.
>
> *NOTE:* I did add a `params.acceptValuePatterns` pattern that is
> null/disabled by default. This might not ever be used but mimic'd the Pattern
> matcher the ParametersInterceptor/CookieInterceptor.
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
