SEC-2742: Race condition in HttpSessionSecurityContextRepository removes a valid context

[spring-projects-issues]

Rob Winch (Migrated from SEC-2742) said:

Hi I think I'm experiencing a race condition in HttpSessionSecurityContextRepository . I'm using Spring-Security in a Vaadin application, i.e. it produces a few concurrent request for one session. Sometimes a programmatic login gets lost in one session. The phenomen produces a log like this:

[http-bio-8080-exec-1] DEBUG context.HttpSessionSecurityContextRepository  - HttpSession returned null object for SPRING_SECURITY_CONTEXT
[http-bio-8080-exec-1] DEBUG context.HttpSessionSecurityContextRepository  - No SecurityContext was available from the HttpSession…
[http-bio-8080-exec-2] Some Login messages
[http-bio-8080-exec-2] DEBUG context.HttpSessionSecurityContextRepository  - SecurityContext stored to HttpSession…
[http-bio-8080-exec-1] DEBUG context.HttpSessionSecurityContextRepository  - SecurityContext is empty or contents are anonymous - context will not be stored in HttpSession.
[http-bio-8080-exec-9] DEBUG context.HttpSessionSecurityContextRepository  - HttpSession returned null object for SPRING_SECURITY_CONTEXT

You can see here that thread 1 has an anonymous context. In between comes thread 2 which does a login on the same session (which doesn't affect the ThreadLocal context from thread 1). After thread 2 finished storing the context, thread 1 continues removing the valid context.

I think the race condition comes from these lines in HttpSessionSecurityContextRepository.saveContext():

if (authentication == null || trustResolver.isAnonymous(authentication)) {
  if (logger.isDebugEnabled()) {
    logger.debug("SecurityContext is empty or contents are anonymous - context will not be stored in HttpSession.");
  }
  if (httpSession != null && !contextObject.equals(contextBeforeExecution)) {
    // SEC-1587 A non-anonymous context may still be in the session
    // SEC-1735 remove if the contextBeforeExecution was not anonymous
    httpSession.removeAttribute(springSecurityContextKey);
  }
  return;
}

I'm guessing that thread 1 yields somewhere before the session.remove(). Thread 2 runs until the context is stored in the session, and thread 1 continues with httpSession.removeAttribute(springSecurityContextKey);.

If the code isn't threadsafe by design I'll repost the issue somewhere towards Vaadin.

I'm using spring-security-3.2.3.RELEASE, but the code exists in master as well.

[spring-projects-issues]

Markus Malkusch said:

I was a bit careless when I reported the issue at github. The cited code is not from HttpSessionSecurityContextRepository.saveContext(). It's from its inner class SaveToSessionResponseWrapper.saveContext().

Furthermore I'm a bit confused about SEC-1735. That closed issue describes perfectly the reported race condition from here. I'm wondering how the code from SEC-1735 fixed the issue.

Maybe it's worth mentioning that I'm experiencing the race condition with the following stack: Grails, grails-spring-security-plugin, Vaadin. The grails plugin has an -early- filter GrailsAnonymousAuthenticationFilter which populates an empty security context with an anonymous context.

[spring-projects-issues]

Rob Winch said:

This sounds like an issue with how the Grails plugin integrates with Spring Security. The problem is that since GrailsAnonymousAuthenticationFilter has already changed the context, saveContext will always evaluate !contextObject.equals(contextBeforeExecution) to true and thus remove the attribute.

In general, the first place that the SecurityContextHolder should be populated is the SecurityContextPersistenceFilter. Perhaps the GrailsAnonymousAuthenticationFilter should be placed where Spring Security's AnonymousAuthenticationFilter is (after the SecurityContextPersistenceFilter). I'm not certain what the reasoning for placing GrailsAnonymousAuthenticationFilter before the SecurityContextPersistenceFilter is (perhaps they have a reason).

[spring-projects-issues]

Markus Malkusch said:

I'm Sorry. I was again too careless. I didn't check the concrete position when I posted that comment. Here's the log output:

INFO web.DefaultSecurityFilterChain - Creating filter chain: Ant [pattern='/**'], [org.springframework.security.web.context.SecurityContextPersistenceFilter@57186e2e, grails.plugin.springsecurity.web.authentication.logout.MutableLogoutFilter@4e9baae, grails.plugin.springsecurity.web.authentication.RequestHolderAuthenticationFilter@cdef807, org.springframework.security.web.authentication.www.DigestAuthenticationFilter@26d9a2b, org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter@69a47e4b, grails.plugin.springsecurity.web.filter.GrailsRememberMeAuthenticationFilter@2e5fa41c, grails.plugin.springsecurity.web.filter.GrailsAnonymousAuthenticationFilter@90f420d, org.springframework.security.web.access.ExceptionTranslationFilter@48b96820, org.springframework.security.web.access.ExceptionTranslationFilter@463322d, org.springframework.security.web.access.intercept.FilterSecurityInterceptor@c732a9b]

So SecurityContextPersistenceFilter is in fact the first filter.