Struts 2
  1. Struts 2
  2. WW-3973

WW-3866 overrides ParameterNameAware decision with interceptor settings

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.3.7
    • Fix Version/s: 2.3.12
    • Component/s: None
    • Labels:
      None

      Description

      The fix for WW-3866 (Revision 1379386) changes the logic for acceptable parameter names from

      com.opensymphony.xwork2.interceptor.ParametersInterceptor, line 282ff.
              boolean acceptableName = acceptableName(name)
                       && (parameterNameAware == null || parameterNameAware.acceptableParameterName(name));
      

      to

      com.opensymphony.xwork2.interceptor.ParametersInterceptor, line 282ff.
              boolean acceptableName = acceptableName(name)
                       || (parameterNameAware != null && parameterNameAware.acceptableParameterName(name));
      

      This might impose a security risk if implementations relied on their actions for parameter name validation (e.g. by explicitly whitelisting parameters).

        Activity

        Hide
        Johno Crawford added a comment -

        Lukasz Lenart new ticket and patch proposal over at WW-4068

        Show
        Johno Crawford added a comment - Lukasz Lenart new ticket and patch proposal over at WW-4068
        Hide
        Johno Crawford added a comment -

        Looks like I do not have permission to reopen the ticket so I will create a new one.

        Show
        Johno Crawford added a comment - Looks like I do not have permission to reopen the ticket so I will create a new one.
        Hide
        Lukasz Lenart added a comment -

        Johno Crawford pleas re-open this issue or create a new one. And propose your solution, where your last option is my favour way

        Show
        Lukasz Lenart added a comment - Johno Crawford pleas re-open this issue or create a new one. And propose your solution, where your last option is my favour way
        Hide
        Johno Crawford added a comment -

        Bit late to the party <insert slowpoke pic>, we would love to see a configuration option for this to return the old behaviour, or a way to invoke the global check from the Action overriding ParameterNameAware, orrrr refactoring ParametersInterceptor so it's easier to extend.

        Show
        Johno Crawford added a comment - Bit late to the party <insert slowpoke pic>, we would love to see a configuration option for this to return the old behaviour, or a way to invoke the global check from the Action overriding ParameterNameAware, orrrr refactoring ParametersInterceptor so it's easier to extend.
        Hide
        Lukasz Lenart added a comment -
        Show
        Lukasz Lenart added a comment - Link to proper Version Notes https://cwiki.apache.org/confluence/display/WW/Version+Notes+2.3.12
        Hide
        Hudson added a comment -

        Integrated in Struts2-JDK6 #635 (See https://builds.apache.org/job/Struts2-JDK6/635/)
        WW-3973 adds disclaimer about ParameterNameAware (Revision 1439410)

        Result = SUCCESS
        lukaszlenart :
        Files :

        • /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Show
        Hudson added a comment - Integrated in Struts2-JDK6 #635 (See https://builds.apache.org/job/Struts2-JDK6/635/ ) WW-3973 adds disclaimer about ParameterNameAware (Revision 1439410) Result = SUCCESS lukaszlenart : Files : /struts/struts2/trunk/xwork-core/src/main/java/com/opensymphony/xwork2/interceptor/ParametersInterceptor.java
        Hide
        Lukasz Lenart added a comment -
        Show
        Lukasz Lenart added a comment - Added info to Version Notes as well https://cwiki.apache.org/confluence/display/WW/Version+Notes+2.3.9
        Hide
        Lukasz Lenart added a comment -

        Thanks for pointing out this issue!

        Show
        Lukasz Lenart added a comment - Thanks for pointing out this issue!
        Hide
        Lukasz Lenart added a comment -
        Show
        Lukasz Lenart added a comment - Added warning to the docs https://cwiki.apache.org/confluence/display/WW/Parameters+Interceptor
        Hide
        Lukasz Lenart added a comment -

        I'm trying deeper understand the problem. With the current solution, ParameterNameAware#acceptableParameterName() is used to obey ParametersInterceptor restrictions. I think this is a good idea as you can define very strong restrictions per application (acceptParamNames, excludeParams) and relax them per action.

        Show
        Lukasz Lenart added a comment - I'm trying deeper understand the problem. With the current solution, ParameterNameAware#acceptableParameterName() is used to obey ParametersInterceptor restrictions. I think this is a good idea as you can define very strong restrictions per application (acceptParamNames, excludeParams) and relax them per action.
        Hide
        Christoph Lenggenhager added a comment -

        I would not opt for a switch, because configuration in this area should be as simple as possible.
        I understand the point with too restrictive rules.
        I think it is definitively worth a disclaimer in the docs (and maybe in the release notes if possible).
        We can live with a won't fix, now that we found out about it.
        Thank you that you took the time to discuss it.

        Show
        Christoph Lenggenhager added a comment - I would not opt for a switch, because configuration in this area should be as simple as possible. I understand the point with too restrictive rules. I think it is definitively worth a disclaimer in the docs (and maybe in the release notes if possible). We can live with a won't fix, now that we found out about it. Thank you that you took the time to discuss it.
        Hide
        Lukasz Lenart added a comment -

        Yes, but quite often the global restrictions was to restrictive, eg.:
        right now acceptedParamNames disallow empty space in param name to avoid remote code execution, but you can have a case where you need to have such a parameter -> 'car name' - in this case you can obey global restriction with ParameterNameAware#acceptableParameterName()

        Anyway, what is your proposal? Updating docs with some disclaimer about security is enough? Or do you want to have the old behaviour? We can think of some switch which will turn the old behaviour on, but thus could be hard to understand.

        Show
        Lukasz Lenart added a comment - Yes, but quite often the global restrictions was to restrictive, eg.: right now acceptedParamNames disallow empty space in param name to avoid remote code execution, but you can have a case where you need to have such a parameter -> 'car name' - in this case you can obey global restriction with ParameterNameAware#acceptableParameterName() Anyway, what is your proposal? Updating docs with some disclaimer about security is enough? Or do you want to have the old behaviour? We can think of some switch which will turn the old behaviour on, but thus could be hard to understand.
        Hide
        Christoph Lenggenhager added a comment -

        Agreed. Before 2.3.7, you could define a strict policy in the configuration and let the developers of actions allow a set of parameters that follow the defined "global" rules. This is now not possible any longer.
        I just think that the change is slightly dangerous for users relying on both mechanisms (configuration and ParameterNameAware actions) to validate their parameter list and that it should not be introduced in a point release without any heads up. This is why I raised the issue.

        Show
        Christoph Lenggenhager added a comment - Agreed. Before 2.3.7, you could define a strict policy in the configuration and let the developers of actions allow a set of parameters that follow the defined "global" rules. This is now not possible any longer. I just think that the change is slightly dangerous for users relying on both mechanisms (configuration and ParameterNameAware actions) to validate their parameter list and that it should not be introduced in a point release without any heads up. This is why I raised the issue.
        Hide
        Lukasz Lenart added a comment -

        But your idea is the same - give more power to user and allow him decided when to accept parameter and when not - you can define excludeParams like '*' and accept parameters with ParameterNameAware.

        Show
        Lukasz Lenart added a comment - But your idea is the same - give more power to user and allow him decided when to accept parameter and when not - you can define excludeParams like '*' and accept parameters with ParameterNameAware.
        Hide
        Christoph Lenggenhager added a comment -

        Obviously, it is not big deal to move the whole validation process into ParameterNameAware actions and configure the interceptor not to accept any parameter. However, we would have been quite exposed if we hadn't detected this during testing as our actions do parameter whitelisting.

        Show
        Christoph Lenggenhager added a comment - Obviously, it is not big deal to move the whole validation process into ParameterNameAware actions and configure the interceptor not to accept any parameter. However, we would have been quite exposed if we hadn't detected this during testing as our actions do parameter whitelisting.

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Christoph Lenggenhager
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development