[s2] Struts 2 and OGNL findings

[Don Brown-2]

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@...@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@...
For additional commands, e-mail: dev-help@...

[Jon Wilmoth-2]

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@...>
To: Struts Developers List <dev@...>
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@...@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@...
For additional commands, e-mail: dev-help@...

[Don Brown-5]

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@...> 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@...>
> To: Struts Developers List <dev@...>
> 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@...@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@...
> For additional commands, e-mail: dev-help@...

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

[Jon Wilmoth-2]

Sounds good.  Thanks.


----- Original Message ----
From: Don Brown <donald.brown@...>
To: Struts Developers List <dev@...>; Jon Wilmoth <jonwilmoth@...>
Sent: Friday, September 7, 2007 9:20:36 AM
Subject: Re: [s2] Struts 2 and OGNL findings


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@...> 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@...>
> To: Struts Developers List <dev@...>
> 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@...@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@...
> For additional commands, e-mail: dev-help@...

[Ted Husted]

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 <mrdon@...> 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@...
For additional commands, e-mail: dev-help@...

[Sikandar123456]

Here is a good article on Struts 2 and OGNL

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

[Philip Luppens]

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
<sikandar.786@...> 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@...
> For additional commands, e-mail: dev-help@...
>
>

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

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

[newton.dave]

--- 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@...
For additional commands, e-mail: dev-help@...

[Lukasz Lenart]

> 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@...
For additional commands, e-mail: dev-help@...