From d9025231f9fb32dfa37858a7da9b7cd8085e5042 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Wed, 1 Feb 2023 14:34:15 +0100 Subject: [PATCH] HTML Injection in Keycloak Admin REST API (#16765) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves #GHSA-m4fv-gm5m-4725 Co-authored-by: Martin Bartoš --- .../ExecuteActionsActionTokenHandler.java | 12 +++++- .../util/RequiredActionsValidator.java | 43 +++++++++++++++++++ .../keycloak/services/messages/Messages.java | 2 + .../resources/admin/UserResource.java | 11 ++++- .../RequiredActionEmailVerificationTest.java | 30 +++++++++++++ .../keycloak/testsuite/admin/UserTest.java | 20 ++++++++- .../login/messages/messages_cs.properties | 2 + .../main/resources/theme/base/login/info.ftl | 2 +- .../login/messages/messages_en.properties | 2 + 9 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 services/src/main/java/org/keycloak/authentication/requiredactions/util/RequiredActionsValidator.java diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java index 9c01546170..a81879c40e 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java @@ -16,10 +16,12 @@ */ package org.keycloak.authentication.actiontoken.execactions; +import org.keycloak.TokenVerifier; import org.keycloak.TokenVerifier.Predicate; import org.keycloak.authentication.RequiredActionFactory; import org.keycloak.authentication.RequiredActionProvider; import org.keycloak.authentication.actiontoken.*; +import org.keycloak.authentication.requiredactions.util.RequiredActionsValidator; import org.keycloak.events.Errors; import org.keycloak.events.EventType; import org.keycloak.forms.login.LoginFormsProvider; @@ -66,7 +68,8 @@ public class ExecuteActionsActionTokenHandler extends AbstractActionTokenHandler Messages.INVALID_REDIRECT_URI ), - verifyEmail(tokenContext) + verifyEmail(tokenContext), + verifyRequiredActions(tokenContext) ); } @@ -129,5 +132,10 @@ public class ExecuteActionsActionTokenHandler extends AbstractActionTokenHandler .noneMatch(RequiredActionFactory::isOneTimeAction); } - + // Verify required actions included in the token are valid + protected TokenVerifier.Predicate verifyRequiredActions(ActionTokenContext tokenContext) { + return TokenUtils.checkThat(t -> RequiredActionsValidator.validRequiredActions(tokenContext.getSession(), t.getRequiredActions()), + Errors.RESOLVE_REQUIRED_ACTIONS, Messages.INVALID_TOKEN_REQUIRED_ACTIONS + ); + } } diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/util/RequiredActionsValidator.java b/services/src/main/java/org/keycloak/authentication/requiredactions/util/RequiredActionsValidator.java new file mode 100644 index 0000000000..fbe871c3ac --- /dev/null +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/util/RequiredActionsValidator.java @@ -0,0 +1,43 @@ +/* + * Copyright 2022 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.authentication.requiredactions.util; + +import org.keycloak.authentication.RequiredActionProvider; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakSessionFactory; + +import java.util.List; + +public class RequiredActionsValidator { + /** + * Validate provided required actions + * + * @param session the {@code KeycloakSession} + * @param requiredActions IDs of tested required actions + */ + public static boolean validRequiredActions(KeycloakSession session, List requiredActions) { + final KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory(); + + for (String action : requiredActions) { + if (sessionFactory.getProviderFactory(RequiredActionProvider.class, action) == null) { + return false; + } + } + return true; + } +} diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java index 1b32b4b65d..50df65d3a1 100755 --- a/services/src/main/java/org/keycloak/services/messages/Messages.java +++ b/services/src/main/java/org/keycloak/services/messages/Messages.java @@ -219,6 +219,8 @@ public class Messages { public static final String MISSING_IDENTITY_PROVIDER = "missingIdentityProviderMessage"; + public static final String INVALID_TOKEN_REQUIRED_ACTIONS = "invalidTokenRequiredActions"; + public static final String INVALID_FEDERATED_IDENTITY_ACTION = "invalidFederatedIdentityActionMessage"; public static final String FEDERATED_IDENTITY_NOT_ACTIVE = "federatedIdentityLinkNotActiveMessage"; diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 12aca4bf42..4b4ad8b8ef 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -20,8 +20,10 @@ import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; import org.keycloak.authentication.RequiredActionProvider; import org.keycloak.authentication.actiontoken.execactions.ExecuteActionsActionToken; +import org.keycloak.authentication.requiredactions.util.RequiredActionsValidator; import org.keycloak.common.ClientConnection; import org.keycloak.common.Profile; +import org.keycloak.common.util.CollectionUtil; import org.keycloak.common.util.Time; import org.keycloak.credential.CredentialModel; import org.keycloak.email.EmailException; @@ -758,7 +760,7 @@ public class UserResource { /** - * Send a update account email to the user + * Send an email to the user with a link they can click to execute particular actions. * * An email contains a link the user can click to perform a set of required actions. * The redirectUri and clientId parameters are optional. If no redirect is given, then there will @@ -768,7 +770,7 @@ public class UserResource { * @param redirectUri Redirect uri * @param clientId Client id * @param lifespan Number of seconds after which the generated token expires - * @param actions required actions the user needs to complete + * @param actions Required actions the user needs to complete * @return */ @Path("execute-actions-email") @@ -798,6 +800,11 @@ public class UserResource { clientId = Constants.ACCOUNT_MANAGEMENT_CLIENT_ID; } + if (CollectionUtil.isNotEmpty(actions) && !RequiredActionsValidator.validRequiredActions(session, actions)) { + throw new WebApplicationException( + ErrorResponse.error("Provided invalid required actions", Status.BAD_REQUEST)); + } + ClientModel client = realm.getClientByClientId(clientId); if (client == null) { logger.debugf("Client %s doesn't exist", clientId); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java index dba8c58ca7..15805efaad 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java @@ -41,6 +41,7 @@ import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.AuthServerTestEnricher; import org.keycloak.testsuite.arquillian.annotation.DisableFeature; +import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.auth.page.AuthRealm; import org.keycloak.testsuite.cluster.AuthenticationSessionFailoverClusterTest; import org.keycloak.testsuite.pages.AppPage; @@ -66,6 +67,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.hamcrest.Matchers; @@ -74,9 +76,11 @@ import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; +import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; /** * @author Stian Thorgersen @@ -1027,4 +1031,30 @@ public class RequiredActionEmailVerificationTest extends AbstractTestRealmKeyclo errorPage.assertCurrent(); assertEquals("The link you clicked is an old stale link and is no longer valid. Maybe you have already verified your email.", errorPage.getError()); } + + @Test + @EnableFeature(value = Profile.Feature.UPDATE_EMAIL, skipRestart = true) + public void actionTokenWithInvalidRequiredActions() throws IOException { + // Send email with required action + testRealm().users().get(testUserId).executeActionsEmail(List.of(RequiredAction.UPDATE_EMAIL.name())); + + Assert.assertEquals(1, greenMail.getReceivedMessages().length); + MimeMessage message = greenMail.getLastReceivedMessage(); + + MailUtils.EmailBody body = MailUtils.getBody(message); + assertThat(body, notNullValue()); + + final String link = MailUtils.getLink(body.getText()); + assertThat(link, notNullValue()); + + // Disable feature and the required action UPDATE_EMAIL provider is not present + testingClient.disableFeature(Profile.Feature.UPDATE_EMAIL); + + driver.navigate().to(link); + + errorPage.assertCurrent(); + + // Required action included in the action token is not valid anymore, because we don't know the provider for it + assertThat(errorPage.getError(), is("Required actions included in the link are not valid")); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java index 908a2b82fa..34ed550f6f 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -1559,7 +1559,25 @@ public class UserTest extends AbstractAdminTest { userRep.setEnabled(true); updateUser(user, userRep); - user.executeActionsEmail("invalidClientId", "invalidUri", actions); + user.executeActionsEmail(Arrays.asList( + UserModel.RequiredAction.UPDATE_PASSWORD.name(), + "invalid\"") + ); + fail("Expected failure"); + } catch (ClientErrorException e) { + assertEquals(400, e.getResponse().getStatus()); + + ErrorRepresentation error = e.getResponse().readEntity(ErrorRepresentation.class); + Assert.assertEquals("Provided invalid required actions", error.getErrorMessage()); + assertAdminEvents.assertEmpty(); + } + + try { + user.executeActionsEmail( + "invalidClientId", + "invalidUri", + Collections.singletonList(UserModel.RequiredAction.UPDATE_PASSWORD.name()) + ); fail("Expected failure"); } catch (ClientErrorException e) { assertEquals(400, e.getResponse().getStatus()); diff --git a/themes/src/main/resources-community/theme/base/login/messages/messages_cs.properties b/themes/src/main/resources-community/theme/base/login/messages/messages_cs.properties index 7b8c7cc143..8a4a5e3362 100644 --- a/themes/src/main/resources-community/theme/base/login/messages/messages_cs.properties +++ b/themes/src/main/resources-community/theme/base/login/messages/messages_cs.properties @@ -341,6 +341,8 @@ requiredAction.UPDATE_PASSWORD=Aktualizace hesla requiredAction.UPDATE_PROFILE=Aktualizovat profil requiredAction.VERIFY_EMAIL=Ověřit e-mail +invalidTokenRequiredActions=Požadované akce obsažené v daném odkazu nejsou validní + doX509Login=Budete přihlášeni jako\: clientCertificate=Klientský X509 certifikát\: noCertificate=[Žádný certifikát] diff --git a/themes/src/main/resources/theme/base/login/info.ftl b/themes/src/main/resources/theme/base/login/info.ftl index 8da0cb7a21..400e8bfdff 100755 --- a/themes/src/main/resources/theme/base/login/info.ftl +++ b/themes/src/main/resources/theme/base/login/info.ftl @@ -8,7 +8,7 @@ <#elseif section = "form">
-

${message.summary}<#if requiredActions??><#list requiredActions>: <#items as reqActionItem>${msg("requiredAction.${reqActionItem}")}<#sep>, <#else>

+

${message.summary}<#if requiredActions??><#list requiredActions>: <#items as reqActionItem>${kcSanitize(msg("requiredAction.${reqActionItem}"))?no_esc}<#sep>, <#else>

<#if skipLink??> <#else> <#if pageRedirectUri?has_content> diff --git a/themes/src/main/resources/theme/base/login/messages/messages_en.properties b/themes/src/main/resources/theme/base/login/messages/messages_en.properties index 7a260a911a..29fad84d2c 100755 --- a/themes/src/main/resources/theme/base/login/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/login/messages/messages_en.properties @@ -394,6 +394,8 @@ requiredAction.VERIFY_EMAIL=Verify Email requiredAction.CONFIGURE_RECOVERY_AUTHN_CODES=Generate Recovery Codes requiredAction.webauthn-register-passwordless=Webauthn Register Passwordless +invalidTokenRequiredActions=Required actions included in the link are not valid + doX509Login=You will be logged in as\: clientCertificate=X509 client certificate\: noCertificate=[No Certificate]