From d94639a1bb93a190870621d4e4cb3c92d8578484 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Tue, 17 May 2022 15:53:18 -0500 Subject: [PATCH] StrictHttpFirewall allows CJKV characters Closes gh-11264 --- .../web/firewall/StrictHttpFirewall.java | 703 +++++++++++++----- .../web/firewall/StrictHttpFirewallTests.java | 588 +++++++++++---- 2 files changed, 976 insertions(+), 315 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java b/web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java index aef2a7b533..18a1e01741 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java +++ b/web/src/main/java/org/springframework/security/web/firewall/StrictHttpFirewall.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,14 +19,19 @@ package org.springframework.security.web.firewall; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Enumeration; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.function.Predicate; +import java.util.regex.Pattern; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.springframework.http.HttpMethod; +import org.springframework.util.Assert; /** *

@@ -37,93 +42,127 @@ import org.springframework.http.HttpMethod; * The following rules are applied to the firewall: *

* * - * @see DefaultHttpFirewall * @author Rob Winch * @author Eddú Meléndez * @since 4.2.4 + * @see DefaultHttpFirewall */ public class StrictHttpFirewall implements HttpFirewall { + /** - * Used to specify to {@link #setAllowedHttpMethods(Collection)} that any HTTP method should be allowed. + * Used to specify to {@link #setAllowedHttpMethods(Collection)} that any HTTP method + * should be allowed. */ - private static final Set ALLOW_ANY_HTTP_METHOD = Collections.unmodifiableSet(Collections.emptySet()); + private static final Set ALLOW_ANY_HTTP_METHOD = Collections.emptySet(); private static final String ENCODED_PERCENT = "%25"; private static final String PERCENT = "%"; - private static final List FORBIDDEN_ENCODED_PERIOD = Collections.unmodifiableList(Arrays.asList("%2e", "%2E")); + private static final List FORBIDDEN_ENCODED_PERIOD = Collections + .unmodifiableList(Arrays.asList("%2e", "%2E")); - private static final List FORBIDDEN_SEMICOLON = Collections.unmodifiableList(Arrays.asList(";", "%3b", "%3B")); + private static final List FORBIDDEN_SEMICOLON = Collections + .unmodifiableList(Arrays.asList(";", "%3b", "%3B")); - private static final List FORBIDDEN_FORWARDSLASH = Collections.unmodifiableList(Arrays.asList("%2f", "%2F")); + private static final List FORBIDDEN_FORWARDSLASH = Collections + .unmodifiableList(Arrays.asList("%2f", "%2F")); - private static final List FORBIDDEN_DOUBLE_FORWARDSLASH = Collections.unmodifiableList(Arrays.asList("//", "%2f%2f", "%2f%2F", "%2F%2f", "%2F%2F")); + private static final List FORBIDDEN_DOUBLE_FORWARDSLASH = Collections + .unmodifiableList(Arrays.asList("//", "%2f%2f", "%2f%2F", "%2F%2f", "%2F%2F")); - private static final List FORBIDDEN_BACKSLASH = Collections.unmodifiableList(Arrays.asList("\\", "%5c", "%5C")); + private static final List FORBIDDEN_BACKSLASH = Collections + .unmodifiableList(Arrays.asList("\\", "%5c", "%5C")); - private Set encodedUrlBlacklist = new HashSet<>(); + private static final List FORBIDDEN_NULL = Collections.unmodifiableList(Arrays.asList("\0", "%00")); - private Set decodedUrlBlacklist = new HashSet<>(); + private static final List FORBIDDEN_LF = Collections.unmodifiableList(Arrays.asList("\n", "%0a", "%0A")); + + private static final List FORBIDDEN_CR = Collections.unmodifiableList(Arrays.asList("\r", "%0d", "%0D")); + + private static final List FORBIDDEN_LINE_SEPARATOR = Collections.unmodifiableList(Arrays.asList("\u2028")); + + private static final List FORBIDDEN_PARAGRAPH_SEPARATOR = Collections + .unmodifiableList(Arrays.asList("\u2029")); + + private Set encodedUrlBlocklist = new HashSet<>(); + + private Set decodedUrlBlocklist = new HashSet<>(); private Set allowedHttpMethods = createDefaultAllowedHttpMethods(); - private Predicate allowedHostnames = hostname -> true; + private Predicate allowedHostnames = (hostname) -> true; + + private static final Pattern ASSIGNED_AND_NOT_ISO_CONTROL_PATTERN = Pattern + .compile("[\\p{IsAssigned}&&[^\\p{IsControl}]]*"); + + private static final Predicate ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE = ( + s) -> ASSIGNED_AND_NOT_ISO_CONTROL_PATTERN.matcher(s).matches(); + + private Predicate allowedHeaderNames = ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE; + + private Predicate allowedHeaderValues = ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE; + + private Predicate allowedParameterNames = ASSIGNED_AND_NOT_ISO_CONTROL_PREDICATE; + + private Predicate allowedParameterValues = (value) -> true; public StrictHttpFirewall() { - urlBlacklistsAddAll(FORBIDDEN_SEMICOLON); - urlBlacklistsAddAll(FORBIDDEN_FORWARDSLASH); - urlBlacklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH); - urlBlacklistsAddAll(FORBIDDEN_BACKSLASH); + urlBlocklistsAddAll(FORBIDDEN_SEMICOLON); + urlBlocklistsAddAll(FORBIDDEN_FORWARDSLASH); + urlBlocklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH); + urlBlocklistsAddAll(FORBIDDEN_BACKSLASH); + urlBlocklistsAddAll(FORBIDDEN_NULL); + urlBlocklistsAddAll(FORBIDDEN_LF); + urlBlocklistsAddAll(FORBIDDEN_CR); - this.encodedUrlBlacklist.add(ENCODED_PERCENT); - this.encodedUrlBlacklist.addAll(FORBIDDEN_ENCODED_PERIOD); - this.decodedUrlBlacklist.add(PERCENT); + this.encodedUrlBlocklist.add(ENCODED_PERCENT); + this.encodedUrlBlocklist.addAll(FORBIDDEN_ENCODED_PERIOD); + this.decodedUrlBlocklist.add(PERCENT); + this.decodedUrlBlocklist.addAll(FORBIDDEN_LINE_SEPARATOR); + this.decodedUrlBlocklist.addAll(FORBIDDEN_PARAGRAPH_SEPARATOR); } /** - * Sets if any HTTP method is allowed. If this set to true, then no validation on the HTTP method will be performed. - * This can open the application up to - * HTTP Verb tampering and XST attacks - * @param unsafeAllowAnyHttpMethod if true, disables HTTP method validation, else resets back to the defaults. Default is false. - * @see #setAllowedHttpMethods(Collection) + * Sets if any HTTP method is allowed. If this set to true, then no validation on the + * HTTP method will be performed. This can open the application up to + * HTTP + * Verb tampering and XST attacks + * @param unsafeAllowAnyHttpMethod if true, disables HTTP method validation, else + * resets back to the defaults. Default is false. * @since 5.1 + * @see #setAllowedHttpMethods(Collection) */ public void setUnsafeAllowAnyHttpMethod(boolean unsafeAllowAnyHttpMethod) { this.allowedHttpMethods = unsafeAllowAnyHttpMethod ? ALLOW_ANY_HTTP_METHOD : createDefaultAllowedHttpMethods(); @@ -131,40 +170,36 @@ public class StrictHttpFirewall implements HttpFirewall { /** *

- * Determines which HTTP methods should be allowed. The default is to allow "DELETE", "GET", "HEAD", "OPTIONS", - * "PATCH", "POST", and "PUT". + * Determines which HTTP methods should be allowed. The default is to allow "DELETE", + * "GET", "HEAD", "OPTIONS", "PATCH", "POST", and "PUT". *

- * - * @param allowedHttpMethods the case-sensitive collection of HTTP methods that are allowed. - * @see #setUnsafeAllowAnyHttpMethod(boolean) + * @param allowedHttpMethods the case-sensitive collection of HTTP methods that are + * allowed. * @since 5.1 + * @see #setUnsafeAllowAnyHttpMethod(boolean) */ public void setAllowedHttpMethods(Collection allowedHttpMethods) { - if (allowedHttpMethods == null) { - throw new IllegalArgumentException("allowedHttpMethods cannot be null"); - } - if (allowedHttpMethods == ALLOW_ANY_HTTP_METHOD) { - this.allowedHttpMethods = ALLOW_ANY_HTTP_METHOD; - } else { - this.allowedHttpMethods = new HashSet<>(allowedHttpMethods); - } + Assert.notNull(allowedHttpMethods, "allowedHttpMethods cannot be null"); + this.allowedHttpMethods = (allowedHttpMethods != ALLOW_ANY_HTTP_METHOD) ? new HashSet<>(allowedHttpMethods) + : ALLOW_ANY_HTTP_METHOD; } /** *

* Determines if semicolon is allowed in the URL (i.e. matrix variables). The default * is to disable this behavior because it is a common way of attempting to perform - * Reflected File Download Attacks. - * It is also the source of many exploits which bypass URL based security. + * Reflected File + * Download Attacks. It is also the source of many exploits which bypass URL based + * security. *

- *

For example, the following CVEs are a subset of the issues related - * to ambiguities in the Servlet Specification on how to treat semicolons that - * led to CVEs: + *

+ * For example, the following CVEs are a subset of the issues related to ambiguities + * in the Servlet Specification on how to treat semicolons that led to CVEs: *

* * *

@@ -177,17 +212,16 @@ public class StrictHttpFirewall implements HttpFirewall { * any sensitive information) in a URL as it can lead to leaking. Instead use Cookies. * *

  • Matrix Variables - Users wanting to leverage Matrix Variables should consider - * using HTTP parameters instead. - *
    • + * using HTTP parameters instead. * - * * @param allowSemicolon should semicolons be allowed in the URL. Default is false */ public void setAllowSemicolon(boolean allowSemicolon) { if (allowSemicolon) { - urlBlacklistsRemoveAll(FORBIDDEN_SEMICOLON); - } else { - urlBlacklistsAddAll(FORBIDDEN_SEMICOLON); + urlBlocklistsRemoveAll(FORBIDDEN_SEMICOLON); + } + else { + urlBlocklistsAddAll(FORBIDDEN_SEMICOLON); } } @@ -202,32 +236,32 @@ public class StrictHttpFirewall implements HttpFirewall { * parsed consistently which results in different values in {@code HttpServletRequest} * path related values which allow bypassing certain security constraints. *

    - * * @param allowUrlEncodedSlash should a slash "/" that is URL encoded "%2F" be allowed * in the path or not. Default is false. */ public void setAllowUrlEncodedSlash(boolean allowUrlEncodedSlash) { if (allowUrlEncodedSlash) { - urlBlacklistsRemoveAll(FORBIDDEN_FORWARDSLASH); - } else { - urlBlacklistsAddAll(FORBIDDEN_FORWARDSLASH); + urlBlocklistsRemoveAll(FORBIDDEN_FORWARDSLASH); + } + else { + urlBlocklistsAddAll(FORBIDDEN_FORWARDSLASH); } } /** *

    - * Determines if double slash "//" that is URL encoded "%2F%2F" should be allowed in the path or - * not. The default is to not allow. + * Determines if double slash "//" that is URL encoded "%2F%2F" should be allowed in + * the path or not. The default is to not allow. *

    - * - * @param allowUrlEncodedDoubleSlash should a slash "//" that is URL encoded "%2F%2F" be allowed - * in the path or not. Default is false. + * @param allowUrlEncodedDoubleSlash should a slash "//" that is URL encoded "%2F%2F" + * be allowed in the path or not. Default is false. */ public void setAllowUrlEncodedDoubleSlash(boolean allowUrlEncodedDoubleSlash) { if (allowUrlEncodedDoubleSlash) { - urlBlacklistsRemoveAll(FORBIDDEN_DOUBLE_FORWARDSLASH); - } else { - urlBlacklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH); + urlBlocklistsRemoveAll(FORBIDDEN_DOUBLE_FORWARDSLASH); + } + else { + urlBlocklistsAddAll(FORBIDDEN_DOUBLE_FORWARDSLASH); } } @@ -240,19 +274,19 @@ public class StrictHttpFirewall implements HttpFirewall { *

    * For example, due to ambiguities in the servlet specification a URL encoded period * might lead to bypassing security constraints through a directory traversal attack. - * This is because the path is not parsed consistently which results in different + * This is because the path is not parsed consistently which results in different * values in {@code HttpServletRequest} path related values which allow bypassing * certain security constraints. *

    - * * @param allowUrlEncodedPeriod should a period "." that is URL encoded "%2E" be * allowed in the path or not. Default is false. */ public void setAllowUrlEncodedPeriod(boolean allowUrlEncodedPeriod) { if (allowUrlEncodedPeriod) { - this.encodedUrlBlacklist.removeAll(FORBIDDEN_ENCODED_PERIOD); - } else { - this.encodedUrlBlacklist.addAll(FORBIDDEN_ENCODED_PERIOD); + this.encodedUrlBlocklist.removeAll(FORBIDDEN_ENCODED_PERIOD); + } + else { + this.encodedUrlBlocklist.addAll(FORBIDDEN_ENCODED_PERIOD); } } @@ -265,19 +299,38 @@ public class StrictHttpFirewall implements HttpFirewall { *

    * For example, due to ambiguities in the servlet specification a URL encoded period * might lead to bypassing security constraints through a directory traversal attack. - * This is because the path is not parsed consistently which results in different + * This is because the path is not parsed consistently which results in different * values in {@code HttpServletRequest} path related values which allow bypassing * certain security constraints. *

    - * * @param allowBackSlash a backslash "\" or a URL encoded backslash "%5C" be allowed * in the path or not. Default is false */ public void setAllowBackSlash(boolean allowBackSlash) { if (allowBackSlash) { - urlBlacklistsRemoveAll(FORBIDDEN_BACKSLASH); - } else { - urlBlacklistsAddAll(FORBIDDEN_BACKSLASH); + urlBlocklistsRemoveAll(FORBIDDEN_BACKSLASH); + } + else { + urlBlocklistsAddAll(FORBIDDEN_BACKSLASH); + } + } + + /** + *

    + * Determines if a null "\0" or a URL encoded nul "%00" should be allowed in the path + * or not. The default is not to allow this behavior because it is a frequent source + * of security exploits. + *

    + * @param allowNull a null "\0" or a URL encoded null "%00" be allowed in the path or + * not. Default is false + * @since 5.3.14 + */ + public void setAllowNull(boolean allowNull) { + if (allowNull) { + urlBlocklistsRemoveAll(FORBIDDEN_NULL); + } + else { + urlBlocklistsAddAll(FORBIDDEN_NULL); } } @@ -291,75 +344,177 @@ public class StrictHttpFirewall implements HttpFirewall { * For example, this can lead to exploits that involve double URL encoding that lead * to bypassing security constraints. *

    - * * @param allowUrlEncodedPercent if a percent "%" that is URL encoded "%25" should be * allowed in the path or not. Default is false */ public void setAllowUrlEncodedPercent(boolean allowUrlEncodedPercent) { if (allowUrlEncodedPercent) { - this.encodedUrlBlacklist.remove(ENCODED_PERCENT); - this.decodedUrlBlacklist.remove(PERCENT); - } else { - this.encodedUrlBlacklist.add(ENCODED_PERCENT); - this.decodedUrlBlacklist.add(PERCENT); + this.encodedUrlBlocklist.remove(ENCODED_PERCENT); + this.decodedUrlBlocklist.remove(PERCENT); } + else { + this.encodedUrlBlocklist.add(ENCODED_PERCENT); + this.decodedUrlBlocklist.add(PERCENT); + } + } + + /** + * Determines if a URL encoded Carriage Return is allowed in the path or not. The + * default is not to allow this behavior because it is a frequent source of security + * exploits. + * @param allowUrlEncodedCarriageReturn if URL encoded Carriage Return is allowed in + * the URL or not. Default is false. + */ + public void setAllowUrlEncodedCarriageReturn(boolean allowUrlEncodedCarriageReturn) { + if (allowUrlEncodedCarriageReturn) { + urlBlocklistsRemoveAll(FORBIDDEN_CR); + } + else { + urlBlocklistsAddAll(FORBIDDEN_CR); + } + } + + /** + * Determines if a URL encoded Line Feed is allowed in the path or not. The default is + * not to allow this behavior because it is a frequent source of security exploits. + * @param allowUrlEncodedLineFeed if URL encoded Line Feed is allowed in the URL or + * not. Default is false. + */ + public void setAllowUrlEncodedLineFeed(boolean allowUrlEncodedLineFeed) { + if (allowUrlEncodedLineFeed) { + urlBlocklistsRemoveAll(FORBIDDEN_LF); + } + else { + urlBlocklistsAddAll(FORBIDDEN_LF); + } + } + + /** + * Determines if a URL encoded paragraph separator is allowed in the path or not. The + * default is not to allow this behavior because it is a frequent source of security + * exploits. + * @param allowUrlEncodedParagraphSeparator if URL encoded paragraph separator is + * allowed in the URL or not. Default is false. + */ + public void setAllowUrlEncodedParagraphSeparator(boolean allowUrlEncodedParagraphSeparator) { + if (allowUrlEncodedParagraphSeparator) { + this.decodedUrlBlocklist.removeAll(FORBIDDEN_PARAGRAPH_SEPARATOR); + } + else { + this.decodedUrlBlocklist.addAll(FORBIDDEN_PARAGRAPH_SEPARATOR); + } + } + + /** + * Determines if a URL encoded line separator is allowed in the path or not. The + * default is not to allow this behavior because it is a frequent source of security + * exploits. + * @param allowUrlEncodedLineSeparator if URL encoded line separator is allowed in the + * URL or not. Default is false. + */ + public void setAllowUrlEncodedLineSeparator(boolean allowUrlEncodedLineSeparator) { + if (allowUrlEncodedLineSeparator) { + this.decodedUrlBlocklist.removeAll(FORBIDDEN_LINE_SEPARATOR); + } + else { + this.decodedUrlBlocklist.addAll(FORBIDDEN_LINE_SEPARATOR); + } + } + + /** + *

    + * Determines which header names should be allowed. The default is to reject header + * names that contain ISO control characters and characters that are not defined. + *

    + * @param allowedHeaderNames the predicate for testing header names + * @since 5.3.14 + * @see Character#isISOControl(int) + * @see Character#isDefined(int) + */ + public void setAllowedHeaderNames(Predicate allowedHeaderNames) { + Assert.notNull(allowedHeaderNames, "allowedHeaderNames cannot be null"); + this.allowedHeaderNames = allowedHeaderNames; + } + + /** + *

    + * Determines which header values should be allowed. The default is to reject header + * values that contain ISO control characters and characters that are not defined. + *

    + * @param allowedHeaderValues the predicate for testing hostnames + * @since 5.3.14 + * @see Character#isISOControl(int) + * @see Character#isDefined(int) + */ + public void setAllowedHeaderValues(Predicate allowedHeaderValues) { + Assert.notNull(allowedHeaderValues, "allowedHeaderValues cannot be null"); + this.allowedHeaderValues = allowedHeaderValues; + } + + /** + * Determines which parameter names should be allowed. The default is to reject header + * names that contain ISO control characters and characters that are not defined. + * @param allowedParameterNames the predicate for testing parameter names + * @since 5.3.14 + * @see Character#isISOControl(int) + * @see Character#isDefined(int) + */ + public void setAllowedParameterNames(Predicate allowedParameterNames) { + Assert.notNull(allowedParameterNames, "allowedParameterNames cannot be null"); + this.allowedParameterNames = allowedParameterNames; + } + + /** + *

    + * Determines which parameter values should be allowed. The default is to allow any + * parameter value. + *

    + * @param allowedParameterValues the predicate for testing parameter values + * @since 5.3.14 + */ + public void setAllowedParameterValues(Predicate allowedParameterValues) { + Assert.notNull(allowedParameterValues, "allowedParameterValues cannot be null"); + this.allowedParameterValues = allowedParameterValues; } /** *

    * Determines which hostnames should be allowed. The default is to allow any hostname. *

    - * * @param allowedHostnames the predicate for testing hostnames * @since 5.2 */ public void setAllowedHostnames(Predicate allowedHostnames) { - if (allowedHostnames == null) { - throw new IllegalArgumentException("allowedHostnames cannot be null"); - } + Assert.notNull(allowedHostnames, "allowedHostnames cannot be null"); this.allowedHostnames = allowedHostnames; } - private void urlBlacklistsAddAll(Collection values) { - this.encodedUrlBlacklist.addAll(values); - this.decodedUrlBlacklist.addAll(values); + private void urlBlocklistsAddAll(Collection values) { + this.encodedUrlBlocklist.addAll(values); + this.decodedUrlBlocklist.addAll(values); } - private void urlBlacklistsRemoveAll(Collection values) { - this.encodedUrlBlacklist.removeAll(values); - this.decodedUrlBlacklist.removeAll(values); + private void urlBlocklistsRemoveAll(Collection values) { + this.encodedUrlBlocklist.removeAll(values); + this.decodedUrlBlocklist.removeAll(values); } @Override public FirewalledRequest getFirewalledRequest(HttpServletRequest request) throws RequestRejectedException { rejectForbiddenHttpMethod(request); - rejectedBlacklistedUrls(request); + rejectedBlocklistedUrls(request); rejectedUntrustedHosts(request); - if (!isNormalized(request)) { throw new RequestRejectedException("The request was rejected because the URL was not normalized."); } - - String requestUri = request.getRequestURI(); - if (!containsOnlyPrintableAsciiCharacters(requestUri)) { - throw new RequestRejectedException("The requestURI was rejected because it can only contain printable ASCII characters."); - } rejectNonPrintableAsciiCharactersInFieldName(request.getRequestURI(), "requestURI"); - rejectNonPrintableAsciiCharactersInFieldName(request.getServletPath(), "servletPath"); - rejectNonPrintableAsciiCharactersInFieldName(request.getPathInfo(), "pathInfo"); - rejectNonPrintableAsciiCharactersInFieldName(request.getContextPath(), "contextPath"); - - return new FirewalledRequest(request) { - @Override - public void reset() { - } - }; + return new StrictFirewalledRequest(request); } private void rejectNonPrintableAsciiCharactersInFieldName(String toCheck, String propertyName) { if (!containsOnlyPrintableAsciiCharacters(toCheck)) { - throw new RequestRejectedException( - String.format("The %s was rejected because it can only contain printable ASCII characters.", propertyName)); + throw new RequestRejectedException(String.format( + "The %s was rejected because it can only contain printable ASCII characters.", propertyName)); } } @@ -368,22 +523,25 @@ public class StrictHttpFirewall implements HttpFirewall { return; } if (!this.allowedHttpMethods.contains(request.getMethod())) { - throw new RequestRejectedException("The request was rejected because the HTTP method \"" + - request.getMethod() + - "\" was not included within the whitelist " + - this.allowedHttpMethods); + throw new RequestRejectedException( + "The request was rejected because the HTTP method \"" + request.getMethod() + + "\" was not included within the list of allowed HTTP methods " + this.allowedHttpMethods); } } - private void rejectedBlacklistedUrls(HttpServletRequest request) { - for (String forbidden : this.encodedUrlBlacklist) { + private void rejectedBlocklistedUrls(HttpServletRequest request) { + for (String forbidden : this.encodedUrlBlocklist) { if (encodedUrlContains(request, forbidden)) { - throw new RequestRejectedException("The request was rejected because the URL contained a potentially malicious String \"" + forbidden + "\""); + throw new RequestRejectedException( + "The request was rejected because the URL contained a potentially malicious String \"" + + forbidden + "\""); } } - for (String forbidden : this.decodedUrlBlacklist) { + for (String forbidden : this.decodedUrlBlocklist) { if (decodedUrlContains(request, forbidden)) { - throw new RequestRejectedException("The request was rejected because the URL contained a potentially malicious String \"" + forbidden + "\""); + throw new RequestRejectedException( + "The request was rejected because the URL contained a potentially malicious String \"" + + forbidden + "\""); } } } @@ -391,7 +549,8 @@ public class StrictHttpFirewall implements HttpFirewall { private void rejectedUntrustedHosts(HttpServletRequest request) { String serverName = request.getServerName(); if (serverName != null && !this.allowedHostnames.test(serverName)) { - throw new RequestRejectedException("The request was rejected because the domain " + serverName + " is untrusted."); + throw new RequestRejectedException( + "The request was rejected because the domain " + serverName + " is untrusted."); } } @@ -451,12 +610,11 @@ public class StrictHttpFirewall implements HttpFirewall { } int length = uri.length(); for (int i = 0; i < length; i++) { - char c = uri.charAt(i); - if (c < '\u0020' || c > '\u007e') { + char ch = uri.charAt(i); + if (ch < '\u0020' || ch > '\u007e') { return false; } } - return true; } @@ -465,51 +623,236 @@ public class StrictHttpFirewall implements HttpFirewall { } /** - * 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. + * 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 static boolean isNormalized(String path) { if (path == null) { return true; } - - for (int j = path.length(); j > 0;) { - int i = path.lastIndexOf('/', j - 1); - int gap = j - i; - - if (gap == 2 && path.charAt(i + 1) == '.') { - // ".", "/./" or "/." - return false; - } else if (gap == 3 && path.charAt(i + 1) == '.' && path.charAt(i + 2) == '.') { + 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; } - - j = i; + i = slashIndex; } - return true; } /** - * Provides the existing encoded url blacklist which can add/remove entries from - * - * @return the existing encoded url blacklist, never null + * Provides the existing encoded url blocklist which can add/remove entries from + * @return the existing encoded url blocklist, never null */ - public Set getEncodedUrlBlacklist() { - return encodedUrlBlacklist; + public Set getEncodedUrlBlocklist() { + return this.encodedUrlBlocklist; } /** - * Provides the existing decoded url blacklist which can add/remove entries from + * Provides the existing decoded url blocklist which can add/remove entries from + * @return the existing decoded url blocklist, never null + */ + public Set getDecodedUrlBlocklist() { + return this.decodedUrlBlocklist; + } + + /** + * Provides the existing encoded url blocklist which can add/remove entries from + * @return the existing encoded url blocklist, never null + * @deprecated Use {@link #getEncodedUrlBlocklist()} instead + */ + @Deprecated + public Set getEncodedUrlBlacklist() { + return getEncodedUrlBlocklist(); + } + + /** + * Provides the existing decoded url blocklist which can add/remove entries from + * @return the existing decoded url blocklist, never null * - * @return the existing decoded url blacklist, never null */ public Set getDecodedUrlBlacklist() { - return decodedUrlBlacklist; + return getDecodedUrlBlocklist(); } + + /** + * Strict {@link FirewalledRequest}. + */ + private class StrictFirewalledRequest extends FirewalledRequest { + + StrictFirewalledRequest(HttpServletRequest request) { + super(request); + } + + @Override + public long getDateHeader(String name) { + if (name != null) { + validateAllowedHeaderName(name); + } + return super.getDateHeader(name); + } + + @Override + public int getIntHeader(String name) { + if (name != null) { + validateAllowedHeaderName(name); + } + return super.getIntHeader(name); + } + + @Override + public String getHeader(String name) { + if (name != null) { + validateAllowedHeaderName(name); + } + String value = super.getHeader(name); + if (value != null) { + validateAllowedHeaderValue(value); + } + return value; + } + + @Override + public Enumeration getHeaders(String name) { + if (name != null) { + validateAllowedHeaderName(name); + } + Enumeration headers = super.getHeaders(name); + return new Enumeration() { + + @Override + public boolean hasMoreElements() { + return headers.hasMoreElements(); + } + + @Override + public String nextElement() { + String value = headers.nextElement(); + validateAllowedHeaderValue(value); + return value; + } + + }; + } + + @Override + public Enumeration getHeaderNames() { + Enumeration names = super.getHeaderNames(); + return new Enumeration() { + + @Override + public boolean hasMoreElements() { + return names.hasMoreElements(); + } + + @Override + public String nextElement() { + String headerNames = names.nextElement(); + validateAllowedHeaderName(headerNames); + return headerNames; + } + + }; + } + + @Override + public String getParameter(String name) { + if (name != null) { + validateAllowedParameterName(name); + } + String value = super.getParameter(name); + if (value != null) { + validateAllowedParameterValue(value); + } + return value; + } + + @Override + public Map getParameterMap() { + Map parameterMap = super.getParameterMap(); + for (Map.Entry entry : parameterMap.entrySet()) { + String name = entry.getKey(); + String[] values = entry.getValue(); + validateAllowedParameterName(name); + for (String value : values) { + validateAllowedParameterValue(value); + } + } + return parameterMap; + } + + @Override + public Enumeration getParameterNames() { + Enumeration paramaterNames = super.getParameterNames(); + return new Enumeration() { + + @Override + public boolean hasMoreElements() { + return paramaterNames.hasMoreElements(); + } + + @Override + public String nextElement() { + String name = paramaterNames.nextElement(); + validateAllowedParameterName(name); + return name; + } + + }; + } + + @Override + public String[] getParameterValues(String name) { + if (name != null) { + validateAllowedParameterName(name); + } + String[] values = super.getParameterValues(name); + if (values != null) { + for (String value : values) { + validateAllowedParameterValue(value); + } + } + return values; + } + + private void validateAllowedHeaderName(String headerNames) { + if (!StrictHttpFirewall.this.allowedHeaderNames.test(headerNames)) { + throw new RequestRejectedException( + "The request was rejected because the header name \"" + headerNames + "\" is not allowed."); + } + } + + private void validateAllowedHeaderValue(String value) { + if (!StrictHttpFirewall.this.allowedHeaderValues.test(value)) { + throw new RequestRejectedException( + "The request was rejected because the header value \"" + value + "\" is not allowed."); + } + } + + private void validateAllowedParameterName(String name) { + if (!StrictHttpFirewall.this.allowedParameterNames.test(name)) { + throw new RequestRejectedException( + "The request was rejected because the parameter name \"" + name + "\" is not allowed."); + } + } + + private void validateAllowedParameterValue(String value) { + if (!StrictHttpFirewall.this.allowedParameterValues.test(value)) { + throw new RequestRejectedException( + "The request was rejected because the parameter value \"" + value + "\" is not allowed."); + } + } + + @Override + public void reset() { + } + + }; + } diff --git a/web/src/test/java/org/springframework/security/web/firewall/StrictHttpFirewallTests.java b/web/src/test/java/org/springframework/security/web/firewall/StrictHttpFirewallTests.java index d91818af2a..809f1cc3f1 100644 --- a/web/src/test/java/org/springframework/security/web/firewall/StrictHttpFirewallTests.java +++ b/web/src/test/java/org/springframework/security/web/firewall/StrictHttpFirewallTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,26 +16,27 @@ package org.springframework.security.web.firewall; -import static org.assertj.core.api.Assertions.assertThatCode; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assertions.fail; - import java.util.Arrays; import java.util.List; +import javax.servlet.http.HttpServletRequest; + import org.junit.Test; + import org.springframework.http.HttpMethod; import org.springframework.mock.web.MockHttpServletRequest; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + /** * @author Rob Winch * @author Eddú Meléndez */ public class StrictHttpFirewallTests { - public String[] unnormalizedPaths = { "/..", "/./path/", "/path/path/.", "/path/path//.", "./path/../path//.", - "./path", ".//path", ".", "//path", "//path/path", "//path//path", "/path//path" }; + public String[] unnormalizedPaths = { "/..", "/./path/", "/path/path/.", "/path/path//.", "./path/../path//.", + "./path", ".//path", ".", "//path", "//path/path", "//path//path", "/path//path" }; private StrictHttpFirewall firewall = new StrictHttpFirewall(); @@ -44,32 +45,32 @@ public class StrictHttpFirewallTests { @Test public void getFirewalledRequestWhenInvalidMethodThenThrowsRequestRejectedException() { this.request.setMethod("INVALID"); - assertThatThrownBy(() -> this.firewall.getFirewalledRequest(this.request)) - .isInstanceOf(RequestRejectedException.class); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } // blocks XST attacks @Test public void getFirewalledRequestWhenTraceMethodThenThrowsRequestRejectedException() { this.request.setMethod(HttpMethod.TRACE.name()); - assertThatThrownBy(() -> this.firewall.getFirewalledRequest(this.request)) - .isInstanceOf(RequestRejectedException.class); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test // blocks XST attack if request is forwarded to a Microsoft IIS web server public void getFirewalledRequestWhenTrackMethodThenThrowsRequestRejectedException() { this.request.setMethod("TRACK"); - assertThatThrownBy(() -> this.firewall.getFirewalledRequest(this.request)) - .isInstanceOf(RequestRejectedException.class); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test // HTTP methods are case sensitive public void getFirewalledRequestWhenLowercaseGetThenThrowsRequestRejectedException() { this.request.setMethod("get"); - assertThatThrownBy(() -> this.firewall.getFirewalledRequest(this.request)) - .isInstanceOf(RequestRejectedException.class); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test @@ -77,8 +78,7 @@ public class StrictHttpFirewallTests { List allowedMethods = Arrays.asList("DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT"); for (String allowedMethod : allowedMethods) { this.request = new MockHttpServletRequest(allowedMethod, ""); - assertThatCode(() -> this.firewall.getFirewalledRequest(this.request)) - .doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(this.request); } } @@ -86,8 +86,7 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenInvalidMethodAndAnyMethodThenNoException() { this.firewall.setUnsafeAllowAnyHttpMethod(true); this.request.setMethod("INVALID"); - assertThatCode(() -> this.firewall.getFirewalledRequest(this.request)) - .doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(this.request); } @Test @@ -95,11 +94,8 @@ public class StrictHttpFirewallTests { for (String path : this.unnormalizedPaths) { this.request = new MockHttpServletRequest("GET", ""); this.request.setRequestURI(path); - try { - this.firewall.getFirewalledRequest(this.request); - fail(path + " is un-normalized"); - } catch (RequestRejectedException expected) { - } + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } } @@ -108,11 +104,8 @@ public class StrictHttpFirewallTests { for (String path : this.unnormalizedPaths) { this.request = new MockHttpServletRequest("GET", ""); this.request.setContextPath(path); - try { - this.firewall.getFirewalledRequest(this.request); - fail(path + " is un-normalized"); - } catch (RequestRejectedException expected) { - } + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } } @@ -121,11 +114,8 @@ public class StrictHttpFirewallTests { for (String path : this.unnormalizedPaths) { this.request = new MockHttpServletRequest("GET", ""); this.request.setServletPath(path); - try { - this.firewall.getFirewalledRequest(this.request); - fail(path + " is un-normalized"); - } catch (RequestRejectedException expected) { - } + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } } @@ -134,105 +124,99 @@ public class StrictHttpFirewallTests { for (String path : this.unnormalizedPaths) { this.request = new MockHttpServletRequest("GET", ""); this.request.setPathInfo(path); - try { - this.firewall.getFirewalledRequest(this.request); - fail(path + " is un-normalized"); - } catch (RequestRejectedException expected) { - } + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } } - // --- ; --- - - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenSemicolonInContextPathThenThrowsRequestRejectedException() { this.request.setContextPath(";/context"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenSemicolonInServletPathThenThrowsRequestRejectedException() { this.request.setServletPath("/spring;/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenSemicolonInPathInfoThenThrowsRequestRejectedException() { this.request.setPathInfo("/path;/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenSemicolonInRequestUriThenThrowsRequestRejectedException() { this.request.setRequestURI("/path;/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenEncodedSemicolonInContextPathThenThrowsRequestRejectedException() { this.request.setContextPath("%3B/context"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenEncodedSemicolonInServletPathThenThrowsRequestRejectedException() { this.request.setServletPath("/spring%3B/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenEncodedSemicolonInPathInfoThenThrowsRequestRejectedException() { this.request.setPathInfo("/path%3B/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenEncodedSemicolonInRequestUriThenThrowsRequestRejectedException() { this.request.setRequestURI("/path%3B/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenLowercaseEncodedSemicolonInContextPathThenThrowsRequestRejectedException() { this.request.setContextPath("%3b/context"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenLowercaseEncodedSemicolonInServletPathThenThrowsRequestRejectedException() { this.request.setServletPath("/spring%3b/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenLowercaseEncodedSemicolonInPathInfoThenThrowsRequestRejectedException() { this.request.setPathInfo("/path%3b/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenLowercaseEncodedSemicolonInRequestUriThenThrowsRequestRejectedException() { this.request.setRequestURI("/path%3b/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test public void getFirewalledRequestWhenSemicolonInContextPathAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setContextPath(";/context"); - this.firewall.getFirewalledRequest(this.request); } @@ -240,7 +224,6 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenSemicolonInServletPathAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setServletPath("/spring;/"); - this.firewall.getFirewalledRequest(this.request); } @@ -248,7 +231,6 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenSemicolonInPathInfoAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setPathInfo("/path;/"); - this.firewall.getFirewalledRequest(this.request); } @@ -256,7 +238,6 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenSemicolonInRequestUriAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setRequestURI("/path;/"); - this.firewall.getFirewalledRequest(this.request); } @@ -265,7 +246,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setContextPath("%3B/context"); - this.firewall.getFirewalledRequest(this.request); } @@ -274,7 +254,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setServletPath("/spring%3B/"); - this.firewall.getFirewalledRequest(this.request); } @@ -283,7 +262,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setPathInfo("/path%3B/"); - this.firewall.getFirewalledRequest(this.request); } @@ -291,7 +269,6 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenEncodedSemicolonInRequestUriAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setRequestURI("/path%3B/"); - this.firewall.getFirewalledRequest(this.request); } @@ -300,7 +277,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setContextPath("%3b/context"); - this.firewall.getFirewalledRequest(this.request); } @@ -309,7 +285,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setServletPath("/spring%3b/"); - this.firewall.getFirewalledRequest(this.request); } @@ -318,7 +293,6 @@ public class StrictHttpFirewallTests { this.firewall.setAllowUrlEncodedPercent(true); this.firewall.setAllowSemicolon(true); this.request.setPathInfo("/path%3b/"); - this.firewall.getFirewalledRequest(this.request); } @@ -326,38 +300,35 @@ public class StrictHttpFirewallTests { public void getFirewalledRequestWhenLowercaseEncodedSemicolonInRequestUriAndAllowSemicolonThenNoException() { this.firewall.setAllowSemicolon(true); this.request.setRequestURI("/path%3b/"); - this.firewall.getFirewalledRequest(this.request); } - // --- encoded . --- - - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenEncodedPeriodInThenThrowsRequestRejectedException() { this.request.setRequestURI("/%2E/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenLowercaseEncodedPeriodInThenThrowsRequestRejectedException() { this.request.setRequestURI("/%2e/"); - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test public void getFirewalledRequestWhenAllowEncodedPeriodAndEncodedPeriodInThenNoException() { this.firewall.setAllowUrlEncodedPeriod(true); this.request.setRequestURI("/%2E/"); - this.firewall.getFirewalledRequest(this.request); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenExceedsLowerboundAsciiThenException() { this.request.setRequestURI("/\u0019"); - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test @@ -372,13 +343,46 @@ public class StrictHttpFirewallTests { this.firewall.getFirewalledRequest(this.request); } - @Test(expected = RequestRejectedException.class) - public void getFirewalledRequestWhenExceedsUpperboundAsciiThenException() { - this.request.setRequestURI("/\u007f"); + @Test + public void getFirewalledRequestWhenJapaneseCharacterThenNoException() { + this.request.setServletPath("/\u3042"); this.firewall.getFirewalledRequest(this.request); } - // --- from DefaultHttpFirewallTests --- + @Test + public void getFirewalledRequestWhenExceedsUpperboundAsciiThenException() { + this.request.setRequestURI("/\u007f"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsNullThenException() { + this.request.setRequestURI("/\0"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsEncodedNullThenException() { + this.request.setRequestURI("/something%00/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsLowercaseEncodedLineFeedThenException() { + this.request.setRequestURI("/something%0a/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsUppercaseEncodedLineFeedThenException() { + this.request.setRequestURI("/something%0A/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } @Test public void getFirewalledRequestWhenContainsLineFeedThenException() { @@ -394,6 +398,20 @@ public class StrictHttpFirewallTests { .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } + @Test + public void getFirewalledRequestWhenContainsLowercaseEncodedCarriageReturnThenException() { + this.request.setRequestURI("/something%0d/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsUppercaseEncodedCarriageReturnThenException() { + this.request.setRequestURI("/something%0D/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + @Test public void getFirewalledRequestWhenContainsCarriageReturnThenException() { this.request.setRequestURI("/something\r/"); @@ -408,29 +426,119 @@ public class StrictHttpFirewallTests { .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } + @Test + public void getFirewalledRequestWhenServletPathContainsLineSeparatorThenException() { + this.request.setServletPath("/something\u2028/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenServletPathContainsParagraphSeparatorThenException() { + this.request.setServletPath("/something\u2029/"); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenContainsLowercaseEncodedLineFeedAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedLineFeed(true); + this.request.setRequestURI("/something%0a/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenContainsUppercaseEncodedLineFeedAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedLineFeed(true); + this.request.setRequestURI("/something%0A/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenContainsLineFeedAndAllowedThenException() { + this.firewall.setAllowUrlEncodedLineFeed(true); + this.request.setRequestURI("/something\n/"); + // Expected an error because the line feed is decoded in an encoded part of the + // URL + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenServletPathContainsLineFeedAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedLineFeed(true); + this.request.setServletPath("/something\n/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenContainsLowercaseEncodedCarriageReturnAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedCarriageReturn(true); + this.request.setRequestURI("/something%0d/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenContainsUppercaseEncodedCarriageReturnAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedCarriageReturn(true); + this.request.setRequestURI("/something%0D/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenContainsCarriageReturnAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedCarriageReturn(true); + this.request.setRequestURI("/something\r/"); + // Expected an error because the carriage return is decoded in an encoded part of + // the URL + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestWhenServletPathContainsCarriageReturnAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedCarriageReturn(true); + this.request.setServletPath("/something\r/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenServletPathContainsLineSeparatorAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedLineSeparator(true); + this.request.setServletPath("/something\u2028/"); + this.firewall.getFirewalledRequest(this.request); + } + + @Test + public void getFirewalledRequestWhenServletPathContainsParagraphSeparatorAndAllowedThenNoException() { + this.firewall.setAllowUrlEncodedParagraphSeparator(true); + this.request.setServletPath("/something\u2029/"); + this.firewall.getFirewalledRequest(this.request); + } + /** - * 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. + * 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) + @Test public void getFirewalledRequestWhenLowercaseEncodedPathThenException() { this.request.setRequestURI("/context-root/a/b;%2f1/c"); this.request.setContextPath("/context-root"); this.request.setServletPath(""); this.request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } - @Test(expected = RequestRejectedException.class) + @Test public void getFirewalledRequestWhenUppercaseEncodedPathThenException() { this.request.setRequestURI("/context-root/a/b;%2F1/c"); this.request.setContextPath("/context-root"); this.request.setServletPath(""); this.request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI - - this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); } @Test @@ -442,7 +550,6 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI - this.firewall.getFirewalledRequest(request); } @@ -455,7 +562,6 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b;/1/c"); // URL decoded requestURI - this.firewall.getFirewalledRequest(request); } @@ -468,7 +574,7 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b//c"); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -480,7 +586,7 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b//c"); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -492,7 +598,7 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b//c"); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -504,7 +610,7 @@ public class StrictHttpFirewallTests { request.setContextPath("/context-root"); request.setServletPath(""); request.setPathInfo("/a/b//c"); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -513,7 +619,7 @@ public class StrictHttpFirewallTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.setRequestURI("/context-root/a/b%2F%2Fc"); this.firewall.getEncodedUrlBlacklist().removeAll(Arrays.asList("%2F%2F")); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -522,7 +628,7 @@ public class StrictHttpFirewallTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.setRequestURI("/context-root/a/b%2f%2fc"); this.firewall.getEncodedUrlBlacklist().removeAll(Arrays.asList("%2f%2f")); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -531,7 +637,7 @@ public class StrictHttpFirewallTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.setRequestURI("/context-root/a/b%2f%2Fc"); this.firewall.getEncodedUrlBlacklist().removeAll(Arrays.asList("%2f%2F")); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -540,7 +646,7 @@ public class StrictHttpFirewallTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.setRequestURI("/context-root/a/b%2F%2fc"); this.firewall.getEncodedUrlBlacklist().removeAll(Arrays.asList("%2F%2f")); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); } @Test @@ -548,22 +654,234 @@ public class StrictHttpFirewallTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.setPathInfo("/a/b//c"); this.firewall.getDecodedUrlBlacklist().removeAll(Arrays.asList("//")); - assertThatCode(() -> this.firewall.getFirewalledRequest(request)).doesNotThrowAnyException(); + this.firewall.getFirewalledRequest(request); + } + + // blocklist + @Test + public void getFirewalledRequestWhenRemoveFromUpperCaseEncodedUrlBlocklistThenNoException() { + this.firewall.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + request.setRequestURI("/context-root/a/b%2F%2Fc"); + this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2F%2F")); + this.firewall.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenRemoveFromLowerCaseEncodedUrlBlocklistThenNoException() { + this.firewall.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + request.setRequestURI("/context-root/a/b%2f%2fc"); + this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2f%2f")); + this.firewall.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenRemoveFromLowerCaseAndUpperCaseEncodedUrlBlocklistThenNoException() { + this.firewall.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + request.setRequestURI("/context-root/a/b%2f%2Fc"); + this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2f%2F")); + this.firewall.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenRemoveFromUpperCaseAndLowerCaseEncodedUrlBlocklistThenNoException() { + this.firewall.setAllowUrlEncodedSlash(true); + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + request.setRequestURI("/context-root/a/b%2F%2fc"); + this.firewall.getEncodedUrlBlocklist().removeAll(Arrays.asList("%2F%2f")); + this.firewall.getFirewalledRequest(request); + } + + @Test + public void getFirewalledRequestWhenRemoveFromDecodedUrlBlocklistThenNoException() { + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + request.setPathInfo("/a/b//c"); + this.firewall.getDecodedUrlBlocklist().removeAll(Arrays.asList("//")); + this.firewall.getFirewalledRequest(request); } @Test public void getFirewalledRequestWhenTrustedDomainThenNoException() { this.request.addHeader("Host", "example.org"); - this.firewall.setAllowedHostnames(hostname -> hostname.equals("example.org")); - - assertThatCode(() -> this.firewall.getFirewalledRequest(this.request)).doesNotThrowAnyException(); - } - - @Test(expected = RequestRejectedException.class) - public void getFirewalledRequestWhenUntrustedDomainThenException() { - this.request.addHeader("Host", "example.org"); - this.firewall.setAllowedHostnames(hostname -> hostname.equals("myexample.org")); - + this.firewall.setAllowedHostnames((hostname) -> hostname.equals("example.org")); this.firewall.getFirewalledRequest(this.request); } + + @Test + public void getFirewalledRequestWhenUntrustedDomainThenException() { + this.request.addHeader("Host", "example.org"); + this.firewall.setAllowedHostnames((hostname) -> hostname.equals("myexample.org")); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> this.firewall.getFirewalledRequest(this.request)); + } + + @Test + public void getFirewalledRequestGetHeaderWhenNotAllowedHeaderNameThenException() { + this.firewall.setAllowedHeaderNames((name) -> !name.equals("bad name")); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("bad name")); + } + + @Test + public void getFirewalledRequestGetHeaderWhenNotAllowedHeaderValueThenException() { + this.request.addHeader("good name", "bad value"); + this.firewall.setAllowedHeaderValues((value) -> !value.equals("bad value")); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("good name")); + } + + @Test + public void getFirewalledRequestGetDateHeaderWhenControlCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getDateHeader("Bad\0Name")); + } + + @Test + public void getFirewalledRequestGetIntHeaderWhenControlCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getIntHeader("Bad\0Name")); + } + + @Test + public void getFirewalledRequestGetHeaderWhenControlCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("Bad\0Name")); + } + + @Test + public void getFirewalledRequestGetHeaderWhenUndefinedCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\uFFFEName", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("Bad\uFFFEName")); + } + + @Test + public void getFirewalledRequestGetHeadersWhenControlCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeaders("Bad\0Name")); + } + + @Test + public void getFirewalledRequestGetHeaderNamesWhenControlCharacterInHeaderNameThenException() { + this.request.addHeader("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> request.getHeaderNames().nextElement()); + } + + @Test + public void getFirewalledRequestGetHeaderWhenControlCharacterInHeaderValueThenException() { + this.request.addHeader("Something", "bad\0value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("Something")); + } + + @Test + public void getFirewalledRequestGetHeaderWhenUndefinedCharacterInHeaderValueThenException() { + this.request.addHeader("Something", "bad\uFFFEvalue"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getHeader("Something")); + } + + @Test + public void getFirewalledRequestGetHeadersWhenControlCharacterInHeaderValueThenException() { + this.request.addHeader("Something", "bad\0value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> request.getHeaders("Something").nextElement()); + } + + @Test + public void getFirewalledRequestGetParameterWhenControlCharacterInParameterNameThenException() { + this.request.addParameter("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> request.getParameter("Bad\0Name")); + } + + @Test + public void getFirewalledRequestGetParameterMapWhenControlCharacterInParameterNameThenException() { + this.request.addParameter("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(request::getParameterMap); + } + + @Test + public void getFirewalledRequestGetParameterNamesWhenControlCharacterInParameterNameThenException() { + this.request.addParameter("Bad\0Name", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(request.getParameterNames()::nextElement); + } + + @Test + public void getFirewalledRequestGetParameterNamesWhenUndefinedCharacterInParameterNameThenException() { + this.request.addParameter("Bad\uFFFEName", "some value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(request.getParameterNames()::nextElement); + } + + @Test + public void getFirewalledRequestGetParameterValuesWhenNotAllowedInParameterValueThenException() { + this.firewall.setAllowedParameterValues((value) -> !value.equals("bad value")); + this.request.addParameter("Something", "bad value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> request.getParameterValues("Something")); + } + + @Test + public void getFirewalledRequestGetParameterValuesWhenNotAllowedInParameterNameThenException() { + this.firewall.setAllowedParameterNames((value) -> !value.equals("bad name")); + this.request.addParameter("bad name", "good value"); + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(RequestRejectedException.class) + .isThrownBy(() -> request.getParameterValues("bad name")); + } + + // gh-9598 + @Test + public void getFirewalledRequestGetParameterWhenNameIsNullThenIllegalArgumentException() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> request.getParameter(null)); + } + + // gh-9598 + @Test + public void getFirewalledRequestGetParameterValuesWhenNameIsNullThenIllegalArgumentException() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(() -> request.getParameterValues(null)); + } + + // gh-9598 + @Test + public void getFirewalledRequestGetHeaderWhenNameIsNullThenNull() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThat(request.getHeader(null)).isNull(); + } + + // gh-9598 + @Test + public void getFirewalledRequestGetHeadersWhenNameIsNullThenEmptyEnumeration() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThat(request.getHeaders(null).hasMoreElements()).isFalse(); + } + + // gh-9598 + @Test + public void getFirewalledRequestGetIntHeaderWhenNameIsNullThenNegativeOne() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThat(request.getIntHeader(null)).isEqualTo(-1); + } + + @Test + public void getFirewalledRequestGetDateHeaderWhenNameIsNullThenNegativeOne() { + HttpServletRequest request = this.firewall.getFirewalledRequest(this.request); + assertThat(request.getDateHeader(null)).isEqualTo(-1); + } + }