From 7d97adc68789f4a191cee15de482530dd9d5c31d Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Sun, 3 Oct 2010 01:04:26 +0100 Subject: [PATCH] SEC-1584: Addition of HttpFirewall strategy to FilterChainProxy to reject un-normalized requests and wrap the incoming request object before processing by the security filter chain to provide a more consistent representation of paths than is guaranteed by the servlet spec. The wrapper strips path parameters from pathInfo and servletPath to provide consistency of URL matching across servlet containers and protect against bypassing security constraints by the malicious addition of such parameters to the URL. The paths are canonicalized further by replacing of multiple sequences of "/" characters with a single "/". --- .gitignore | 1 + .../config/FilterChainProxyConfigTests.java | 6 +- .../security/web/FilterChainProxy.java | 160 ++++++++++++------ .../web/firewall/DefaultHttpFirewall.java | 67 ++++++++ .../web/firewall/FirewalledRequest.java | 34 ++++ .../security/web/firewall/HttpFirewall.java | 32 ++++ .../firewall/RequestRejectedException.java | 10 ++ .../security/web/firewall/RequestWrapper.java | 103 +++++++++++ .../security/web/FilterChainProxyTests.java | 116 +++++++++++++ .../firewall/DefaultHttpFirewallTests.java | 44 +++++ .../web/firewall/RequestWrapperTests.java | 62 +++++++ 11 files changed, 583 insertions(+), 52 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java create mode 100644 web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java create mode 100644 web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java create mode 100644 web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java create mode 100644 web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java create mode 100644 web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java create mode 100644 web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java create mode 100644 web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java diff --git a/.gitignore b/.gitignore index 7ab06b3337..9144c45efc 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ target/ .project .DS_Store .settings/ +.idea/ out/ intellij/ build/ diff --git a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java index ceaeb41120..33d83e460d 100644 --- a/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java +++ b/config/src/test/java/org/springframework/security/config/FilterChainProxyConfigTests.java @@ -116,18 +116,18 @@ public class FilterChainProxyConfigTests { } private void checkPathAndFilterOrder(FilterChainProxy filterChainProxy) throws Exception { - List filters = filterChainProxy.getFilters("/foo/blah"); + List filters = filterChainProxy.getFilters("/foo/blah;x=1"); assertEquals(1, filters.size()); assertTrue(filters.get(0) instanceof SecurityContextHolderAwareRequestFilter); - filters = filterChainProxy.getFilters("/some/other/path/blah"); + filters = filterChainProxy.getFilters("/some;x=2,y=3/other/path;z=4/blah"); assertNotNull(filters); assertEquals(3, filters.size()); assertTrue(filters.get(0) instanceof SecurityContextPersistenceFilter); assertTrue(filters.get(1) instanceof SecurityContextHolderAwareRequestFilter); assertTrue(filters.get(2) instanceof SecurityContextHolderAwareRequestFilter); - filters = filterChainProxy.getFilters("/do/not/filter"); + filters = filterChainProxy.getFilters("/do/not/filter;x=7"); assertEquals(0, filters.size()); filters = filterChainProxy.getFilters("/another/nonspecificmatch"); diff --git a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java index 27edef4515..5d18483226 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -17,64 +17,100 @@ package org.springframework.security.web; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.security.web.firewall.DefaultHttpFirewall; +import org.springframework.security.web.firewall.FirewalledRequest; +import org.springframework.security.web.firewall.HttpFirewall; import org.springframework.security.web.util.AnyRequestMatcher; import org.springframework.security.web.util.RequestMatcher; +import org.springframework.security.web.util.UrlUtils; import org.springframework.util.Assert; import org.springframework.web.filter.DelegatingFilterProxy; import org.springframework.web.filter.GenericFilterBean; -import javax.servlet.*; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletRequestWrapper; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.util.*; /** - * Delegates Filter requests to a list of Spring-managed beans. - * As of version 2.0, you shouldn't need to explicitly configure a FilterChainProxy bean in your application + * Delegates {@code Filter} requests to a list of Spring-managed filter beans. + * As of version 2.0, you shouldn't need to explicitly configure a {@code FilterChainProxy} bean in your application * context unless you need very fine control over the filter chain contents. Most cases should be adequately covered - * by the default <security:http /> namespace configuration options. + * by the default {@code <security:http />} namespace configuration options. *

- * The FilterChainProxy is loaded via a standard Spring {@link DelegatingFilterProxy} declaration in - * web.xml. + * The {@code FilterChainProxy} is linked into the servlet container filter chain by adding a standard + * Spring {@link DelegatingFilterProxy} declaration in the application {@code web.xml} file. + * + *

Configuration

*

- * As of version 3.1, FilterChainProxy is configured using an ordered Map of {@link RequestMatcher} instances - * to Lists of Filters. The Map instance will normally be created while parsing the namespace - * configuration, so doesn't have to be set explicitly. Instead the <filter-chain-map> element should be used - * within the FilterChainProxy bean declaration. - * This in turn should have a list of child <filter-chain> elements which each define a URI pattern and the list - * of filters (as comma-separated bean names) which should be applied to requests which match the pattern. - * An example configuration might look like this: + * As of version 3.1, {@code FilterChainProxy} is configured using an ordered Map of {@link RequestMatcher} instances + * to {@code List}s of {@code Filter}s. The Map instance will normally be created while parsing the namespace + * configuration, so doesn't have to be set explicitly. Instead the {@code <filter-chain-map>} + * element should be used within the bean declaration. + * This in turn should have a list of child {@code <filter-chain>} elements which each define a URI pattern and + * the list of filters (as comma-separated bean names) which should be applied to requests which match the pattern. + * The default pattern matching strategy is to use {@link org.springframework.security.web.util.AntPathRequestMatcher + * Ant-style paths}. An example configuration might look like this: * * 

  <bean id="myfilterChainProxy" class="org.springframework.security.util.FilterChainProxy">
      <security:filter-chain-map request-matcher="ant">
-         <security:filter-chain pattern="/do/not/filter" filters="none"/>
+         <security:filter-chain pattern="/do/not/filter*" filters="none"/>
          <security:filter-chain pattern="/**" filters="filter1,filter2,filter3"/>
      </security:filter-chain-map>
  </bean>
  *
* - * The names "filter1", "filter2", "filter3" should be the bean names of Filter instances defined in the + * The names "filter1", "filter2", "filter3" should be the bean names of {@code Filter} instances defined in the * application context. The order of the names defines the order in which the filters will be applied. As shown above, * use of the value "none" for the "filters" can be used to exclude a request pattern from the security filter chain * entirely. Please consult the security namespace schema file for a full list of available configuration options. + * + *

Request Handling

*

- * Each possible pattern that FilterChainProxy should service must be entered. - * The first match for a given request will be used to define all of the Filters that apply to that - * request. This means you must put most specific matches at the top of the list, and ensure all Filters + * Each possible pattern that the {@code FilterChainProxy} should service must be entered. + * The first match for a given request will be used to define all of the {@code Filter}s that apply to that + * request. This means you must put most specific matches at the top of the list, and ensure all {@code Filter}s * that should apply for a given matcher are entered against the respective entry. - * The FilterChainProxy will not iterate through the remainder of the map entries to locate additional - * Filters. + * The {@code FilterChainProxy} will not iterate through the remainder of the map entries to locate additional + * {@code Filter}s. *

- * FilterChainProxy respects normal handling of Filters that elect not to call {@link + * {@code FilterChainProxy} respects normal handling of {@code Filter}s that elect not to call {@link * javax.servlet.Filter#doFilter(javax.servlet.ServletRequest, javax.servlet.ServletResponse, - * javax.servlet.FilterChain)}, in that the remainder of the original or FilterChainProxy-declared filter + * javax.servlet.FilterChain)}, in that the remainder of the original or {@code FilterChainProxy}-declared filter * chain will not be called. + * + *

Request Firewalling

+ * + * An {@link HttpFirewall} instance is used to validate incoming requests and create a wrapped request which provides + * consistent path values for matching against. See {@link DefaultHttpFirewall}, for more information on the type of + * attacks which the default implementation protects against. A custom implementation can be injected to provide + * stricter control over the request contents or if an application needs to support certain types of request which + * are rejected by default. *

- * Note the Filter lifecycle mismatch between the servlet container and IoC - * container. As described in the {@link DelegatingFilterProxy} JavaDocs, we recommend you allow the IoC - * container to manage the lifecycle instead of the servlet container. + * Note that this means that you must use the Spring Security filters in combination with a {@code FilterChainProxy} + * if you want this protection. Don't define them explicitly in your {@code web.xml} file. + *

+ * {@code FilterChainProxy} will use the firewall instance to obtain both request and response objects which will be + * fed down the filter chain, so it is also possible to use this functionality to control the functionality of the + * response. When the request has passed through the security filter chain, the {@code reset} method will be called. + * With the default implementation this means that the original values of {@code servletPath} and {@code pathInfo} will + * be returned thereafter, instead of the modified ones used for security pattern matching. + * + * + *

Filter Lifecycle

+ *

+ * Note the {@code Filter} lifecycle mismatch between the servlet container and IoC + * container. As described in the {@link DelegatingFilterProxy} Javadocs, we recommend you allow the IoC + * container to manage the lifecycle instead of the servlet container. {@code FilterChainProxy} does not invoke the + * standard filter lifecycle methods on any filter beans that you add to the application context. * * @author Carlos Sanchez * @author Ben Alex @@ -92,6 +128,8 @@ public class FilterChainProxy extends GenericFilterBean { private FilterChainValidator filterChainValidator = new NullFilterChainValidator(); + private HttpFirewall firewall = new DefaultHttpFirewall(); + //~ Methods ======================================================================================================== @Override @@ -103,25 +141,27 @@ public class FilterChainProxy extends GenericFilterBean { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - FilterInvocation fi = new FilterInvocation(request, response, chain); - List filters = getFilters(fi.getRequest()); + FirewalledRequest fwRequest = firewall.getFirewalledRequest((HttpServletRequest) request); + HttpServletResponse fwResponse = firewall.getFirewalledResponse((HttpServletResponse) response); + String url = UrlUtils.buildRequestUrl(fwRequest); + + List filters = getFilters(fwRequest); if (filters == null || filters.size() == 0) { if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() + + logger.debug(url + (filters == null ? " has no matching filters" : " has an empty filter list")); } - chain.doFilter(request, response); + chain.doFilter(fwRequest, fwResponse); return; } - VirtualFilterChain virtualFilterChain = new VirtualFilterChain(fi, filters); - virtualFilterChain.doFilter(fi.getRequest(), fi.getResponse()); + VirtualFilterChain vfc = new VirtualFilterChain(url, chain, filters); + vfc.doFilter(fwRequest, fwResponse); } - /** * Returns the first filter chain matching the supplied URL. * @@ -147,20 +187,20 @@ public class FilterChainProxy extends GenericFilterBean { * @return matching filter list */ public List getFilters(String url) { - return getFilters(new FilterInvocation(url, null).getRequest()); + return getFilters(firewall.getFirewalledRequest((new FilterInvocation(url, null).getRequest()))); } /** * Sets the mapping of URL patterns to filter chains. * - * The map keys should be the paths and the values should be arrays of Filter objects. + * The map keys should be the paths and the values should be arrays of {@code Filter} objects. * It's VERY important that the type of map used preserves ordering - the order in which the iterator * returns the entries must be the same as the order they were added to the map, otherwise you have no way * of guaranteeing that the most specific patterns are returned before the more general ones. So make sure - * the Map used is an instance of LinkedHashMap or an equivalent, rather than a plain HashMap, for + * the Map used is an instance of {@code LinkedHashMap} or an equivalent, rather than a plain {@code HashMap}, for * example. * - * @param filterChainMap the map of path Strings to List<Filter>s. + * @param filterChainMap the map of path Strings to {@code List<Filter>}s. */ @SuppressWarnings("unchecked") public void setFilterChainMap(Map filterChainMap) { @@ -198,7 +238,7 @@ public class FilterChainProxy extends GenericFilterBean { /** * Returns a copy of the underlying filter chain map. Modifications to the map contents - * will not affect the FilterChainProxy state - to change the map call setFilterChainMap. + * will not affect the FilterChainProxy state - to change the map call {@code setFilterChainMap}. * * @return the map of path pattern Strings to filter chain lists (with ordering guaranteed). */ @@ -216,6 +256,16 @@ public class FilterChainProxy extends GenericFilterBean { this.filterChainValidator = filterChainValidator; } + /** + * Sets the "firewall" implementation which will be used to validate and wrap (or potentially reject) the + * incoming requests. The default implementation should be satisfactory for most requirements. + * + * @param firewall + */ + public void setFirewall(HttpFirewall firewall) { + this.firewall = firewall; + } + public String toString() { StringBuilder sb = new StringBuilder(); sb.append("FilterChainProxy["); @@ -230,18 +280,18 @@ public class FilterChainProxy extends GenericFilterBean { /** * Internal {@code FilterChain} implementation that is used to pass a request through the additional - * internal list of filters which match the request. Records the position in the additional filter chain and, when - * completed, passes the request back to the original {@code FilterChain} supplied by the servlet container. + * internal list of filters which match the request. */ private static class VirtualFilterChain implements FilterChain { - private final FilterInvocation fi; + private final FilterChain originalChain; private final List additionalFilters; + private final String url; private final int size; private int currentPosition = 0; - - private VirtualFilterChain(FilterInvocation filterInvocation, List additionalFilters) { - this.fi = filterInvocation; + private VirtualFilterChain(String url, FilterChain chain, List additionalFilters) { + this.originalChain = chain; + this.url = url; this.additionalFilters = additionalFilters; this.size = additionalFilters.size(); } @@ -249,23 +299,35 @@ public class FilterChainProxy extends GenericFilterBean { public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { if (currentPosition == size) { if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() - + " reached end of additional filter chain; proceeding with original chain"); + logger.debug(url + " reached end of additional filter chain; proceeding with original chain"); } - fi.getChain().doFilter(request, response); + // Deactivate path stripping as we exit the security filter chain + resetWrapper(request); + + originalChain.doFilter(request, response); } else { currentPosition++; Filter nextFilter = additionalFilters.get(currentPosition - 1); if (logger.isDebugEnabled()) { - logger.debug(fi.getRequestUrl() + " at position " + currentPosition + " of " + logger.debug(url + " at position " + currentPosition + " of " + size + " in additional filter chain; firing Filter: '" - + nextFilter + "'"); + + nextFilter.getClass().getSimpleName() + "'"); } - nextFilter.doFilter(request, response, this); + nextFilter.doFilter(request, response, this); + } + } + + private void resetWrapper(ServletRequest request) { + while (request instanceof ServletRequestWrapper) { + if (request instanceof FirewalledRequest) { + ((FirewalledRequest)request).reset(); + break; + } + request = ((ServletRequestWrapper)request).getRequest(); } } } diff --git a/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java b/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java new file mode 100644 index 0000000000..a40fa435dc --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java @@ -0,0 +1,67 @@ +package org.springframework.security.web.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * Default implementation which wraps requests in order to provide consistent values of the {@code servletPath} and + * {@code pathInfo}, which do not contain path parameters (as defined in + * RFC 2396). Different servlet containers + * interpret the servlet spec differently as to how path parameters are treated and it is possible they might be added + * in order to bypass particular security constraints. When using this implementation, they will be removed for all + * requests as the request passes through the security filter chain. Note that this means that any segments in the + * decoded path which contain a semi-colon, will have the part following the semi-colon removed for + * request matching. Your application should not contain any valid paths which contain semi-colons. + *

+ * If any un-normalized paths are found (containing directory-traversal character sequences), the request will be + * rejected immediately. Most containers normalize the paths before performing the servlet-mapping, but again this is + * not guaranteed by the servlet spec. + * + * @author Luke Taylor + */ +public class DefaultHttpFirewall implements HttpFirewall { + + public FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException { + FirewalledRequest fwr = new RequestWrapper(request); + + if (!isNormalized(fwr.getServletPath()) || !isNormalized(fwr.getPathInfo())) { + throw new RequestRejectedException("Un-normalized paths are not supported: " + fwr.getServletPath() + + (fwr.getPathInfo() != null ? fwr.getPathInfo() : "")); + } + + return fwr; + } + + public HttpServletResponse getFirewalledResponse(HttpServletResponse response) { + return response; + } + + /** + * Checks whether a path is normalized (doesn't contain path traversal sequences like "./", "/../" or "/.") + * + * @param path the path to test + * @return true if the path doesn't contain any path-traversal character sequences. + */ + private boolean isNormalized(String path) { + if (path == null) { + return true; + } + + for (int j = path.length(); j > 0;) { + int i = path.lastIndexOf('/', j - 1); + int gap = j - i; + + if (gap == 2 && path.charAt(i+1) == '.') { + // ".", "/./" or "/." + return false; + } else if (gap == 3 && path.charAt(i+1) == '.'&& path.charAt(i+2) == '.') { + return false; + } + + j = i; + } + + return true; + } + +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java b/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java new file mode 100644 index 0000000000..48cf40acc2 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/FirewalledRequest.java @@ -0,0 +1,34 @@ +package org.springframework.security.web.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; + +/** + * Request wrapper which is returned by the {@code HttpFirewall} interface. + *

+ * The only difference is the {@code reset} method which allows some + * or all of the state to be reset by the {@code FilterChainProxy} when the + * request leaves the security filter chain. + * + * @author Luke Taylor + */ +public abstract class FirewalledRequest extends HttpServletRequestWrapper { + /** + * Constructs a request object wrapping the given request. + * + * @throws IllegalArgumentException if the request is null + */ + public FirewalledRequest(HttpServletRequest request) { + super(request); + } + + /** + * This method will be called once the request has passed through the + * security filter chain, when it is about to proceed to the application + * proper. + *

+ * An implementation can thus choose to modify the state of the request + * for the security infrastructure, while still maintaining the + */ + public abstract void reset(); +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java b/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java new file mode 100644 index 0000000000..de7b9af17e --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/HttpFirewall.java @@ -0,0 +1,32 @@ +package org.springframework.security.web.firewall; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * Interface which can be used to reject potentially dangerous requests and/or wrap them to + * control their behaviour. + *

+ * The implementation is injected into the {@code FilterChainProxy} and will be invoked before + * sending any request through the filter chain. It can also provide a response wrapper if the response + * behaviour should also be restricted. + * + * @author Luke Taylor + */ +public interface HttpFirewall { + + /** + * Provides the request object which will be passed through the filter chain. + * + * @throws RequestRejectedException if the request should be rejected immediately + */ + FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException; + + /** + * Provides the response which will be passed through the filter chain. + * + * @param response the original response + * @return either the original response or a replacement/wrapper. + */ + HttpServletResponse getFirewalledResponse(HttpServletResponse response); +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java new file mode 100644 index 0000000000..fe334219a0 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedException.java @@ -0,0 +1,10 @@ +package org.springframework.security.web.firewall; + +/** + * @author Luke Taylor + */ +public class RequestRejectedException extends RuntimeException { + public RequestRejectedException(String message) { + super(message); + } +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java b/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java new file mode 100644 index 0000000000..9cc3921905 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestWrapper.java @@ -0,0 +1,103 @@ +package org.springframework.security.web.firewall; + +import org.springframework.security.web.util.UrlUtils; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; +import java.util.*; + +/** + * Request wrapper which ensures values of {@code servletPath} and {@code pathInfo} are returned which are suitable for + * pattern matching against. It strips out path parameters and extra consecutive '/' characters. + * + *

Path Parameters

+ * Parameters (as defined in RFC 2396) are stripped from the path + * segments of the {@code servletPath} and {@code pathInfo} values of the request. + *

+ * The parameter sequence is demarcated by a semi-colon, so each segment is checked for the occurrence of a ";" + * character and truncated at that point if it is present. + *

+ * The behaviour differs between servlet containers in how they interpret the servlet spec, which + * does not clearly state what the behaviour should be. For consistency, we make sure they are always removed, to + * avoid the risk of URL matching rules being bypassed by the malicious addition of parameters to the path component. + * + * @author Luke Taylor + */ +final class RequestWrapper extends FirewalledRequest { + private final String strippedServletPath; + private final String strippedPathInfo; + private boolean stripPaths = true; + + public RequestWrapper(HttpServletRequest request) { + super(request); + strippedServletPath = strip(request.getServletPath()); + String pathInfo = strip(request.getPathInfo()); + if (pathInfo != null && pathInfo.length() == 0) { + pathInfo = null; + } + strippedPathInfo = pathInfo; + } + + /** + * Removes path parameters from each path segment in the supplied path and truncates sequences of multiple '/' + * characters to a single '/'. + * + * @param path either the {@code servletPath} and {@code pathInfo} from the original request + * + * @return the supplied value, with path parameters removed and sequences of multiple '/' characters truncated, + * or null if the supplied path was null. + */ + private String strip(String path) { + if (path == null) { + return null; + } + + int scIndex = path.indexOf(';'); + + if (scIndex < 0) { + int doubleSlashIndex = path.indexOf("//"); + if (doubleSlashIndex < 0) { + // Most likely case, no parameters in any segment and no '//', so no stripping required + return path; + } + } + + StringTokenizer st = new StringTokenizer(path, "/"); + StringBuilder stripped = new StringBuilder(path.length()); + + if (path.charAt(0) == '/') { + stripped.append('/'); + } + + while(st.hasMoreTokens()) { + String segment = st.nextToken(); + scIndex = segment.indexOf(';'); + + if (scIndex >= 0) { + segment = segment.substring(0, scIndex); + } + stripped.append(segment).append('/'); + } + + // Remove the trailing slash if the original path didn't have one + if (path.charAt(path.length() - 1) != '/') { + stripped.deleteCharAt(stripped.length() - 1); + } + + return stripped.toString(); + } + + @Override + public String getPathInfo() { + return stripPaths ? strippedPathInfo : super.getPathInfo(); + } + + @Override + public String getServletPath() { + return stripPaths ? strippedServletPath : super.getServletPath(); + } + + public void reset() { + this.stripPaths = false; + } +} diff --git a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java new file mode 100644 index 0000000000..917dc47063 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -0,0 +1,116 @@ +package org.springframework.security.web; + +import static org.junit.Assert.*; +import static org.mockito.Mockito.*; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.security.web.firewall.FirewalledRequest; +import org.springframework.security.web.util.RequestMatcher; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequestWrapper; +import javax.servlet.http.HttpServletResponse; +import java.util.*; + +/** + * @author Luke Taylor + */ +@SuppressWarnings({"unchecked"}) +public class FilterChainProxyTests { + private FilterChainProxy fcp; + private RequestMatcher matcher; + private MockHttpServletRequest request; + private MockHttpServletResponse response; + private FilterChain chain; + private Filter filter; + + @Before + public void setup() throws Exception { + fcp = new FilterChainProxy(); + fcp.setFilterChainValidator(mock(FilterChainProxy.FilterChainValidator.class)); + matcher = mock(RequestMatcher.class); + filter = mock(Filter.class); + doAnswer(new Answer() { + public Object answer(InvocationOnMock inv) throws Throwable { + Object[] args = inv.getArguments(); + FilterChain fc = (FilterChain) args[2]; + HttpServletRequestWrapper extraWrapper = + new HttpServletRequestWrapper((HttpServletRequest) args[0]); + fc.doFilter(extraWrapper, (HttpServletResponse) args[1]); + return null; + } + }).when(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + LinkedHashMap map = new LinkedHashMap(); + map.put(matcher, Arrays.asList(filter)); + fcp.setFilterChainMap(map); + request = new MockHttpServletRequest(); + request.setServletPath("/path"); + response = new MockHttpServletResponse(); + chain = mock(FilterChain.class); + } + + @Test + public void toStringCallSucceeds() throws Exception { + fcp.afterPropertiesSet(); + fcp.toString(); + } + + @Test + public void securityFilterChainIsNotInvokedIfMatchFails() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(false); + fcp.doFilter(request, response, chain); + assertEquals(1, fcp.getFilterChainMap().size()); + assertSame(filter, fcp.getFilterChainMap().get(matcher).get(0)); + + verifyZeroInteractions(filter); + // The actual filter chain should be invoked though + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void originalChainIsInvokedAfterSecurityChainIfMatchSucceeds() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + fcp.doFilter(request, response, chain); + + verify(filter).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void originalFilterChainIsInvokedIfMatchingSecurityChainIsEmpty() throws Exception { + LinkedHashMap map = new LinkedHashMap(); + map.put(matcher, Collections.emptyList()); + fcp.setFilterChainMap(map); + + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + fcp.doFilter(request, response, chain); + + verify(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void requestIsWrappedForMatchingAndFilteringWhenMatchIsFound() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(true); + fcp.doFilter(request, response, chain); + verify(matcher).matches(any(FirewalledRequest.class)); + verify(filter).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class), any(FilterChain.class)); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); + } + + @Test + public void requestIsWrappedForMatchingAndFilteringWhenMatchIsNotFound() throws Exception { + when(matcher.matches(any(HttpServletRequest.class))).thenReturn(false); + fcp.doFilter(request, response, chain); + verify(matcher).matches(any(FirewalledRequest.class)); + verifyZeroInteractions(filter); + verify(chain).doFilter(any(FirewalledRequest.class), any(HttpServletResponse.class)); + } + +} diff --git a/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java b/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java new file mode 100644 index 0000000000..8b3df587e9 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java @@ -0,0 +1,44 @@ +package org.springframework.security.web.firewall; + +import static org.junit.Assert.fail; + +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +/** + * @author Luke Taylor + */ +public class DefaultHttpFirewallTests { + public String[] unnormalizedPaths = { + "/..", + "/./path/", + "/path/path/.", + "/path/path//.", + "./path/../path//.", + "./path", + ".//path", + "." + }; + + @Test + public void unnormalizedPathsAreRejected() throws Exception { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + + MockHttpServletRequest request; + for (String path : unnormalizedPaths) { + request = new MockHttpServletRequest(); + request.setServletPath(path); + try { + fw.getFirewalledRequest(request); + fail(path + " is un-normalized"); + } catch (RequestRejectedException expected) { + } + request.setPathInfo(path); + try { + fw.getFirewalledRequest(request); + fail(path + " is un-normalized"); + } catch (RequestRejectedException expected) { + } + } + } +} diff --git a/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java b/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java new file mode 100644 index 0000000000..b462251440 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/firewall/RequestWrapperTests.java @@ -0,0 +1,62 @@ +package org.springframework.security.web.firewall; + +import static org.junit.Assert.assertEquals; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.springframework.mock.web.MockHttpServletRequest; + +import java.util.*; + +/** + * @author Luke Taylor + */ +public class RequestWrapperTests { + private static Map testPaths = new LinkedHashMap(); + + @BeforeClass + // Some of these may be unrealistic values, but we can't be sure because of the + // inconsistency in the spec. + public static void createTestMap() { + testPaths.put("/path1;x=y;z=w/path2;x=y/path3;x=y", "/path1/path2/path3"); + testPaths.put("/path1;x=y/path2;x=y/", "/path1/path2/"); + testPaths.put("/path1//path2/", "/path1/path2/"); + testPaths.put("//path1/path2//", "/path1/path2/"); + testPaths.put(";x=y;z=w", ""); + } + + @Test + public void pathParametersAreRemovedFromServletPath() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + for (Map.Entry entry : testPaths.entrySet()) { + String path = entry.getKey(); + String expectedResult = entry.getValue(); + request.setServletPath(path); + RequestWrapper wrapper = new RequestWrapper(request); + assertEquals(expectedResult, wrapper.getServletPath()); + wrapper.reset(); + assertEquals(path, wrapper.getServletPath()); + } + } + + @Test + public void pathParametersAreRemovedFromPathInfo() { + MockHttpServletRequest request = new MockHttpServletRequest(); + + for (Map.Entry entry : testPaths.entrySet()) { + String path = entry.getKey(); + String expectedResult = entry.getValue(); + // Should be null when stripped value is empty + if (expectedResult.length() == 0) { + expectedResult = null; + } + request.setPathInfo(path); + RequestWrapper wrapper = new RequestWrapper(request); + assertEquals(expectedResult, wrapper.getPathInfo()); + wrapper.reset(); + assertEquals(path, wrapper.getPathInfo()); + } + } + +}