diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilter.java b/java/org/apache/catalina/filters/CsrfPreventionFilter.java index 8f131439eec0..41bd83a8f29d 100644 --- a/java/org/apache/catalina/filters/CsrfPreventionFilter.java +++ b/java/org/apache/catalina/filters/CsrfPreventionFilter.java @@ -544,6 +544,8 @@ public CsrfResponseWrapper(HttpServletResponse response, String nonceRequestPara @Override public String encodeRedirectURL(String url) { + url = removeQueryParameters(url, nonceRequestParameterName); + if (shouldAddNonce(url)) { return addNonce(super.encodeRedirectURL(url)); } else { @@ -553,6 +555,8 @@ public String encodeRedirectURL(String url) { @Override public String encodeURL(String url) { + url = removeQueryParameters(url, nonceRequestParameterName); + if (shouldAddNonce(url)) { return addNonce(super.encodeURL(url)); } else { @@ -574,6 +578,89 @@ private boolean shouldAddNonce(String url) { return true; } + /** + * Removes zero or more query parameters from a URL. + * + * All instances of the query parameter and any associated values will be + * removed. + * + * @param url The URL whose query parameters should be removed. + * + * @param parameterName The name of the parameter to remove. + * + * @return The URL without any instances of the query parameter + * parameterName present. + */ + public static String removeQueryParameters(String url, String parameterName) { + if (null != parameterName) { + // Check for query string + int q = url.indexOf('?'); + if (q > -1) { + + // Look for parameter end + int start = q + 1; + int pos = url.indexOf('&', start); + + int iterations = 0; + + while (-1 < pos) { + // Process all parameters + if (++iterations > 100000) { + // Just in case things get out of control + throw new IllegalStateException("Way too many loop iterations"); + } + + int eq = url.indexOf('=', start); + int paramNameEnd; + if (-1 == eq || eq > pos) { + paramNameEnd = pos; + // Found no equal sign at all or past next & ; Parameter is all name. + } else { + // Found this param's equal sign + paramNameEnd = eq; + } + if (parameterName.equals(url.substring(start, paramNameEnd))) { + // Remove the parameter + url = url.substring(0, start) + url.substring(pos + 1); // +1 to consume the & + } else { + start = pos + 1; // Go to next parameter + } + pos = url.indexOf('&', start); + } + + // Check final parameter + String paramName; + pos = url.indexOf('=', start); + + if (-1 < pos) { + paramName = url.substring(start, pos); + } else { + paramName = url.substring(start); + // No value + } + if (paramName.equals(parameterName)) { + // Remove this parameter + + // Remove any trailing ? or & as well + char c = url.charAt(start - 1); + if ('?' == c || '&' == c) { + start--; + } + + url = url.substring(0, start); + } else { + // Remove trailing ? if it's there. Is this worth it? + int length = url.length(); + if (length == q + 1) { + url = url.substring(0, length - 1); + } + } + } + } + + return url; + } + /* * Return the specified URL with the nonce added to the query string. * diff --git a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java index 8cf872dda87e..d053552a2ec3 100644 --- a/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java +++ b/test/org/apache/catalina/filters/TestCsrfPreventionFilter.java @@ -25,6 +25,8 @@ import java.util.Collections; import java.util.Iterator; import java.util.function.Predicate; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import jakarta.servlet.http.HttpServletResponse; @@ -196,6 +198,106 @@ public void testNoNonceMimeMatcher() { Assert.assertEquals("/foo/images/home.jpg", response.encodeURL("/foo/images/home.jpg")); } + @Test + public void testMultipleTokens() { + String nonce = "TESTNONCE"; + String testURL = "/foo/bar?" + Constants.CSRF_NONCE_SESSION_ATTR_NAME + "=sample"; + CsrfPreventionFilter.CsrfResponseWrapper response = new CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(), + Constants.CSRF_NONCE_SESSION_ATTR_NAME, nonce, null); + + Assert.assertTrue("Original URL does not contain CSRF token", + testURL.contains(Constants.CSRF_NONCE_SESSION_ATTR_NAME)); + + String result = response.encodeURL(testURL); + + int pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME); + Assert.assertTrue("Result URL does not contain CSRF token", + pos >= 0); + pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME, pos + 1); + Assert.assertFalse("Result URL contains multiple CSRF tokens: " + result, + pos >= 0); + } + + @Test + public void testURLCleansing() { + String[] urls = new String[] { + "/foo/bar", + "/foo/bar?", + "/foo/bar?csrf", + "/foo/bar?csrf&", + "/foo/bar?csrf=", + "/foo/bar?csrf=&", + "/foo/bar?csrf=abc", + "/foo/bar?csrf=abc&bar=foo", + "/foo/bar?bar=foo&csrf=abc", + "/foo/bar?bar=foo&csrf=abc&foo=bar", + "/foo/bar?csrfx=foil&bar=foo&csrf=abc&foo=bar", + "/foo/bar?csrfx=foil&bar=foo&csrf=abc&foo=bar&csrf=def", + "/foo/bar?csrf=&csrf&csrf&csrf&csrf=abc&csrf=", + "/foo/bar?xcsrf=&xcsrf&xcsrf&xcsrf&xcsrf=abc&xcsrf=", + "/foo/bar?xcsrf=&xcsrf&xcsrf&csrf=foo&xcsrf&xcsrf=abc&csrf=bar&xcsrf=&", + "/foo/bar?bar=fo?&csrf=abc", + "/foo/bar?bar=fo?&csrf=abc&baz=boh", + }; + + String csrfParameterName = "csrf"; + + for(String url : urls) { + String result = CsrfPreventionFilter.CsrfResponseWrapper.removeQueryParameters(url, csrfParameterName); + + Assert.assertEquals("Failed to cleanse URL '" + url + "' properly", dumbButAccurateCleanse(url, csrfParameterName), result); + } + + } + + private static String dumbButAccurateCleanse(String url, String csrfParameterName) { + // Get rid of [&csrf=value] with optional =value + Pattern p = Pattern.compile(Pattern.quote("&") + Pattern.quote(csrfParameterName) + "(=[^&]*)?(&|$)"); + + // Do this iteratively to get everything + Matcher m = p.matcher(url); + + while (m.find()) { + url = m.replaceFirst("$2"); + m = p.matcher(url); + } + + // Get rid of [?csrf=value] with optional =value + url = url.replaceAll(Pattern.quote("?") + csrfParameterName + "(=[^&]*)?(&|$)", "?"); + + if (url.endsWith("?")) { + // Special case: it's possible to have multiple ? in a URL: + // the query-string is technically allowed to contain ? characters. + // Only trim the trailing ? if it is actually the quest-string + // separator. + if(url.lastIndexOf('?', url.length() - 2) < 0) { + url = url.substring(0, url.length() - 1); + } + } + + return url; + } + + @Test + public void testDuplicateTokens() { + String nonce = "TESTNONCE"; + String testURL = "/foo/bar?" + Constants.CSRF_NONCE_SESSION_ATTR_NAME + "=" + nonce; + CsrfPreventionFilter.CsrfResponseWrapper response = new CsrfPreventionFilter.CsrfResponseWrapper(new NonEncodingResponse(), + Constants.CSRF_NONCE_SESSION_ATTR_NAME, nonce, null); + + Assert.assertTrue("Original URL does not contain CSRF token", + testURL.contains(Constants.CSRF_NONCE_SESSION_ATTR_NAME)); + + String result = response.encodeURL(testURL); + + int pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME); + Assert.assertTrue("Result URL does not contain CSRF token", + pos >= 0); + pos = result.indexOf(Constants.CSRF_NONCE_SESSION_ATTR_NAME, pos + 1); + Assert.assertFalse("Result URL contains multiple CSRF tokens: " + result, + pos >= 0); + } + private static class MimeTypeServletContext extends TesterServletContext { private String mimeType;