This is an automated email from the ASF dual-hosted git repository. bdemers pushed a commit to branch 1.7.x in repository https://gitbox.apache.org/repos/asf/shiro.git
commit a28300448ae6c4bb78a8ba626b0cacb00f82d5f8 Author: Brian Demers <[email protected]> AuthorDate: Thu Sep 3 14:58:45 2020 -0400 Adds configuration to toggle the normalization of backslashes This is normally handled by the container Update the InvalidRequestFilter to use WebUtils.ALLOW_BACKSLASH (new system property: org.apache.shiro.web.ALLOW_BACKSLASH) Fixes: SHIRO-794 --- .../shiro/web/filter/InvalidRequestFilter.java | 22 +++++-- .../java/org/apache/shiro/web/util/WebUtils.java | 4 +- .../web/filter/InvalidRequestFilterTest.groovy | 48 +++++++++++++-- .../org/apache/shiro/web/util/WebUtilsTest.groovy | 52 ++++++++++++++++ .../apache/shiro/web/RestoreSystemProperties.java | 69 ++++++++++++++++++++++ 5 files changed, 182 insertions(+), 13 deletions(-) diff --git a/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java b/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java index 3d229e9..e352d5c 100644 --- a/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java +++ b/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java @@ -19,10 +19,12 @@ package org.apache.shiro.web.filter; +import org.apache.shiro.util.StringUtils; import org.apache.shiro.web.util.WebUtils; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -48,16 +50,24 @@ public class InvalidRequestFilter extends AccessControlFilter { private boolean blockSemicolon = true; - private boolean blockBackslash = true; + private boolean blockBackslash = !Boolean.getBoolean(WebUtils.ALLOW_BACKSLASH); private boolean blockNonAscii = true; @Override - protected boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) throws Exception { - String uri = WebUtils.toHttp(request).getRequestURI(); - return !containsSemicolon(uri) - && !containsBackslash(uri) - && !containsNonAsciiCharacters(uri); + protected boolean isAccessAllowed(ServletRequest req, ServletResponse response, Object mappedValue) throws Exception { + HttpServletRequest request = WebUtils.toHttp(req); + // check the original and decoded values + return isValid(request.getRequestURI()) // user request string (not decoded) + && isValid(request.getServletPath()) // decoded servlet part + && isValid(request.getPathInfo()); // decoded path info (may be null) + } + + private boolean isValid(String uri) { + return !StringUtils.hasText(uri) + || ( !containsSemicolon(uri) + && !containsBackslash(uri) + && !containsNonAsciiCharacters(uri)); } @Override diff --git a/web/src/main/java/org/apache/shiro/web/util/WebUtils.java b/web/src/main/java/org/apache/shiro/web/util/WebUtils.java index b9c9f62..ea0974f 100644 --- a/web/src/main/java/org/apache/shiro/web/util/WebUtils.java +++ b/web/src/main/java/org/apache/shiro/web/util/WebUtils.java @@ -57,6 +57,8 @@ public class WebUtils { public static final String SERVLET_REQUEST_KEY = ServletRequest.class.getName() + "_SHIRO_THREAD_CONTEXT_KEY"; public static final String SERVLET_RESPONSE_KEY = ServletResponse.class.getName() + "_SHIRO_THREAD_CONTEXT_KEY"; + public static final String ALLOW_BACKSLASH = "org.apache.shiro.web.ALLOW_BACKSLASH"; + /** * {@link org.apache.shiro.session.Session Session} key used to save a request and later restore it, for example when redirecting to a * requested page after login, equal to {@code shiroSavedRequest}. @@ -163,7 +165,7 @@ public class WebUtils { * @return normalized path */ public static String normalize(String path) { - return normalize(path, true); + return normalize(path, Boolean.getBoolean(ALLOW_BACKSLASH)); } /** diff --git a/web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy b/web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy index 8d0b1c0..c7a0525 100644 --- a/web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy +++ b/web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy @@ -19,6 +19,7 @@ package org.apache.shiro.web.filter +import org.apache.shiro.web.RestoreSystemProperties import org.junit.Test import javax.servlet.http.HttpServletRequest @@ -39,6 +40,25 @@ class InvalidRequestFilterTest { } @Test + void systemPropertyAllowBackslash() { + RestoreSystemProperties.withProperties(["org.apache.shiro.web.ALLOW_BACKSLASH": "true"]) { + InvalidRequestFilter filter = new InvalidRequestFilter() + assertThat "filter.blockBackslash expected to be false", !filter.isBlockBackslash() + } + + RestoreSystemProperties.withProperties(["org.apache.shiro.web.ALLOW_BACKSLASH": ""]) { + InvalidRequestFilter filter = new InvalidRequestFilter() + assertThat "filter.blockBackslash expected to be false", filter.isBlockBackslash() + } + + RestoreSystemProperties.withProperties(["org.apache.shiro.web.ALLOW_BACKSLASH": "false"]) { + InvalidRequestFilter filter = new InvalidRequestFilter() + assertThat "filter.blockBackslash expected to be false", filter.isBlockBackslash() + } + } + + + @Test void testFilterBlocks() { InvalidRequestFilter filter = new InvalidRequestFilter() assertPathBlocked(filter, "/\\something") @@ -48,6 +68,9 @@ class InvalidRequestFilterTest { assertPathBlocked(filter, "/%3bsomething") assertPathBlocked(filter, "/%3Bsomething") assertPathBlocked(filter, "/\u0019something") + + assertPathBlocked(filter, "/something", "/;something") + assertPathBlocked(filter, "/something", "/something", "/;") } @Test @@ -61,6 +84,9 @@ class InvalidRequestFilterTest { assertPathBlocked(filter, "/%3bsomething") assertPathBlocked(filter, "/%3Bsomething") assertPathBlocked(filter, "/\u0019something") + + assertPathAllowed(filter, "/something", "/\\something") + assertPathAllowed(filter, "/something", "/something", "/\\") } @Test @@ -74,6 +100,9 @@ class InvalidRequestFilterTest { assertPathBlocked(filter, "/%3bsomething") assertPathBlocked(filter, "/%3Bsomething") assertPathAllowed(filter, "/\u0019something") + + assertPathAllowed(filter, "/something", "/\u0019something") + assertPathAllowed(filter, "/something", "/something", "/\u0019") } @Test void testFilterAllowsSemicolon() { @@ -86,20 +115,27 @@ class InvalidRequestFilterTest { assertPathAllowed(filter, "/%3bsomething") assertPathAllowed(filter, "/%3Bsomething") assertPathBlocked(filter, "/\u0019something") + + assertPathAllowed(filter, "/something", "/;something") + assertPathAllowed(filter, "/something", "/something", "/;") } - static void assertPathBlocked(InvalidRequestFilter filter, String path) { - assertThat "Expected path '${path}', to be blocked", !filter.isAccessAllowed(mockRequest(path), null, null) + static void assertPathBlocked(InvalidRequestFilter filter, String requestUri, String servletPath = requestUri, String pathInfo = null) { + assertThat "Expected path '${requestUri}', to be blocked", !filter.isAccessAllowed(mockRequest(requestUri, servletPath, pathInfo), null, null) } - static void assertPathAllowed(InvalidRequestFilter filter, String path) { - assertThat "Expected path '${path}', to be allowed", filter.isAccessAllowed(mockRequest(path), null, null) + static void assertPathAllowed(InvalidRequestFilter filter, String requestUri, String servletPath = requestUri, String pathInfo = null) { + assertThat "Expected requestUri '${requestUri}', to be allowed", filter.isAccessAllowed(mockRequest(requestUri, servletPath, pathInfo), null, null) } - static HttpServletRequest mockRequest(String path) { + static HttpServletRequest mockRequest(String requestUri, String servletPath, String pathInfo) { HttpServletRequest request = mock(HttpServletRequest) - expect(request.getRequestURI()).andReturn(path) + expect(request.getRequestURI()).andReturn(requestUri) + expect(request.getServletPath()).andReturn(servletPath).anyTimes() + expect(request.getPathInfo()).andReturn(pathInfo).anyTimes() + expect(request.getAttribute("javax.servlet.include.servlet_path")).andReturn(servletPath) + expect(request.getAttribute("javax.servlet.include.path_info")).andReturn(pathInfo) replay(request) return request } diff --git a/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy b/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy index 7956a10..b501605 100644 --- a/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy +++ b/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy @@ -18,12 +18,15 @@ */ package org.apache.shiro.web.util +import org.apache.shiro.web.RestoreSystemProperties +import org.hamcrest.CoreMatchers import org.junit.Test import javax.servlet.http.HttpServletRequest import static org.easymock.EasyMock.* import static org.junit.Assert.* +import static org.hamcrest.CoreMatchers.* /** * Tests for {@link WebUtils}. @@ -193,6 +196,55 @@ public class WebUtilsTest { doTestGetRequestURI("/context path/foobar", "/context path/foobar"); } + @Test + void testNormalize() { + doNormalizeTest"/foobar", "/foobar" + doNormalizeTest "/foobar/", "/foobar/" + doNormalizeTest"", "/" + doNormalizeTest"foobar", "/foobar" + doNormalizeTest"//foobar", "/foobar" + doNormalizeTest"//foobar///", "/foobar/" + doNormalizeTest"/context-path/foobar", "/context-path/foobar" + doNormalizeTest"/context-path/foobar/", "/context-path/foobar/" + doNormalizeTest"//context-path/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//foobar" ,"/context-path/foobar" + doNormalizeTest"//context-path/remove-one/remove-two/../../././/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//../../././/foobar", null + doNormalizeTest"/context path/foobar", "/context path/foobar" + + doNormalizeTest"/context path/\\foobar", "/context path/\\foobar" + doNormalizeTest"//context-path\\..\\../.\\.\\foobar", "/context-path\\..\\../.\\.\\foobar" + doNormalizeTest"//context-path\\..\\..\\.\\.\\foobar", "/context-path\\..\\..\\.\\.\\foobar" + doNormalizeTest"\\context-path\\..\\foobar", "/\\context-path\\..\\foobar" + } + + @Test + void testNormalize_allowBackslashes() { + RestoreSystemProperties.withProperties(["org.apache.shiro.web.ALLOW_BACKSLASH": "true"]) { + doNormalizeTest"/foobar", "/foobar" + doNormalizeTest "/foobar/", "/foobar/" + doNormalizeTest"", "/" + doNormalizeTest"foobar", "/foobar" + doNormalizeTest"//foobar", "/foobar" + doNormalizeTest"//foobar///", "/foobar/" + doNormalizeTest"/context-path/foobar", "/context-path/foobar" + doNormalizeTest"/context-path/foobar/", "/context-path/foobar/" + doNormalizeTest"//context-path/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//foobar" ,"/context-path/foobar" + doNormalizeTest"//context-path/remove-one/remove-two/../../././/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//../../././/foobar", null + doNormalizeTest"/context path/foobar", "/context path/foobar" + doNormalizeTest"/context path/\\foobar", "/context path/foobar" + doNormalizeTest"//context-path\\..\\..\\.\\.\\foobar", null + doNormalizeTest"\\context-path\\..\\foobar", "/foobar" + + } + } + + void doNormalizeTest(String path, String expected) { + assertThat WebUtils.normalize(path), equalTo(expected) + } + void doTestGetPathWithinApplication(String servletPath, String pathInfo, String expectedValue) { def request = createMock(HttpServletRequest) diff --git a/web/src/test/java/org/apache/shiro/web/RestoreSystemProperties.java b/web/src/test/java/org/apache/shiro/web/RestoreSystemProperties.java new file mode 100644 index 0000000..882e2e9 --- /dev/null +++ b/web/src/test/java/org/apache/shiro/web/RestoreSystemProperties.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.shiro.web; + +import groovy.lang.Closure; + +import java.io.Closeable; +import java.util.Collections; +import java.util.Map; +import java.util.Properties; + +/** + * Wrapper that will restore System properties after test methods. + * + * Based on: https://github.com/stefanbirkner/system-rules/blob/master/src/main/java/org/junit/contrib/java/lang/system/RestoreSystemProperties.java + */ +public class RestoreSystemProperties implements Closeable { + + private final Properties originalProperties; + + public RestoreSystemProperties() { + originalProperties = System.getProperties(); + System.setProperties(copyOf(originalProperties)); + } + + public void restore() { + System.setProperties(originalProperties); + } + + private Properties copyOf(Properties source) { + Properties copy = new Properties(); + copy.putAll(source); + return copy; + } + + public static <T> T withProperties(Closure<T> closure) { + return withProperties(Collections.emptyMap(), closure); + } + + public static <T> T withProperties(Map<String, String> properties, Closure<T> closure) { + + try (RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties()) { + properties.forEach(System::setProperty); + + return closure.call(); + } + } + + @Override + public void close() { + restore(); + } +}
