Skip to content

Remove Servlet 2.5 Support for Session Fixation #6297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,7 @@ public SessionManagementConfigurer<H> sessionCreationPolicy(

/**
* Allows explicitly specifying the {@link SessionAuthenticationStrategy}.
* The default is to use {@link SessionFixationProtectionStrategy} for Servlet 3.1 or
* {@link ChangeSessionIdAuthenticationStrategy} for Servlet 3.1+.
* The default is to use {@link ChangeSessionIdAuthenticationStrategy}.
* If restricting the maximum number of sessions is configured, then
* {@link CompositeSessionAuthenticationStrategy} delegating to
* {@link ConcurrentSessionControlAuthenticationStrategy},
Expand Down Expand Up @@ -305,13 +304,11 @@ public SessionManagementConfigurer<H> migrateSession() {

/**
* Specifies that the Servlet container-provided session fixation protection
* should be used. When a session authenticates, the Servlet 3.1 method
* should be used. When a session authenticates, the Servlet method
* {@code HttpServletRequest#changeSessionId()} is called to change the session ID
* and retain all session attributes. Using this option in a Servlet 3.0 or older
* container results in an {@link IllegalStateException}.
* and retain all session attributes.
*
* @return the {@link SessionManagementConfigurer} for further customizations
* @throws IllegalStateException if the container is not Servlet 3.1 or newer.
*/
public SessionManagementConfigurer<H> changeSessionId() {
setSessionFixationAuthenticationStrategy(
Expand Down Expand Up @@ -664,11 +661,6 @@ private boolean isConcurrentSessionControlEnabled() {
* @return the default {@link SessionAuthenticationStrategy} for session fixation
*/
private static SessionAuthenticationStrategy createDefaultSessionFixationProtectionStrategy() {
try {
return new ChangeSessionIdAuthenticationStrategy();
}
catch (IllegalStateException e) {
return new SessionFixationProtectionStrategy();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@
*/
package org.springframework.security.config.http;

import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.List;

import javax.servlet.ServletRequest;
import javax.servlet.http.HttpServletRequest;

import org.w3c.dom.Element;

Expand Down Expand Up @@ -73,7 +71,6 @@
import org.springframework.security.web.session.SimpleRedirectSessionInformationExpiredStrategy;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.util.ClassUtils;
import org.springframework.util.ReflectionUtils;
import org.springframework.util.StringUtils;
import org.springframework.util.xml.DomUtils;

Expand Down Expand Up @@ -350,10 +347,7 @@ private void createSessionManagementFilters() {
}

if (!StringUtils.hasText(sessionFixationAttribute)) {
Method changeSessionIdMethod = ReflectionUtils.findMethod(
HttpServletRequest.class, "changeSessionId");
sessionFixationAttribute = changeSessionIdMethod == null ? OPT_SESSION_FIXATION_MIGRATE_SESSION
: OPT_CHANGE_SESSION_ID;
sessionFixationAttribute = OPT_CHANGE_SESSION_ID;
}
else if (StringUtils.hasText(sessionAuthStratRef)) {
pc.getReaderContext().error(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@

import java.lang.reflect.Method;
import javax.servlet.Filter;
import javax.servlet.http.HttpServletRequest;

import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import org.springframework.context.ConfigurableApplicationContext;
Expand All @@ -44,21 +42,14 @@
import org.springframework.security.web.context.HttpSessionSecurityContextRepository;
import org.springframework.security.web.csrf.CsrfToken;
import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository;
import org.springframework.util.ReflectionUtils;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.same;
import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.spy;
import static org.powermock.api.mockito.PowerMockito.verifyStatic;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;

/**
*
* @author Rob Winch
*/
@RunWith(PowerMockRunner.class)
@PrepareForTest({ ReflectionUtils.class, Method.class })
@PowerMockIgnore({ "org.w3c.dom.*", "org.xml.sax.*", "org.apache.xerces.*", "javax.xml.parsers.*" })
public class SessionManagementConfigurerServlet31Tests {
@Mock
Expand Down Expand Up @@ -87,10 +78,9 @@ public void teardown() {
}

@Test
public void changeSessionIdDefaultsInServlet31Plus() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
public void changeSessionIdThenPreserveParameters() throws Exception {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
String id = request.getSession().getId();
request.getSession();
request.setServletPath("/login");
request.setMethod("POST");
Expand All @@ -100,15 +90,14 @@ public void changeSessionIdDefaultsInServlet31Plus() throws Exception {
CsrfToken token = repository.generateToken(request);
repository.saveToken(token, request, response);
request.setParameter(token.getParameterName(), token.getToken());
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method);
request.getSession().setAttribute("attribute1", "value1");

loadConfig(SessionManagementDefaultSessionFixationServlet31Config.class);

springSecurityFilterChain.doFilter(request, response, chain);

verifyStatic(ReflectionUtils.class);
ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class));
assertThat(!request.getSession().getId().equals(id));
assertThat(request.getSession().getAttribute("attribute1").equals("value1"));
}

@EnableWebSecurity
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import java.lang.reflect.Method;
import javax.servlet.Filter;
import javax.servlet.http.HttpServletRequest;

import org.junit.After;
import org.junit.Before;
Expand All @@ -39,12 +38,7 @@
import org.springframework.security.web.context.HttpSessionSecurityContextRepository;
import org.springframework.util.ReflectionUtils;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.same;
import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.spy;
import static org.powermock.api.mockito.PowerMockito.verifyStatic;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

/**
* @author Rob Winch
Expand Down Expand Up @@ -86,40 +80,40 @@ public void teardown() {
}

@Test
public void changeSessionIdDefaultsInServlet31Plus() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
public void changeSessionIdThenPreserveParameters() throws Exception {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
request.getSession();
request.setServletPath("/login");
request.setMethod("POST");
request.setParameter("username", "user");
request.setParameter("password", "password");
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method);

request.getSession().setAttribute("attribute1", "value1");

String id = request.getSession().getId();

loadContext("<http>\n" + " <form-login/>\n"
+ " <session-management/>\n" + " <csrf disabled='true'/>\n"
+ " </http>" + XML_AUTHENTICATION_MANAGER);

springSecurityFilterChain.doFilter(request, response, chain);

verifyStatic(ReflectionUtils.class);
ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class));

assertThat(!request.getSession().getId().equals(id));
assertThat(request.getSession().getAttribute("attribute1").equals("value1"));
}

@Test
public void changeSessionId() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);

MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
request.getSession();
request.setServletPath("/login");
request.setMethod("POST");
request.setParameter("username", "user");
request.setParameter("password", "password");
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method);

String id = request.getSession().getId();

loadContext("<http>\n"
+ " <form-login/>\n"
Expand All @@ -129,8 +123,8 @@ public void changeSessionId() throws Exception {

springSecurityFilterChain.doFilter(request, response, chain);

verifyStatic(ReflectionUtils.class);
ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class));
assertThat(!request.getSession().getId().equals(id));

}

private void loadContext(String context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,18 @@
*/
package org.springframework.security.web.authentication.session;

import java.lang.reflect.Method;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;

import org.springframework.util.ReflectionUtils;

/**
* Uses {@code HttpServletRequest.changeSessionId()} to protect against session fixation
* attacks. This is the default implementation for Servlet 3.1+.
* attacks. This is the default implementation.
*
* @author Rob Winch
* @since 3.2
*/
public final class ChangeSessionIdAuthenticationStrategy
extends AbstractSessionFixationProtectionStrategy {
private final Method changeSessionIdMethod;

public ChangeSessionIdAuthenticationStrategy() {
Method changeSessionIdMethod = ReflectionUtils
.findMethod(HttpServletRequest.class, "changeSessionId");
if (changeSessionIdMethod == null) {
throw new IllegalStateException(
"HttpServletRequest.changeSessionId is undefined. Are you using a Servlet 3.1+ environment?");
}
this.changeSessionIdMethod = changeSessionIdMethod;
}

/*
* (non-Javadoc)
Expand All @@ -52,7 +37,7 @@ public ChangeSessionIdAuthenticationStrategy() {
*/
@Override
HttpSession applySessionFixation(HttpServletRequest request) {
ReflectionUtils.invokeMethod(this.changeSessionIdMethod, request);
request.changeSessionId();
return request.getSession();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import javax.servlet.http.HttpSession;

/**
* The default implementation of {@link SessionAuthenticationStrategy} when using &lt;
* The implementation of {@link SessionAuthenticationStrategy} when using &lt;
* Servlet 3.1.
* <p>
* Creates a new session for the newly authenticated user if they already have a session
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
import javax.servlet.http.HttpServletResponse;

/**
* Internal interface for creating a {@link HttpServletRequest}. This allows for creating
* a different implementation for Servlet 2.5 and Servlet 3.0 environments.
* Internal interface for creating a {@link HttpServletRequest}.
*
* @author Rob Winch
* @since 3.2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,55 +15,26 @@
*/
package org.springframework.security.web.authentication.session;

import static org.mockito.Matchers.*;
import static org.powermock.api.mockito.PowerMockito.*;

import java.lang.reflect.Method;

import javax.servlet.http.HttpServletRequest;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.util.ReflectionUtils;

/**
* @author Rob Winch
*
*/
@RunWith(PowerMockRunner.class)
@PrepareForTest({ ReflectionUtils.class, Method.class })
public class ChangeSessionIdAuthenticationStrategyTests {
@Mock
private Method method;

@Test(expected = IllegalStateException.class)
public void constructChangeIdMethodNotFound() {
spy(ReflectionUtils.class);
MockHttpServletRequest request = new MockHttpServletRequest();
request.getSession();
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(null);

new ChangeSessionIdAuthenticationStrategy();
}

@Test
public void applySessionFixation() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
public void applySessionFixation() {
MockHttpServletRequest request = new MockHttpServletRequest();
request.getSession();
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method);
String id = request.getSession().getId();

new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request);
new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request);

verifyStatic(ReflectionUtils.class);
ReflectionUtils.invokeMethod(same(method), eq(request));
Assert.assertNotEquals(id, request.getSession().getId());
}

}