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 "/".
This commit is contained in:
Luke Taylor
@@ -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));
|
||||
}
|
||||
|
||||
}
|
||||
@@ -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) {
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<String, String> testPaths = new LinkedHashMap<String,String>();
|
||||
|
||||
@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<String,String> 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<String,String> 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());
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
Reference in New Issue
Block a user