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
