Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-2264

A session value is overwrited by requesting.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Not A Problem
    • Affects Version/s: 2.0.9
    • Fix Version/s: 2.0.9
    • Component/s: Value Stack
    • Labels:
      None
    • Environment:

      I tested in struts2.0.9

      Description

      The attacker can inject the given value into session map by clicking following URL.

      http://example.com/SomeAction.action?session.somekey=someValue

      [[A session value is overwrited by demanding a browser. ]]
      FROM: hisato.killing@gmail.com
      TO: struts-dev
      >>>>
      1.This problem is caused in struts 2.0.9 and others perhaps.

      In that case, it is assumed that it is as follows.
      i. SomeAction is implements SessionAware.
      ii. And It is defined in struts-default.
      iii. devMode is true or false.

      ["someValue"] of the name of "someKey" enters in SessionMap when the
      request shown in that URL is processed.
      It is meant that ["someValue"] is an array including "someValue".
      This causes ClassCastException in case of almost.

      hisato.killing@gmail.com
      It is thought that this only has to be my mistake ,setting etc.

      Thanks

      1. s2inject.zip
        6 kB
        Hisato Killing

        Activity

        Hide
        hisato.killing Hisato Killing added a comment -

        Here is a sample.

        Show
        hisato.killing Hisato Killing added a comment - Here is a sample.
        Hide
        pluppens Philip Luppens added a comment -

        Like Jim Cushing pointed out, the most obvious fix would be to include a flag for OGNL to enable/disable access to private/protected methods and variables, and adding a warning that both the field and getter for the Session map for Actions that implement SessionAware should be private or protected.

        Show
        pluppens Philip Luppens added a comment - Like Jim Cushing pointed out, the most obvious fix would be to include a flag for OGNL to enable/disable access to private/protected methods and variables, and adding a warning that both the field and getter for the Session map for Actions that implement SessionAware should be private or protected.
        Hide
        mrdon Don Brown added a comment -

        I guess I"m not seeing the problem - if you don't declare a getSession() method, then why would you be at risk? The SessionAware interface only requires a setSession(Map). Also, we only allow OGNL access to public methods anyways, so I don't see how the additional flag would help.

        Show
        mrdon Don Brown added a comment - I guess I"m not seeing the problem - if you don't declare a getSession() method, then why would you be at risk? The SessionAware interface only requires a setSession(Map). Also, we only allow OGNL access to public methods anyways, so I don't see how the additional flag would help.
        Hide
        pluppens Philip Luppens added a comment -

        Ah, I was under the impression that OGNL would fall back on reflective field access (and bypass any private modifier) - I guess I misunderstood Tom's reply then. The additional flag would be for those that use OGNL in a non-struts 2/WW enviroment, where one can specify this behaviour. But that is meant as an enhancement for OGNL 2.6.x (once again, if the private access statement holds any ground), not S2.

        Nonetheless, I believe a warning (be it in the wiki) is necessary.

        Show
        pluppens Philip Luppens added a comment - Ah, I was under the impression that OGNL would fall back on reflective field access (and bypass any private modifier) - I guess I misunderstood Tom's reply then. The additional flag would be for those that use OGNL in a non-struts 2/WW enviroment, where one can specify this behaviour. But that is meant as an enhancement for OGNL 2.6.x (once again, if the private access statement holds any ground), not S2. Nonetheless, I believe a warning (be it in the wiki) is necessary.
        Hide
        mrdon Don Brown added a comment -

        Agreed, we can't do enough to emphasize the importance of being careful what values you expose via public methods on an action, but to verify this ticket is invalid as written, I added unit tests to the ParametersInterceptorTest to ensure that private fields and protected accessor methods are not affected.

        Show
        mrdon Don Brown added a comment - Agreed, we can't do enough to emphasize the importance of being careful what values you expose via public methods on an action, but to verify this ticket is invalid as written, I added unit tests to the ParametersInterceptorTest to ensure that private fields and protected accessor methods are not affected.
        Hide
        jimothy Jim Cushing added a comment -

        When I take Hisato's sample code as is, I get a ClassCastException. If I remove the cast (see below), "[Ljava.lang.String;@4d98b4" prints to the console, but the rendered JSP still displays "someValue is someValue".

        So it looks like SessionUser's local variable 'session' does get changed (it's trying to set a String array? why?), but THE session (associated with JSESSIONID) does not.

        It still seems like OGNL might be messing around with private variables.

        Show
        jimothy Jim Cushing added a comment - When I take Hisato's sample code as is, I get a ClassCastException. If I remove the cast (see below), "[Ljava.lang.String;@4d98b4" prints to the console, but the rendered JSP still displays "someValue is someValue". So it looks like SessionUser's local variable 'session' does get changed (it's trying to set a String array? why?), but THE session (associated with JSESSIONID) does not. It still seems like OGNL might be messing around with private variables.
        Hide
        jimothy Jim Cushing added a comment -

        My trivial change to Hisato's code:

        Object sessionValue = session.get("someKey");
        System.out.println(sessionValue);

        (Change sessionValue to an Object instead of a String)

        Show
        jimothy Jim Cushing added a comment - My trivial change to Hisato's code: Object sessionValue = session.get("someKey"); System.out.println(sessionValue); (Change sessionValue to an Object instead of a String)
        Hide
        pluppens Philip Luppens added a comment -

        Indeed, I can confirm this behaviour - I am able to set a property on a non-public map property with a public setter. There definitely is a fallback to the property, even if it's declared private. So the security problems still stands. Quite strange, because from the first looks of it, our memberaccess declared in XW shouldn't allow it.

        When a public setter is available, access to a protected/private map is in fact granted, and we are allowed to change/set variables.

        To test this, I added the following in SimpleAction:

        public void setThePrivateMap(Map map) {
        this.protectedMap = map;
        }

        Note, no public getter.

        And in the testcase ParametersInterceptorTest:
        public void testParametersNotAccessProtectedMethods() throws Exception {
        ...
        params.put("thePrivateMap.koo", "This is blah");
        ...
        }

        Show
        pluppens Philip Luppens added a comment - Indeed, I can confirm this behaviour - I am able to set a property on a non-public map property with a public setter. There definitely is a fallback to the property, even if it's declared private. So the security problems still stands. Quite strange, because from the first looks of it, our memberaccess declared in XW shouldn't allow it. When a public setter is available, access to a protected/private map is in fact granted, and we are allowed to change/set variables. To test this, I added the following in SimpleAction: public void setThePrivateMap(Map map) { this.protectedMap = map; } Note, no public getter. And in the testcase ParametersInterceptorTest: public void testParametersNotAccessProtectedMethods() throws Exception { ... params.put("thePrivateMap.koo", "This is blah"); ... }
        Hide
        mrdon Don Brown added a comment -

        Getting closer OGNL still does not access private or protected variables or methods. What is happening is OGNL only sees a setter, so it creates a new map, adds the request parameter to it, then sets it on the Action. Therefore, the session map in question is being overridden, but not modified directly by OGNL. I added a new unit test to ParametersInterceptorTest to prove this.

        So the question remains, is this a gaping security hole? I'd argue not because the session isn't being attacked. Is it a possible source of errors and perhaps in some limited cases, security breaches? Yep, so I'd definitely consider it a bug, but not one to warrant an immediate security release.

        As for a quick solution, we could modify the parametersinterceptor config to by default ignore the 'session' parameters. This can be done in struts-default.xml or in the user's specific interceptor stack. Another workaround for the user is to implement ParameterNameAware, which lets them specify a list of acceptable parameters.

        A new feature we could add would be a new annotation so that a user could annotate which setters/getters can be accessed, which is probably a good idea regardless.

        Show
        mrdon Don Brown added a comment - Getting closer OGNL still does not access private or protected variables or methods. What is happening is OGNL only sees a setter, so it creates a new map, adds the request parameter to it, then sets it on the Action. Therefore, the session map in question is being overridden, but not modified directly by OGNL. I added a new unit test to ParametersInterceptorTest to prove this. So the question remains, is this a gaping security hole? I'd argue not because the session isn't being attacked. Is it a possible source of errors and perhaps in some limited cases, security breaches? Yep, so I'd definitely consider it a bug, but not one to warrant an immediate security release. As for a quick solution, we could modify the parametersinterceptor config to by default ignore the 'session' parameters. This can be done in struts-default.xml or in the user's specific interceptor stack. Another workaround for the user is to implement ParameterNameAware, which lets them specify a list of acceptable parameters. A new feature we could add would be a new annotation so that a user could annotate which setters/getters can be accessed, which is probably a good idea regardless.
        Hide
        brenmcguire Antonio Petrelli added a comment -

        It could be a security hole anyway: when it is evaluated, is the original session or the overridden one used?
        It is a security hole in the latter case, because it evaluates a possible malicious code.

        Show
        brenmcguire Antonio Petrelli added a comment - It could be a security hole anyway: when it is evaluated, is the original session or the overridden one used? It is a security hole in the latter case, because it evaluates a possible malicious code.
        Hide
        pluppens Philip Luppens added a comment -

        Don: after some sleep, I came to the same conclusion (actually, my tests had been reporting it all along, I just looked over it ..).

        Antonio: It's definitely a security hole, since your local session variables is overwritten with the new map. I guess those using a security system (where they store the access rights in the session) are indeed at risk.

        Show
        pluppens Philip Luppens added a comment - Don: after some sleep, I came to the same conclusion (actually, my tests had been reporting it all along, I just looked over it ..). Antonio: It's definitely a security hole, since your local session variables is overwritten with the new map. I guess those using a security system (where they store the access rights in the session) are indeed at risk.
        Hide
        pledge Martin Gilday added a comment -

        "A new feature we could add would be a new annotation so that a user could annotate which setters/getters can be accessed, which is probably a good idea regardless".

        I think this would be a great addition. Many other frameworks have this feature. I'd rather annotate what should be allowed to be set per Action, than have to define inclusions/exclusions on the interceptor definition (or implement the interface). Would you see this as an addition to the existing parameters interceptor or somewhere else?

        Show
        pledge Martin Gilday added a comment - "A new feature we could add would be a new annotation so that a user could annotate which setters/getters can be accessed, which is probably a good idea regardless". I think this would be a great addition. Many other frameworks have this feature. I'd rather annotate what should be allowed to be set per Action, than have to define inclusions/exclusions on the interceptor definition (or implement the interface). Would you see this as an addition to the existing parameters interceptor or somewhere else?
        Hide
        mrdon Don Brown added a comment -

        Philip: agreed, but that is why I would not consider it a security hole. The session remains untouched; only the action is affected as it gets a new map that it thinks is backed by the session. If the action was really worried about security, it would implement the ParameterNameAware interface. Anyways, I think the fix here is to add "session" to the list of parameters that aren't accepted by the ParametersInterceptor, even though this may mean breaking some existing applications. I don't think it is enough to warrant a hurried security patch and downgrading of existing GA releases, but it should be backported to the 2.0.x branch and released with the next version.

        Show
        mrdon Don Brown added a comment - Philip: agreed, but that is why I would not consider it a security hole. The session remains untouched; only the action is affected as it gets a new map that it thinks is backed by the session. If the action was really worried about security, it would implement the ParameterNameAware interface. Anyways, I think the fix here is to add "session" to the list of parameters that aren't accepted by the ParametersInterceptor, even though this may mean breaking some existing applications. I don't think it is enough to warrant a hurried security patch and downgrading of existing GA releases, but it should be backported to the 2.0.x branch and released with the next version.
        Hide
        brenmcguire Antonio Petrelli added a comment -

        Don, probably I have not been clear with what I meant.
        The problem is not in the session values itself, but how they are evaluated.
        If the session remains untouched, but you evaluate the "overridden" session map, and in this map there is a malicious code (for example, System.exit), there is a security hole indeed.
        The question that I raised is: are these "overridden" session attributes evaluated by OGNL as OGNL expressions? Or do they remain only strings?

        Show
        brenmcguire Antonio Petrelli added a comment - Don, probably I have not been clear with what I meant. The problem is not in the session values itself, but how they are evaluated. If the session remains untouched, but you evaluate the "overridden" session map, and in this map there is a malicious code (for example, System.exit), there is a security hole indeed. The question that I raised is: are these "overridden" session attributes evaluated by OGNL as OGNL expressions? Or do they remain only strings?
        Hide
        mrdon Don Brown added a comment -

        The values in the map are evaluated as simple Strings. There are other protections in place to ensure it is not possible to execute methods simply by putting ognl expressions in the query string, and this case is no exception. Therefore, the most a malicious user could do is call your setSession() method with a map of a single String key/value pair. While this is annoying and could potentially be an issue (which is why I suggest we block the 'session' parameter in the next release), it does not allow any method execution or server-side state change, and therefore, I don't consider it a security hole.

        Show
        mrdon Don Brown added a comment - The values in the map are evaluated as simple Strings. There are other protections in place to ensure it is not possible to execute methods simply by putting ognl expressions in the query string, and this case is no exception. Therefore, the most a malicious user could do is call your setSession() method with a map of a single String key/value pair. While this is annoying and could potentially be an issue (which is why I suggest we block the 'session' parameter in the next release), it does not allow any method execution or server-side state change, and therefore, I don't consider it a security hole.
        Hide
        hisato.killing Hisato Killing added a comment -

        At last, this problem was able to be understood thanks to everybody

        I will close this issue for the following reasons.

        1. The query above never puts the value in HttpSession itself.
        2. Map passed to Action has the possibility to be orver-written. However,
        this is a quite new map, and the processing continuance of Action is
        done difficultly.
        3. The problem of 2 can be blocked. Because query evaluted as simple String.

        Thank you !!

        Show
        hisato.killing Hisato Killing added a comment - At last, this problem was able to be understood thanks to everybody I will close this issue for the following reasons. 1. The query above never puts the value in HttpSession itself. 2. Map passed to Action has the possibility to be orver-written. However, this is a quite new map, and the processing continuance of Action is done difficultly. 3. The problem of 2 can be blocked. Because query evaluted as simple String. Thank you !!
        Hide
        brenmcguire Antonio Petrelli added a comment -

        <snip>
        Don: The values in the map are evaluated as simple Strings.
        </snip>

        This is exactly what I wanted to read!
        It's nice that we do not need to downgrade Struts 2.0.9

        Show
        brenmcguire Antonio Petrelli added a comment - <snip> Don: The values in the map are evaluated as simple Strings. </snip> This is exactly what I wanted to read! It's nice that we do not need to downgrade Struts 2.0.9

          People

          • Assignee:
            Unassigned
            Reporter:
            hisato.killing Hisato Killing
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development