Struts 2
  1. Struts 2
  2. WW-2107

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.9
    • Fix Version/s: 2.0.10
    • Component/s: Plugin - Tags
    • Labels:
      None
    • Flags:
      Important

      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.

        Issue Links

          Activity

          Show
          Don Brown added a comment - See also http://forums.opensymphony.com/thread.jspa?messageID=176037
          Hide
          Don Brown added a comment -

          A similar issue, also can be triggered by validation errors

          Show
          Don Brown added a comment - A similar issue, also can be triggered by validation errors
          Hide
          musachy added a comment -

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

          {...}
          Show
          musachy added a comment - I know I've said this before, but I think we are better off blocking any parameter with a value that looks like % {...}
          Hide
          Dale Newfield added a comment -

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

          Show
          Dale Newfield added a comment - I'm just a struts user, and not a developer/committer, but I agree with Musachy.
          Hide
          Don Brown added a comment -

          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.

          Show
          Don Brown added a comment - 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.
          Hide
          Erlend Oftedal added a comment - - edited

          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.

          Show
          Erlend Oftedal added a comment - - edited 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.
          Hide
          Dale Newfield added a comment -

          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.

          Show
          Dale Newfield added a comment - 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.
          Hide
          Don Brown added a comment -

          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.

          Show
          Don Brown added a comment - 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.
          Hide
          James Holmes added a comment - - edited

          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.

          Show
          James Holmes added a comment - - edited 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.
          Hide
          Don Brown added a comment -

          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.rowattr.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...

          Show
          Don Brown added a comment - 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...
          Hide
          Erlend Oftedal added a comment -

          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?

          Show
          Erlend Oftedal added a comment - 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?
          Hide
          Don Brown added a comment -

          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.

          Show
          Don Brown added a comment - 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.
          Hide
          Don Brown added a comment -

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

          Show
          Don Brown added a comment - Well, I think this has been open long enough. If anyone has any ideas how to better resolve it, feel free to reopen.
          Hide
          Nestor Boscan added a comment -

          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

          Show
          Nestor Boscan added a comment - 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
          Hide
          Nestor Boscan added a comment -

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

          Show
          Nestor Boscan added a comment - Another solution (much easier) will be to let the developer tell Struts 2 to deactivate OGNL evaluation on tags and use only JSTL.
          Hide
          Don Brown added a comment -

          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

          Show
          Don Brown added a comment - 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
          Hide
          Adam Hardy added a comment -

          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.

          Show
          Adam Hardy added a comment - 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.
          Hide
          Ted Husted added a comment -

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

          Show
          Ted Husted added a comment - > 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.

            People

            • Assignee:
              Don Brown
              Reporter:
              Don Brown
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development