diff --git a/docs/documentation/release_notes/index.adoc b/docs/documentation/release_notes/index.adoc index 7a179de8d17..15f69ac86c1 100644 --- a/docs/documentation/release_notes/index.adoc +++ b/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] diff --git a/docs/documentation/release_notes/topics/21_1_2.adoc b/docs/documentation/release_notes/topics/21_1_2.adoc new file mode 100644 index 00000000000..161a1d6e4ff --- /dev/null +++ b/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. diff --git a/docs/documentation/server_admin/topics/threat/open-redirect.adoc b/docs/documentation/server_admin/topics/threat/open-redirect.adoc index 98a08705c6d..a7417146079 100644 --- a/docs/documentation/server_admin/topics/threat/open-redirect.adoc +++ b/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. 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 1cdcbbb7879..447685e6d8b 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 @@ -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 resolveValidRedirects(KeycloakSession session, String rootUrl, Set validRedirects) { // If the valid redirect URI is relative (no scheme, host, port) then use the request's scheme, host, and port - Set resolveValidRedirects = new HashSet<>(); + // the set is ordered by length to get the longest match first + Set 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 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 validRedirects, String redirect, boolean allowWildcards) { + // return the String that matched the redirect or null if not matched + private static String matchesRedirects(Set 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 validRedirects) { 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 new file mode 100644 index 00000000000..ea23040bade --- /dev/null +++ b/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; + +/** + *

Little test class for RedirectUtils methods.

+ * + * @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 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 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 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)); + } +} diff --git a/services/src/test/java/org/keycloak/protocol/oidc/utils/ResteasyTestProvider.java b/services/src/test/java/org/keycloak/protocol/oidc/utils/ResteasyTestProvider.java new file mode 100644 index 00000000000..5d4f5d705ee --- /dev/null +++ b/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; + +/** + *

Resteasy provider to be used for the utils class.

+ * @author rmartinc + */ +public class ResteasyTestProvider implements ResteasyProvider { + + @Override + public R getContextData(Class 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() { + } +} diff --git a/services/src/test/resources/META-INF/services/org.keycloak.common.util.ResteasyProvider b/services/src/test/resources/META-INF/services/org.keycloak.common.util.ResteasyProvider new file mode 100644 index 00000000000..d8ffb8a92f0 --- /dev/null +++ b/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 diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java index c70d8d5ee80..f9f835091f2 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java +++ b/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 { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java index d9e18c73113..c63f02efd04 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java +++ b/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")); + } + } }