Perform exact string match if redirect URI contains userinfo, encoded…

… slashes or parent access (#114) Closes keycloak/keycloak-private#113 Closes keycloak/keycloak-private#134 Signed-off-by: rmartinc <[email protected]>
eosc-kc · Mar 23, 2024 · e310604 · e310604
1 parent e3598a5
commit e310604
Show file tree

Hide file tree

Showing 10 changed files with 212 additions and 94 deletions.
diff --git a/common/src/main/java/org/keycloak/common/util/Encode.java b/common/src/main/java/org/keycloak/common/util/Encode.java
@@ -46,6 +46,7 @@ public class Encode
    private static final String[] matrixParameterEncoding = new String[128];
    private static final String[] queryNameValueEncoding = new String[128];
    private static final String[] queryStringEncoding = new String[128];
+   private static final String[] userInfoStringEncoding = new String[128];
 
    static
    {
@@ -158,6 +159,44 @@ public class Encode
          }
          queryStringEncoding[i] = URLEncoder.encode(String.valueOf((char) i));
       }
+
+     /*
+      * userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )
+      * unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
+      * pct-encoded   = "%" HEXDIG HEXDIG
+      * sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
+      *                   / "*" / "+" / "," / ";" / "="
+      */
+      for (int i = 0; i < 128; i++)
+      {
+         if (i >= 'a' && i <= 'z') continue;
+         if (i >= 'A' && i <= 'Z') continue;
+         if (i >= '0' && i <= '9') continue;
+         switch ((char) i)
+         {
+            case '-':
+            case '.':
+            case '_':
+            case '~':
+            case '!':
+            case '$':
+            case '&':
+            case '\'':
+            case '(':
+            case ')':
+            case '*':
+            case '+':
+            case ',':
+            case ';':
+            case '=':
+            case ':':
+               continue;
+            case ' ':
+               userInfoStringEncoding[i] = "%20";
+               continue;
+         }
+         userInfoStringEncoding[i] = URLEncoder.encode(String.valueOf((char) i));
+      }
    }
 
    /**
@@ -177,6 +216,24 @@ public static String encodeQueryStringNotTemplateParameters(String value) {
       return encodeNonCodes(encodeFromArray(value, queryStringEncoding, false));
    }
 
+   /**
+    * Keep encoded values "%..." and template parameters intact.
+    * @param value The user-info value to encode
+    * @return The user-info encoded
+    */
+   public static String encodeUserInfo(String value) {
+      return encodeValue(value, userInfoStringEncoding);
+   }
+
+   /**
+    * Keep encoded values "%..." but not the template parameters.
+    * @param value The user-info to encode
+    * @return The user-info encoded
+    */
+   public static String encodeUserInfoNotTemplateParameters(String value) {
+      return encodeNonCodes(encodeFromArray(value, userInfoStringEncoding, false));
+   }
+
    /**
     * Keep encoded values "%...", matrix parameters, template parameters, and '/' characters intact.
     */
@@ -424,6 +481,30 @@ public static String encodeQueryParamSaveEncodings(String segment)
       return result;
    }
 
