struts-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jon Wilmoth <>
Subject Re: [s2] Struts 2 and OGNL findings
Date Fri, 07 Sep 2007 16:10:08 GMT
Does #2 include access to static fields (i.e. Constants) or are these not affected by your


----- 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:


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
'=' - 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.



To unsubscribe, e-mail:
For additional commands, e-mail:
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message