You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by bd...@apache.org on 2020/11/04 15:48:12 UTC
[shiro] 02/03: Adds configuration to toggle the normalization of
backslashes
This is an automated email from the ASF dual-hosted git repository.
bdemers pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shiro.git
commit 042c59356cc6442345a9f935aed3e7603cb4dfad
Author: Brian Demers <bd...@apache.org>
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..63da6d4 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.lang.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 abc27c4..992f7ad 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();
+ }
+}