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]
