SEC-834: Session fixation attack protection will cause problems with URL rewriting

http://jira.springframework.org/browse/SEC-834. Changed position of SessionFixationProtectionFilter and modified it to make a decision about whether authentication has taken place prior to calling doFilter(). Previously it did this on the return through the filter chain, which caused the problem described in this issue.

core/src/main/java/org/springframework/security/ui/FilterChainOrder.java

@@ -22,8 +22,7 @@ public abstract class FilterChainOrder {
 
     public static final int CHANNEL_FILTER              = FILTER_CHAIN_FIRST;
     public static final int CONCURRENT_SESSION_FILTER   = FILTER_CHAIN_FIRST + INTERVAL * i++;
-    public static final int HTTP_SESSION_CONTEXT_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++;
-    public static final int SESSION_FIXATION_FILTER     = FILTER_CHAIN_FIRST + INTERVAL * i++;    
+    public static final int HTTP_SESSION_CONTEXT_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++;    
     public static final int LOGOUT_FILTER               = FILTER_CHAIN_FIRST + INTERVAL * i++;
     public static final int X509_FILTER                 = FILTER_CHAIN_FIRST + INTERVAL * i++;
     public static final int PRE_AUTH_FILTER             = FILTER_CHAIN_FIRST + INTERVAL * i++;
@@ -37,6 +36,7 @@ public abstract class FilterChainOrder {
     public static final int ANONYMOUS_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++;
     public static final int EXCEPTION_TRANSLATION_FILTER = FILTER_CHAIN_FIRST + INTERVAL * i++;
     public static final int NTLM_FILTER                 = FILTER_CHAIN_FIRST + INTERVAL * i++;
+    public static final int SESSION_FIXATION_FILTER     = FILTER_CHAIN_FIRST + INTERVAL * i++;    
     public static final int FILTER_SECURITY_INTERCEPTOR = FILTER_CHAIN_FIRST + INTERVAL * i++;
     public static final int SWITCH_USER_FILTER          = FILTER_CHAIN_FIRST + INTERVAL * i++;
 
@@ -46,7 +46,6 @@ public abstract class FilterChainOrder {
         filterNameToOrder.put("FIRST", new Integer(Integer.MIN_VALUE));
         filterNameToOrder.put("CHANNEL_FILTER", new Integer(CHANNEL_FILTER));
         filterNameToOrder.put("CONCURRENT_SESSION_FILTER", new Integer(CONCURRENT_SESSION_FILTER));
-        filterNameToOrder.put("SESSION_CONTEXT_INTEGRATION_FILTER", new Integer(HTTP_SESSION_CONTEXT_FILTER));
         filterNameToOrder.put("LOGOUT_FILTER", new Integer(LOGOUT_FILTER));
         filterNameToOrder.put("X509_FILTER", new Integer(X509_FILTER));
         filterNameToOrder.put("PRE_AUTH_FILTER", new Integer(PRE_AUTH_FILTER));
@@ -59,6 +58,7 @@ public abstract class FilterChainOrder {
         filterNameToOrder.put("ANONYMOUS_FILTER", new Integer(ANONYMOUS_FILTER));
         filterNameToOrder.put("EXCEPTION_TRANSLATION_FILTER", new Integer(EXCEPTION_TRANSLATION_FILTER));
         filterNameToOrder.put("NTLM_FILTER", new Integer(NTLM_FILTER));
+        filterNameToOrder.put("SESSION_CONTEXT_INTEGRATION_FILTER", new Integer(HTTP_SESSION_CONTEXT_FILTER));        
         filterNameToOrder.put("FILTER_SECURITY_INTERCEPTOR", new Integer(FILTER_SECURITY_INTERCEPTOR));
         filterNameToOrder.put("SWITCH_USER_FILTER", new Integer(SWITCH_USER_FILTER));
         filterNameToOrder.put("LAST", new Integer(Integer.MAX_VALUE));

core/src/main/java/org/springframework/security/ui/SessionFixationProtectionFilter.java

@@ -6,24 +6,23 @@ import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpServletResponseWrapper;
+import javax.servlet.http.HttpSession;
 
 import org.springframework.security.Authentication;
 import org.springframework.security.AuthenticationTrustResolver;
 import org.springframework.security.AuthenticationTrustResolverImpl;
 import org.springframework.security.concurrent.SessionRegistry;
+import org.springframework.security.context.HttpSessionContextIntegrationFilter;
+import org.springframework.security.context.SecurityContext;
 import org.springframework.security.context.SecurityContextHolder;
 import org.springframework.security.util.SessionUtils;
 
 /**
  * Detects that a user has been authenticated since the start of the request and starts a new session.
  * <p>
- * This is essentially a generalization of the functionality that was implemented for SEC-399. Additionally, it will
- * update the configured SessionRegistry if one is in use, thus preventing problems when used with Spring Security's
- * concurrent session control.
- * <p>
- * If the response has already been committed when the filter checks the authentication state, then it isn't possible 
- * to create a new session and the filter will print a warning to that effect. 
+ * This is essentially a generalization of the functionality that was implemented for SEC-399. 
+ * Additionally, it will update the configured SessionRegistry if one is in use, thus preventing problems when used 
+ * with Spring Security's concurrent session control. 
  * 
  * @author Martin Algesten
  * @author Luke Taylor
@@ -55,22 +54,17 @@ public class SessionFixationProtectionFilter extends SpringSecurityFilter {
         }
         
         request.setAttribute(FILTER_APPLIED, Boolean.TRUE);
-                
-        if (isAuthenticated()) {
-            // We don't have to worry about session fixation attack if already authenticated 
-            chain.doFilter(request, response);
-            return;            
-        }
+
+        HttpSession session = request.getSession();
+        SecurityContext sessionSecurityContext = 
+            (SecurityContext) session.getAttribute(HttpSessionContextIntegrationFilter.SPRING_SECURITY_CONTEXT_KEY);
         
-        SessionFixationProtectionResponseWrapper wrapper = 
-            new SessionFixationProtectionResponseWrapper(response, request);
-        try {
-            chain.doFilter(request, wrapper);
-        } finally {
-            if (!wrapper.isNewSessionStarted()) {
-                startNewSessionIfRequired(request, response);
-            }
+        if (sessionSecurityContext == null && isAuthenticated()) {
+            // The user has been authenticated during the current request, so do the session migration
+            startNewSessionIfRequired(request, response);
         }
+
+        chain.doFilter(request, response);
     }
     
     private boolean isAuthenticated() {
@@ -92,83 +86,12 @@ public class SessionFixationProtectionFilter extends SpringSecurityFilter {
     }
     
     /**
-     * Called when the an initially unauthenticated request completes or a redirect or sendError occurs.
+     * Called when the a user wasn't authenticated at the start of the request but has been during it
      * <p>
-     * If the user is now authenticated, a new session will be created, the session attributes copied to it (if 
-     * <tt>migrateSessionAttributes</tt> is set and the sessionRegistry updated with the new session information.
+     * A new session will be created, the session attributes copied to it (if 
+     * <tt>migrateSessionAttributes</tt> is set) and the sessionRegistry updated with the new session information.
      */
-    protected void startNewSessionIfRequired(HttpServletRequest request, HttpServletResponse response) {
-        if (isAuthenticated()) {
-            if (request.getSession(false) != null && response.isCommitted()) {
-                logger.warn("Response is already committed. Unable to create new session.");
-            }
-            
-            SessionUtils.startNewSessionIfRequired(request, migrateSessionAttributes, sessionRegistry);
-        }
-    }
-    
-    /**
-     * Response wrapper to handle the situation where we need to migrate the session after a redirect or sendError.
-     * Similar in function to Martin Algesten's OnRedirectUpdateSessionResponseWrapper used in 
-     * HttpSessionContextIntegrationFilter.
-     * <p>
-     * Only used to wrap the response if the conditions are right at the start of the request to potentially 
-     * require starting a new session, i.e. that the user isn't authenticated and a session existed to begin with.  
-     */
-    class SessionFixationProtectionResponseWrapper extends HttpServletResponseWrapper {
-        private HttpServletRequest request;
-        private boolean newSessionStarted;
-
-        SessionFixationProtectionResponseWrapper(HttpServletResponse response, HttpServletRequest request) {
-            super(response);
-            this.request = request;
-        }
-        
-        /**
-         * Makes sure a new session is created before calling the
-         * superclass <code>sendError()</code>
-         */
-        public void sendError(int sc) throws IOException {
-            startNewSession();
-            super.sendError(sc);
-        }
-
-        /**
-         * Makes sure a new session is created before calling the
-         * superclass <code>sendError()</code>
-         */
-        public void sendError(int sc, String msg) throws IOException {
-            startNewSession();
-            super.sendError(sc, msg);
-        }
-
-        /**
-         * Makes sure a new session is created before calling the
-         * superclass <code>sendRedirect()</code>
-         */
-        public void sendRedirect(String location) throws IOException {
-            startNewSession();
-            super.sendRedirect(location);
-        }
-        
-        public void flushBuffer() throws IOException {
-            startNewSession();
-            super.flushBuffer();
-        }
-
-        /**
-         * Calls <code>startNewSessionIfRequired()</code>
-         */
-        private void startNewSession() {
-            if (newSessionStarted) {
-                return;
-            }
-            startNewSessionIfRequired(request, this);
-            newSessionStarted = true;
-        }
-
-        boolean isNewSessionStarted() {
-            return newSessionStarted;
-        }
+    protected void startNewSessionIfRequired(HttpServletRequest request, HttpServletResponse response) {            
+        SessionUtils.startNewSessionIfRequired(request, migrateSessionAttributes, sessionRegistry);
     }
 }

core/src/test/java/org/springframework/security/config/HttpSecurityBeanDefinitionParserTests.java

@@ -96,8 +96,7 @@ public class HttpSecurityBeanDefinitionParserTests {
 
         Iterator filters = filterList.iterator();
 
-        assertTrue(filters.next() instanceof HttpSessionContextIntegrationFilter);
-        assertTrue(filters.next() instanceof SessionFixationProtectionFilter);        
+        assertTrue(filters.next() instanceof HttpSessionContextIntegrationFilter);     
         assertTrue(filters.next() instanceof LogoutFilter);
         Object authProcFilter = filters.next();
         assertTrue(authProcFilter instanceof AuthenticationProcessingFilter);
@@ -112,6 +111,7 @@ public class HttpSecurityBeanDefinitionParserTests {
         assertTrue(filters.next() instanceof RememberMeProcessingFilter);
         assertTrue(filters.next() instanceof AnonymousProcessingFilter);
         assertTrue(filters.next() instanceof ExceptionTranslationFilter);
+        assertTrue(filters.next() instanceof SessionFixationProtectionFilter);           
         Object fsiObj = filters.next();
         assertTrue(fsiObj instanceof FilterSecurityInterceptor);
         FilterSecurityInterceptor fsi = (FilterSecurityInterceptor) fsiObj;
@@ -173,7 +173,7 @@ public class HttpSecurityBeanDefinitionParserTests {
                 "   <form-login default-target-url='/default' always-use-default-target='true' />" +
                 "</http>" + AUTH_PROVIDER_XML);
         // These will be matched by the default pattern "/**"
-        AuthenticationProcessingFilter filter = (AuthenticationProcessingFilter) getFilters("/anything").get(2);
+        AuthenticationProcessingFilter filter = (AuthenticationProcessingFilter) getFilters("/anything").get(1);
         assertEquals("/default", filter.getDefaultTargetUrl());
         assertEquals(Boolean.TRUE, FieldUtils.getFieldValue(filter, "alwaysUseDefaultTargetUrl"));
     }
@@ -264,7 +264,7 @@ public class HttpSecurityBeanDefinitionParserTests {
         setContext("<http access-denied-page='/access-denied'><http-basic /></http>" + AUTH_PROVIDER_XML);
         List filters = getFilters("/someurl");
         
-        ExceptionTranslationFilter etf = (ExceptionTranslationFilter) filters.get(filters.size() - 2);
+        ExceptionTranslationFilter etf = (ExceptionTranslationFilter) filters.get(filters.size() - 3);
         
         assertEquals("/access-denied", FieldUtils.getFieldValue(etf, "accessDeniedHandler.errorPage"));
     }
@@ -324,7 +324,7 @@ public class HttpSecurityBeanDefinitionParserTests {
         assertEquals(14, filters.size());
         assertTrue(filters.get(0) instanceof MockFilter);        
         assertTrue(filters.get(1) instanceof SecurityContextHolderAwareRequestFilter);
-        assertTrue(filters.get(5) instanceof SecurityContextHolderAwareRequestFilter);
+        assertTrue(filters.get(4) instanceof SecurityContextHolderAwareRequestFilter);
     }
     
     @Test(expected=BeanCreationException.class)
@@ -368,7 +368,7 @@ public class HttpSecurityBeanDefinitionParserTests {
                 "</http>"  + AUTH_PROVIDER_XML);
         List filters = getFilters("/someurl");
 
-        assertTrue(filters.get(3) instanceof X509PreAuthenticatedProcessingFilter);
+        assertTrue(filters.get(2) instanceof X509PreAuthenticatedProcessingFilter);
     }
 
     @Test
@@ -422,7 +422,7 @@ public class HttpSecurityBeanDefinitionParserTests {
                 "<b:bean id='entryPoint' class='org.springframework.security.MockAuthenticationEntryPoint'>" +
                 "    <b:constructor-arg value='/customlogin'/>" +
                 "</b:bean>" + AUTH_PROVIDER_XML);
-        ExceptionTranslationFilter etf = (ExceptionTranslationFilter) getFilters("/someurl").get(9);
+        ExceptionTranslationFilter etf = (ExceptionTranslationFilter) getFilters("/someurl").get(8);
         assertTrue("ExceptionTranslationFilter should be configured with custom entry point", 
                 etf.getAuthenticationEntryPoint() instanceof MockAuthenticationEntryPoint);
     }

core/src/test/java/org/springframework/security/ui/SessionFixationProtectionFilterTests.java

@@ -2,19 +2,14 @@ package org.springframework.security.ui;
 
 import static org.junit.Assert.*;
 
-import java.io.IOException;
-
-import javax.servlet.FilterChain;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
 
 import org.junit.After;
 import org.junit.Test;
 import org.springframework.mock.web.MockFilterChain;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.security.context.HttpSessionContextIntegrationFilter;
 import org.springframework.security.context.SecurityContextHolder;
 import org.springframework.security.providers.TestingAuthenticationToken;
 
@@ -34,6 +29,7 @@ public class SessionFixationProtectionFilterTests {
     public void newSessionShouldNotBeCreatedIfNoSessionExists() throws Exception {
         SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
         HttpServletRequest request = new MockHttpServletRequest();
+        authenticateUser();
         
         filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain());
         
@@ -41,14 +37,15 @@ public class SessionFixationProtectionFilterTests {
     }
 
     @Test
-    public void newSessionShouldNotBeCreatedIfUserIsAuthenticated() throws Exception {
+    public void newSessionBeCreatedIfAuthenticatedOccurredDuringRequest() throws Exception {
         SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
         HttpServletRequest request = new MockHttpServletRequest();
         String sessionId = request.getSession().getId();
+        authenticateUser();
         
         filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain());
         
-        assertEquals(sessionId, request.getSession().getId());
+        assertFalse(sessionId.equals(request.getSession().getId()));
     }    
 
     @Test
@@ -68,91 +65,15 @@ public class SessionFixationProtectionFilterTests {
         HttpServletRequest request = new MockHttpServletRequest();
         String sessionId = request.getSession().getId();
         authenticateUser();
-        
+        request.getSession().setAttribute(HttpSessionContextIntegrationFilter.SPRING_SECURITY_CONTEXT_KEY, 
+                SecurityContextHolder.getContext());
+
         filter.doFilter(request, new MockHttpServletResponse(), new MockFilterChain());
         
         assertEquals(sessionId, request.getSession().getId());
     }    
-    
-    @Test
-    public void newSessionShouldBeCreatedIfAuthenticationOccursDuringRequest() throws Exception {
-        SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
-        HttpServletRequest request = new MockHttpServletRequest();
-        String sessionId = request.getSession().getId();
-        
-        filter.doFilter(request, new MockHttpServletResponse(), new UserAuthenticatingFilterChain());
-        
-        assertFalse(sessionId.equals(request.getSession().getId()));         
-    }
-    
-    @Test
-    public void newSessionShouldBeCreatedIfAuthenticationAndRedirectOccursDuringRequest() throws Exception {
-        SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
-        HttpServletRequest request = new MockHttpServletRequest();
-        String sessionId = request.getSession().getId();
-        
-        AuthenticateAndRedirectFilterChain chain = new AuthenticateAndRedirectFilterChain();
-        filter.doFilter(request, new MockHttpServletResponse(), chain);
-        
-        assertTrue(chain.getResponse() instanceof 
-                SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper);
-        assertTrue("New session should have been created by session wrapper",
-                ((SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper)chain.getResponse()).isNewSessionStarted());
-        assertFalse(sessionId.equals(request.getSession().getId()));
-    }
-    
-    @Test
-    public void wrapperSendErrorCreatesNewSession() throws Exception {
-        authenticateUser();
-        SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
-        HttpServletRequest request = new MockHttpServletRequest();
-        String sessionId = request.getSession().getId();
-        SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper wrapper = 
-            filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request);
-        wrapper.sendError(HttpServletResponse.SC_FORBIDDEN);
-        assertFalse(sessionId.equals(request.getSession().getId()));
-        
-        // Message version
-        request = new MockHttpServletRequest();
-        sessionId = request.getSession().getId();
-        wrapper = filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request);
-        wrapper.sendError(HttpServletResponse.SC_FORBIDDEN, "Hi. I'm your friendly forbidden message.");
-        assertFalse(sessionId.equals(request.getSession().getId()));        
-    }
 
-    @Test
-    public void wrapperRedirectCreatesNewSession() throws Exception {
-        authenticateUser();
-        SessionFixationProtectionFilter filter = new SessionFixationProtectionFilter();
-        HttpServletRequest request = new MockHttpServletRequest();
-        String sessionId = request.getSession().getId();
-        SessionFixationProtectionFilter.SessionFixationProtectionResponseWrapper wrapper = 
-            filter.new SessionFixationProtectionResponseWrapper(new MockHttpServletResponse(), request);
-        wrapper.sendRedirect("/somelocation");
-        assertFalse(sessionId.equals(request.getSession().getId()));
-    }
-    
     private void authenticateUser() {
         SecurityContextHolder.getContext().setAuthentication(new TestingAuthenticationToken("user", "pass", null));
     }
-    
-    private class UserAuthenticatingFilterChain implements FilterChain {
-        public void doFilter(ServletRequest request, ServletResponse response) throws IOException { 
-            authenticateUser();
-        }
-    }
-    
-    private class AuthenticateAndRedirectFilterChain extends UserAuthenticatingFilterChain{
-        HttpServletResponse response;
-        
-        public void doFilter(ServletRequest request, ServletResponse response) throws IOException {
-            super.doFilter(request, response);
-            this.response = (HttpServletResponse)response;
-            this.response.sendRedirect("/someUrl");
-        }
-
-        public HttpServletResponse getResponse() {
-            return response;
-        }
-    }
 }