- restored backwards compatibility

- using URLDecode() before patch matching to be even more secure by default
- allow unencoded semicolons by default

Author: lprimak

web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java

@@ -46,14 +46,13 @@
  * @since 1.6
  */
 public class InvalidRequestFilter extends AccessControlFilter {
-
-    enum PathTraversalBlockMode {
+    public enum PathTraversalBlockMode {
         STRICT,
         NORMAL,
         NO_BLOCK;
     }
 
-    private static final List<String> SEMICOLON = Collections.unmodifiableList(Arrays.asList(";", "%3b", "%3B"));
+    private static final List<String> SEMICOLON = Collections.unmodifiableList(Arrays.asList("%3b", "%3B"));
 
     private static final List<String> BACKSLASH = Collections.unmodifiableList(Arrays.asList("\\", "%5c", "%5C"));
 
@@ -98,7 +97,8 @@ protected boolean onAccessDenied(ServletRequest request, ServletResponse respons
 
     private boolean containsSemicolon(String uri) {
         if (isBlockSemicolon()) {
-            return SEMICOLON.stream().anyMatch(uri::contains);
+            return SEMICOLON.stream().anyMatch(uri::contains)
+                    || (pathTraversalBlockMode == PathTraversalBlockMode.STRICT && uri.contains(";"));
         }
         return false;
     }
@@ -129,10 +129,10 @@ private static boolean containsOnlyPrintableAsciiCharacters(String uri) {
     }
 
     private boolean containsTraversal(String uri) {
-        if (isBlockTraversalNormal()) {
+        if (pathTraversalBlockMode == PathTraversalBlockMode.NORMAL) {
             return !(isNormalized(uri));
         }
-        if (isBlockTraversalStrict()) {
+        if (pathTraversalBlockMode == PathTraversalBlockMode.STRICT) {
             return !(isNormalized(uri)
                     && PERIOD.stream().noneMatch(uri::contains)
                     && FORWARDSLASH.stream().noneMatch(uri::contains));
@@ -190,21 +190,49 @@ public void setBlockNonAscii(boolean blockNonAscii) {
         this.blockNonAscii = blockNonAscii;
     }
 
-    public boolean isBlockTraversalNormal() {
-        return pathTraversalBlockMode == PathTraversalBlockMode.NORMAL;
+    public PathTraversalBlockMode getPathTraversalBlockMode() {
+        return pathTraversalBlockMode;
+    }
+
+    public void setBlockPathTraversal(PathTraversalBlockMode mode) {
+        this.pathTraversalBlockMode = mode;
     }
 
-    public boolean isBlockTraversalStrict() {
+    public boolean isBlockEncodedPeriod() {
         return pathTraversalBlockMode == PathTraversalBlockMode.STRICT;
     }
 
-    public void setPathTraversalBlockMode(PathTraversalBlockMode mode) {
-        this.pathTraversalBlockMode = mode;
+    public void setBlockEncodedPeriod(boolean blockEncodedPeriod) {
+        setBlockPathTraversal(blockEncodedPeriod ? PathTraversalBlockMode.STRICT : PathTraversalBlockMode.NORMAL);
+    }
+
+    public boolean isBlockEncodedForwardSlash() {
+        return pathTraversalBlockMode == PathTraversalBlockMode.STRICT;
+    }
+
+    public void setBlockEncodedForwardSlash(boolean blockEncodedForwardSlash) {
+        setBlockPathTraversal(blockEncodedForwardSlash ? PathTraversalBlockMode.STRICT : PathTraversalBlockMode.NORMAL);
+    }
+
+    public boolean isBlockRewriteTraversal() {
+        return pathTraversalBlockMode == PathTraversalBlockMode.NORMAL;
+    }
+
+    public void setBlockRewriteTraversal(boolean blockRewriteTraversal) {
+        setBlockPathTraversal(blockRewriteTraversal ? PathTraversalBlockMode.NORMAL : PathTraversalBlockMode.NO_BLOCK);
+    }
+
+    /**
+     * @deprecated use {@link #getPathTraversalBlockMode()} instead
+     */
+    @Deprecated
+    public boolean isBlockTraversal() {
+        return pathTraversalBlockMode != PathTraversalBlockMode.NO_BLOCK;
     }
 
     /**
      *
-     * @deprecated Use {@link #setPathTraversalBlockMode(PathTraversalBlockMode)}
+     * @deprecated Use {@link #setBlockPathTraversal(PathTraversalBlockMode)}
      */
     @Deprecated
     public void setBlockTraversal(boolean blockTraversal) {

web/src/main/java/org/apache/shiro/web/util/WebUtils.java

@@ -116,7 +116,7 @@ private WebUtils() {
      * @return the path within the web application
      */
     public static String getPathWithinApplication(HttpServletRequest request) {
-        return normalize(removeSemicolon(getServletPath(request) + getPathInfo(request)));
+        return normalize(decodeAndCleanUriString(request, getServletPath(request) + getPathInfo(request)));
     }
 
     /**

web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy

@@ -39,7 +39,8 @@ class InvalidRequestFilterTest {
         assertThat "filter.blockBackslash expected to be true", filter.isBlockBackslash()
         assertThat "filter.blockNonAscii expected to be true", filter.isBlockNonAscii()
         assertThat "filter.blockSemicolon expected to be true", filter.isBlockSemicolon()
-        assertThat "filter.blockTraversal expected to be NORMAL", filter.isBlockTraversalNormal()
+        assertThat "filter.blockTraversal expected to be NORMAL",
+                filter.getPathTraversalBlockMode() == InvalidRequestFilter.PathTraversalBlockMode.NORMAL
     }
 
     @Test
@@ -67,13 +68,9 @@ class InvalidRequestFilterTest {
         assertPathBlocked(filter, "/\\something")
         assertPathBlocked(filter, "/%5csomething")
         assertPathBlocked(filter, "/%5Csomething")
-        assertPathBlocked(filter, "/;something")
         assertPathBlocked(filter, "/%3bsomething")
         assertPathBlocked(filter, "/%3Bsomething")
         assertPathBlocked(filter, "/\u0019something")
-
-        assertPathBlocked(filter, "/something", "/;something")
-        assertPathBlocked(filter, "/something", "/something", "/;")
     }
 
     @Test
@@ -101,12 +98,14 @@ class InvalidRequestFilterTest {
         assertPathAllowed(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
         assertPathAllowed(filter, "/something/%2e%2E/bar/")
         assertPathAllowed(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
+        assertPathAllowed(filter, "/;something")
+        assertPathAllowed(filter, "/something", "/;something")
     }
 
     @Test
     void testBlocksTraversalStrict() {
         InvalidRequestFilter filter = new InvalidRequestFilter()
-        filter.setPathTraversalBlockMode(InvalidRequestFilter.PathTraversalBlockMode.STRICT)
+        filter.setBlockPathTraversal(InvalidRequestFilter.PathTraversalBlockMode.STRICT)
         assertPathBlocked(filter, "/something/../")
         assertPathBlocked(filter, "/something/../bar")
         assertPathBlocked(filter, "/something/../bar/")
@@ -129,6 +128,9 @@ class InvalidRequestFilterTest {
         assertPathBlocked(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
         assertPathBlocked(filter, "/something/%2e%2E/bar/")
         assertPathBlocked(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
+        assertPathBlocked(filter, "/;something")
+        assertPathBlocked(filter, "/something", "/;something")
+        assertPathBlocked(filter, "/something", "/something", "/;")
     }
 
     @Test
@@ -138,7 +140,6 @@ class InvalidRequestFilterTest {
         assertPathAllowed(filter, "/\\something")
         assertPathAllowed(filter, "/%5csomething")
         assertPathAllowed(filter, "/%5Csomething")
-        assertPathBlocked(filter, "/;something")
         assertPathBlocked(filter, "/%3bsomething")
         assertPathBlocked(filter, "/%3Bsomething")
         assertPathBlocked(filter, "/\u0019something")
@@ -154,7 +155,6 @@ class InvalidRequestFilterTest {
         assertPathBlocked(filter, "/\\something")
         assertPathBlocked(filter, "/%5csomething")
         assertPathBlocked(filter, "/%5Csomething")
-        assertPathBlocked(filter, "/;something")
         assertPathBlocked(filter, "/%3bsomething")
         assertPathBlocked(filter, "/%3Bsomething")
         assertPathAllowed(filter, "/\u0019something")
@@ -182,7 +182,7 @@ class InvalidRequestFilterTest {
     @Test
     void testAllowTraversal() {
         InvalidRequestFilter filter = new InvalidRequestFilter()
-        filter.setPathTraversalBlockMode(InvalidRequestFilter.PathTraversalBlockMode.NO_BLOCK);
+        filter.setBlockPathTraversal(InvalidRequestFilter.PathTraversalBlockMode.NO_BLOCK);
 
         assertPathAllowed(filter, "/something/../")
         assertPathAllowed(filter, "/something/../bar")
@@ -207,6 +207,9 @@ class InvalidRequestFilterTest {
         assertPathAllowed(filter, "/something/http:%2f%2fmydomain.example.com%2foidc/bar/")
         assertPathAllowed(filter, "/something/%2e%2E/bar/")
         assertPathAllowed(filter, "/something/http:%2f%2fmydomain%2eexample%2ecom%2foidc/bar/")
+        assertPathAllowed(filter, "/;something")
+        assertPathAllowed(filter, "/something", "/;something")
+        assertPathAllowed(filter, "/something", "/something", "/;")
     }
 
     static void assertPathBlocked(InvalidRequestFilter filter, String requestUri, String servletPath = requestUri, String pathInfo = null) {

web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy

@@ -23,6 +23,7 @@ import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.parallel.Isolated
 
 import javax.servlet.http.HttpServletRequest
+import java.nio.charset.StandardCharsets
 
 import static org.easymock.EasyMock.*
 import static org.hamcrest.MatcherAssert.*
@@ -252,6 +253,7 @@ class WebUtilsTest {
         def request = createMock(HttpServletRequest)
         expect(request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE)).andReturn(servletPath)
         expect(request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE)).andReturn(pathInfo)
+        expect(request.getCharacterEncoding()).andReturn(StandardCharsets.UTF_8.name())
         if (pathInfo == null) {
             expect(request.getPathInfo()).andReturn(null) // path info can be null
         }