struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Don Brown" <donald.br...@gmail.com>
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
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 <jonwilmoth@yahoo.com> 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 <mrdon@twdata.org>
> 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:
>
> ?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] 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: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org


Mime
View raw message