From c3f34c3515ffdc122c1bfb3f1d980e60859b0025 Mon Sep 17 00:00:00 2001 From: Kosiorkosa47 Date: Thu, 21 May 2026 17:23:27 +0200 Subject: [PATCH] Tighten UNSAFE_PATH_PATTERN against encoded path-traversal terminators (#49000) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Tighten UNSAFE_PATH_PATTERN against encoded path-traversal terminators Fixes #48978 Extends the regex to cover encoded forms that previously bypassed detection: - %3B / %3b (encoded semicolon) - %09, %0A, %0D, %00 (control characters) - %252E (double-encoded dot) These encodings do not produce actual path traversal on conformant servers per RFC 3986 (percent-encoded characters are literals, not delimiters), but are semantically close enough to the patterns the regex was designed to block to warrant defense-in-depth coverage. The end-of-input anchor ($) is moved into the terminator class to collapse the two pattern alternatives into one, keeping the diff minimal. Test changes: - 8 new assertions covering encoded semicolons, control character terminators, and double-encoded dots. - 3 prior assertEquals flipped to assertNull (lines that previously asserted %252E%252E/, %252E%252E/#fragment, and ..%3Bsomething/ were allowed are now expected to be blocked). - 1 new negative test confirming %3B as legitimate path content (not following a parent-folder sequence) still resolves. Triple-encoded variants (e.g., %25252E) remain allowed; out of scope for this issue. Signed-off-by: Michał Kosiorek * Update OAuthRedirectUriTest expectations for double-encoded dots Follow-up to 36b0b10dd2 — Base IT (6) CI run for #49000 caught a cross-module integration test that needed updating alongside the regex change. Local verification of the previous commit covered the services module (RedirectUtilsTest); testsuite/integration-arquillian was outside that scope, so the existing OAuthRedirectUriTest.testWildcard expectations for %252E%252E variants didn't flip with the regex. Four assertions in testWildcard flipped from true → false to match the Option A semantic introduced in 36b0b10dd2 (double-encoded dots are now blocked by UNSAFE_PATH_PATTERN): http://example.com/foo/%252E%252E/ http://example.com/foo/%252E%252E/?some_query_param=some_value http://example.com/foo/%252E%252E/?encodeTest=a%3Cb http://example.com/foo/%252E%252E/#encodeTest=a%3Cb Triple-encoded (%25252E) and septuple-encoded variants remain expected:true — recursive decoding is explicitly out of scope for #48978. Verified locally: - mvn -pl services -Dtest=RedirectUtilsTest test → 11/11 green. - Direct regex match against the four flipped URIs confirms UNSAFE_PATH_PATTERN matches each rawPath, mechanically equivalent to the verifyRedirectUri code path exercised by the arquillian test. Refs #48978 Signed-off-by: Michał Kosiorek --------- Signed-off-by: Michał Kosiorek --- .../protocol/oidc/utils/RedirectUtils.java | 5 ++++- .../oidc/utils/RedirectUtilsTest.java | 19 +++++++++++++++---- .../testsuite/oauth/OAuthRedirectUriTest.java | 12 ++++++------ 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java index d0d64caccb8..51432b13e81 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java @@ -150,8 +150,11 @@ public class RedirectUtils { } // any access to parent folder /../ is unsafe with or without encoding + // = / | %2F | %5C | \ + // = "..", including %2E and %252E (double-encoded) variants + // = / | %2F | %5C | \ | ; | %3B | %09 | %0A | %0D | %00 | end-of-input private final static Pattern UNSAFE_PATH_PATTERN = Pattern.compile( - "(/|%2[fF]|%5[cC]|\\\\)(%2[eE]|\\.){2}(/|%2[fF]|%5[cC]|\\\\|;)|(/|%2[fF]|%5[cC]|\\\\)(%2[eE]|\\.){2}$"); + "(/|%2[fF]|%5[cC]|\\\\)(%2[eE]|%252[eE]|\\.){2}(/|%2[fF]|%5[cC]|\\\\|;|%3[bB]|%09|%0[aAdD]|%00|$)"); private static boolean areWildcardsAllowed(URI redirectUri) { // wildcars are only allowed if no user-info and no unparsed authority and no unsafe pattern in path diff --git a/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java b/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java index 4ec2daee568..38b7f4977c7 100644 --- a/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java +++ b/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java @@ -269,15 +269,26 @@ public class RedirectUtilsTest { Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test%5C..", set, false)); Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test%5C..;/", set, false)); Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%2F%2E%2E%2Fdocumentation", set, false)); + + // Issue #48978 — encoded terminators that previously bypassed the regex. + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%3B/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%3b/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%09/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%0A/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%0D/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%00/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%3Bsomething/", set, false)); + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/", set, false)); // double-encoded dots + Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/#encodeTest=a%3Cb", set, false)); + Assert.assertEquals("https://keycloak.org/test/.../", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/.../", set, false)); Assert.assertEquals("https://keycloak.org/test/%2E../", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%2E../", set, false)); // encoded Assert.assertEquals("https://keycloak.org/test/some%2Fthing/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/some%2Fthing/", set, false)); // encoded Assert.assertEquals("https://keycloak.org/test/./", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/./", set, false)); - Assert.assertEquals("https://keycloak.org/test/%252E%252E/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/", set, false)); // double-encoded - Assert.assertEquals("https://keycloak.org/test/%252E%252E/#encodeTest=a%3Cb", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/#encodeTest=a%3Cb", set, false)); // double-encoded - Assert.assertEquals("https://keycloak.org/test/%25252E%25252E/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%25252E%25252E/", set, false)); // triple-encoded + Assert.assertEquals("https://keycloak.org/test/%25252E%25252E/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%25252E%25252E/", set, false)); // triple-encoded; out of scope for #48978 Assert.assertEquals("https://keycloak.org/exact/%5C%2F/..", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/exact/%5C%2F/..", set, false)); - Assert.assertEquals("https://keycloak.org/test/..%3Bsomething/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/..%3Bsomething/", set, false)); + // Negative test: %3B as legitimate path content (not following `..`) must still resolve. + Assert.assertEquals("https://keycloak.org/test/file%3Bname/", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/file%3Bname/", set, false)); Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak%2Eorg/test/", set, false)); Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org%2Ftest%2F%40sample.com", set, false)); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java index c6d75eb9f21..0ff4f448b97 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java @@ -357,12 +357,12 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { checkRedirectUri("http://example.com/foo/../", false); checkRedirectUri("http://example.com/foo/%2E%2E/", false); // url-encoded "http://example.com/foobar/../" checkRedirectUri("http://example.com/foo%2F%2E%2E%2F", false); // url-encoded "http://example.com/foobar/../" - checkRedirectUri("http://example.com/foo/%252E%252E/", true); // double-encoded "http://example.com/foobar/../" - checkRedirectUri("http://example.com/foo/%252E%252E/?some_query_param=some_value", true); // double-encoded "http://example.com/foobar/../?some_query_param=some_value" - checkRedirectUri("http://example.com/foo/%252E%252E/?encodeTest=a%3Cb", true); // double-encoded "http://example.com/foobar/../?encodeTest=a