[s2] Struts 2 and OGNL findings

[Don Brown <mr...@twdata.org>]

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

Re: [s2] Struts 2 and OGNL findings

[Ted Husted <hu...@apache.org>]

Thanks so much, Don. OGNL has been a point of concern, and I'm glad
you were able to look into it.

With TP5 moving away from OGNL, do we know of any other projects other
than XW still using it?

If not, then if we (meaning XW) starts to tweak OGNL, would it make
sense to bring OGNL into the XW codebase, eliminating a dependency?

-Ted.

On 9/7/07, Don Brown <mr...@twdata.org> wrote:
> 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.

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

Re: Struts 2 and OGNL findings

[Lukasz Lenart <lu...@googlemail.com>]

> Yeah, Struts 2 in Action isn't bad.
>
> :/
>
> Dave

I can confirm, it's better to have orginal version ;-)


Regards
-- 
Lukasz
http://www.lenart.org.pl/

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

Re: Struts 2 and OGNL findings

[Dave Newton <ne...@yahoo.com>]

--- On Thu, 9/25/08, Philip Luppens wrote:
> But all in all, a nice resort, and perhaps a good introduction for new 
> developers besides the current guides.

Yeah, Struts 2 in Action isn't bad.

:/

Dave


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

Re: Struts 2 and OGNL findings

[Philip Luppens <ph...@gmail.com>]

Thanks for letting us know, but I'd dare to say this is better
submitted to the user mailing list and added to the confluence wiki.
Also note (if you're the author) that your page numbering for the
articles is .. slightly confusing. Part 5 points to page2, Part 3 is
named differently, etc.

But all in all, a nice resort, and perhaps a good introduction for new
developers besides the current guides.

Thanks,

Phil

On Thu, Sep 25, 2008 at 9:48 AM, Sikandar123456
<si...@hotmail.com> wrote:
>
> Here is a good article on Struts 2 and OGNL
>
> http://struts2-java.blogspot.com/2008/09/struts-2-and-ognl.html
>
> --
> View this message in context: http://www.nabble.com/-s2--Struts-2-and-OGNL-findings-tp12558251p19664629.html
> Sent from the Struts - Dev mailing list archive at Nabble.com.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>



-- 
"We cannot change the cards we are dealt, just how we play the hand."
- Randy Pausch

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

Struts 2 and OGNL findings

[Sikandar123456 <si...@hotmail.com>]

Here is a good article on Struts 2 and OGNL

http://struts2-java.blogspot.com/2008/09/struts-2-and-ognl.html

-- 
View this message in context: http://www.nabble.com/-s2--Struts-2-and-OGNL-findings-tp12558251p19664629.html
Sent from the Struts - Dev mailing list archive at Nabble.com.


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