Sounds good. Thanks.
----- Original Message ---- From: Don Brown <[EMAIL PROTECTED]> To: Struts Developers List <dev@struts.apache.org>; Jon Wilmoth <[EMAIL PROTECTED]> Sent: Friday, September 7, 2007 9:20:36 AM Subject: Re: [s2] Struts 2 and OGNL findings Correct, static fields are still accessible, and actually, so are static methods if you specify struts.ognl.allowStaticMethodAccess=true in your struts.properties/web.xml/struts.xml configuration. I just made the default be false as a security precaution. Don On 9/8/07, Jon Wilmoth <[EMAIL PROTECTED]> wrote: > Does #2 include access to static fields (i.e. Constants) or are these not > affected by your change? > > Thanks, > Jon > > ----- Original Message ---- > From: Don Brown <[EMAIL PROTECTED]> > To: Struts Developers List <dev@struts.apache.org> > Sent: Friday, September 7, 2007 9:04:37 AM > Subject: [s2] Struts 2 and OGNL findings > > > I spent the last couple days looking into the OGNL situation in Struts > 2 and XWork, with the intent on if not eliminating it, at least > blocking it cleanly off. The summary is this: > 1. Refactored the TypeConverter system away from OGNL into new XWork API [1] > 2. Turned off static method access in OGNL expressions for Struts 2.1 [2] [3] > 3. Finally convinced myself OGNL, if used right, is safe > 4. Again realized how deeply OGNL is used within XWork and how much > the Struts 2 featureset depends on it > > While 1 and 2 should be clear from the tickets and commits, 3 and 4 > require more explanation. My concern about OGNL, from a fundamental > security perspective, was that it treats everything as expressions. > Every request parameter is parsed as an OGNL expression, and OGNL > expressions can have operators, method calls, object creation, and > even functions. Turns out, you are safe here because of two things: > > 1. OGNL treats expressions meant for "set" operations differently that > "get". In processing an expression, OGNL converts the string into AST > objects - ASTMethod, ASTAssign, ASTAdd, etc. The next step is > executing these objects, but it does so via their setValue() and > getValue() methods. So yes, in constructing a request parameter name, > you can create an OGNL expression that contains a static method, say: > > [EMAIL PROTECTED]@exit(1)]=bar > > but, while the System.exit(1) call is parsed correctly into a static > method, the ASTStaticMethod object doesn't execute it when its > setValue() method is called. On the other hand, if you used this > expression in a tag like so: > > <s:inputtransferselect list="@[EMAIL PROTECTED](1)" > name="favouriteNumbers" /> > > the getValue() method will be called, in which case, the static > System.exit() method is executed. So, while it still seems a bit > dodgy to be treating request parameter names as OGNL expressions, I > didn't find a way to exploit this. > > 2. But what about the variable assignment and object creation you say? > Well, all parameters are checked for illegal characters before being > processed: > '=' - Don't allow sneaky assignments > ',' - Dont' allow multiple statements > '#' - Don't allow object creation > ':' - Don't allow functions or as the OGNL docs call them, > "Pseudo-Lambda Expressions" > > Finally, for #4, I again discovered how deeply OGNL is core to XWork > and therefore Struts 2. We build some of our core features such as > automatic object creation (null handling), property access, value > stack operations, and type conversion (although I changed that in #1) > off what OGNL provides and allows us to extend. > > Even if we abstracted it to the level of allowing us to switch OGNL > for another EL, I'm not sure that would really get us. We'd have to > go back and re-implement all these features on the new EL and hope it > was flexible enough to make it possible. Furthermore, there is a good > chance the EL would be tied in some way to a view technology (Unified > EL - JSP), so we'd have to find ways to keep our Velocity and > Freemarker friends equally supported. > > My conclusion is OGNL is like Maven 2 - sometimes it really pisses you > off, and you probably generally don't like the thing, but you've > invested so much into it that it would be too painful to switch, and > really, it does 95% of what you want anyways. Switching the EL for > the sake of switching isn't interesting to me, so I'd rather hear a > list of things OGNL can't or isn't doing for us now and see if either > we could fix OGNL or XWork to handle it better, or if we do, in fact, > need to throw it out the door. > > Don > > > [1] http://jira.opensymphony.com/browse/XW-561 > [2] https://issues.apache.org/struts/browse/WW-2160 > [3] http://jira.opensymphony.com/browse/XW-562 > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED]
