Remove Servlet 2.5 Support for Session Fixation

This commit removes existence validation of a method only available in Servlet 3.1.
Spring Framework baseline is Servlet 3.1 so is not longer required.

Fixes: gh-6259
This commit is contained in:
Rafael Dominguez 2018-12-17 13:03:18 -06:00 committed by Josh Cummings
parent 4123d96cd5
commit 086b105273
No known key found for this signature in database
GPG Key ID: 49EF60DD7FF83443
7 changed files with 32 additions and 107 deletions

View File

@ -212,8 +212,7 @@ public final class SessionManagementConfigurer<H extends HttpSecurityBuilder<H>>
/** /**
* Allows explicitly specifying the {@link SessionAuthenticationStrategy}. * Allows explicitly specifying the {@link SessionAuthenticationStrategy}.
* The default is to use {@link SessionFixationProtectionStrategy} for Servlet 3.1 or * The default is to use {@link ChangeSessionIdAuthenticationStrategy}.
* {@link ChangeSessionIdAuthenticationStrategy} for Servlet 3.1+.
* If restricting the maximum number of sessions is configured, then * If restricting the maximum number of sessions is configured, then
* {@link CompositeSessionAuthenticationStrategy} delegating to * {@link CompositeSessionAuthenticationStrategy} delegating to
* {@link ConcurrentSessionControlAuthenticationStrategy}, * {@link ConcurrentSessionControlAuthenticationStrategy},
@ -305,13 +304,11 @@ public final class SessionManagementConfigurer<H extends HttpSecurityBuilder<H>>
/** /**
* Specifies that the Servlet container-provided session fixation protection * 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 * {@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 * and retain all session attributes.
* container results in an {@link IllegalStateException}.
* *
* @return the {@link SessionManagementConfigurer} for further customizations * @return the {@link SessionManagementConfigurer} for further customizations
* @throws IllegalStateException if the container is not Servlet 3.1 or newer.
*/ */
public SessionManagementConfigurer<H> changeSessionId() { public SessionManagementConfigurer<H> changeSessionId() {
setSessionFixationAuthenticationStrategy( setSessionFixationAuthenticationStrategy(
@ -664,11 +661,6 @@ public final class SessionManagementConfigurer<H extends HttpSecurityBuilder<H>>
* @return the default {@link SessionAuthenticationStrategy} for session fixation * @return the default {@link SessionAuthenticationStrategy} for session fixation
*/ */
private static SessionAuthenticationStrategy createDefaultSessionFixationProtectionStrategy() { private static SessionAuthenticationStrategy createDefaultSessionFixationProtectionStrategy() {
try {
return new ChangeSessionIdAuthenticationStrategy(); return new ChangeSessionIdAuthenticationStrategy();
}
catch (IllegalStateException e) {
return new SessionFixationProtectionStrategy();
}
} }
} }

View File

@ -15,12 +15,10 @@
*/ */
package org.springframework.security.config.http; package org.springframework.security.config.http;
import java.lang.reflect.Method;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.List; import java.util.List;
import javax.servlet.ServletRequest; import javax.servlet.ServletRequest;
import javax.servlet.http.HttpServletRequest;
import org.w3c.dom.Element; import org.w3c.dom.Element;
@ -73,7 +71,6 @@ import org.springframework.security.web.session.SimpleRedirectInvalidSessionStra
import org.springframework.security.web.session.SimpleRedirectSessionInformationExpiredStrategy; import org.springframework.security.web.session.SimpleRedirectSessionInformationExpiredStrategy;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.util.ClassUtils; import org.springframework.util.ClassUtils;
import org.springframework.util.ReflectionUtils;
import org.springframework.util.StringUtils; import org.springframework.util.StringUtils;
import org.springframework.util.xml.DomUtils; import org.springframework.util.xml.DomUtils;
@ -350,10 +347,7 @@ class HttpConfigurationBuilder {
} }
if (!StringUtils.hasText(sessionFixationAttribute)) { if (!StringUtils.hasText(sessionFixationAttribute)) {
Method changeSessionIdMethod = ReflectionUtils.findMethod( sessionFixationAttribute = OPT_CHANGE_SESSION_ID;
HttpServletRequest.class, "changeSessionId");
sessionFixationAttribute = changeSessionIdMethod == null ? OPT_SESSION_FIXATION_MIGRATE_SESSION
: OPT_CHANGE_SESSION_ID;
} }
else if (StringUtils.hasText(sessionAuthStratRef)) { else if (StringUtils.hasText(sessionAuthStratRef)) {
pc.getReaderContext().error( pc.getReaderContext().error(

View File

@ -17,7 +17,6 @@ package org.springframework.security.config.annotation.web.configurers;
import java.lang.reflect.Method; import java.lang.reflect.Method;
import javax.servlet.Filter; import javax.servlet.Filter;
import javax.servlet.http.HttpServletRequest;
import org.junit.After; import org.junit.After;
import org.junit.Before; import org.junit.Before;
@ -25,7 +24,6 @@ import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.mockito.Mock; import org.mockito.Mock;
import org.powermock.core.classloader.annotations.PowerMockIgnore; import org.powermock.core.classloader.annotations.PowerMockIgnore;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner; import org.powermock.modules.junit4.PowerMockRunner;
import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.ConfigurableApplicationContext;
@ -44,21 +42,14 @@ import org.springframework.security.web.context.HttpRequestResponseHolder;
import org.springframework.security.web.context.HttpSessionSecurityContextRepository; import org.springframework.security.web.context.HttpSessionSecurityContextRepository;
import org.springframework.security.web.csrf.CsrfToken; import org.springframework.security.web.csrf.CsrfToken;
import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository; import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository;
import org.springframework.util.ReflectionUtils;
import static org.mockito.Matchers.any; import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
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;
/** /**
* *
* @author Rob Winch * @author Rob Winch
*/ */
@RunWith(PowerMockRunner.class) @RunWith(PowerMockRunner.class)
@PrepareForTest({ ReflectionUtils.class, Method.class })
@PowerMockIgnore({ "org.w3c.dom.*", "org.xml.sax.*", "org.apache.xerces.*", "javax.xml.parsers.*" }) @PowerMockIgnore({ "org.w3c.dom.*", "org.xml.sax.*", "org.apache.xerces.*", "javax.xml.parsers.*" })
public class SessionManagementConfigurerServlet31Tests { public class SessionManagementConfigurerServlet31Tests {
@Mock @Mock
@ -87,10 +78,9 @@ public class SessionManagementConfigurerServlet31Tests {
} }
@Test @Test
public void changeSessionIdDefaultsInServlet31Plus() throws Exception { public void changeSessionIdThenPreserveParameters() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
String id = request.getSession().getId();
request.getSession(); request.getSession();
request.setServletPath("/login"); request.setServletPath("/login");
request.setMethod("POST"); request.setMethod("POST");
@ -100,15 +90,14 @@ public class SessionManagementConfigurerServlet31Tests {
CsrfToken token = repository.generateToken(request); CsrfToken token = repository.generateToken(request);
repository.saveToken(token, request, response); repository.saveToken(token, request, response);
request.setParameter(token.getParameterName(), token.getToken()); request.setParameter(token.getParameterName(), token.getToken());
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) request.getSession().setAttribute("attribute1", "value1");
.thenReturn(method);
loadConfig(SessionManagementDefaultSessionFixationServlet31Config.class); loadConfig(SessionManagementDefaultSessionFixationServlet31Config.class);
springSecurityFilterChain.doFilter(request, response, chain); springSecurityFilterChain.doFilter(request, response, chain);
verifyStatic(ReflectionUtils.class); assertThat(!request.getSession().getId().equals(id));
ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); assertThat(request.getSession().getAttribute("attribute1").equals("value1"));
} }
@EnableWebSecurity @EnableWebSecurity

View File

@ -17,7 +17,6 @@ package org.springframework.security.config.http;
import java.lang.reflect.Method; import java.lang.reflect.Method;
import javax.servlet.Filter; import javax.servlet.Filter;
import javax.servlet.http.HttpServletRequest;
import org.junit.After; import org.junit.After;
import org.junit.Before; import org.junit.Before;
@ -39,12 +38,7 @@ import org.springframework.security.web.context.HttpRequestResponseHolder;
import org.springframework.security.web.context.HttpSessionSecurityContextRepository; import org.springframework.security.web.context.HttpSessionSecurityContextRepository;
import org.springframework.util.ReflectionUtils; import org.springframework.util.ReflectionUtils;
import static org.mockito.Matchers.any; import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
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;
/** /**
* @author Rob Winch * @author Rob Winch
@ -86,17 +80,17 @@ public class SessionManagementConfigServlet31Tests {
} }
@Test @Test
public void changeSessionIdDefaultsInServlet31Plus() throws Exception { public void changeSessionIdThenPreserveParameters() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
request.getSession(); request.getSession();
request.setServletPath("/login"); request.setServletPath("/login");
request.setMethod("POST"); request.setMethod("POST");
request.setParameter("username", "user"); request.setParameter("username", "user");
request.setParameter("password", "password"); 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" loadContext("<http>\n" + " <form-login/>\n"
+ " <session-management/>\n" + " <csrf disabled='true'/>\n" + " <session-management/>\n" + " <csrf disabled='true'/>\n"
@ -104,22 +98,22 @@ public class SessionManagementConfigServlet31Tests {
springSecurityFilterChain.doFilter(request, response, chain); 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 @Test
public void changeSessionId() throws Exception { public void changeSessionId() throws Exception {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); MockHttpServletRequest request = new MockHttpServletRequest("GET", "");
request.getSession(); request.getSession();
request.setServletPath("/login"); request.setServletPath("/login");
request.setMethod("POST"); request.setMethod("POST");
request.setParameter("username", "user"); request.setParameter("username", "user");
request.setParameter("password", "password"); request.setParameter("password", "password");
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method); String id = request.getSession().getId();
loadContext("<http>\n" loadContext("<http>\n"
+ " <form-login/>\n" + " <form-login/>\n"
@ -129,8 +123,8 @@ public class SessionManagementConfigServlet31Tests {
springSecurityFilterChain.doFilter(request, response, chain); springSecurityFilterChain.doFilter(request, response, chain);
verifyStatic(ReflectionUtils.class); assertThat(!request.getSession().getId().equals(id));
ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class));
} }
private void loadContext(String context) { private void loadContext(String context) {

View File

@ -15,33 +15,18 @@
*/ */
package org.springframework.security.web.authentication.session; package org.springframework.security.web.authentication.session;
import java.lang.reflect.Method;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession; import javax.servlet.http.HttpSession;
import org.springframework.util.ReflectionUtils;
/** /**
* Uses {@code HttpServletRequest.changeSessionId()} to protect against session fixation * 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 * @author Rob Winch
* @since 3.2 * @since 3.2
*/ */
public final class ChangeSessionIdAuthenticationStrategy public final class ChangeSessionIdAuthenticationStrategy
extends AbstractSessionFixationProtectionStrategy { 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) * (non-Javadoc)
@ -52,7 +37,7 @@ public final class ChangeSessionIdAuthenticationStrategy
*/ */
@Override @Override
HttpSession applySessionFixation(HttpServletRequest request) { HttpSession applySessionFixation(HttpServletRequest request) {
ReflectionUtils.invokeMethod(this.changeSessionIdMethod, request); request.changeSessionId();
return request.getSession(); return request.getSession();
} }
} }

View File

@ -24,7 +24,7 @@ import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession; 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. * Servlet 3.1.
* <p> * <p>
* Creates a new session for the newly authenticated user if they already have a session * Creates a new session for the newly authenticated user if they already have a session

View File

@ -15,55 +15,26 @@
*/ */
package org.springframework.security.web.authentication.session; package org.springframework.security.web.authentication.session;
import static org.mockito.Matchers.*; import org.junit.Assert;
import static org.powermock.api.mockito.PowerMockito.*;
import java.lang.reflect.Method;
import javax.servlet.http.HttpServletRequest;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner; import org.powermock.modules.junit4.PowerMockRunner;
import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.util.ReflectionUtils;
/** /**
* @author Rob Winch * @author Rob Winch
* *
*/ */
@RunWith(PowerMockRunner.class) @RunWith(PowerMockRunner.class)
@PrepareForTest({ ReflectionUtils.class, Method.class })
public class ChangeSessionIdAuthenticationStrategyTests { 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 @Test
public void applySessionFixation() throws Exception { public void applySessionFixation() {
spy(ReflectionUtils.class);
Method method = mock(Method.class);
MockHttpServletRequest request = new MockHttpServletRequest(); MockHttpServletRequest request = new MockHttpServletRequest();
request.getSession(); String id = request.getSession().getId();
when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId"))
.thenReturn(method);
new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request); new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request);
verifyStatic(ReflectionUtils.class); Assert.assertNotEquals(id, request.getSession().getId());
ReflectionUtils.invokeMethod(same(method), eq(request));
} }
} }