keycloak · stianst · Dec 6, 2023 · Dec 6, 2023
@@ -381,7 +381,7 @@ public String buildHtmlForm(String actionUrl, Map<String, String> inputTypes) {
                 .append("</HEAD>")
                 .append("<BODY Onload=\"document.forms[0].submit()\">")
 
-                .append("<FORM METHOD=\"POST\" ACTION=\"").append(actionUrl).append("\">");
+                .append("<FORM METHOD=\"POST\" ACTION=\"").append(escapeAttribute(actionUrl)).append("\">");
 
         builder.append("<p>Redirecting, please wait.</p>");
 

@@ -160,7 +160,9 @@ public Response build() {
             builder.append("  </HEAD>");
             builder.append("  <BODY Onload=\"document.forms[0].submit()\">");
 
-            builder.append("    <FORM METHOD=\"POST\" ACTION=\"" + redirectUri.toString() + "\">");
+            builder.append("    <FORM METHOD=\"POST\" ACTION=\"")
+                    .append(HtmlUtils.escapeAttribute(redirectUri.toString()))
+                    .append("\">");
 
             for (Map.Entry<String, String> param : params.entrySet()) {
                 builder.append("  <INPUT TYPE=\"HIDDEN\" NAME=\"")

@@ -248,13 +248,24 @@ 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 query component - we don't check them when wildcards are effective
-                String r = redirect.contains("?") ? redirect.substring(0, redirect.indexOf("?")) : redirect;
+                // strip off the userInfo, query or fragment components - we don't check them when wildcards are effective
+                String r = stripOffRedirectForWildcard(redirect);
                 // strip off *
                 int length = validRedirect.length() - 1;
                 validRedirect = validRedirect.substring(0, length);

@@ -174,4 +174,24 @@ public void testRelativeRedirectUri() {
         Assert.assertEquals("https://keycloak.org/path", RedirectUtils.verifyRedirectUri(session, "https://keycloak.org", "path", set, false));
     }
 
+    @Test
+    public void testUserInfo() {
+        Set<String> set = Stream.of(
+                "https://keycloak.org/*",
+                "https://test*",
+                "https://[email protected]/exact"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("https://keycloak.org/index.html", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/index.html", set, false));
+        Assert.assertEquals("https://test.com/index.html", RedirectUtils.verifyRedirectUri(session, null, "https://test.com/index.html", set, false));
+        Assert.assertEquals("https://[email protected]/path", RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]/path", set, false));
+        Assert.assertEquals("https://some%[email protected]/path", RedirectUtils.verifyRedirectUri(session, null, "https://some%[email protected]/path", set, false));
+        Assert.assertEquals("https://[email protected]/exact", RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]/exact", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]/", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org%[email protected]", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://[email protected]", set, false));
+    }
 }
@@ -1394,6 +1394,9 @@ public String getCurrentRequest() {
         int index = driver.getCurrentUrl().indexOf('?');
         if (index == -1) {
             index = driver.getCurrentUrl().indexOf('#');
+            if (index == -1) {
+                index = driver.getCurrentUrl().length();
+            }
         }
         return driver.getCurrentUrl().substring(0, index);
     }

@@ -639,6 +639,20 @@ public static SAMLDocumentHolder extractSamlResponseFromForm(String responsePage
         return SAMLRequestParser.parseResponsePostBinding(respElement.val());
     }
 
+    /**
+     * Extracts the form element from a Post binding.
+     *
+     * @param responsePage HTML code in the page
+     * @return The element that is the form
+     */
+    public static Element extractFormFromPostResponse(String responsePage) {
+        org.jsoup.nodes.Document theResponsePage = Jsoup.parse(responsePage);
+        Elements form = theResponsePage.select("form");
+        assertThat("Checking uniqueness of SAMLResponse/SAMLRequest form in Post binding", form.size(), is(1));
+
+        return form.first();
+    }
+
     /**
      * Extracts and parses value of RelayState input field of a form present in the given page.
      *

@@ -16,6 +16,8 @@
  */
 package org.keycloak.testsuite.oauth;
 
+import org.hamcrest.MatcherAssert;
+import org.hamcrest.Matchers;
 import org.jboss.arquillian.graphene.page.Page;
 import org.junit.Assert;
 import org.junit.Before;
@@ -25,6 +27,7 @@
 import org.keycloak.OAuthErrorException;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
+import org.keycloak.events.EventType;
 import org.keycloak.models.Constants;
 import org.keycloak.protocol.oidc.utils.OIDCResponseMode;
 import org.keycloak.representations.idm.RealmRepresentation;
@@ -35,6 +38,7 @@
 import org.keycloak.testsuite.updaters.ClientAttributeUpdater;
 import org.keycloak.testsuite.util.ClientManager;
 import org.keycloak.testsuite.util.OAuthClient;
+import org.keycloak.testsuite.util.WaitUtils;
 import org.openqa.selenium.By;
 
 import jakarta.ws.rs.core.UriBuilder;
@@ -249,6 +253,28 @@ public void authorizationRequestFormPostResponseModeInvalidRedirectUri() throws
         }
     }
 