+   /**
+    * Encodes everything in user-info
+    *
+    * @param nameOrValue
+    * @return
+    */
+   public static String encodeUserInfoAsIs(String nameOrValue)
+   {
+      return encodeFromArray(nameOrValue, userInfoStringEncoding, true);
+   }
+
+   /**
+    * Keep any valid encodings from string i.e. keep "%2D" but don't keep "%p"
+    *
+    * @param segment
+    * @return
+    */
+   public static String encodeUserInfoSaveEncodings(String segment)
+   {
+      String result = encodeFromArray(segment, userInfoStringEncoding, false);
+      result = encodeNonCodes(result);
+      return result;
+   }
+
    public static String encodeFragmentAsIs(String nameOrValue)
    {
       return encodeFromArray(nameOrValue, queryNameValueEncoding, true);

diff --git a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java
@@ -150,7 +150,7 @@ protected KeycloakUriBuilder parseHierarchicalUri(String uri, Matcher match, boo
             if (at > -1) {
                 String user = host.substring(0, at);
                 host = host.substring(at + 1);
-                this.userInfo = user;
+                replaceUserInfo(user, template);
             }
             Matcher hostPortMatch = hostPortPattern.matcher(host);
             if (hostPortMatch.matches()) {
@@ -459,7 +459,7 @@ private String buildString(Map<String, ?> paramMap, boolean fromEncodedMap, bool
         } else if (userInfo != null || host != null || port != -1) {
             buffer.append("//");
             if (userInfo != null)
-                replaceParameter(paramMap, fromEncodedMap, isTemplate, userInfo, buffer, encodeSlash).append("@");
+                replaceUserInfoParameter(paramMap, fromEncodedMap, isTemplate, userInfo, buffer).append("@");
             if (host != null) {
                 if ("".equals(host)) throw new RuntimeException("empty host name");
                 replaceParameter(paramMap, fromEncodedMap, isTemplate, host, buffer, encodeSlash);
@@ -571,6 +571,33 @@ protected StringBuffer replaceQueryStringParameter(Map<String, ?> paramMap, bool
         return buffer;
     }
 
+    protected StringBuffer replaceUserInfoParameter(Map<String, ?> paramMap, boolean fromEncodedMap, boolean isTemplate, String string, StringBuffer buffer) {
+        Matcher matcher = createUriParamMatcher(string);
+        while (matcher.find()) {
+            String param = matcher.group(1);
+            Object valObj = paramMap.get(param);
+            if (valObj == null && !isTemplate) {
+                throw new IllegalArgumentException("NULL value for template parameter: " + param);
+            } else if (valObj == null && isTemplate) {
+                matcher.appendReplacement(buffer, matcher.group());
+                continue;
+            }
+            String value = valObj.toString();
+            if (value != null) {
+                if (!fromEncodedMap) {
+                    value = Encode.encodeUserInfoAsIs(value);
+                } else {
+                    value = Encode.encodeUserInfoSaveEncodings(value);
+                }
+                matcher.appendReplacement(buffer, value);
+            } else {
+                throw new IllegalArgumentException("path param " + param + " has not been provided by the parameter map");
+            }
+        }
+        matcher.appendTail(buffer);
+        return buffer;
+    }
+
     /**
      * Return a unique order list of path params
      *
@@ -742,6 +769,15 @@ public KeycloakUriBuilder replacePath(String path, boolean template) {
         return this;
     }
 
+    public KeycloakUriBuilder replaceUserInfo(String userInfo, boolean template) {
+        if (userInfo == null) {
+            this.userInfo = null;
+            return this;
+        }
+        this.userInfo = template? Encode.encodeUserInfo(userInfo) : Encode.encodeUserInfoNotTemplateParameters(userInfo);
+        return this;
+    }
+
     public URI build(Object[] values, boolean encodeSlashInPath) throws IllegalArgumentException {
         if (values == null) throw new IllegalArgumentException("values param is null");
         return buildFromValues(encodeSlashInPath, false, values);

diff --git a/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java b/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java
@@ -80,4 +80,16 @@ public void testTemplateAndNotTemplate() {
         Assert.assertEquals("https://localhost:8443/%7Bpath%7D?key=%7Bquery%7D#%7Bfragment%7D", KeycloakUriBuilder.fromUri(
                 "https://localhost:8443/{path}?key={query}#{fragment}", false).buildAsString());
     }
+
+    @Test
+    public void testUserInfo() {
+        Assert.assertEquals("https://user-info@localhost:8443/path?key=query#fragment", KeycloakUriBuilder.fromUri(
+                "https://{userinfo}@localhost:8443/{path}?key={query}#{fragment}").buildAsString("user-info", "path", "query", "fragment"));
+        Assert.assertEquals("https://user%20info%40%2F@localhost:8443/path?key=query#fragment", KeycloakUriBuilder.fromUri(
+                "https://{userinfo}@localhost:8443/{path}?key={query}#{fragment}").buildAsString("user info@/", "path", "query", "fragment"));
+        Assert.assertEquals("https://user-info%E2%82%AC@localhost:8443", KeycloakUriBuilder.fromUri(
+                "https://user-info%E2%82%AC@localhost:8443", false).buildAsString());
+        Assert.assertEquals("https://user-info%E2%82%AC@localhost:8443", KeycloakUriBuilder.fromUri(
+                "https://user-info€@localhost:8443", false).buildAsString());
+    }
 }
diff --git a/docs/documentation/server_admin/topics/clients/oidc/con-basic-settings.adoc b/docs/documentation/server_admin/topics/clients/oidc/con-basic-settings.adoc
@@ -24,9 +24,13 @@ the name, set up a replacement string value. For example, a string value such as
 
 *Home URL*:: Provides the default URL for when the auth server needs to redirect or link back to the client.
 
-*Valid Redirect URIs*:: Required field.  Enter a URL pattern and click *+* to add and *-* to remove existing URLs and click *Save*. You can use wildcards at the end of the URL pattern. For example $$http://host.com/*$$
+*Valid Redirect URIs*:: Required field.  Enter a URL pattern and click *+* to add and *-* to remove existing URLs and click *Save*. Exact (case sensitive) string matching is used to compare valid redirect URIs.
 +
-Exclusive redirect URL patterns are typically more secure.  See xref:unspecific-redirect-uris_{context}[Unspecific Redirect URIs] for more information.
+You can use wildcards at the end of the URL pattern. For example `$$http://host.com/path/*$$`. To avoid security issues, if the passed redirect URI contains the *userinfo* part or its *path* manages access to parent directory (`/../`) no wildcard comparison is performed but the standard and secure exact string matching.
++
+The full wildcard `$$*$$` valid redirect URI can also be configured to allow any *http* or *https* redirect URI. Please do not use it in production environments.
++
+Exclusive redirect URI patterns are typically more secure. See xref:unspecific-redirect-uris_{context}[Unspecific Redirect URIs] for more information.
 
 Web Origins:: Enter a URL pattern and click + to add and - to remove existing URLs. Click Save.
 +

diff --git a/docs/documentation/upgrading/topics/keycloak/changes-22_0_10.adoc b/docs/documentation/upgrading/topics/keycloak/changes-22_0_10.adoc
@@ -0,0 +1,7 @@
+= Changes in redirect URI verification when using wildcards
+
+Because of security concerns, the redirect URI verification now performs a exact string matching (no wildcard involved) if the passed redirect uri contains a `userinfo` part or its `path` accesses parent directory (`/../`).
+
+The full wildcard `*` can still be used as a valid redirect in development for http(s) URIs with those characteristics. In production environments a exact valid redirect URI without wildcard needs to be configured for any URI of that type.
+
+Please note that wildcard valid redirect URIs are not recommended for production and not covered by the OAuth 2.0 specification.
diff --git a/docs/documentation/upgrading/topics/keycloak/changes.adoc b/docs/documentation/upgrading/topics/keycloak/changes.adoc
@@ -1,5 +1,9 @@
 == Migration Changes
 
+=== Migrating to 22.0.10
+
+include::changes-22_0_10.adoc[leveloffset=3]
+
 === Migrating to 22.0.7
 
 include::changes-22_0_7.adoc[leveloffset=3]

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,8 +18,6 @@
 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;
@@ -34,6 +32,7 @@
 import java.util.Collection;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 /**
@@ -111,16 +110,13 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
                 return null;
             }
 
-            // Make the validations against fully decoded and normalized redirect-url. This also allows wildcards (case when client configured "Valid redirect-urls" contain wildcards)
-            String decodedRedirectUri = decodeRedirectUri(redirectUri);
-            URI decodedRedirect = toUri(decodedRedirectUri);
-            decodedRedirectUri = getNormalizedRedirectUri(decodedRedirect);
-            if (decodedRedirectUri == null) return null;
+            // check if the passed URI allows wildcards
+            boolean allowWildcards = areWildcardsAllowed(originalRedirect);
 
-            String r = decodedRedirectUri;
+            String r = redirectUri;
             Set<String> resolveValidRedirects = resolveValidRedirects(session, rootUrl, validRedirects);
 
-            String valid = matchesRedirects(resolveValidRedirects, r, true);
+            String valid = matchesRedirects(resolveValidRedirects, r, allowWildcards);
 
             if (valid == null && (r.startsWith(Constants.INSTALLED_APP_URL) || r.startsWith(Constants.INSTALLED_APP_LOOPBACK)) && r.indexOf(':', Constants.INSTALLED_APP_URL.length()) >= 0) {
                 int i = r.indexOf(':', Constants.INSTALLED_APP_URL.length());
@@ -135,15 +131,7 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
 
                 r = sb.toString();
 
-                valid = matchesRedirects(resolveValidRedirects, r, true);
-            }
-
-            // Return the original redirectUri, which can be partially encoded - for example http://localhost:8280/foo/bar%20bar%2092%2F72/3 . Just make sure it is normalized
-            redirectUri = getNormalizedRedirectUri(originalRedirect);
-
-            // We try to check validity also for original (encoded) redirectUrl, but just in case it exactly matches some "Valid Redirect URL" specified for client (not wildcards allowed)
-            if (valid == null) {
-                valid = matchesRedirects(resolveValidRedirects, redirectUri, false);
+                valid = matchesRedirects(resolveValidRedirects, r, allowWildcards);
             }
 
             if (valid != null && !originalRedirect.isAbsolute()) {
@@ -154,7 +142,7 @@ public static String verifyRedirectUri(KeycloakSession session, String rootUrl,
                 redirectUri = relativeToAbsoluteURI(session, rootUrl, redirectUri);
             }
 
-            String scheme = decodedRedirect.getScheme();
+            String scheme = originalRedirect.getScheme();
             if (valid != null && scheme != null) {
                 // check the scheme is valid, it should be http(s) or explicitly allowed by the validation
                 if (!valid.startsWith(scheme + ":") && !"http".equalsIgnoreCase(scheme) && !"https".equalsIgnoreCase(scheme)) {
@@ -179,51 +167,22 @@ private static URI toUri(String redirectUri) {
             try {
                 uri = URI.create(redirectUri);
             } catch (IllegalArgumentException cause) {
-                logger.debug("Invalid redirect uri", cause);
+                logger.debugf(cause, "Invalid redirect uri %s", redirectUri);
             } catch (Exception cause) {
-                logger.debug("Unexpected error when parsing redirect uri", cause);
+                logger.debugf(cause, "Unexpected error when parsing redirect uri %s", redirectUri);
             }
         }
         return uri;
     }
 
-    private static String getNormalizedRedirectUri(URI uri) {
-        String redirectUri = null;
-        if (uri != null) {
-            redirectUri = uri.normalize().toString();
-        }
-        return redirectUri;
-    }
-
-    // Decode redirectUri. Only path is decoded as other elements can be encoded in the original URL or cannot be encoded at all.
-    // 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)
+    // any access to parent folder /../ is unsafe with or without encoding
+    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}$");
 
-        try {
-            KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri, false).preserveDefaultPort();
-            if (uriBuilder.getPath() == null) {
-                return redirectUri;
-            }
-            String encodedPath = uriBuilder.getPath();
-            String decodedPath;
-
-            for (int i = 0; i < MAX_DECODING_COUNT; i++) {
-                decodedPath = Encode.decode(encodedPath);
-                if (decodedPath.equals(encodedPath)) {
-                    // URL path is decoded. We can return it in the original redirect URI
-                    return uriBuilder.replacePath(decodedPath, false).buildAsString();
-                } else {
-                    // Next attempt
-                    encodedPath = decodedPath;
-                }
-            }
-        } 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 boolean areWildcardsAllowed(URI redirectUri) {
+        // wildcars are only allowed if no user-info and no unsafe pattern in path
+        return redirectUri.getRawUserInfo() == null
+                && (redirectUri.getRawPath() == null || !UNSAFE_PATH_PATTERN.matcher(redirectUri.getRawPath()).find());
     }
 
     private static String relativeToAbsoluteURI(KeycloakSession session, String rootUrl, String relative) {
@@ -240,24 +199,20 @@ private static String relativeToAbsoluteURI(KeycloakSession session, String root
         return sb.toString();
     }
 
-    // removes the queryString, fragment and userInfo from the redirect
-    // to avoid comparing this when wildcards are used
-    private static String stripOffRedirectForWildcard(String redirect) {
-        return KeycloakUriBuilder.fromUri(redirect, false)
-                .preserveDefaultPort()
-                .userInfo(null)
-                .replaceQuery(null)
-                .fragment(null)
-                .buildAsString();
-    }
-
     // return the String that matched the redirect or null if not matched
     private static String matchesRedirects(Set<String> validRedirects, String redirect, boolean allowWildcards) {
         logger.tracef("matchesRedirects: redirect URL to check: %s, allow wildcards: %b, Configured valid redirect URLs: %s", redirect, allowWildcards, validRedirects);
         for (String validRedirect : validRedirects) {
-            if (validRedirect.endsWith("*") && !validRedirect.contains("?") && allowWildcards) {
-                // strip off the userInfo, query or fragment components - we don't check them when wildcards are effective
-                String r = stripOffRedirectForWildcard(redirect);
+            if ("*".equals(validRedirect)) {
+                // the valid redirect * is a full wildcard for http(s) even if the redirect URI does not allow wildcards
+                return validRedirect;
+            } else if (validRedirect.endsWith("*") && !validRedirect.contains("?") && allowWildcards) {
+                // strip off the query or fragment components - we don't check them when wildcards are effective
+                int idx = redirect.indexOf('?');
+                if (idx == -1) {
+                    idx = redirect.indexOf('#');
+                }
+                String r = idx == -1 ? redirect : redirect.substring(0, idx);
                 // strip off *
                 int length = validRedirect.length() - 1;
                 validRedirect = validRedirect.substring(0, length);