From 5fa5485ef246516016c197ed2951f8e3ca2ca396 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 13 Dec 2023 11:38:55 +0100 Subject: [PATCH] Recovery codes modifications to not tamper sent values Closes #26104 Closes #26105 Signed-off-by: rmartinc --- .../RecoveryAuthnCodesFormAuthenticator.java | 3 + .../RecoveryAuthnCodesAction.java | 24 ++++--- .../FreeMarkerLoginFormsProvider.java | 7 +- .../pages/SetupRecoveryAuthnCodesPage.java | 27 ++++++- .../RecoveryAuthnCodesAuthenticatorTest.java | 72 +++++++++++++++++-- 5 files changed, 119 insertions(+), 14 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java index ac90479a8040..bf47a6d9ddc8 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/RecoveryAuthnCodesFormAuthenticator.java @@ -27,6 +27,9 @@ public class RecoveryAuthnCodesFormAuthenticator implements Authenticator { + public static final String GENERATED_RECOVERY_AUTHN_CODES_NOTE = "RecoveryAuthnCodes.generatedRecoveryAuthnCodes"; + public static final String GENERATED_AT_NOTE = "RecoveryAuthnCodes.generatedAt"; + public RecoveryAuthnCodesFormAuthenticator(KeycloakSession keycloakSession) { } diff --git a/services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java b/services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java index b133293b9b07..cbac962136fe 100644 --- a/services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java +++ b/services/src/main/java/org/keycloak/authentication/requiredactions/RecoveryAuthnCodesAction.java @@ -1,6 +1,5 @@ package org.keycloak.authentication.requiredactions; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import org.keycloak.Config; @@ -10,6 +9,7 @@ import org.keycloak.authentication.RequiredActionContext; import org.keycloak.authentication.RequiredActionFactory; import org.keycloak.authentication.RequiredActionProvider; +import org.keycloak.authentication.authenticators.browser.RecoveryAuthnCodesFormAuthenticator; import org.keycloak.common.Profile; import org.keycloak.credential.RecoveryAuthnCodesCredentialProviderFactory; import org.keycloak.credential.CredentialProvider; @@ -89,8 +89,6 @@ public void processAction(RequiredActionContext reqActionContext) { CredentialProvider recoveryCodeCredentialProvider; MultivaluedMap httpReqParamsMap; - Long generatedAtTime; - String generatedUserLabel; recoveryCodeCredentialProvider = reqActionContext.getSession().getProvider(CredentialProvider.class, RecoveryAuthnCodesCredentialProviderFactory.PROVIDER_ID); @@ -98,12 +96,22 @@ public void processAction(RequiredActionContext reqActionContext) { event.detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE); httpReqParamsMap = reqActionContext.getHttpRequest().getDecodedFormParameters(); - List generatedCodes = new ArrayList<>( - Arrays.asList(httpReqParamsMap.getFirst(FIELD_GENERATED_RECOVERY_AUTHN_CODES_HIDDEN).split(","))); - generatedAtTime = Long.parseLong(httpReqParamsMap.getFirst(FIELD_GENERATED_AT_HIDDEN)); - generatedUserLabel = httpReqParamsMap.getFirst(FIELD_USER_LABEL_HIDDEN); + final String generatedCodesString = httpReqParamsMap.getFirst(FIELD_GENERATED_RECOVERY_AUTHN_CODES_HIDDEN); + final String generatedAtTimeString = httpReqParamsMap.getFirst(FIELD_GENERATED_AT_HIDDEN); + final String generatedUserLabel = httpReqParamsMap.getFirst(FIELD_USER_LABEL_HIDDEN); + + if (!generatedAtTimeString.equals(reqActionContext.getAuthenticationSession().getAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE)) + || !generatedCodesString.equals(reqActionContext.getAuthenticationSession().getAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE))) { + // authn codes have been tampered, sent them again + requiredActionChallenge(reqActionContext); + return; + } + + reqActionContext.getAuthenticationSession().removeAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE); + reqActionContext.getAuthenticationSession().removeAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE); - RecoveryAuthnCodesCredentialModel credentialModel = createFromValues(generatedCodes, generatedAtTime, generatedUserLabel); + List generatedCodes = Arrays.asList(generatedCodesString.split(",")); + RecoveryAuthnCodesCredentialModel credentialModel = createFromValues(generatedCodes, Long.valueOf(generatedAtTimeString), generatedUserLabel); if ("on".equals(httpReqParamsMap.getFirst("logout-sessions"))) { AuthenticatorUtil.logoutOtherSessions(reqActionContext); diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java b/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java index d238d73901c0..77e7c207ca54 100755 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java +++ b/services/src/main/java/org/keycloak/forms/login/freemarker/FreeMarkerLoginFormsProvider.java @@ -27,6 +27,7 @@ import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.authentication.authenticators.browser.AbstractUsernameFormAuthenticator; import org.keycloak.authentication.authenticators.browser.OTPFormAuthenticator; +import org.keycloak.authentication.authenticators.browser.RecoveryAuthnCodesFormAuthenticator; import org.keycloak.authentication.forms.RegistrationPage; import org.keycloak.authentication.requiredactions.util.UpdateProfileContext; import org.keycloak.authentication.requiredactions.util.UserUpdateProfileContext; @@ -261,7 +262,11 @@ protected Response createResponse(LoginFormsPages page) { attributes.put("totp", totpBean); break; case LOGIN_RECOVERY_AUTHN_CODES_CONFIG: - attributes.put("recoveryAuthnCodesConfigBean", new RecoveryAuthnCodesBean()); + // generate the recovery codes and assign to the auth session + RecoveryAuthnCodesBean recoveryAuthnCodesBean = new RecoveryAuthnCodesBean(); + attributes.put("recoveryAuthnCodesConfigBean", recoveryAuthnCodesBean); + authenticationSession.setAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_RECOVERY_AUTHN_CODES_NOTE, recoveryAuthnCodesBean.getGeneratedRecoveryAuthnCodesAsString()); + authenticationSession.setAuthNote(RecoveryAuthnCodesFormAuthenticator.GENERATED_AT_NOTE, Long.toString(recoveryAuthnCodesBean.getGeneratedAt())); break; case LOGIN_RECOVERY_AUTHN_CODES_INPUT: attributes.put("recoveryAuthnCodesInputBean", new RecoveryAuthnCodeInputLoginBean(session, realm, user)); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java index 667dc7cbc557..f96d9265f418 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/SetupRecoveryAuthnCodesPage.java @@ -2,6 +2,7 @@ import org.keycloak.testsuite.util.UIUtils; import org.openqa.selenium.By; +import org.openqa.selenium.JavascriptExecutor; import org.openqa.selenium.NoSuchElementException; import org.openqa.selenium.WebElement; import org.openqa.selenium.support.FindBy; @@ -18,14 +19,38 @@ public class SetupRecoveryAuthnCodesPage extends LogoutSessionsPage { @FindBy(id = "saveRecoveryAuthnCodesBtn") private WebElement saveRecoveryAuthnCodesButton; - @FindBy(id="kcRecoveryCodesConfirmationCheck") + @FindBy(id = "kcRecoveryCodesConfirmationCheck") private WebElement kcRecoveryCodesConfirmationCheck; + @FindBy(name = "generatedRecoveryAuthnCodes") + private WebElement generatedRecoveryAuthnCodesHidden; + + @FindBy(name = "generatedAt") + private WebElement generatedAtHidden; + public void clickSaveRecoveryAuthnCodesButton() { UIUtils.switchCheckbox(kcRecoveryCodesConfirmationCheck, true); UIUtils.clickLink(saveRecoveryAuthnCodesButton); } + public String getGeneratedRecoveryAuthnCodesHidden() { + return generatedRecoveryAuthnCodesHidden.getAttribute("value"); + } + + public void setGeneratedRecoveryAuthnCodesHidden(String codes) { + final JavascriptExecutor js = (JavascriptExecutor) driver; + js.executeScript("document.getElementsByName('generatedRecoveryAuthnCodes')[0].value='" + codes + "'"); + } + + public String getGeneratedAtHidden() { + return generatedAtHidden.getAttribute("value"); + } + + public void setGeneratedAtHidden(String at) { + final JavascriptExecutor js = (JavascriptExecutor) driver; + js.executeScript("document.getElementsByName('generatedAt')[0].value='" + at + "'"); + } + public List getRecoveryAuthnCodes() { String recoveryAuthnCodesText = recoveryAuthnCodesList.getText(); List recoveryAuthnCodesList = new ArrayList<>(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RecoveryAuthnCodesAuthenticatorTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RecoveryAuthnCodesAuthenticatorTest.java index 7eb139dfacfd..5fdb1914ad1b 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RecoveryAuthnCodesAuthenticatorTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RecoveryAuthnCodesAuthenticatorTest.java @@ -17,6 +17,7 @@ import org.keycloak.credential.CredentialModel; import org.keycloak.events.Details; import org.keycloak.events.EventType; +import org.keycloak.forms.login.freemarker.model.RecoveryAuthnCodesBean; import org.keycloak.models.AuthenticationExecutionModel; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; @@ -185,9 +186,73 @@ public void test02SetupRecoveryAuthnCodesLogoutOtherSessionsNotChecked() { testSetupRecoveryAuthnCodesLogoutOtherSessions(false); } + @Test + public void test03SetupRecoveryAuthnCodesModifyGeneratedAt() { + // add the configure recovery codes action + UserResource testUser = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost"); + UserRepresentation userRepresentation = testUser.toRepresentation(); + userRepresentation.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name())); + testUser.update(userRepresentation); + + oauth.openLoginForm(); + loginPage.assertCurrent(); + loginPage.login("test-user@localhost", "password"); + setupRecoveryAuthnCodesPage.assertCurrent(); + + // modify generatedAt to a fixed value + setupRecoveryAuthnCodesPage.setGeneratedAtHidden("10000"); + setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton(); + + // the recovery codes are regerated as they were tampered + setupRecoveryAuthnCodesPage.assertCurrent(); + setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton(); + + assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); + EventRepresentation event = events.expectRequiredAction(EventType.UPDATE_CREDENTIAL) + .user(userRepresentation.getId()) + .detail(Details.USERNAME, "test-user@localhost") + .detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE) + .assertEvent(); + events.expectLogin().user(event.getUserId()).session(event.getDetails().get(Details.CODE_ID)) + .detail(Details.USERNAME, "test-user@localhost") + .assertEvent(); + } + + @Test + public void test04SetupRecoveryAuthnCodesModifyGeneratedCodes() { + // add the configure recovery codes action + UserResource testUser = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost"); + UserRepresentation userRepresentation = testUser.toRepresentation(); + userRepresentation.setRequiredActions(Arrays.asList(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name())); + testUser.update(userRepresentation); + + oauth.openLoginForm(); + loginPage.assertCurrent(); + loginPage.login("test-user@localhost", "password"); + setupRecoveryAuthnCodesPage.assertCurrent(); + + // modify the codes with a new generated ones + setupRecoveryAuthnCodesPage.setGeneratedRecoveryAuthnCodesHidden(new RecoveryAuthnCodesBean().getGeneratedRecoveryAuthnCodesAsString()); + setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton(); + + // the recovery codes are regerated as they were tampered + setupRecoveryAuthnCodesPage.assertCurrent(); + setupRecoveryAuthnCodesPage.clickSaveRecoveryAuthnCodesButton(); + + assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); + EventRepresentation event = events.expectRequiredAction(EventType.UPDATE_CREDENTIAL) + .user(userRepresentation.getId()) + .detail(Details.USERNAME, "test-user@localhost") + .detail(Details.CREDENTIAL_TYPE, RecoveryAuthnCodesCredentialModel.TYPE) + .assertEvent(); + events.expectLogin().user(event.getUserId()).session(event.getDetails().get(Details.CODE_ID)) + .detail(Details.USERNAME, "test-user@localhost") + .assertEvent(); + } + // In a sub-flow with alternative credential executors, test whether Recovery Authentication Codes are working @Test - public void test03AuthenticateRecoveryAuthnCodes() { + public void test05AuthenticateRecoveryAuthnCodes() { try { configureBrowserFlowWithRecoveryAuthnCodes(testingClient); testRealm().flows().removeRequiredAction(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()); @@ -231,7 +296,7 @@ public void test03AuthenticateRecoveryAuthnCodes() { @Test @IgnoreBrowserDriver(FirefoxDriver.class) @IgnoreBrowserDriver(ChromeDriver.class) - public void test04SetupRecoveryAuthnCodes() { + public void test06SetupRecoveryAuthnCodes() { try { configureBrowserFlowWithRecoveryAuthnCodes(testingClient); RequiredActionProviderSimpleRepresentation simpleRepresentation = new RequiredActionProviderSimpleRepresentation(); @@ -262,7 +327,7 @@ public void test04SetupRecoveryAuthnCodes() { @Test @IgnoreBrowserDriver(FirefoxDriver.class) // TODO: https://github.com/keycloak/keycloak/issues/13543 @IgnoreBrowserDriver(ChromeDriver.class) - public void test05BruteforceProtectionRecoveryAuthnCodes() { + public void test07BruteforceProtectionRecoveryAuthnCodes() { try { configureBrowserFlowWithRecoveryAuthnCodes(testingClient); RealmRepresentation rep = testRealm().toRepresentation(); @@ -314,5 +379,4 @@ public void test05BruteforceProtectionRecoveryAuthnCodes() { BrowserFlowTest.revertFlows(testRealm(), BROWSER_FLOW_WITH_RECOVERY_AUTHN_CODES); } } - }