From ec8109112e67208c13e13f6d1f8706a5a3ba8d4c Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Wed, 1 Mar 2023 19:35:24 +0100 Subject: [PATCH] Make sure the code is bound to the user session (#18) (#17380) Co-authored-by: Pedro Igor --- .../protocol/oidc/OIDCLoginProtocol.java | 3 +- .../protocol/oidc/utils/OAuth2Code.java | 12 ++++++-- .../protocol/oidc/utils/OAuth2CodeParser.java | 11 +++++-- .../org/keycloak/testsuite/forms/SSOTest.java | 30 +++++++++++++++++++ 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java index db98a183bf9..8bf7deacf4e 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java @@ -241,7 +241,8 @@ public Response authenticated(AuthenticationSessionModel authSession, UserSessio authSession.getClientNote(OAuth2Constants.SCOPE), authSession.getClientNote(OIDCLoginProtocol.REDIRECT_URI_PARAM), authSession.getClientNote(OIDCLoginProtocol.CODE_CHALLENGE_PARAM), - authSession.getClientNote(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM)); + authSession.getClientNote(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM), + userSession.getId()); code = OAuth2CodeParser.persistCode(session, clientSession, codeData); redirectUri.addParam(OAuth2Constants.CODE, code); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2Code.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2Code.java index 83050aaec15..36ac2870cd9 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2Code.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2Code.java @@ -38,6 +38,7 @@ public class OAuth2Code { private static final String REDIRECT_URI_PARAM_NOTE = "redirectUri"; private static final String CODE_CHALLENGE_NOTE = "code_challenge"; private static final String CODE_CHALLENGE_METHOD_NOTE = "code_challenge_method"; + private static final String USER_SESSION_ID_NOTE = "user_session_id"; private final String id; @@ -52,10 +53,10 @@ public class OAuth2Code { private final String codeChallenge; private final String codeChallengeMethod; - + private final String userSessionId; public OAuth2Code(String id, int expiration, String nonce, String scope, String redirectUriParam, - String codeChallenge, String codeChallengeMethod) { + String codeChallenge, String codeChallengeMethod, String userSessionId) { this.id = id; this.expiration = expiration; this.nonce = nonce; @@ -63,6 +64,7 @@ public OAuth2Code(String id, int expiration, String nonce, String scope, String this.redirectUriParam = redirectUriParam; this.codeChallenge = codeChallenge; this.codeChallengeMethod = codeChallengeMethod; + this.userSessionId = userSessionId; } @@ -74,6 +76,7 @@ private OAuth2Code(Map data) { redirectUriParam = data.get(REDIRECT_URI_PARAM_NOTE); codeChallenge = data.get(CODE_CHALLENGE_NOTE); codeChallengeMethod = data.get(CODE_CHALLENGE_METHOD_NOTE); + userSessionId = data.get(USER_SESSION_ID_NOTE); } @@ -92,6 +95,7 @@ public Map serializeCode() { result.put(REDIRECT_URI_PARAM_NOTE, redirectUriParam); result.put(CODE_CHALLENGE_NOTE, codeChallenge); result.put(CODE_CHALLENGE_METHOD_NOTE, codeChallengeMethod); + result.put(USER_SESSION_ID_NOTE, userSessionId); return result; } @@ -124,4 +128,8 @@ public String getCodeChallenge() { public String getCodeChallengeMethod() { return codeChallengeMethod; } + + public String getUserSessionId() { + return userSessionId; + } } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2CodeParser.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2CodeParser.java index 1c4c59daed5..4a747d90e13 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2CodeParser.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/OAuth2CodeParser.java @@ -121,16 +121,23 @@ public static ParseResult parseCode(KeycloakSession session, String code, RealmM return result.illegalCode(); } - logger.tracef("Successfully verified code '%s'. User session: '%s', client: '%s'", codeUUID, userSessionId, clientUUID); - result.codeData = OAuth2Code.deserializeCode(codeData); + String persistedUserSessionId = result.codeData.getUserSessionId(); + + if (!userSessionId.equals(persistedUserSessionId)) { + logger.warnf("Code '%s' is bound to a different session", codeUUID); + return result.illegalCode(); + } + // Finally doublecheck if code is not expired int currentTime = Time.currentTime(); if (currentTime > result.codeData.getExpiration()) { return result.expiredCode(); } + logger.tracef("Successfully verified code '%s'. User session: '%s', client: '%s'", codeUUID, userSessionId, clientUUID); + return result; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/SSOTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/SSOTest.java index 35a274f106d..ca81f763c4d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/SSOTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/SSOTest.java @@ -16,6 +16,7 @@ */ package org.keycloak.testsuite.forms; +import org.apache.http.impl.client.CloseableHttpClient; import org.jboss.arquillian.drone.api.annotation.Drone; import org.jboss.arquillian.graphene.page.Page; import org.junit.Assert; @@ -39,6 +40,7 @@ import org.keycloak.testsuite.pages.AppPage.RequestType; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; +import org.keycloak.testsuite.util.MutualTLSUtils; import org.keycloak.testsuite.util.OAuthClient; import org.openqa.selenium.WebDriver; @@ -46,6 +48,9 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; +import java.io.IOException; +import javax.ws.rs.core.Response; + /** * @author Stian Thorgersen * @author Stan Silvert ssilvert@redhat.com (C) 2016 Red Hat Inc. @@ -209,4 +214,29 @@ public void loginWithRequiredActionAddedInTheMeantime() { } + @Test + public void failIfUsingCodeFromADifferentSession() throws IOException { + // first client user login + oauth.openLoginForm(); + oauth.doLogin("test-user@localhost", "password"); + String firstCode = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + + // second client user login + OAuthClient oauth2 = new OAuthClient(); + oauth2.init(driver2); + oauth2.doLogin("john-doh@localhost", "password"); + String secondCode = oauth2.getCurrentQuery().get(OAuth2Constants.CODE); + String[] firstCodeParts = firstCode.split("\\."); + String[] secondCodeParts = secondCode.split("\\."); + secondCodeParts[1] = firstCodeParts[1]; + secondCode = String.join(".", secondCodeParts); + + OAuthClient.AccessTokenResponse tokenResponse; + + try (CloseableHttpClient client = MutualTLSUtils.newCloseableHttpClientWithOtherKeyStoreAndTrustStore()) { + tokenResponse = oauth2.doAccessTokenRequest(secondCode, "password", client); + } + + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), tokenResponse.getStatusCode()); + } }