[
https://issues.apache.org/jira/browse/WW-5184?focusedWorklogId=812808&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-812808
]
ASF GitHub Bot logged work on WW-5184:
--------------------------------------
Author: ASF GitHub Bot
Created on: 28/Sep/22 09:18
Start Date: 28/Sep/22 09:18
Worklog Time Spent: 10m
Work Description: github-code-scanning[bot] commented on code in PR #607:
URL: https://github.com/apache/struts/pull/607#discussion_r982159941
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -351,19 +351,24 @@ private boolean isIgnoredDMI(String name) {
* * 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)));
+ 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);
+ String message = "Value [{}] of parameter [{}] was not accepted
and will be dropped!";
+ if (devMode) {
+ LOG.warn(message, value, name);
+ } else {
+ LOG.debug(message, value, name);
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AYODY9tvQiP6RP8X9SmM-->Change this code to not log
user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AYODY9tvQiP6RP8X9SmM&open=AYODY9tvQiP6RP8X9SmM&pullRequest=607";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/123)
##########
core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java:
##########
@@ -351,19 +351,24 @@ private boolean isIgnoredDMI(String name) {
* * 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)));
+ 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);
+ String message = "Value [{}] of parameter [{}] was not accepted
and will be dropped!";
+ if (devMode) {
+ LOG.warn(message, value, name);
Review Comment:
## Logging should not be vulnerable to injection attacks
<!--SONAR_ISSUE_KEY:AYODY9tvQiP6RP8X9SmN-->Change this code to not log
user-controlled data. <p>See more on <a
href="https://sonarcloud.io/project/issues?id=apache_struts&issues=AYODY9tvQiP6RP8X9SmN&open=AYODY9tvQiP6RP8X9SmN&pullRequest=607";>SonarCloud</a></p>
[Show more
details](https://github.com/apache/struts/security/code-scanning/122)
Issue Time Tracking
-------------------
Worklog Id: (was: 812808)
Time Spent: 5h 20m (was: 5h 10m)
> 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
> Affects Versions: 6.0.0
> Reporter: Brian Andle
> Priority: Major
> Fix For: 6.1.0
>
> Time Spent: 5h 20m
> 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 issue 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.10#820010)
