Re: Characters allowed in map keys in parameters

2015-09-15 Thread Lukasz Lenart 
2015-09-14 21:53 GMT+02:00 Steven Willis :
> 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-unsubscr...@struts.apache.org
For additional commands, e-mail: user-h...@struts.apache.org

Re: Characters allowed in map keys in parameters

2015-09-15 Thread Christoph Nenning 
> 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 <lukaszlen...@apache.org>
> To: Struts Users Mailing List <user@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 <swil...@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-unsubscr...@struts.apache.org
> For additional commands, e-mail: user-h...@struts.apache.org
> 

This Email was scanned by Sophos Anti Virus