Escape action in the form_post response mode (keycloak#60)

rmartinc · abstractj · commit 2ba7a51da60d · 2023-12-18T18:10:41.000-03:00
Closes keycloak/keycloak-private#31 Closes https://issues.redhat.com/browse/RHBK-652 Signed-off-by: rmartinc <rmartinc@redhat.com>
diff --git a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java
@@ -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>");
 
diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java
@@ -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=\"")
diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java
@@ -1398,6 +1398,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);
     }
diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/SamlClient.java
@@ -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.
      *
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
@@ -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());
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
@@ -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');"));
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -1398,6 +1398,9 @@ public String getCurrentRequest() {`
`1398`	`1398`	`int index = driver.getCurrentUrl().indexOf('?');`
`1399`	`1399`	`if (index == -1) {`
`1400`	`1400`	`index = driver.getCurrentUrl().indexOf('#');`
	`1401`	`+ if (index == -1) {`
	`1402`	`+ index = driver.getCurrentUrl().length();`
	`1403`	`+ }`
`1401`	`1404`	`}`
`1402`	`1405`	`return driver.getCurrentUrl().substring(0, index);`
`1403`	`1406`	`}`