Arbitrary user-submitted OGNL possible when using JSP EL or FreeMarker

• Key: WW-2107
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Blocker
• Assignee: Don Brown
• Reporter: Don Brown
• Component/s: Plugin - Tags
• Affects Version/s: 2.0.9
• Fix Version/s: 2.0.10

Description

It is possible for a user to submit malicious OGNL that could be executed in a page that uses JSP EL expressions in Struts tag attributes. FreeMarker pages that use FreeMarker expressions in Struts tag attributes are also affected. Velocity pages are not affected.

For example, say you had this JSP page fragement:

<s:text name="foo" value="${bar}" />

And a user submitted, via a validation error or request url query parameter, the value:

bar=%{1+1}

What happens is the JSP processor gets the page first and processes the JSP EL expression resulting in:

<s:text name="foo" value="%{1+1}" />

Then, the Struts 2 tag receives the 'value' attribute value and processes the OGNL expression, resulting in this:

<input type="text" name="foo" value="2" />

The workaround is to ensure you don't use JSP EL or FreeMarker expressions in Struts tag attributes because you could be unwittingly allowing arbitrary code execution.

The proposed solution is to turn off, via the TLD, JSP EL expressions in all Struts tag attributes. This will mostly likely break many Struts 2 applications, but the severity of the issue needs to be taken into account. This solution doesn't unfortunately resolve the FreeMarker issue.

Comments

[Don Brown]

See also http://forums.opensymphony.com/thread.jspa?messageID=176037

[Don Brown]

A similar issue, also can be triggered by validation errors

[Musachy Barroso]

I know I've said this before, but I think we are better off blocking any parameter with a value that looks like %{...}

[Dale Newfield]

I'm just a struts user, and not a developer/committer, but I agree with Musachy.

[Don Brown]

I thought about that as well, but it wouldn't help. For one thing, we would have to block the string '%{' anywhere in the submitted data, which is very intrusive, but the kicker is it wouldn't block other attacks. For example, any attribute that evaluations to a non-string value is automatically evaluated as OGNL with no '%{}' delimiter necessary. Therefore, blocking '%{}' would help some cases, but still leave you open for attack in others.

[Erlend Oftedal]

This seems like a metacharacter problem, just like Cross site scripting or SQL-injection. The correct solution would be to escape all OGNL metacharacters when you first create the OGNL expression. Just like escaping ' and " in SQL statements or <,'," and > in HTML. The data must then be unescaped when the OGNL-expression parsing/execution is done.

[Dale Newfield]

Indeed, this, and the fact that OGNL chooses to evaluate stuff that doesn't ask for it seems like a bad idea to me.
(IMHO, not including "%{}" in your code is poor style and, moreover, having that work leads to this security hole.)
If we're going to make a change that forces people to update their code to have it continue to work, I would argue that a better change would be to force them to be explicit about whether the tag attribute should be passed through an OGNL evaluation.

[Don Brown]

Regarding escaping, I'm not sure how that would help us here. The issue is that the tag attributes expect OGNL expressions, but we can't distinguish between the original values and those that are the product of a JSP EL expression.

Regarding requiring an explicit notation, unfortunately, that still wouldn't address the core issue, although it might be a good idea. As noted above, we would need to be able to determine who entered the OGNL expression, but because of the way JSP works where we can't get the value before it is processed by JSP EL, we can't.

[James Holmes]

This change definitely breaks the application I am working on. I am using JSP EL to dynamically specify an OGNL property. Here is an example:

<display:table id="row" name="groupSeekers" requestURI="${tableUrl}" defaultsort="${defaultSortColumnIndex}" defaultorder="${defaultSortOrder}" class="listing">
    <s:iterator value="selectedColumns">
        <s:if test="%{dateColumn}">
            <display:column title="${title}" sortable="true" headerClass="sortable">
                <s:date name="#attr.row.${property}" format="MMM d, yyyy, hh:mm a"/>
            </display:column>
        </s:if>
        <s:else>
            <display:column property="${property}" title="${title}" sortable="true" headerClass="sortable" autolink="true"/>
        </s:else>
    </s:iterator>
</display:table>

Notice the <s:date> tag. Basically I have a configurable set of columns for my table and each column is represented by a TableColumn object. One of the fields on the TableColumn object is "property" which specifies the field name on the object representing the row.

I'm not an OGNL expert, so I don't know if there is a way to represent "#attr.row.${property}" using a pure OGNL expression instead of incorporating JSP EL to dynamically specify the name of the property. I'm happy to use just OGNL if I can find a solution.

[Don Brown]

Yes, this change can be very disruptive, I agree. As for your particular issue, yes, you can do it with pure ognl, something like:
#attr.row[#attr.property]

What I don't like about this change is it really forces users to use ognl where I know I personally would prefer to stick to one EL in a JSP app. But as I said, I can't think of a better way...

[Erlend Oftedal]

It's disruptive, but having worms or similar constantly taking down your site would be a lot worse :-)

Is it possible to add a wrapper over freemarker or jsp el, or an interceptor that is only run when freemaker or jsp EL has been running?

[Don Brown]

But what would this interceptor/wrapper do? Unfortunately, it isn't as simple as looking for '%{}' strings because non-string attributes don't require those delimiters. The best you could do is customize the TLD for yourself, turning EL on in all attributes, then ensuring you are writing your tags correctly. This bug is one that only really shows up if you use JSP EL in certain erroneous ways, but if you are careful, the two could co-exist.

[Don Brown]

Well, I think this has been open long enough. If anyone has any ideas how to better resolve it, feel free to reopen.

[Nestor Boscan]

So basically the problem is that it's evaluating two expression languages JSTL + OGNL. I don't think that a good solution is to simply eliminate JSTL, why not eliminate OGNL some will say. One solution that I propose is to let the developer choose which language it wants to evaluate INSIDE the tag. I can choose between OGNL, JSTL, etc. Inside the tag you will evaluate the expression using the choosen languages. This way we can choose the language that we like and make Struts 2 extensible for other expression languages.

Regards,

Néstor Boscán

[Nestor Boscan]

Another solution (much easier) will be to let the developer tell Struts 2 to deactivate OGNL evaluation on tags and use only JSTL.

[Don Brown]

Unfortunately, just disabling OGNL isn't possible, even just for tag evaluation. However, as an additional measure, I disabled (in 2.1 at least, could backport it if there is interest) static method access in OGNL expressions, which is the OGNL feature that makes this issue so severe. Most users don't even know that the feature is there, so I think it makes sense to turn it off, and let the user turn it on if they need. See WW-2160

[Adam Hardy]

I suggest that the Struts1 HTML taglib with all the form tags be ported to S2 and implemented without OGNL. This would provide an alternative to Struts users who wish to use JSTL.

[Ted Husted]

> I suggest that the Struts1 HTML taglib with all the form tags be ported to S2 and implemented without OGNL.
> This would provide an alternative to Struts users who wish to use JSTL.

Adam,

If you would like to give this a try, please feel free (under a new ticket).

It would be interesting to see conventional JSP tags implemented under Struts 2 without OGNL. Of course, the tags would not be available to Velocity templates, but they could still be used by FreeMarker, and we could also port the "Velocity Tools" to Struts 2. It would also be interesting to see how a S1 JSP taglib for S2 would interact with the S2/S1 Action class plugin.

Ted.