You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@struts.apache.org by Steven Willis <sw...@cargurus.com> on 2015/09/14 21:53:22 UTC
Characters allowed in map keys in parameters
I've been having issues with map keys in struts and I finally tracked it down to the pattern here:
https://github.com/apache/struts/blob/master/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java#L19
Which is:
"\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'\\])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*"
This apparently restricts map keys to strings of word characters [a-zA-Z0-9_] or virtually all of the 20,950 characters in the CJK Unified Ideographs (why does it stop at 9FA5 and not 9FFF or 9FD5?). Is there some justification for which characters were allowed here, and why things like spaces and slashes are excluded? It seems like you could allow anything except for single quotes and you'd be safe, right?
I've read through all of the following which seemed to be either directly or indirectly related to the issue:
https://issues.apache.org/jira/browse/WW-4250
http://markmail.org/message/y7d6hgftyf2jauz5
https://cwiki.apache.org/confluence/display/WW/S2-003
https://cwiki.apache.org/confluence/display/WW/S2-005
https://cwiki.apache.org/confluence/display/WW/S2-008
https://cwiki.apache.org/confluence/display/WW/S2-009
https://issues.apache.org/jira/browse/WW-3729
https://issues.apache.org/jira/browse/WW-3668
https://issues.apache.org/jira/browse/WW-4257
Some of the messages say that allowing spaces is a security vulnerability, but how could that be? Isn't foo['hello world'] and foo['hello/world'] just as safe as foo['hello_world'] ? The actual security vulnerabilities seem related to other forms parameter values (using #, or forms that aren't inside single quoted strings).
This commit:
https://github.com/apache/struts/commit/8a93df10c4f5f3f22f1837c47b4ca9b4facc4f94#diff-d6b23e0dce6eef0d9662cbfacbc8c916L376
Changed the testParametersWithSpacesInTheName test to expect spaces not to work rather than to work. But the commit message doesn't explain why.
Actually looks like the test has flipped back and forth between expecting spaces to work and not work a few times. I just found what I believe is the earliest commit that has a reference to not accepting spaces (only accessible via tags that are not ancestors of master):
https://github.com/apache/struts/commit/41f90ae39d0783f64641726e7e6b4741663c04bd#diff-d6b23e0dce6eef0d9662cbfacbc8c916
That commit also doesn't explain why spaces shouldn't be allowed.
-Steven Willis
Re: Characters allowed in map keys in parameters
Posted by Christoph Nenning <Ch...@lex-com.net>.
> That being said, I think we should try change this pattern to allow
> params like this "map['my key']"
This would really make my day :)
> From: Lukasz Lenart <lu...@apache.org>
> To: Struts Users Mailing List <us...@struts.apache.org>,
> Date: 15.09.2015 08:54
> Subject: Re: Characters allowed in map keys in parameters
>
> 2015-09-14 21:53 GMT+02:00 Steven Willis <sw...@cargurus.com>:
> > I've been having issues with map keys in struts and I finally
> tracked it down to the pattern here:
> >
> > https://github.com/apache/struts/blob/master/core/src/main/
>
java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java#L19
> >
> >
> > Which is:
> >
> > "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\
> \u9fa5])+'\\])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*"
> >
> > This apparently restricts map keys to strings of word characters
> [a-zA-Z0-9_] or virtually all of the 20,950 characters in the CJK
> Unified Ideographs (why does it stop at 9FA5 and not 9FFF or 9FD5?).
> Is there some justification for which characters were allowed here,
> and why things like spaces and slashes are excluded? It seems like
> you could allow anything except for single quotes and you'd be safe,
right?
> >
> > I've read through all of the following which seemed to be either
> directly or indirectly related to the issue:
> >
> > https://issues.apache.org/jira/browse/WW-4250
> >
> > http://markmail.org/message/y7d6hgftyf2jauz5
> >
> > https://cwiki.apache.org/confluence/display/WW/S2-003
> > https://cwiki.apache.org/confluence/display/WW/S2-005
> >
> > https://cwiki.apache.org/confluence/display/WW/S2-008
> > https://cwiki.apache.org/confluence/display/WW/S2-009
> > https://issues.apache.org/jira/browse/WW-3729
> > https://issues.apache.org/jira/browse/WW-3668
> > https://issues.apache.org/jira/browse/WW-4257
> >
> > Some of the messages say that allowing spaces is a security
> vulnerability, but how could that be? Isn't foo['hello world'] and
> foo['hello/world'] just as safe as foo['hello_world'] ? The actual
> security vulnerabilities seem related to other forms parameter
> values (using #, or forms that aren't inside single quoted strings).
> >
> >
> > This commit:
> >
> > https://github.com/apache/struts/commit/
> 8a93df10c4f5f3f22f1837c47b4ca9b4facc4f94#diff-
> d6b23e0dce6eef0d9662cbfacbc8c916L376
> >
> > Changed the testParametersWithSpacesInTheName test to expect
> spaces not to work rather than to work. But the commit message
> doesn't explain why.
> >
> > Actually looks like the test has flipped back and forth between
> expecting spaces to work and not work a few times. I just found what
> I believe is the earliest commit that has a reference to not
> accepting spaces (only accessible via tags that are not ancestors of
master):
> >
> > https://github.com/apache/struts/commit/
>
41f90ae39d0783f64641726e7e6b4741663c04bd#diff-d6b23e0dce6eef0d9662cbfacbc8c916
> >
> > That commit also doesn't explain why spaces shouldn't be allowed.
> >
> > -Steven Willis
>
> There is no simple answer :) There was a lot of examples which were
> using the 'new' keyword to create instances, ie. 'x=new
> org.apache.struts2.ImportantObject(); #x.unsecureOperation()' and also
> examples of accessing internal scopes, ie. '#' + 'application' and so
> on. So we dropped support for spaces in param names 'just in case' :)
>
> Also using a RegEx to filter them out it's just a temporary solution
> and not the best one (I'm working on better solution to simple
> distinguish origins of each parameter - external or internal).
>
> And I think after we have introduced the Internal Security mechanism
> [1] a lot of those RegExs are invalid now, but we afraid too much to
> relax them :\
>
> That being said, I think we should try change this pattern to allow
> params like this "map['my key']"
>
> [1] http://struts.apache.org/docs/security.html#Security-
> Internalsecuritymechanism
>
>
> Regards
> --
> Łukasz
> + 48 606 323 122 http://www.lenart.org.pl/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
> For additional commands, e-mail: user-help@struts.apache.org
>
This Email was scanned by Sophos Anti Virus
Re: Characters allowed in map keys in parameters
Posted by Lukasz Lenart <lu...@apache.org>.
2015-09-14 21:53 GMT+02:00 Steven Willis <sw...@cargurus.com>:
> I've been having issues with map keys in struts and I finally tracked it down to the pattern here:
>
> https://github.com/apache/struts/blob/master/core/src/main/java/com/opensymphony/xwork2/security/DefaultAcceptedPatternsChecker.java#L19
>
>
> Which is:
>
> "\\w+((\\.\\w+)|(\\[\\d+\\])|(\\(\\d+\\))|(\\['(\\w|[\\u4e00-\\u9fa5])+'\\])|(\\('(\\w|[\\u4e00-\\u9fa5])+'\\)))*"
>
> This apparently restricts map keys to strings of word characters [a-zA-Z0-9_] or virtually all of the 20,950 characters in the CJK Unified Ideographs (why does it stop at 9FA5 and not 9FFF or 9FD5?). Is there some justification for which characters were allowed here, and why things like spaces and slashes are excluded? It seems like you could allow anything except for single quotes and you'd be safe, right?
>
> I've read through all of the following which seemed to be either directly or indirectly related to the issue:
>
> https://issues.apache.org/jira/browse/WW-4250
>
> http://markmail.org/message/y7d6hgftyf2jauz5
>
> https://cwiki.apache.org/confluence/display/WW/S2-003
> https://cwiki.apache.org/confluence/display/WW/S2-005
>
> https://cwiki.apache.org/confluence/display/WW/S2-008
> https://cwiki.apache.org/confluence/display/WW/S2-009
> https://issues.apache.org/jira/browse/WW-3729
> https://issues.apache.org/jira/browse/WW-3668
> https://issues.apache.org/jira/browse/WW-4257
>
> Some of the messages say that allowing spaces is a security vulnerability, but how could that be? Isn't foo['hello world'] and foo['hello/world'] just as safe as foo['hello_world'] ? The actual security vulnerabilities seem related to other forms parameter values (using #, or forms that aren't inside single quoted strings).
>
>
> This commit:
>
> https://github.com/apache/struts/commit/8a93df10c4f5f3f22f1837c47b4ca9b4facc4f94#diff-d6b23e0dce6eef0d9662cbfacbc8c916L376
>
> Changed the testParametersWithSpacesInTheName test to expect spaces not to work rather than to work. But the commit message doesn't explain why.
>
> Actually looks like the test has flipped back and forth between expecting spaces to work and not work a few times. I just found what I believe is the earliest commit that has a reference to not accepting spaces (only accessible via tags that are not ancestors of master):
>
> https://github.com/apache/struts/commit/41f90ae39d0783f64641726e7e6b4741663c04bd#diff-d6b23e0dce6eef0d9662cbfacbc8c916
>
> That commit also doesn't explain why spaces shouldn't be allowed.
>
> -Steven Willis
There is no simple answer :) There was a lot of examples which were
using the 'new' keyword to create instances, ie. 'x=new
org.apache.struts2.ImportantObject(); #x.unsecureOperation()' and also
examples of accessing internal scopes, ie. '#' + 'application' and so
on. So we dropped support for spaces in param names 'just in case' :)
Also using a RegEx to filter them out it's just a temporary solution
and not the best one (I'm working on better solution to simple
distinguish origins of each parameter - external or internal).
And I think after we have introduced the Internal Security mechanism
[1] a lot of those RegExs are invalid now, but we afraid too much to
relax them :\
That being said, I think we should try change this pattern to allow
params like this "map['my key']"
[1] http://struts.apache.org/docs/security.html#Security-Internalsecuritymechanism
Regards
--
Łukasz
+ 48 606 323 122 http://www.lenart.org.pl/
---------------------------------------------------------------------
To unsubscribe, e-mail: user-unsubscribe@struts.apache.org
For additional commands, e-mail: user-help@struts.apache.org