Check the redirect URI is http(s) when used for a form Post (#22)

Closes https://github.com/keycloak/security/issues/22

Co-authored-by: Stian Thorgersen <stianst@gmail.com>
Signed-off-by: Peter Skopek <pskopek@redhat.com>

docs/documentation/release_notes/index.adoc

@@ -16,13 +16,15 @@ include::topics/templates/release-header.adoc[]
 // == {project_name_full} 22.0.0
 // include::topics/22_0_0.adoc[leveloffset=2]
 
+== {project_name_full} 21.1.2
+include::topics/21_1_2.adoc[leveloffset=2]
+
 == {project_name_full} 21.1.0
 include::topics/21_1_0.adoc[leveloffset=2]
 
 == {project_name_full} 21.0.0
 include::topics/21_0_0.adoc[leveloffset=2]
 
-
 == {project_name_full} 20.0.0
 include::topics/20_0_0.adoc[leveloffset=2]
 

docs/documentation/release_notes/topics/21_1_2.adoc

@@ -0,0 +1,3 @@
+= Changes in validating schemes for valid redirect URIs
+
+If an application client is using non http(s) custom schemes, from now on the validation requires that a valid redirect pattern explicitly allows that scheme. Example patterns for allowing `custom` scheme are `custom:/test`, `custom:/test/\*` or `custom:*`. For security reasons a general pattern like `*` does not cover them anymore.

docs/documentation/server_admin/topics/threat/open-redirect.adoc

@@ -4,3 +4,5 @@
 An open redirector is an endpoint using a parameter to automatically redirect a user agent to the location specified by the parameter value without validation. An attacker can use the end-user authorization endpoint and the redirect URI parameter to use the authorization server as an open redirector, using a user's trust in an authorization server to launch a phishing attack.
 
 {project_name} requires that all registered applications and clients register at least one redirection URI pattern. When a client requests that {project_name} performs a redirect, {project_name} checks the redirect URI against the list of valid registered URI patterns. Clients and applications must register as specific a URI pattern as possible to mitigate open redirector attacks.
+
+If an application requires a non http(s) custom scheme, it should be an explicit part of the validation pattern (for example `custom:/app/\*`). For security reasons a general pattern like `*` does not cover non http(s) schemes.

services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java

@@ -32,8 +32,8 @@ import org.keycloak.services.util.ResolveRelative;
 
 import java.net.URI;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.stream.Collectors;
 
 /**
@@ -66,7 +66,8 @@ public class RedirectUtils {
 
     public static Set<String> resolveValidRedirects(KeycloakSession session, String rootUrl, Set<String> validRedirects) {
         // If the valid redirect URI is relative (no scheme, host, port) then use the request's scheme, host, and port
-        Set<String> resolveValidRedirects = new HashSet<>();
+        // the set is ordered by length to get the longest match first
+        Set<String> resolveValidRedirects = new TreeSet<>((String s1, String s2) -> s1.length() == s2.length()? s1.compareTo(s2) : s1.length() < s2.length()? 1 : -1);
         for (String validRedirect : validRedirects) {
             if (validRedirect.startsWith("/")) {
                 validRedirect = relativeToAbsoluteURI(session, rootUrl, validRedirect);
@@ -108,15 +109,16 @@ public class RedirectUtils {
         } else {
             // 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);
-            decodedRedirectUri = getNormalizedRedirectUri(decodedRedirectUri);
+            URI decodedRedirect = toUri(decodedRedirectUri);
+            decodedRedirectUri = getNormalizedRedirectUri(decodedRedirect);
             if (decodedRedirectUri == null) return null;
 
             String r = decodedRedirectUri;
             Set<String> resolveValidRedirects = resolveValidRedirects(session, rootUrl, validRedirects);
 
-            boolean valid = matchesRedirects(resolveValidRedirects, r, true);
+            String valid = matchesRedirects(resolveValidRedirects, r, true);
 
-            if (!valid && (r.startsWith(Constants.INSTALLED_APP_URL) || r.startsWith(Constants.INSTALLED_APP_LOOPBACK)) && r.indexOf(':', Constants.INSTALLED_APP_URL.length()) >= 0) {
+            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());
 
                 StringBuilder sb = new StringBuilder();
@@ -133,17 +135,28 @@ public class RedirectUtils {
             }
 
             // 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(redirectUri);
+            URI redirect = toUri(redirectUri);
+            redirectUri = getNormalizedRedirectUri(redirect);
 
             // 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) {
+            if (valid == null) {
                 valid = matchesRedirects(resolveValidRedirects, redirectUri, false);
             }
 
-            if (valid && redirectUri.startsWith("/")) {
+            if (valid != null && redirectUri.startsWith("/")) {
                 redirectUri = relativeToAbsoluteURI(session, rootUrl, redirectUri);
             }
-            redirectUri = valid ? redirectUri : null;
+
+            String scheme = decodedRedirect.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)) {
+                    logger.debugf("Invalid URI because scheme is not allowed: %s", redirectUri);
+                    valid = null;
+                }
+            }
+
+            redirectUri = valid != null ? redirectUri : null;
         }
 
         if (Constants.INSTALLED_APP_URN.equals(redirectUri)) {
@@ -153,18 +166,24 @@ public class RedirectUtils {
         }
     }
 
-    private static String getNormalizedRedirectUri(String redirectUri) {
+    private static URI toUri(String redirectUri) {
+        URI uri = null;
         if (redirectUri != null) {
-            try {
-                URI uri = URI.create(redirectUri);
-                redirectUri = uri.normalize().toString();
+        try {
+                uri = URI.create(redirectUri);
             } catch (IllegalArgumentException cause) {
                 logger.debug("Invalid redirect uri", cause);
-                return null;
             } catch (Exception cause) {
                 logger.debug("Unexpected error when parsing redirect uri", cause);
-                return null;
             }
+        }
+        return uri;
+    }
+
+    private static String getNormalizedRedirectUri(URI uri) {
+        String redirectUri = null;
+        if (uri != null) {
+            redirectUri = uri.normalize().toString();
             redirectUri = lowerCaseHostname(redirectUri);
         }
         return redirectUri;
@@ -229,7 +248,8 @@ public class RedirectUtils {
         return sb.toString();
     }
 
-    private static boolean matchesRedirects(Set<String> validRedirects, String redirect, boolean allowWildcards) {
+    // 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) {
@@ -238,14 +258,14 @@ public class RedirectUtils {
                 // strip off *
                 int length = validRedirect.length() - 1;
                 validRedirect = validRedirect.substring(0, length);
-                if (r.startsWith(validRedirect)) return true;
+                if (r.startsWith(validRedirect)) return validRedirect;
                 // strip off trailing '/'
                 if (length - 1 > 0 && validRedirect.charAt(length - 1) == '/') length--;
                 validRedirect = validRedirect.substring(0, length);
-                if (validRedirect.equals(r)) return true;
-            } else if (validRedirect.equals(redirect)) return true;
+                if (validRedirect.equals(r)) return validRedirect;
+            } else if (validRedirect.equals(redirect)) return validRedirect;
         }
-        return false;
+        return null;
     }
 
     private static String getSingleValidRedirectUri(Collection<String> validRedirects) {

services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright 2023 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.keycloak.protocol.oidc.utils;
+
+import java.net.URI;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.jboss.resteasy.core.ResteasyContext;
+import org.jboss.resteasy.mock.MockHttpRequest;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.keycloak.common.Profile;
+import org.keycloak.common.crypto.CryptoIntegration;
+import org.keycloak.common.crypto.CryptoProvider;
+import org.keycloak.http.HttpRequest;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.services.DefaultKeycloakSession;
+import org.keycloak.services.DefaultKeycloakSessionFactory;
+import org.keycloak.services.HttpRequestImpl;
+
+/**
+ * <p>Little test class for RedirectUtils methods.</p>
+ *
+ * @author rmartinc
+ */
+public class RedirectUtilsTest {
+
+    private static KeycloakSession session;
+
+    @BeforeClass
+    public static void beforeClass() {
+        HttpRequest httpRequest = new HttpRequestImpl(MockHttpRequest.create("GET", URI.create("https://keycloak.org/"), URI.create("https://keycloak.org")));
+        ResteasyContext.getContextDataMap().put(HttpRequest.class, httpRequest);
+        Profile.defaults();
+        CryptoIntegration.init(CryptoProvider.class.getClassLoader());
+        DefaultKeycloakSessionFactory sessionFactory = new DefaultKeycloakSessionFactory();
+        sessionFactory.init();
+        session = new DefaultKeycloakSession(sessionFactory);
+    }
+
+    @Test
+    public void testverifyRedirectUriHttps() {
+        Set<String> set = Stream.of(
+                "https://keycloak.org/test1",
+                "https://keycloak.org/test2",
+                "https://keycloak.org/parent/*"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("https://keycloak.org/test1", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test1", set, false));
+        Assert.assertEquals("https://keycloak.org/test2", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test2", set, false));
+        Assert.assertEquals("https://keycloak.org/parent", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/parent", set, false));
+        Assert.assertEquals("https://keycloak.org/parent/child", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/parent/child", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test1/child", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.com/test", set, false));
+    }
+
+    @Test
+    public void testverifyRedirectUriMixedSchemes() {
+        Set<String> set = Stream.of(
+                "https://keycloak.org/*",
+                "custom1:/test1",
+                "custom1:/test2",
+                "custom1:/parent/*",
+                "custom2:*"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("custom1:/test1", RedirectUtils.verifyRedirectUri(session, null, "custom1:/test1", set, false));
+        Assert.assertEquals("custom1:/test2", RedirectUtils.verifyRedirectUri(session, null, "custom1:/test2", set, false));
+        Assert.assertEquals("custom1:/parent/child", RedirectUtils.verifyRedirectUri(session, null, "custom1:/parent/child", set, false));
+        Assert.assertEquals("custom2:/something", RedirectUtils.verifyRedirectUri(session, null, "custom2:/something", set, false));
+        Assert.assertEquals("https://keycloak.org/test", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "custom1:/test", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "custom1:/test1/test", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "custom3:/test", set, false));
+    }
+
+    @Test
+    public void testverifyRedirectUriInvalidScheme() {
+        Set<String> set = Stream.of(
+                "custom1:/test1",
+                "custom1:/test2",
+                "custom1:/parent/*",
+                "custom2:*",
+                "*"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("custom1:/test1", RedirectUtils.verifyRedirectUri(session, null, "custom1:/test1", set, false));
+        Assert.assertEquals("custom1:/test2", RedirectUtils.verifyRedirectUri(session, null, "custom1:/test2", set, false));
+        Assert.assertEquals("custom1:/parent/child", RedirectUtils.verifyRedirectUri(session, null, "custom1:/parent/child", set, false));
+        Assert.assertEquals("custom2:/something", RedirectUtils.verifyRedirectUri(session, null, "custom2:/something", set, false));
+        Assert.assertEquals("https://keycloak.org/test", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test", set, false));
+        Assert.assertEquals("http://keycloak.org/test", RedirectUtils.verifyRedirectUri(session, null, "http://keycloak.org/test", set, false));
+        Assert.assertEquals("https://keycloak.org/test", RedirectUtils.verifyRedirectUri(session, null, "/test", set, false));
+        Assert.assertEquals("https://keycloak.com/test", RedirectUtils.verifyRedirectUri(session, "https://keycloak.com", "/test", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "custom3:/test", set, false));
+    }
+}

services/src/test/java/org/keycloak/protocol/oidc/utils/ResteasyTestProvider.java

@@ -0,0 +1,45 @@
+/*
+ * Copyright 2023 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.protocol.oidc.utils;
+
+import org.jboss.resteasy.core.ResteasyContext;
+import org.keycloak.common.util.ResteasyProvider;
+
+/**
+ * <p>Resteasy provider to be used for the utils class.</p>
+ * @author rmartinc
+ */
+public class ResteasyTestProvider implements ResteasyProvider {
+
+    @Override
+    public <R> R getContextData(Class<R> type) {
+        return ResteasyContext.getContextData(type);
+    }
+
+    @Override
+    public void pushDefaultContextObject(Class type, Object instance) {
+    }
+
+    @Override
+    public void pushContext(Class type, Object instance) {
+    }
+
+    @Override
+    public void clearContextData() {
+    }
+}

services/src/test/resources/META-INF/services/org.keycloak.common.util.ResteasyProvider

@@ -0,0 +1,17 @@
+#
+# Copyright 2023 Red Hat, Inc. and/or its affiliates
+# and other contributors as indicated by the @author tags.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+org.keycloak.protocol.oidc.utils.ResteasyTestProvider

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java

@@ -32,6 +32,7 @@ import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.AssertEvents;
 import org.keycloak.testsuite.pages.ErrorPage;
 import org.keycloak.testsuite.pages.InstalledAppRedirectPage;
+import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
 import org.keycloak.testsuite.util.ClientManager;
 import org.keycloak.testsuite.util.OAuthClient;
 import org.openqa.selenium.By;
@@ -39,6 +40,7 @@ import org.openqa.selenium.By;
 import jakarta.ws.rs.core.UriBuilder;
 import java.io.IOException;
 import java.net.URI;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -227,6 +229,22 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
         String codeId = events.expectLogin().assertEvent().getDetails().get(Details.CODE_ID);
     }
 
+    @Test
+    public void authorizationRequestFormPostResponseModeInvalidRedirectUri() throws IOException {
+        try (var c = ClientAttributeUpdater.forClient(adminClient, "test", "test-app")
+                .setRedirectUris(Collections.singletonList("*"))
+                .update()) {
+            oauth.responseMode(OIDCResponseMode.FORM_POST.value());
+            oauth.responseType(OAuth2Constants.CODE);
+            oauth.redirectUri("javascript:alert('XSS')");
+            oauth.openLoginForm();
+
+            errorPage.assertCurrent();
+            assertEquals("Invalid parameter: redirect_uri", errorPage.getError());
+
+            events.expectLogin().error(Errors.INVALID_REDIRECT_URI).user((String) null).session((String) null).clearDetails().assertEvent();
+        }
+    }
 
     @Test
     public void authorizationRequestFormPostResponseModeWithCustomState() throws IOException {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java

@@ -26,6 +26,7 @@ import org.keycloak.testsuite.util.SamlClientBuilder;
 import java.io.IOException;
 import java.net.URI;
 import java.security.Signature;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.logging.Level;
@@ -327,4 +328,21 @@ public class BasicSamlTest extends AbstractSamlTest {
             assertThat(signature, matchesRegex("^[A-Za-z0-9+/ ]+[= ]*$"));
         }
     }
+
+    @Test
+    public void testInvalidAssertionConsumerServiceURL() throws IOException {
+        try (var c = ClientAttributeUpdater.forClient(adminClient, REALM_NAME, SAML_CLIENT_ID_SALES_POST)
+                .setRedirectUris(Collections.singletonList("*"))
+                .update()) {
+
+            String page = new SamlClientBuilder()
+                    .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, "javascript:alert('XSS')", Binding.POST)
+                    .build()
+                    .executeAndTransform(response -> {
+                        assertThat(response, statusCodeIsHC(Status.BAD_REQUEST));
+                        return EntityUtils.toString(response.getEntity(), "UTF-8");
+                    });
+            assertThat(page, containsString("Invalid redirect uri"));
+        }
+    }
 }