From c3ede3f94efb442acb0795714a022c2c121d1da0 Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Wed, 31 May 2023 13:54:37 -0400 Subject: [PATCH] Adds improved path filter --- .../web/filter/InvalidRequestFilter.java | 51 ++++++++++++++++++- .../filter/InvalidRequestFilterTest.groovy | 51 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) 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 e352d5c143..97133500fe 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 @@ -37,6 +37,7 @@ *
  • Semicolon - can be disabled by setting {@code blockSemicolon = false}
    • *
  • Backslash - can be disabled by setting {@code blockBackslash = false}
    • *
  • Non-ASCII characters - can be disabled by setting {@code blockNonAscii = false}, the ability to disable this check will be removed in future version.
    • + *
  • Path traversals - can be disabled by setting {@code blockTraversal = false}
    • * * * @see This class was inspired by Spring Security StrictHttpFirewall @@ -48,12 +49,18 @@ public class InvalidRequestFilter extends AccessControlFilter { private static final List BACKSLASH = Collections.unmodifiableList(Arrays.asList("\\", "%5c", "%5C")); + private static final List FORWARDSLASH = Collections.unmodifiableList(Arrays.asList("%2f", "%2F")); + + private static final List PERIOD = Collections.unmodifiableList(Arrays.asList("%2e", "%2E")); + private boolean blockSemicolon = true; private boolean blockBackslash = !Boolean.getBoolean(WebUtils.ALLOW_BACKSLASH); private boolean blockNonAscii = true; + private boolean blockTraversal = true; + @Override protected boolean isAccessAllowed(ServletRequest req, ServletResponse response, Object mappedValue) throws Exception { HttpServletRequest request = WebUtils.toHttp(req); @@ -67,7 +74,8 @@ private boolean isValid(String uri) { return !StringUtils.hasText(uri) || ( !containsSemicolon(uri) && !containsBackslash(uri) - && !containsNonAsciiCharacters(uri)); + && !containsNonAsciiCharacters(uri)) + && !containsTraversal(uri); } @Override @@ -108,6 +116,39 @@ private static boolean containsOnlyPrintableAsciiCharacters(String uri) { return true; } + private boolean containsTraversal(String uri) { + if (isBlockTraversal()) { + return !(isNormalized(uri) + && PERIOD.stream().noneMatch(uri::contains) + && FORWARDSLASH.stream().noneMatch(uri::contains)); + } + 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) { + return true; + } + for (int i = path.length(); i > 0;) { + int slashIndex = path.lastIndexOf('/', i - 1); + int gap = i - slashIndex; + if (gap == 2 && path.charAt(slashIndex + 1) == '.') { + return false; // ".", "/./" or "/." + } + if (gap == 3 && path.charAt(slashIndex + 1) == '.' && path.charAt(slashIndex + 2) == '.') { + return false; + } + i = slashIndex; + } + return true; + } + public boolean isBlockSemicolon() { return blockSemicolon; } @@ -131,4 +172,12 @@ public boolean isBlockNonAscii() { public void setBlockNonAscii(boolean blockNonAscii) { this.blockNonAscii = blockNonAscii; } + + public boolean isBlockTraversal() { + return blockTraversal; + } + + public void setBlockTraversal(boolean blockTraversal) { + this.blockTraversal = blockTraversal; + } } 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 c7a052504f..9e37b3fef2 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 @@ -37,6 +37,7 @@ 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 true", filter.isBlockTraversal() } @Test @@ -73,6 +74,31 @@ class InvalidRequestFilterTest { assertPathBlocked(filter, "/something", "/something", "/;") } + @Test + void testBlocksTraversal() { + InvalidRequestFilter filter = new InvalidRequestFilter() + assertPathBlocked(filter, "/something/../") + assertPathBlocked(filter, "/something/../bar") + assertPathBlocked(filter, "/something/../bar/") + assertPathBlocked(filter, "/something/%2e%2E/bar/") + assertPathBlocked(filter, "/something/..") + assertPathBlocked(filter, "/..") + assertPathBlocked(filter, "..") + assertPathBlocked(filter, "../") + assertPathBlocked(filter, "%2E./") + assertPathBlocked(filter, "%2F./") + assertPathBlocked(filter, "/something/./") + assertPathBlocked(filter, "/something/./bar") + assertPathBlocked(filter, "/something/\u002e/bar") + assertPathBlocked(filter, "/something/./bar/") + assertPathBlocked(filter, "/something/%2e/bar/") + assertPathBlocked(filter, "/something/%2f/bar/") + assertPathBlocked(filter, "/something/.") + assertPathBlocked(filter, "/.") + assertPathBlocked(filter, "/something/../something/.") + assertPathBlocked(filter, "/something/../something/.") + } + @Test void testFilterAllowsBackslash() { InvalidRequestFilter filter = new InvalidRequestFilter() @@ -120,6 +146,31 @@ class InvalidRequestFilterTest { assertPathAllowed(filter, "/something", "/something", "/;") } + @Test + void testAllowTraversal() { + InvalidRequestFilter filter = new InvalidRequestFilter() + filter.setBlockTraversal(false) + + assertPathAllowed(filter, "/something/../") + assertPathAllowed(filter, "/something/../bar") + assertPathAllowed(filter, "/something/../bar/") + assertPathAllowed(filter, "/something/..") + assertPathAllowed(filter, "/..") + assertPathAllowed(filter, "..") + assertPathAllowed(filter, "../") + assertPathAllowed(filter, "%2E./") + assertPathAllowed(filter, "%2F./") + assertPathAllowed(filter, "/something/./") + assertPathAllowed(filter, "/something/./bar") + assertPathAllowed(filter, "/something/\u002e/bar") + assertPathAllowed(filter, "/something\u002fbar") + assertPathAllowed(filter, "/something/./bar/") + assertPathAllowed(filter, "/something/%2e/bar/") + assertPathAllowed(filter, "/something/%2f/bar/") + assertPathAllowed(filter, "/something/.") + assertPathAllowed(filter, "/.") + assertPathAllowed(filter, "/something/../something/.") + } 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)