From 0f2ca3bfddeeeece06cae4cbd1f7122adb261eed Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Wed, 14 Dec 2022 07:46:17 +0100 Subject: [PATCH] fixes from release/20 (#15982) * Avoid path traversal vis double-url encoding of redirect URI (#8) (cherry picked from commit a2128fb9e940d96c2f9a64edcd4fbcc768eedb4f) * Do not resolve user session if corresponding auth session does not exist (#7) * Stabilizing the ConcurrentLoginTest when running with JPA map storage by locking user sessions (#9) Co-authored-by: Marek Posolda Co-authored-by: Pedro Igor Co-authored-by: Alexander Schwartz --- .../oidc/endpoints/LogoutEndpoint.java | 8 +- .../protocol/oidc/utils/RedirectUtils.java | 39 ++++++ .../AuthenticationSessionManager.java | 14 +- .../managers/UserSessionCrossDCManager.java | 14 ++ .../services/resources/SessionCodeChecks.java | 15 +++ .../testsuite/oauth/OAuthRedirectUriTest.java | 17 ++- .../oauth/RPInitiatedLogoutTest.java | 12 +- .../testsuite/oauth/RefreshTokenTest.java | 123 ++++++++++++++++++ 8 files changed, 233 insertions(+), 9 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java index 14a2941c5b4..42582f35deb 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java @@ -370,6 +370,8 @@ public class LogoutEndpoint { session.getProvider(LoginFormsProvider.class).setAttribute(Constants.SKIP_LINK, true); } + event.error(Errors.SESSION_EXPIRED); + return ErrorPage.error(session, logoutSession, Response.Status.BAD_REQUEST, Messages.FAILED_LOGOUT); } @@ -405,6 +407,7 @@ public class LogoutEndpoint { AuthenticationManager.AuthResult authResult = AuthenticationManager.authenticateIdentityCookie(session, realm, false); if (authResult != null) { + event.error(Errors.LOGOUT_FAILED); return ErrorPage.error(session, logoutSession, Response.Status.BAD_REQUEST, Messages.FAILED_LOGOUT); } else { // Probably changing locale on logout screen after logout was already performed. If there is no session in the browser, we can just display that logout was already finished @@ -431,7 +434,10 @@ public class LogoutEndpoint { try { userSession = lockUserSessionsForModification(session, () -> session.sessions().getUserSession(realm, userSessionIdFromIdToken)); - if (userSession != null) { + if (userSession == null) { + event.event(EventType.LOGOUT); + event.error(Errors.SESSION_EXPIRED); + } else { Integer idTokenIssuedAt = Integer.parseInt(idTokenIssuedAtStr); checkTokenIssuedAt(idTokenIssuedAt, userSession); } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java index 01d6cc4c0d5..9cb314b2e48 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java @@ -18,6 +18,8 @@ package org.keycloak.protocol.oidc.utils; import org.jboss.logging.Logger; +import org.keycloak.common.util.Encode; +import org.keycloak.common.util.KeycloakUriBuilder; import org.keycloak.common.util.UriUtils; import org.keycloak.models.ClientModel; import org.keycloak.models.Constants; @@ -91,6 +93,7 @@ public class RedirectUtils { KeycloakUriInfo uriInfo = session.getContext().getUri(); RealmModel realm = session.getContext().getRealm(); + redirectUri = decodeRedirectUri(redirectUri); if (redirectUri != null) { try { URI uri = URI.create(redirectUri); @@ -152,6 +155,42 @@ public class RedirectUtils { } } + // Decode redirectUri. We don't decode query and fragment as those can be encoded in the original URL. + // URL can be decoded multiple times (in case it was encoded multiple times, or some of it's parts were encoded multiple times) + private static String decodeRedirectUri(String redirectUri) { + if (redirectUri == null) return null; + int MAX_DECODING_COUNT = 5; // Max count of attempts for decoding URL (in case it was encoded multiple times) + + try { + KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri); + String origQuery = uriBuilder.getQuery(); + String origFragment = uriBuilder.getFragment(); + String encodedRedirectUri = uriBuilder + .replaceQuery(null) + .fragment(null) + .buildAsString(); + String decodedRedirectUri = null; + + for (int i = 0; i < MAX_DECODING_COUNT; i++) { + decodedRedirectUri = Encode.decode(encodedRedirectUri); + if (decodedRedirectUri.equals(encodedRedirectUri)) { + // URL is decoded. We can return it (after attach original query and fragment) + return KeycloakUriBuilder.fromUri(decodedRedirectUri) + .replaceQuery(origQuery) + .fragment(origFragment) + .buildAsString(); + } else { + // Next attempt + encodedRedirectUri = decodedRedirectUri; + } + } + } catch (IllegalArgumentException iae) { + logger.debugf("Illegal redirect URI used: %s, Details: %s", redirectUri, iae.getMessage()); + } + logger.debugf("Was not able to decode redirect URI: %s", redirectUri); + return null; + } + private static String lowerCaseHostname(String redirectUri) { int n = redirectUri.indexOf('/', 7); if (n == -1) { diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java index fc07aaf14ff..f815259185e 100644 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java @@ -34,6 +34,7 @@ import javax.ws.rs.core.UriInfo; import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import static org.keycloak.utils.LockObjectsForModification.lockUserSessionsForModification; @@ -196,7 +197,18 @@ public class AuthenticationSessionManager { log.debugf("Not found AUTH_SESSION_ID cookie"); } - return authSessionIds; + return authSessionIds.stream().filter(new Predicate() { + @Override + public boolean test(String id) { + StickySessionEncoderProvider encoder = session.getProvider(StickySessionEncoderProvider.class); + // in case the id is encoded with a route when running in a cluster + String decodedId = encoder.decodeSessionId(cookiesVal.iterator().next()); + // we can't blindly trust the cookie and assume it is valid and referencing a valid root auth session + // but make sure the root authentication session actually exists + // without this check there is a risk of resolving user sessions from invalid root authentication sessions as they share the same id + return session.authenticationSessions().getRootAuthenticationSession(realm, decodedId) != null; + } + }).collect(Collectors.toList()); } diff --git a/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java b/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java index 9e3eda42be2..6d6cacbdda6 100644 --- a/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java +++ b/services/src/main/java/org/keycloak/services/managers/UserSessionCrossDCManager.java @@ -17,6 +17,7 @@ package org.keycloak.services.managers; +import java.util.Collections; import java.util.List; import java.util.Objects; @@ -26,6 +27,7 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserSessionModel; +import static org.keycloak.services.managers.AuthenticationManager.authenticateIdentityCookie; import static org.keycloak.utils.LockObjectsForModification.lockUserSessionsForModification; /** @@ -65,6 +67,18 @@ public class UserSessionCrossDCManager { public UserSessionModel getUserSessionIfExistsRemotely(AuthenticationSessionManager asm, RealmModel realm) { List sessionCookies = asm.getAuthSessionCookies(realm); + if (sessionCookies.isEmpty()) { + // ideally, we should not rely on auth session id to retrieve user sessions + // in case the auth session was removed, we fall back to the identity cookie + // we are here doing the user session lookup twice, however the second lookup is going to make sure the + // session exists in remote caches + AuthenticationManager.AuthResult authResult = lockUserSessionsForModification(kcSession, () -> authenticateIdentityCookie(kcSession, realm, true)); + + if (authResult != null && authResult.getSession() != null) { + sessionCookies = Collections.singletonList(authResult.getSession().getId()); + } + } + return sessionCookies.stream().map(oldEncodedId -> { AuthSessionId authSessionId = asm.decodeAuthSessionId(oldEncodedId); String sessionId = authSessionId.getDecodedId(); diff --git a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java index e7e6c82c732..999de87625a 100644 --- a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java +++ b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java @@ -17,6 +17,10 @@ package org.keycloak.services.resources; +import static org.keycloak.services.managers.AuthenticationManager.KEYCLOAK_IDENTITY_COOKIE; +import static org.keycloak.services.managers.AuthenticationManager.authenticateIdentityCookie; +import static org.keycloak.utils.LockObjectsForModification.lockUserSessionsForModification; + import java.net.URI; import javax.ws.rs.core.Cookie; @@ -42,11 +46,13 @@ import org.keycloak.protocol.AuthorizationEndpointBase; import org.keycloak.protocol.RestartLoginCookie; import org.keycloak.services.ErrorPage; import org.keycloak.services.ServicesLogger; +import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.services.managers.ClientSessionCode; import org.keycloak.services.messages.Messages; import org.keycloak.services.util.BrowserHistoryHelper; import org.keycloak.services.util.AuthenticationFlowURLHelper; +import org.keycloak.services.util.CookieHelper; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.sessions.RootAuthenticationSessionModel; @@ -178,6 +184,15 @@ public class SessionCodeChecks { // See if we are already authenticated and userSession with same ID exists. UserSessionModel userSession = authSessionManager.getUserSessionFromAuthCookie(realm); + if (userSession == null) { + // fallback to check if there is an identity cookie + AuthenticationManager.AuthResult authResult = lockUserSessionsForModification(session, () -> authenticateIdentityCookie(session, realm, false)); + + if (authResult != null) { + userSession = authResult.getSession(); + } + } + if (userSession != null) { LoginFormsProvider loginForm = session.getProvider(LoginFormsProvider.class).setAuthenticationSession(authSession) .setSuccess(Messages.ALREADY_LOGGED_IN); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java index 93022679d9a..2959c4977b8 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java @@ -337,6 +337,20 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { checkRedirectUri("http://localhost:8280/foo/bar", true, true); checkRedirectUri("http://example.com/foobar", false); checkRedirectUri("http://localhost:8280/foobar", false, true); + + checkRedirectUri("http://example.com/foo/../", false); + checkRedirectUri("http://example.com/foo/%2E%2E/", false); // url-encoded "http://example.com/foobar/../" + checkRedirectUri("http://example.com/foo%2F%2E%2E%2F", false); // url-encoded "http://example.com/foobar/../" + checkRedirectUri("http://example.com/foo/%252E%252E/", false); // double-encoded "http://example.com/foobar/../" + checkRedirectUri("http://example.com/foo/%252E%252E/?some_query_param=some_value", false); // double-encoded "http://example.com/foobar/../?some_query_param=some_value" + checkRedirectUri("http://example.com/foo/%252E%252E/?encodeTest=a%3Cb", false); // double-encoded "http://example.com/foobar/../?encodeTest=a