SEC-2067: Security context incorrectly removed from session when asynchronous servlet used #2290

Closed
spring-issuemaster opened this Issue Oct 19, 2012 · 6 comments

2 participants

@spring-issuemaster

Tomasz Nurkiewicz (Migrated from SEC-2067) said:

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.

@spring-issuemaster

Rob Winch said:

Duplicate of SEC-1998

@spring-issuemaster

Tomasz Nurkiewicz said:

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?

@spring-issuemaster

Rob Winch said:

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.

@spring-issuemaster

Tomasz Nurkiewicz said:

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.

@spring-issuemaster

Rob Winch said:

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.

@spring-issuemaster

This issue duplicates #2223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment