From ed2ae21074b7850d386371b2ab9e29268ef2f0c0 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Wed, 7 Dec 2016 14:32:41 -0600 Subject: [PATCH] Block URL Encoded "/" in DefaultHttpFirewall Fixes gh-4170 --- .../web/firewall/DefaultHttpFirewall.java | 87 ++++++++++++++----- .../firewall/DefaultHttpFirewallTests.java | 72 +++++++++++++-- 2 files changed, 126 insertions(+), 33 deletions(-) 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 index 2847c282ab..a63035a038 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java +++ b/web/src/main/java/org/springframework/security/web/firewall/DefaultHttpFirewall.java @@ -19,49 +19,89 @@ 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. + * 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. + * 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 { + private boolean allowUrlEncodedSlash; - public FirewalledRequest getFirewalledRequest(HttpServletRequest request) - throws RequestRejectedException { + @Override + 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() + throw new RequestRejectedException("Un-normalized paths are not supported: " + fwr.getServletPath() + (fwr.getPathInfo() != null ? fwr.getPathInfo() : "")); } + String requestURI = fwr.getRequestURI(); + if (containsInvalidUrlEncodedSlash(requestURI)) { + throw new RequestRejectedException("The requestURI cannot contain encoded slash. Got " + requestURI); + } + return fwr; } + @Override public HttpServletResponse getFirewalledResponse(HttpServletResponse response) { return new FirewalledResponse(response); } /** - * Checks whether a path is normalized (doesn't contain path traversal sequences like - * "./", "/../" or "/.") + *

+ * Sets if the application should allow a URL encoded slash character. + *

+ *

+ * If true (default is false), a URL encoded slash will be allowed in the + * URL. Allowing encoded slashes can cause security vulnerabilities in some + * situations depending on how the container constructs the + * HttpServletRequest. + *

* - * @param path the path to test - * @return true if the path doesn't contain any path-traversal character sequences. + * @param allowUrlEncodedSlash + * the new value (default false) + */ + public void setAllowUrlEncodedSlash(boolean allowUrlEncodedSlash) { + this.allowUrlEncodedSlash = allowUrlEncodedSlash; + } + + private boolean containsInvalidUrlEncodedSlash(String uri) { + if (this.allowUrlEncodedSlash || uri == null) { + return false; + } + + if (uri.contains("%2f") || uri.contains("%2F")) { + return true; + } + + return false; + } + + /** + * 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) { @@ -75,8 +115,7 @@ public class DefaultHttpFirewall implements HttpFirewall { if (gap == 2 && path.charAt(i + 1) == '.') { // ".", "/./" or "/." return false; - } - else if (gap == 3 && path.charAt(i + 1) == '.' && path.charAt(i + 2) == '.') { + } else if (gap == 3 && path.charAt(i + 1) == '.' && path.charAt(i + 2) == '.') { return false; } 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 index 9892f6cfc9..6e2e152dd1 100644 --- a/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java +++ b/web/src/test/java/org/springframework/security/web/firewall/DefaultHttpFirewallTests.java @@ -15,39 +15,93 @@ */ package org.springframework.security.web.firewall; -import static org.assertj.core.api.Assertions.fail; - import org.junit.Test; + import org.springframework.mock.web.MockHttpServletRequest; +import static org.assertj.core.api.Assertions.fail; + /** * @author Luke Taylor */ public class DefaultHttpFirewallTests { - public String[] unnormalizedPaths = { "/..", "/./path/", "/path/path/.", - "/path/path//.", "./path/../path//.", "./path", ".//path", "." }; + 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) { + for (String path : this.unnormalizedPaths) { request = new MockHttpServletRequest(); request.setServletPath(path); try { fw.getFirewalledRequest(request); fail(path + " is un-normalized"); - } - catch (RequestRejectedException expected) { + } catch (RequestRejectedException expected) { } request.setPathInfo(path); try { fw.getFirewalledRequest(request); fail(path + " is un-normalized"); - } - catch (RequestRejectedException expected) { + } catch (RequestRejectedException expected) { } } } + + /** + * On WebSphere 8.5 a URL like /context-root/a/b;%2f1/c can bypass a rule on + * /a/b/c because the pathInfo is /a/b;/1/c which ends up being /a/b/1/c + * while Spring MVC will strip the ; content from requestURI before the path + * is URL decoded. + */ + @Test(expected = RequestRejectedException.class) + public void getFirewalledRequestWhenLowercaseEncodedPathThenException() { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/context-root/a/b;%2f1/c"); + request.setContextPath("/context-root"); + request.setServletPath(""); + request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI + fw.getFirewalledRequest(request); + } + + @Test(expected = RequestRejectedException.class) + public void getFirewalledRequestWhenUppercaseEncodedPathThenException() { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/context-root/a/b;%2F1/c"); + request.setContextPath("/context-root"); + request.setServletPath(""); + request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI + + fw.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenAllowUrlEncodedSlashAndLowercaseEncodedPathThenNoException() { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + fw.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/context-root/a/b;%2f1/c"); + request.setContextPath("/context-root"); + request.setServletPath(""); + request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI + + fw.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenAllowUrlEncodedSlashAndUppercaseEncodedPathThenNoException() { + DefaultHttpFirewall fw = new DefaultHttpFirewall(); + fw.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/context-root/a/b;%2F1/c"); + request.setContextPath("/context-root"); + request.setServletPath(""); + request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI + + fw.getFirewalledRequest(request); + } }