Avoid path traversal vis double-url encoding of redirect URI (#8)

(cherry picked from commit a2128fb9e940d96c2f9a64edcd4fbcc768eedb4f)
keycloak · Dec 13, 2022 · 1987c94 · 1987c94
1 parent 1ed81fa
commit 1987c94
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 1 deletion.
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
@@ -18,6 +18,8 @@
 package org.keycloak.protocol.oidc.utils;
 
 import org.jboss.logging.Logger;
+import org.keycloak.common.util.Encode;
+import org.keycloak.common.util.KeycloakUriBuilder;
 import org.keycloak.common.util.UriUtils;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.Constants;
@@ -91,6 +93,7 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
         KeycloakUriInfo uriInfo = session.getContext().getUri();
         RealmModel realm = session.getContext().getRealm();
 
+        redirectUri = decodeRedirectUri(redirectUri);
         if (redirectUri != null) {
             try {
                 URI uri = URI.create(redirectUri);
@@ -152,6 +155,42 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
         }
     }
 
+    // Decode redirectUri. We don't decode query and fragment as those can be encoded in the original URL.
+    // URL can be decoded multiple times (in case it was encoded multiple times, or some of it's parts were encoded multiple times)
+    private static String decodeRedirectUri(String redirectUri) {
+        if (redirectUri == null) return null;
+        int MAX_DECODING_COUNT = 5; // Max count of attempts for decoding URL (in case it was encoded multiple times)
+
+        try {
+            KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri);
+            String origQuery = uriBuilder.getQuery();
+            String origFragment = uriBuilder.getFragment();
+            String encodedRedirectUri = uriBuilder
+                    .replaceQuery(null)
+                    .fragment(null)
+                    .buildAsString();
+            String decodedRedirectUri = null;
+
+            for (int i = 0; i < MAX_DECODING_COUNT; i++) {
+                decodedRedirectUri = Encode.decode(encodedRedirectUri);
+                if (decodedRedirectUri.equals(encodedRedirectUri)) {
+                    // URL is decoded. We can return it (after attach original query and fragment)
+                    return KeycloakUriBuilder.fromUri(decodedRedirectUri)
+                            .replaceQuery(origQuery)
+                            .fragment(origFragment)
+                            .buildAsString();
+                } else {
+                    // Next attempt
+                    encodedRedirectUri = decodedRedirectUri;
+                }
+            }
+        } catch (IllegalArgumentException iae) {
+            logger.debugf("Illegal redirect URI used: %s, Details: %s", redirectUri, iae.getMessage());
+        }
+        logger.debugf("Was not able to decode redirect URI: %s", redirectUri);
+        return null;
+    }
+
     private static String lowerCaseHostname(String redirectUri) {
         int n = redirectUri.indexOf('/', 7);
         if (n == -1) {

diff --git a/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/...rquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java
@@ -337,6 +337,20 @@ public void testWildcard() throws IOException {
         checkRedirectUri("http://localhost:8280/foo/bar", true, true);
         checkRedirectUri("http://example.com/foobar", false);
         checkRedirectUri("http://localhost:8280/foobar", false, true);
+
+        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/", false); // double-encoded "http://example.com/foobar/../"
+        checkRedirectUri("http://example.com/foo/%252E%252E/?some_query_param=some_value", false); // double-encoded "http://example.com/foobar/../?some_query_param=some_value"
+        checkRedirectUri("http://example.com/foo/%252E%252E/?encodeTest=a%3Cb", false); // double-encoded "http://example.com/foobar/../?encodeTest=a<b"
+        checkRedirectUri("http://example.com/foo/%252E%252E/#encodeTest=a%3Cb", false); // double-encoded "http://example.com/foobar/../?encodeTest=a<b"
+        checkRedirectUri("http://example.com/foo/%25252E%25252E/", false); // triple-encoded "http://example.com/foobar/../"
+        checkRedirectUri("http://example.com/foo/%2525252525252E%2525252525252E/", false); // seventh-encoded "http://example.com/foobar/../"
+
+        checkRedirectUri("http://example.com/foo?encodeTest=a%3Cb", true);
+        checkRedirectUri("http://example.com/foo?encodeTest=a%3Cb#encode2=a%3Cb", true);
+        checkRedirectUri("http://example.com/foo/#encode2=a%3Cb", true);
     }
 
     @Test
@@ -381,7 +395,7 @@ public void testRelative() throws IOException {
         oauth.clientId("test-relative-url");
 
         checkRedirectUri("http://with-dash.example.local/foo", false);
-        checkRedirectUri("http://localhost:8180/auth", true);
+        checkRedirectUri(OAuthClient.AUTH_SERVER_ROOT, true);
     }
 
     @Test
@@ -455,6 +469,7 @@ private void checkRedirectUri(String redirectUri, boolean expectValid, boolean c
             if (!checkCodeToToken) {
                 oauth.openLoginForm();
                 Assert.assertTrue(loginPage.isCurrent());
+                Assert.assertFalse(errorPage.isCurrent());
             } else {
                 oauth.doLogin("test-user@localhost", "password");