struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Don Brown" <>
Subject Re: [s2] Struts 2 and OGNL findings
Date Fri, 07 Sep 2007 16:20:36 GMT
Correct, static fields are still accessible, and actually, so are
static methods if you specify
struts.ognl.allowStaticMethodAccess=true in your configuration.  I just made the
default be false as a security precaution.


On 9/8/07, Jon Wilmoth <> 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 <>
> To: Struts Developers List <>
> 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:
> ?errors[%22bob%22%2b@java.lang.System@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="@java.lang.System@exit(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]
> [2]
> [3]
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message