struts-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lukasz Lenart <lukaszlen...@apache.org>
Subject Re: Characters allowed in map keys in parameters
Date Tue, 15 Sep 2015 06:54:01 GMT
2015-09-14 21:53 GMT+02:00 Steven Willis <swillis@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


Mime
View raw message