+    @Test
+    public void authorizationRequestFormPostResponseModeHTMLEntitiesRedirectUri() 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("/test?p=&gt;"); // set HTML entity &gt;
+            oauth.doLogin("test-user@localhost", "password");
+
+            WaitUtils.waitForPageToLoad();
+            // if not properly encoded %3E would be received instead of &gt;
+            MatcherAssert.assertThat("Redirect page was not encoded", oauth.getDriver().getCurrentUrl(), Matchers.endsWith("/test?p=&gt;"));
+
+            events.expect(EventType.LOGIN)
+                    .user(AssertEvents.isUUID())
+                    .session(AssertEvents.isUUID())
+                    .detail(Details.USERNAME, "test-user@localhost")
+                    .assertEvent();
+        }
+    }
+
     @Test
     public void authorizationRequestFormPostResponseModeWithCustomState() throws IOException {
         oauth.responseMode(OIDCResponseMode.FORM_POST.value());

@@ -50,6 +50,7 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.matchesRegex;
 import static org.keycloak.saml.common.constants.JBossSAMLURIConstants.NAMEID_FORMAT_TRANSIENT;
@@ -345,4 +346,28 @@ public void testInvalidAssertionConsumerServiceURL() throws IOException {
             assertThat(page, containsString("Invalid redirect uri"));
         }
     }
+
+    @Test
+    public void testConsumerServiceURLHtmlEntities() throws IOException {
+        try (var c = ClientAttributeUpdater.forClient(adminClient, REALM_NAME, SAML_CLIENT_ID_SALES_POST)
+                .setRedirectUris(Collections.singletonList("*"))
+                .update()) {
+
+            String action = new SamlClientBuilder()
+                    .authnRequest(getAuthServerSamlEndpoint(REALM_NAME), SAML_CLIENT_ID_SALES_POST, "javascript&colon;alert('xss');", Binding.POST)
+                    .build()
+                    .login().user(bburkeUser).build()
+                    .executeAndTransform(response -> {
+                        assertThat(response, statusCodeIsHC(Response.Status.OK));
+                        String responsePage = EntityUtils.toString(response.getEntity(), "UTF-8");
+                        return SamlClient.extractFormFromPostResponse(responsePage)
+                                .attributes().asList().stream()
+                                .filter(a -> "action".equalsIgnoreCase(a.getKey()))
+                                .map(org.jsoup.nodes.Attribute::getValue)
+                                .findAny().orElse(null);
+                    });
+            // if not encoded properly jsoup returns ":" instead of "&colon;"
+            assertThat(action, endsWith("javascript&colon;alert('xss');"));
+        }
+    }
 }