Wicket
  1. Wicket
  2. WICKET-1767

Protection against Session Fixation

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.4
    • Fix Version/s: 1.4-RC1
    • Component/s: wicket
    • Labels:
      None

      Description

      Securing a Wicket application against Session Fixation attacks (http://www.owasp.org/index.php/Session_Fixation) is currently not trivial. This is especially problematic as most Java webservers fall back to URL rewriting when the user disabled cookies. The session is gets appended to the URL and its trivial to steal a session.

      To protect against session fixation, the HTTP session must be invalidated and recreated on login, giving the user a new session id. The following code does exactly that, it must be called before loggin in the user (eg. store credentials). A redirect isn't required, though it should be part of the login-form anyway.

      ISessionStore store = Application.get().getSessionStore();
      Request request = RequestCycle.get().getRequest();
      store.invalidate(request);
      Session session = Application.get().newSession(request, RequestCycle.get().getResponse());
      session.bind();
      store.bind(request, session);

      Calling session.invalidateNow() does NOT work (I have no idea why).

      I'd like to see support for this as part of Wicket - it took me about 6 hours to figure out the Wicket internals and produce these 6 lines of code. Others shouldn't have to bother with that.

      I can't provide a testcase. Applications work fine without the addition, but leave users vulnerable to the session-fixation. Manual testing has to look at the session id (eg. via Firebug's Net tab) before and after a login.

        Activity

        Hide
        Johan Compagner added a comment -

        the first 3 lines are exactly what session.invalidate does

        But we dont create a new session right away. And i think that is the problem
        But what you coud try is doing this:

        session.invalidateNow()
        Session.unset();

        Show
        Johan Compagner added a comment - the first 3 lines are exactly what session.invalidate does But we dont create a new session right away. And i think that is the problem But what you coud try is doing this: session.invalidateNow() Session.unset();
        Hide
        Jörn Zaefferer added a comment -

        Thanks for the comment Johan.

        invalidateNow actually does more then just store.invalidate(request). It also sets the invalited flag, which has some, in this case, undesired side effects. I didn't understand what exactly happens, but its definitely not the same.

        Calling Session.unset() didn't help either.

        Show
        Jörn Zaefferer added a comment - Thanks for the comment Johan. invalidateNow actually does more then just store.invalidate(request). It also sets the invalited flag, which has some, in this case, undesired side effects. I didn't understand what exactly happens, but its definitely not the same. Calling Session.unset() didn't help either.
        Hide
        Johan Compagner added a comment -

        that flag shouldnt be in the way one bit
        It just says that the session is invalidate or should invalidate

        so for the rest it does exactly teh same is the 3 first lines:

        getSessionStore().invalidate(RequestCycle.get().getRequest());

        Then i you call Session.unset()
        and then you ask for a session again (when you login in)
        You should get a new one. I guess you really also should call bind() on it because you do want to store it and i guess we dont know that yet

        So
        session.invalidatNow()
        Session.unset()
        Session.get().bind();

        If that doenst work then i guess that something goes wrong in the bind() call and i guess thats related with:

        if (store.lookup(request) == null)

        Maybe that somehow sees still the old session.

        Show
        Johan Compagner added a comment - that flag shouldnt be in the way one bit It just says that the session is invalidate or should invalidate so for the rest it does exactly teh same is the 3 first lines: getSessionStore().invalidate(RequestCycle.get().getRequest()); Then i you call Session.unset() and then you ask for a session again (when you login in) You should get a new one. I guess you really also should call bind() on it because you do want to store it and i guess we dont know that yet So session.invalidatNow() Session.unset() Session.get().bind(); If that doenst work then i guess that something goes wrong in the bind() call and i guess thats related with: if (store.lookup(request) == null) Maybe that somehow sees still the old session.
        Hide
        Jörn Zaefferer added a comment -

        I lack the necessary knowledge about Wicket internals, so I was trying out things while reading bits of Wicket source code.

        session.invalidatNow()
        Session.unset()
        Session.get().bind();

        Those didn't work. One possible issue with Session.get(), called after invalidate/unset could be that get() actually creates the session, but without creating a HTTP session.

        Also store.lookup has to return null, as store.invalidate(request) kills the HTTP session, and lookup takes the session id and uses getAttribute to access the HTTP session.

        My code seems to work because, in constract to the various "alternatives", it first invalidates the HTTP session, then creates a new Wicket Session (via Application#newSession, then binds it to the HTTP session (creating a new one), then binding it to the SessionStore.

        What do you think? Would a simple test application, that does nothing but login in with a single button and outputting the session id help?

        Show
        Jörn Zaefferer added a comment - I lack the necessary knowledge about Wicket internals, so I was trying out things while reading bits of Wicket source code. session.invalidatNow() Session.unset() Session.get().bind(); Those didn't work. One possible issue with Session.get(), called after invalidate/unset could be that get() actually creates the session, but without creating a HTTP session. Also store.lookup has to return null, as store.invalidate(request) kills the HTTP session, and lookup takes the session id and uses getAttribute to access the HTTP session. My code seems to work because, in constract to the various "alternatives", it first invalidates the HTTP session, then creates a new Wicket Session (via Application#newSession, then binds it to the HTTP session (creating a new one), then binding it to the SessionStore. What do you think? Would a simple test application, that does nothing but login in with a single button and outputting the session id help?
        Hide
        Johan Compagner added a comment -

        no this is just weird
        It has to be in the last bind() call that you do the rest is exactly the same

        session.invalidatNow()
        Session.unset()
        session = Session.get();
        session.bind()

        i guess that last call is just a NOP. doesnt do anything because store.lookup() still returns something.

        for example after that sesion.bind() call does the session have an id?

        this code has to work it is just exactly the same as yours i cant see any difference and then it is just the rebinding in the same request that goes wrong

        session.invalidatNow()
        Session.unset()
        session = Session.get(); << this should now be another instance of session!
        Application.get().getSessionStore().bind(request, session);

        the only thing is that session doesnt have an id then i think.

        Show
        Johan Compagner added a comment - no this is just weird It has to be in the last bind() call that you do the rest is exactly the same session.invalidatNow() Session.unset() session = Session.get(); session.bind() i guess that last call is just a NOP. doesnt do anything because store.lookup() still returns something. for example after that sesion.bind() call does the session have an id? this code has to work it is just exactly the same as yours i cant see any difference and then it is just the rebinding in the same request that goes wrong session.invalidatNow() Session.unset() session = Session.get(); << this should now be another instance of session! Application.get().getSessionStore().bind(request, session); the only thing is that session doesnt have an id then i think.
        Hide
        Jörn Zaefferer added a comment -

        Tried this:

        get().invalidateNow();
        Session.unset();
        Session session = Session.get();
        session.bind();
        Application.get().getSessionStore().bind(request(), session);

        Without the session.bind() no new HttpSession is created, resulting in a NullPointerException in SessionStore. With the bind-call, as above, it just doesn't work.

        Some further experimentation showed that this works, somewhat simplifying my initial solution:

        Application.get().getSessionStore().invalidate(RequestCycle.get().getRequest());
        Session session = Session.get();
        session.bind();
        Application.get().getSessionStore().bind(request(), session);

        Replacing the first line with get().invalidateNow() does NOT work. So that sessionInvalidated flag in Session must have some undesired sideeffect, as that is the only difference between Session#invalidateNow and my code.

        Show
        Jörn Zaefferer added a comment - Tried this: get().invalidateNow(); Session.unset(); Session session = Session.get(); session.bind(); Application.get().getSessionStore().bind(request(), session); Without the session.bind() no new HttpSession is created, resulting in a NullPointerException in SessionStore. With the bind-call, as above, it just doesn't work. Some further experimentation showed that this works, somewhat simplifying my initial solution: Application.get().getSessionStore().invalidate(RequestCycle.get().getRequest()); Session session = Session.get(); session.bind(); Application.get().getSessionStore().bind(request(), session); Replacing the first line with get().invalidateNow() does NOT work. So that sessionInvalidated flag in Session must have some undesired sideeffect, as that is the only difference between Session#invalidateNow and my code.
        Hide
        Johan Compagner added a comment -

        that that code works is weird you dont even call unset() !
        So the second line where you do session.get() you should get the same session that is already stored in the thread local

        I still dont get what that sessioninvalidate flag has to do with anything.
        If you call Session.unset()
        and then Session.get() then you have a new session. That that old object with that flag is not used anymore..
        So how can it be in the way??

        So you are saying that you have to call session.bind() else you dont have a new httpsession. then what thought would go wrong that it would go into this if

        if (store.lookup(request) == null)
        {
        // explicitly create a session
        id = store.getSessionId(request, true);
        // bind it
        store.bind(request, this);

        isnt the case. So your call that you do also your self:
        Application.get().getSessionStore().bind(request(), session);

        is already done. so why do you have to call it a second time also yourself? This all doesnt make sense.

        Show
        Johan Compagner added a comment - that that code works is weird you dont even call unset() ! So the second line where you do session.get() you should get the same session that is already stored in the thread local I still dont get what that sessioninvalidate flag has to do with anything. If you call Session.unset() and then Session.get() then you have a new session. That that old object with that flag is not used anymore.. So how can it be in the way?? So you are saying that you have to call session.bind() else you dont have a new httpsession. then what thought would go wrong that it would go into this if if (store.lookup(request) == null) { // explicitly create a session id = store.getSessionId(request, true); // bind it store.bind(request, this); isnt the case. So your call that you do also your self: Application.get().getSessionStore().bind(request(), session); is already done. so why do you have to call it a second time also yourself? This all doesnt make sense.
        Hide
        Jörn Zaefferer added a comment -

        I can only repeat myself - and provide a testapp if that helps. You're welcome to give it a try yourself, I don't see how we could get any further otherwise.

        Show
        Jörn Zaefferer added a comment - I can only repeat myself - and provide a testapp if that helps. You're welcome to give it a try yourself, I don't see how we could get any further otherwise.
        Hide
        Johan Compagner added a comment -

        give me a test case then if you cant debug it what is really different
        Because currently the only thing i see to fix this is doing that Session.unset() in the invalidateNow() call

        Everything should work then (if you call bind() your self in your login code)

        Show
        Johan Compagner added a comment - give me a test case then if you cant debug it what is really different Because currently the only thing i see to fix this is doing that Session.unset() in the invalidateNow() call Everything should work then (if you call bind() your self in your login code)
        Hide
        Jörn Zaefferer added a comment -

        I just noticed an interesting thing here: I'm not calling Session.unset, thats why it works at all. Basically I'm replacing the HTTP session WITHOUT replacing the Wicket Session - it just gets bound to the new HTTP session, and thats it.

        Thats why invalidateNow doesn't work: The Session is marked as invalid and rebinding it fails.

        Good to finalize realize that. All wicket related state is bound to the wicket Session object. By just rebinding it to a new Session, everything continues to work as intentended. Creating a new wicket Session kills the state that has to be maintained.

        Assuming the session-fixation-protection is a desired feature, can we add something to Session that makes this easier to use and much easier to document? Something like Session.get().replace().

        /**

        • Replaces the underlying HTTP Session, invalidating the current one and
        • creating a new one.
        • <p>
        • Call upon login to protect against session fixation.
        • @see http://www.owasp.org/index.php/Session_Fixation
          */
          public void replace() { getSessionStore().invalidate(RequestCycle.get().getRequest()); bind(); getSessionStore().bind(RequestCycle.get().getRequest(), this); }
        Show
        Jörn Zaefferer added a comment - I just noticed an interesting thing here: I'm not calling Session.unset, thats why it works at all. Basically I'm replacing the HTTP session WITHOUT replacing the Wicket Session - it just gets bound to the new HTTP session, and thats it. Thats why invalidateNow doesn't work: The Session is marked as invalid and rebinding it fails. Good to finalize realize that. All wicket related state is bound to the wicket Session object. By just rebinding it to a new Session, everything continues to work as intentended. Creating a new wicket Session kills the state that has to be maintained. Assuming the session-fixation-protection is a desired feature, can we add something to Session that makes this easier to use and much easier to document? Something like Session.get().replace(). /** Replaces the underlying HTTP Session, invalidating the current one and creating a new one. <p> Call upon login to protect against session fixation. @see http://www.owasp.org/index.php/Session_Fixation */ public void replace() { getSessionStore().invalidate(RequestCycle.get().getRequest()); bind(); getSessionStore().bind(RequestCycle.get().getRequest(), this); }
        Hide
        Igor Vaynberg added a comment -

        if you are not replacing the old session instance with the new, then it is possible to still access stale values from the old session instance during that request...

        Show
        Igor Vaynberg added a comment - if you are not replacing the old session instance with the new, then it is possible to still access stale values from the old session instance during that request...
        Hide
        Jörn Zaefferer added a comment -

        I don't consider that a problem. On the hand, the old session got invalidet, so there is no way it still contains stale values. On the other, I don't need to invalidate Wickets Session state, all I need is a new HTTP session. After all, the point isn't to remove Wicket's session state, but to get a new session id.

        Show
        Jörn Zaefferer added a comment - I don't consider that a problem. On the hand, the old session got invalidet, so there is no way it still contains stale values. On the other, I don't need to invalidate Wickets Session state, all I need is a new HTTP session. After all, the point isn't to remove Wicket's session state, but to get a new session id.
        Hide
        Igor Vaynberg added a comment -

        the stale state might not be in the actual httpsession, but it remains in wicket's session object attached to the current thread, and is not synced into httpsession until the end of requests afaik. so there is still a problem there, because between you calling invalidate() and the end of the request you can have business logic acting on old values it stored in wicket's session object.

        Show
        Igor Vaynberg added a comment - the stale state might not be in the actual httpsession, but it remains in wicket's session object attached to the current thread, and is not synced into httpsession until the end of requests afaik. so there is still a problem there, because between you calling invalidate() and the end of the request you can have business logic acting on old values it stored in wicket's session object.
        Hide
        Johan Compagner added a comment -

        so the only thing you want to do is remove the http session but nothing more?

        ((WebRequest)RequestCycle.get().getRequest()).getHttpServletRequest().getSession().invalidate()
        session.bind()

        Show
        Johan Compagner added a comment - so the only thing you want to do is remove the http session but nothing more? ((WebRequest)RequestCycle.get().getRequest()).getHttpServletRequest().getSession().invalidate() session.bind()
        Hide
        Jörn Zaefferer added a comment -

        Thanks Johan, that makes it even shorter.

        This works fine:

        /**

        • Replaces the underlying HTTP Session, invalidating the current one and
        • creating a new one.
        • <p>
        • Call upon login to protect against session fixation.
        • @see http://www.owasp.org/index.php/Session_Fixation
          */
          public void replace() { ((WebRequest)RequestCycle.get().getRequest()).getHttpServletRequest().getSession().invalidate(); bind(); }
        Show
        Jörn Zaefferer added a comment - Thanks Johan, that makes it even shorter. This works fine: /** Replaces the underlying HTTP Session, invalidating the current one and creating a new one. <p> Call upon login to protect against session fixation. @see http://www.owasp.org/index.php/Session_Fixation */ public void replace() { ((WebRequest)RequestCycle.get().getRequest()).getHttpServletRequest().getSession().invalidate(); bind(); }
        Hide
        Johan Compagner added a comment -

        that is something we cant add to Session but maybe to the WebSession subclass.
        i dont like the name replace() to much..
        a bit more explicit like invalidateHttpSession() or replaceHttpSession() (in WebSession not Session)

        igor should we add this to the api what do you think?

        Show
        Johan Compagner added a comment - that is something we cant add to Session but maybe to the WebSession subclass. i dont like the name replace() to much.. a bit more explicit like invalidateHttpSession() or replaceHttpSession() (in WebSession not Session) igor should we add this to the api what do you think?
        Hide
        Jörn Zaefferer added a comment -

        replaceHttpSession() in WebSession sounds good.

        By the way, I'm still wondering why Wicket has the seperatation of a non-web-layer and a WebXXX-layer on top of that. Can you give me an idea about that? I haven't seen any hint at usage of Wicket in non-http scenerios...

        Show
        Jörn Zaefferer added a comment - replaceHttpSession() in WebSession sounds good. By the way, I'm still wondering why Wicket has the seperatation of a non-web-layer and a WebXXX-layer on top of that. Can you give me an idea about that? I haven't seen any hint at usage of Wicket in non-http scenerios...
        Hide
        Johan Compagner added a comment -

        for example there was/is talk about a Wap implementation
        also the portlets are implemented a bit different, but thats only for the special request/response wrappers

        but currently the only impl is really the http/web implementation but the base is http/web free

        Show
        Johan Compagner added a comment - for example there was/is talk about a Wap implementation also the portlets are implemented a bit different, but thats only for the special request/response wrappers but currently the only impl is really the http/web implementation but the base is http/web free
        Hide
        Igor Vaynberg added a comment -

        a while ago someone also used wicket with apache mina instead of wicket running in jetty. so the separation is used, albeit seldomly.

        Show
        Igor Vaynberg added a comment - a while ago someone also used wicket with apache mina instead of wicket running in jetty. so the separation is used, albeit seldomly.
        Hide
        Jörn Zaefferer added a comment -

        Any plans to fix this? It doesn't affect any existing code and its only four lines of code...

        Show
        Jörn Zaefferer added a comment - Any plans to fix this? It doesn't affect any existing code and its only four lines of code...
        Hide
        Johan Compagner added a comment -

        will try to apply it in 1.4

        Show
        Johan Compagner added a comment - will try to apply it in 1.4
        Hide
        Igor Vaynberg added a comment -

        johan, should we just add bind() call to session.invalidatenow()? seems like that is the only missing bit and it would make more sense there then in yet another method.

        Show
        Igor Vaynberg added a comment - johan, should we just add bind() call to session.invalidatenow()? seems like that is the only missing bit and it would make more sense there then in yet another method.
        Hide
        Johan Compagner added a comment -

        that doesnt sound logical to me
        if i call invalidateNow() of a wicket session then it doesnt say to me that that where i just called invalidateNow() on is then added to a new HttpSession so it is kind of still alive not really invalidated at all (at least not the object i called it on)

        And then the boolean in the wicket session
        sessionInvalidated = true; // set this for isSessionInvalidated

        is telling us what?...

        no i guess having a nice method like replaceHttpSession is way better.

        Show
        Johan Compagner added a comment - that doesnt sound logical to me if i call invalidateNow() of a wicket session then it doesnt say to me that that where i just called invalidateNow() on is then added to a new HttpSession so it is kind of still alive not really invalidated at all (at least not the object i called it on) And then the boolean in the wicket session sessionInvalidated = true; // set this for isSessionInvalidated is telling us what?... no i guess having a nice method like replaceHttpSession is way better.
        Hide
        Johan Compagner added a comment -

        i added this method:
        /**

        • Replaces the underlying (Web)Session, invalidating the current one and creating a new one. By
        • calling {@link ISessionStore#invalidate(Request)}

          and

          {@link #bind()}
        • <p>
        • Call upon login to protect against session fixation.
        • @see "http://www.owasp.org/index.php/Session_Fixation"
          */
          public void replaceSession() { getSessionStore().invalidate(RequestCycle.get().getRequest()); bind(); }

        i think thats the nicest thing to have (maybe a better method name anyone?)

        Show
        Johan Compagner added a comment - i added this method: /** Replaces the underlying (Web)Session, invalidating the current one and creating a new one. By calling {@link ISessionStore#invalidate(Request)} and {@link #bind()} <p> Call upon login to protect against session fixation. @see "http://www.owasp.org/index.php/Session_Fixation" */ public void replaceSession() { getSessionStore().invalidate(RequestCycle.get().getRequest()); bind(); } i think thats the nicest thing to have (maybe a better method name anyone?)

          People

          • Assignee:
            Johan Compagner
            Reporter:
            Jörn Zaefferer
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development