Spring Security
  1. Spring Security
  2. SEC-2067

Security context incorrectly removed from session when asynchronous servlet used

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: 3.1.3
    • Fix Version/s: None
    • Component/s: Web
    • Labels:
      None
    • Environment:
      Tomcat 7.0.30, Java 7

      Description

      Take the following sample servlet:

      @Override
      protected void doGet(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException {
      	req.startAsync();
      	new Thread("AsyncThread") {
      		@Override
      		public void run() {
      			try {
      				TimeUnit.SECONDS.sleep(1);
      				resp.getOutputStream().flush();
      			} catch (Exception e) {
      				e.printStackTrace();
      			}
      		}
      	}.start();
      }
      

      As you can see it does nothing except flushing after one second in a separate thread. However this servlet is secured by Spring Security so the resp.getOutputStream() stream is actually an instance of org.springframework.security.web.context.SaveContextOnUpdateOrErrorResponseWrapper.SaveContextServletOutputStream.

      This class calls org.springframework.security.web.context.HttpSessionSecurityContextRepository.SaveToSessionResponseWrapper.saveContext() on flush(). That's where the bug is This method under certain conditions removes security context from session:

      httpSession.removeAttribute(springSecurityContextKey)
      

      This method was heavily rewritten lately (see: See SEC-776, SEC-1587 and SEC-1735) so I can't tell where the actually bug lies. All I see is that since it can't find security context in thread local holder (we are flushing from a different thread), it removes the context from the session as well. Basically asynchronous servlet logs me out from the application.

      I extracted the error and prepared sample, simplistic application exposing that bug (attached). I actually run into it when using Atmosphere Comet library together with Spring Security, but this application shows exact same error.

      Extract and call mvn tomcat7:run. Only one Java class, browse to localhost:8080, login using admin/admin and follow instructions to reproduce.

        Issue Links

          Activity

          Hide
          Rob Winch added a comment -

          Duplicate of SEC-1998

          Show
          Rob Winch added a comment - Duplicate of SEC-1998
          Hide
          Tomasz Nurkiewicz added a comment -

          Thanks, but the sample application attached used to work in 3.1.1 and before but fails in 3.1.2. Does it mean that it was working only by coincidence before?

          Show
          Tomasz Nurkiewicz added a comment - Thanks, but the sample application attached used to work in 3.1.1 and before but fails in 3.1.2. Does it mean that it was working only by coincidence before?
          Hide
          Rob Winch added a comment -

          You are correct it was working by coincidence previously. Currently there is nothing that propagates the Spring Security Context across threads. The other thing that will need to be addressed is that only certain methods are accessible from the Async Context (i.e. see SPR-9433, this thread, etc). In short, any behavior that implied Spring Security was working was working with Async Context was coincidence and until SEC-1998 is resolved I would not recommend its usage as there are likely a number of unexpected consequences that may result in security vulnerabilities.

          Show
          Rob Winch added a comment - You are correct it was working by coincidence previously. Currently there is nothing that propagates the Spring Security Context across threads. The other thing that will need to be addressed is that only certain methods are accessible from the Async Context (i.e. see SPR-9433 , this thread , etc). In short, any behavior that implied Spring Security was working was working with Async Context was coincidence and until SEC-1998 is resolved I would not recommend its usage as there are likely a number of unexpected consequences that may result in security vulnerabilities.
          Hide
          Tomasz Nurkiewicz added a comment -

          Thank you Rob for thorough explanation. If you look at the sample application I provided, the asynchronous servlet is neither accessing the security context nor interacting with Spring security in any way. Removing of security context from HTTP session happens when servlet (from other thread) flushes the response (HttpSessionSecurityContextRepository.SaveToSessionResponseWrapper.saveContext()).

          Nevertheless I understand that it won't work consistently until full Servlet 3.0 support. Thank you again.

          Show
          Tomasz Nurkiewicz added a comment - Thank you Rob for thorough explanation. If you look at the sample application I provided, the asynchronous servlet is neither accessing the security context nor interacting with Spring security in any way. Removing of security context from HTTP session happens when servlet (from other thread) flushes the response ( HttpSessionSecurityContextRepository.SaveToSessionResponseWrapper.saveContext() ). Nevertheless I understand that it won't work consistently until full Servlet 3.0 support. Thank you again.
          Hide
          Rob Winch added a comment - - edited

          I understand it isn't requiring Spring Security in anyway. However, it just shows there is a bit of work to be done for general Async Support. FYI..a "do at your own risk" sort of approach would be to do something like this:

          final AsyncContext startAsync = req.startAsync();
          new Thread("AsyncThread") {
            @Override
            public void run() {
              try {
                TimeUnit.SECONDS.sleep(1);
                startAsync.getResponse().getOutputStream().flush();
              } catch (Exception e) {
                e.printStackTrace();
              }
            }
          }.start();
          

          This uses the original unwrapped request/response which will not impact the OutputStream. As I alluded to this will work in this small example, but there are certainly other things that need to be nailed down for a general approach.

          PS: I am working on Asych support now, so hopefully we will be able to push out a Milestone with these changes. I would love your feedback once it is available so please keep an eye out for it.

          Show
          Rob Winch added a comment - - edited I understand it isn't requiring Spring Security in anyway. However, it just shows there is a bit of work to be done for general Async Support. FYI..a "do at your own risk" sort of approach would be to do something like this: final AsyncContext startAsync = req.startAsync(); new Thread ( "AsyncThread" ) { @Override public void run() { try { TimeUnit.SECONDS.sleep(1); startAsync.getResponse().getOutputStream().flush(); } catch (Exception e) { e.printStackTrace(); } } }.start(); This uses the original unwrapped request/response which will not impact the OutputStream. As I alluded to this will work in this small example, but there are certainly other things that need to be nailed down for a general approach. PS: I am working on Asych support now, so hopefully we will be able to push out a Milestone with these changes. I would love your feedback once it is available so please keep an eye out for it.

            People

            • Assignee:
              Rob Winch
              Reporter:
              Tomasz Nurkiewicz
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: