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 <mposolda@gmail.com>
Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com>
Co-authored-by: Alexander Schwartz <alexander.schwartz@gmx.net>
This commit is contained in:
Stian Thorgersen 2022-12-14 07:46:17 +01:00 committed by GitHub
parent 5ced20e1ee
commit 0f2ca3bfdd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 233 additions and 9 deletions

View file

@ -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);
}

View file

@ -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) {

View file

@ -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<String>() {
@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());
}

View file

@ -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<String> 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();

View file

@ -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);

View file

@ -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<b"
checkRedirectUri("http://example.com/foo/%252E%252E/#encodeTest=a%3Cb", false); // double-encoded "http://example.com/foobar/../?encodeTest=a<b"
checkRedirectUri("http://example.com/foo/%25252E%25252E/", false); // triple-encoded "http://example.com/foobar/../"
checkRedirectUri("http://example.com/foo/%2525252525252E%2525252525252E/", false); // seventh-encoded "http://example.com/foobar/../"
checkRedirectUri("http://example.com/foo?encodeTest=a%3Cb", true);
checkRedirectUri("http://example.com/foo?encodeTest=a%3Cb#encode2=a%3Cb", true);
checkRedirectUri("http://example.com/foo/#encode2=a%3Cb", true);
}
@Test
@ -381,7 +395,7 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest {
oauth.clientId("test-relative-url");
checkRedirectUri("http://with-dash.example.local/foo", false);
checkRedirectUri("http://localhost:8180/auth", true);
checkRedirectUri(OAuthClient.AUTH_SERVER_ROOT, true);
}
@Test
@ -455,6 +469,7 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest {
if (!checkCodeToToken) {
oauth.openLoginForm();
Assert.assertTrue(loginPage.isCurrent());
Assert.assertFalse(errorPage.isCurrent());
} else {
oauth.doLogin("test-user@localhost", "password");

View file

@ -238,7 +238,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
driver.navigate().to(logoutUrl);
logoutConfirmPage.assertCurrent();
logoutConfirmPage.confirmLogout();
events.expectLogout(sessionId2).detail(Details.REDIRECT_URI, redirectUri).assertEvent();
events.expectLogoutError(Errors.SESSION_EXPIRED);
MatcherAssert.assertThat(false, is(isSessionActive(sessionId2)));
assertCurrentUrlEquals(redirectUri + "&state=something");
}
@ -261,7 +261,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
// should not throw an internal server error. But no logout event is sent as nothing was logged-out
appPage.assertCurrent();
events.assertEmpty();
events.expectLogoutError(Errors.SESSION_EXPIRED);
MatcherAssert.assertThat(false, is(isSessionActive(tokenResponse.getSessionState())));
// check if the back channel logout succeeded
@ -318,7 +318,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
// Try logout even if user already logged-out by admin. Should redirect back to the application, but no logout-event should be triggered
String logoutUrl = oauth.getLogoutUrl().postLogoutRedirectUri(APP_REDIRECT_URI).idTokenHint(idTokenString).build();
driver.navigate().to(logoutUrl);
events.assertEmpty();
events.expectLogoutError(Errors.SESSION_EXPIRED);
assertCurrentUrlEquals(APP_REDIRECT_URI);
// Login again in the browser. Ensure to use newest idTokenHint after logout
@ -378,7 +378,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
assertThat(response, Matchers.statusCodeIsHC(Response.Status.FOUND));
assertThat(response.getFirstHeader(HttpHeaders.LOCATION).getValue(), is(APP_REDIRECT_URI));
}
events.assertEmpty();
events.expectLogoutError(Errors.SESSION_EXPIRED);
MatcherAssert.assertThat(false, is(isSessionActive(tokenResponse.getSessionState())));
}
@ -401,7 +401,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
assertThat(response, Matchers.statusCodeIsHC(Response.Status.FOUND));
assertThat(response.getFirstHeader(HttpHeaders.LOCATION).getValue(), is(APP_REDIRECT_URI));
}
events.assertEmpty();
events.expectLogoutError(Errors.SESSION_EXPIRED);
MatcherAssert.assertThat(false, is(isSessionActive(tokenResponse.getSessionState())));
}
@ -932,7 +932,7 @@ public class RPInitiatedLogoutTest extends AbstractTestRealmKeycloakTest {
errorPage.assertCurrent();
Assert.assertEquals("Logout failed", errorPage.getError());
events.expectLogoutError(Errors.SESSION_EXPIRED).assertEvent();
events.expectLogoutError(Errors.LOGOUT_FAILED).assertEvent();
}

View file

@ -17,7 +17,9 @@
package org.keycloak.testsuite.oauth;
import com.fasterxml.jackson.databind.JsonNode;
import com.gargoylesoftware.htmlunit.WebClient;
import org.hamcrest.CoreMatchers;
import org.jboss.arquillian.drone.webdriver.htmlunit.DroneHtmlUnitDriver;
import org.jboss.arquillian.graphene.page.Page;
import org.junit.Assert;
import org.junit.Before;
@ -27,6 +29,7 @@ import org.keycloak.OAuth2Constants;
import org.keycloak.OAuthErrorException;
import org.keycloak.admin.client.resource.ClientResource;
import org.keycloak.admin.client.resource.RealmResource;
import org.keycloak.admin.client.resource.RealmsResource;
import org.keycloak.admin.client.resource.UserResource;
import org.keycloak.common.enums.SslRequired;
import org.keycloak.crypto.Algorithm;
@ -36,6 +39,7 @@ import org.keycloak.jose.jws.JWSHeader;
import org.keycloak.jose.jws.JWSInput;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.models.utils.KeycloakModelUtils;
import org.keycloak.models.utils.SessionTimeoutHelper;
import org.keycloak.protocol.oidc.OIDCConfigAttributes;
import org.keycloak.protocol.oidc.OIDCLoginProtocol;
@ -47,6 +51,7 @@ import org.keycloak.representations.idm.ClientRepresentation;
import org.keycloak.representations.idm.EventRepresentation;
import org.keycloak.representations.idm.RealmRepresentation;
import org.keycloak.representations.idm.UserSessionRepresentation;
import org.keycloak.services.managers.AuthenticationSessionManager;
import org.keycloak.testsuite.AbstractKeycloakTest;
import org.keycloak.testsuite.AssertEvents;
import org.keycloak.testsuite.admin.ApiUtil;
@ -59,11 +64,13 @@ import org.keycloak.testsuite.util.OAuthClient;
import org.keycloak.testsuite.util.RealmBuilder;
import org.keycloak.testsuite.util.RealmManager;
import org.keycloak.testsuite.util.TokenSignatureUtil;
import org.keycloak.testsuite.util.UserBuilder;
import org.keycloak.testsuite.util.UserInfoClientUtil;
import org.keycloak.testsuite.util.UserManager;
import org.keycloak.testsuite.util.WaitUtils;
import org.keycloak.util.BasicAuthHelper;
import org.keycloak.util.JsonSerialization;
import org.openqa.selenium.Cookie;
import javax.ws.rs.client.Client;
import javax.ws.rs.client.Entity;
@ -74,6 +81,7 @@ import javax.ws.rs.core.Response;
import javax.ws.rs.core.UriBuilder;
import java.net.URI;
import java.util.List;
import java.util.concurrent.TimeUnit;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.greaterThan;
@ -83,6 +91,7 @@ import static org.hamcrest.Matchers.lessThanOrEqualTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
@ -283,6 +292,120 @@ public class RefreshTokenTest extends AbstractKeycloakTest {
Assert.assertNotEquals(200, response.getStatusCode());
}
@Test
public void testDoNotResolveOfflineUserSessionIfAuthenticationSessionIsInvalidated() {
oauth.scope("offline_access");
testDoNotResolveUserSessionIfAuthenticationSessionIsInvalidated();
}
@Test
public void testDoNotResolveUserSessionIfAuthenticationSessionIsInvalidated() {
String realmName = KeycloakModelUtils.generateId();
RealmsResource realmsResource = realmsResouce();
realmsResource.create(RealmBuilder.create().name(realmName).build());
RealmResource realmResource = realmsResource.realm(realmName);
RealmRepresentation realm = realmResource.toRepresentation();
try {
realm.setSsoSessionMaxLifespan((int) TimeUnit.MINUTES.toSeconds(2));
realm.setSsoSessionIdleTimeout((int) TimeUnit.MINUTES.toSeconds(2));
realm.setAccessTokenLifespan((int) TimeUnit.MINUTES.toSeconds(1));
realmResource.update(realm);
realmResource.clients().create(org.keycloak.testsuite.util.ClientBuilder.create()
.clientId("public-client")
.redirectUris("*")
.publicClient()
.build());
realmResource.users()
.create(UserBuilder.create().username("alice").password("alice").addRoles("offline_access").build());
realmResource.users()
.create(UserBuilder.create().username("bob").password("bob").addRoles("offline_access").build());
oauth.realm(realmName);
oauth.clientId("public-client");
oauth.doLogin("alice", "alice");
String aliceCode = oauth.getCurrentQuery().get(OAuth2Constants.CODE);
OAuthClient.AccessTokenResponse tokenResponse = oauth.doAccessTokenRequest(aliceCode, "password");
AccessToken aliceAt = oauth.verifyToken(tokenResponse.getAccessToken());
setTimeOffset((int) TimeUnit.MINUTES.toSeconds(2));
oauth.doLogin("bob", "bob");
String bobCode = oauth.getCurrentQuery().get(OAuth2Constants.CODE);
assertNotEquals(aliceCode, bobCode);
tokenResponse = oauth.doAccessTokenRequest(bobCode, "password");
String refreshToken = tokenResponse.getRefreshToken();
tokenResponse = oauth.doRefreshTokenRequest(refreshToken, null);
AccessToken bobAt = oauth.verifyToken(tokenResponse.getAccessToken());
assertNotEquals(aliceAt.getSessionId(), bobAt.getSessionId());
assertEquals("bob", bobAt.getPreferredUsername());
} finally {
setTimeOffset(0);
realm.setSsoSessionMaxLifespan(null);
realm.setSsoSessionIdleTimeout(null);
realm.setAccessTokenLifespan(null);
realmResource.update(realm);
}
}
@Test
public void testTimeoutWhenReUsingPreviousAuthenticationSession() {
String realmName = KeycloakModelUtils.generateId();
RealmsResource realmsResource = realmsResouce();
realmsResource.create(RealmBuilder.create().name(realmName).build());
RealmResource realmResource = realmsResource.realm(realmName);
RealmRepresentation realm = realmResource.toRepresentation();
try {
realm.setSsoSessionMaxLifespan((int) TimeUnit.MINUTES.toSeconds(2));
realm.setSsoSessionIdleTimeout((int) TimeUnit.MINUTES.toSeconds(2));
realm.setAccessTokenLifespan((int) TimeUnit.MINUTES.toSeconds(1));
realmResource.update(realm);
realmResource.clients().create(org.keycloak.testsuite.util.ClientBuilder.create()
.clientId("public-client")
.redirectUris("*")
.publicClient()
.build());
realmResource.users()
.create(UserBuilder.create().username("alice").password("alice").addRoles("offline_access").build());
realmResource.users()
.create(UserBuilder.create().username("bob").password("bob").addRoles("offline_access").build());
oauth.realm(realmName);
oauth.clientId("public-client");
oauth.openLoginForm();
Cookie authSessionCookie = driver.manage().getCookieNamed(AuthenticationSessionManager.AUTH_SESSION_ID);
oauth.fillLoginForm("alice", "alice");
String aliceCode = oauth.getCurrentQuery().get(OAuth2Constants.CODE);
WebClient webClient = DroneHtmlUnitDriver.class.cast(driver).getWebClient();
webClient.getCookieManager().clearCookies();
oauth.openLoginForm();
driver.manage().addCookie(authSessionCookie);
oauth.fillLoginForm("bob", "bob");
assertEquals("Your login attempt timed out. Login will start from the beginning.", loginPage.getError());
} finally {
setTimeOffset(0);
realm.setSsoSessionMaxLifespan(null);
realm.setSsoSessionIdleTimeout(null);
realm.setAccessTokenLifespan(null);
realmResource.update(realm);
}
}
/**
* KEYCLOAK-15437
*/