Encrypted KC_RESTART cookie and removed sensitive notes

This change backports a fix for GHSA-69fp-7c8p-crjr (CVE-2024-4540). Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com> Signed-off-by: Tero Saarni <tero.saarni@est.tech>
Nordix · Jun 17, 2024 · d091f0b · d091f0b
1 parent 1331b1b
commit d091f0b
Show file tree

Hide file tree

Showing 8 changed files with 142 additions and 6 deletions.
diff --git a/docs/documentation/release_notes/index.adoc b/docs/documentation/release_notes/index.adoc
@@ -13,6 +13,9 @@ include::topics/templates/document-attributes.adoc[]
 :release_header_latest_link: {releasenotes_link_latest}
 include::topics/templates/release-header.adoc[]
 
+== {project_name_full} 22.0.11
+include::topics/22_0_11.adoc[leveloffset=2]
+
 == {project_name_full} 22.0.3
 include::topics/22_0_3.adoc[leveloffset=2]
 

diff --git a/docs/documentation/release_notes/topics/22_0_11.adoc b/docs/documentation/release_notes/topics/22_0_11.adoc
@@ -0,0 +1,5 @@
+= Security issue with PAR clients using client_secret_post based authentication
+
+This release contains the fix of the important security issue affecting some OIDC confidential clients using PAR (Pushed authorization request). In case you use OIDC confidential clients together
+with PAR and you use client authentication based on `client_id` and `client_secret` sent as parameters in the HTTP request body (method `client_secret_post` specified in the OIDC specification), it is
+highly encouraged to rotate the client secrets of your clients after upgrading to this version.
diff --git a/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java b/services/src/main/java/org/keycloak/jose/jws/DefaultTokenManager.java
@@ -271,6 +271,8 @@ private String getEncryptedToken(TokenCategory category, String encodedToken) {
     public String cekManagementAlgorithm(TokenCategory category) {
         if (category == null) return null;
         switch (category) {
+            case INTERNAL:
+                return Algorithm.AES;
             case ID:
             case LOGOUT:
                 return getCekManagementAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ALG);
@@ -298,6 +300,8 @@ public String encryptAlgorithm(TokenCategory category) {
         switch (category) {
             case ID:
                 return getEncryptAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ENC, JWEConstants.A128CBC_HS256);
+            case INTERNAL:
+                return JWEConstants.A128CBC_HS256;
             case LOGOUT:
                 return getEncryptAlgorithm(OIDCConfigAttributes.ID_TOKEN_ENCRYPTED_RESPONSE_ENC);
             case AUTHORIZATION_RESPONSE:

diff --git a/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java b/services/src/main/java/org/keycloak/protocol/RestartLoginCookie.java
@@ -22,6 +22,7 @@
 import org.keycloak.Token;
 import org.keycloak.TokenCategory;
 import org.keycloak.common.ClientConnection;
+import org.keycloak.crypto.KeyUse;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.KeycloakContext;
 import org.keycloak.models.KeycloakSession;
@@ -34,6 +35,10 @@
 
 import jakarta.ws.rs.core.Cookie;
 import jakarta.ws.rs.core.UriInfo;
+import org.keycloak.util.TokenUtil;
+
+import javax.crypto.SecretKey;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -122,7 +127,7 @@ public RestartLoginCookie(AuthenticationSessionModel authSession) {
 
     public static void setRestartCookie(KeycloakSession session, RealmModel realm, ClientConnection connection, UriInfo uriInfo, AuthenticationSessionModel authSession) {
         RestartLoginCookie restart = new RestartLoginCookie(authSession);
-        String encoded = session.tokens().encode(restart);
+        String encoded = encodeAndEncrypt(session, restart);
         String path = AuthenticationManager.getRealmCookiePath(realm, uriInfo);
         boolean secureOnly = realm.getSslRequired().isRequired(connection);
         CookieHelper.addCookie(KC_RESTART, encoded, path, null, null, -1, secureOnly, true, session);
@@ -151,7 +156,7 @@ public static AuthenticationSessionModel restartSession(KeycloakSession session,
 
         String encodedCookie = cook.getValue();
 
-        RestartLoginCookie cookie = session.tokens().decode(encodedCookie, RestartLoginCookie.class);
+        RestartLoginCookie cookie = decryptAndDecode(session, encodedCookie);
         if (cookie == null) {
             logger.debug("Failed to verify encoded RestartLoginCookie");
             return null;
@@ -182,6 +187,36 @@ public static AuthenticationSessionModel restartSession(KeycloakSession session,
         return authSession;
     }
 
+    private static RestartLoginCookie decryptAndDecode(KeycloakSession session, String encodedToken) {
+        try {
+            String sigAlgorithm = session.tokens().signatureAlgorithm(TokenCategory.INTERNAL);
+            String algAlgorithm = session.tokens().cekManagementAlgorithm(TokenCategory.INTERNAL);
+            SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, algAlgorithm).getSecretKey();
+            SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey();
+
+            byte[] contentBytes = TokenUtil.jweDirectVerifyAndDecode(encKey, signKey, encodedToken);
+            String jwt = new String(contentBytes, StandardCharsets.UTF_8);
+            return session.tokens().decode(jwt, RestartLoginCookie.class);
+        } catch (Exception e) {
+            // Might be the cookie from the older version
+            return session.tokens().decode(encodedToken, RestartLoginCookie.class);
+        }
+    }
+
+    private static String encodeAndEncrypt(KeycloakSession session, RestartLoginCookie cookie) {
+        try {
+            String sigAlgorithm = session.tokens().signatureAlgorithm(cookie.getCategory());
+            String algAlgorithm = session.tokens().cekManagementAlgorithm(cookie.getCategory());
+            SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, algAlgorithm).getSecretKey();
+            SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey();
+
+            String encodedJwt = session.tokens().encode(cookie);
+            return TokenUtil.jweDirectEncode(encKey, signKey, encodedJwt.getBytes(StandardCharsets.UTF_8));
+        } catch (Exception e) {
+            throw new RuntimeException("Error encoding cookie.", e);
+        }
+    }
+
     @Override
     public TokenCategory getCategory() {
         return TokenCategory.INTERNAL;

diff --git a/...rc/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java b/...rc/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java
@@ -73,6 +73,11 @@ public abstract class AuthzEndpointRequestParser {
         // https://tools.ietf.org/html/rfc7636#section-6.1
         KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_PARAM);
         KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM);
+
+        // Those are not OAuth/OIDC parameters, but they should never be added to the additionalRequestParameters
+        KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_ASSERTION_TYPE);
+        KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_ASSERTION);
+        KNOWN_REQ_PARAMS.add(OAuth2Constants.CLIENT_SECRET);
     }
 
     public void parseRequest(AuthorizationEndpointRequest request) {

diff --git a/...egration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java b/...egration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java
@@ -1149,8 +1149,9 @@ public ParResponse doPushedAuthorizationRequest(String clientId, String clientSe
                 parameters.add(new BasicNameValuePair(OIDCLoginProtocol.RESPONSE_MODE_PARAM, responseMode));
             }
             if (clientId != null && clientSecret != null) {
-                String authorization = BasicAuthHelper.createHeader(clientId, clientSecret);
-                post.setHeader("Authorization", authorization);
+                parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_ID, clientId));
+                parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_SECRET, clientSecret));
+            } else if (clientId != null) {
                 parameters.add(new BasicNameValuePair(OAuth2Constants.CLIENT_ID, clientId));
             }
             if (redirectUri != null) {

diff --git a/...n-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java b/...n-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RestartCookieTest.java
@@ -16,26 +16,39 @@
  */
 
 package org.keycloak.testsuite.forms;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.HashSet;
+import java.util.Set;
 
 import org.jboss.arquillian.graphene.page.Page;
 import org.junit.Rule;
 import org.junit.Test;
+import org.keycloak.OAuth2Constants;
+import org.keycloak.TokenCategory;
+import org.keycloak.crypto.KeyUse;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
 import org.keycloak.jose.jws.JWSBuilder;
 import org.keycloak.models.KeyManager;
 import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.ParConfig;
 import org.keycloak.models.RealmModel;
 import org.keycloak.protocol.RestartLoginCookie;
+import org.keycloak.protocol.oidc.endpoints.AuthorizationEndpoint;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
 import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.AssertEvents;
 import org.keycloak.testsuite.pages.LoginPage;
+import org.keycloak.testsuite.util.ClientBuilder;
+import org.keycloak.testsuite.util.OAuthClient;
+import org.keycloak.util.TokenUtil;
 import org.openqa.selenium.Cookie;
 
-import java.io.IOException;
+import javax.crypto.SecretKey;
 
+import static org.junit.Assert.assertEquals;
 
 /**
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
@@ -70,10 +83,80 @@ public class RestartCookieTest extends AbstractTestRealmKeycloakTest {
             "  }\n" +
             "}";
 
+    public static final Set<String> sensitiveNotes = new HashSet<>();
+    static {
+        sensitiveNotes.add(OAuth2Constants.CLIENT_ASSERTION_TYPE);
+        sensitiveNotes.add(OAuth2Constants.CLIENT_ASSERTION);
+        sensitiveNotes.add(OAuth2Constants.CLIENT_SECRET);
+        sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_ASSERTION_TYPE);
+        sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_ASSERTION);
+        sensitiveNotes.add(AuthorizationEndpoint.LOGIN_SESSION_NOTE_ADDITIONAL_REQ_PARAMS_PREFIX + OAuth2Constants.CLIENT_SECRET);
+    }
+
     @Override
     public void configureTestRealm(RealmRepresentation testRealm) {
     }
 
+    @Test
+    public void testRestartCookie() {
+        loginPage.open();
+        String restartCookie = loginPage.getDriver().manage().getCookieNamed(RestartLoginCookie.KC_RESTART).getValue();
+        assertRestartCookie(restartCookie);
+    }
+
+    @Test
+    public void testRestartCookieWithPar() {
+        String clientId = "par-confidential-client";
+        adminClient.realm("test").clients().create(ClientBuilder.create()
+                .clientId("par-confidential-client")
+                .secret("secret")
+                .redirectUris(oauth.getRedirectUri() + "/*")
+                .attribute(ParConfig.REQUIRE_PUSHED_AUTHORIZATION_REQUESTS, "true")
+                .build());
+
+        oauth.clientId(clientId);
+        String requestUri = null;
+        try {
+            OAuthClient.ParResponse pResp = oauth.doPushedAuthorizationRequest(clientId, "secret");
+            assertEquals(201, pResp.getStatusCode());
+            requestUri = pResp.getRequestUri();
+        }
+        catch (Exception e) {
+            Assert.fail();
+        }
+
+        oauth.redirectUri(null);
+        oauth.scope(null);
+        oauth.responseType(null);
+        oauth.requestUri(requestUri);
+        String state = oauth.stateParamRandom().getState();
+        oauth.stateParamHardcoded(state);
+
+        oauth.openLoginForm();
+        String restartCookie = loginPage.getDriver().manage().getCookieNamed(RestartLoginCookie.KC_RESTART).getValue();
+        assertRestartCookie(restartCookie);
+    }
+
+    private void assertRestartCookie(String restartCookie) {
+        getTestingClient()
+                .server(TEST_REALM_NAME)
+                .run(session ->
+                {
+                    try {
+                        String sigAlgorithm = session.tokens().signatureAlgorithm(TokenCategory.INTERNAL);
+                        String encAlgorithm = session.tokens().cekManagementAlgorithm(TokenCategory.INTERNAL);
+                        SecretKey encKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.ENC, encAlgorithm).getSecretKey();
+                        SecretKey signKey = session.keys().getActiveKey(session.getContext().getRealm(), KeyUse.SIG, sigAlgorithm).getSecretKey();
+
+                        byte[] contentBytes = TokenUtil.jweDirectVerifyAndDecode(encKey, signKey, restartCookie);
+                        String jwt = new String(contentBytes, StandardCharsets.UTF_8);
+                        RestartLoginCookie restartLoginCookie = session.tokens().decode(jwt, RestartLoginCookie.class);
+                        Assert.assertFalse(restartLoginCookie.getNotes().keySet().stream().anyMatch(sensitiveNotes::contains));
+                    } catch (Exception e) {
+                        Assert.fail();
+                    }
+                });
+    }
 
     // KEYCLOAK-5440 -- migration from Keycloak 3.1.0
     @Test

diff --git a/...uillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java b/...uillian/tests/base/src/test/java/org/keycloak/testsuite/keys/FallbackKeyProviderTest.java
@@ -81,7 +81,7 @@ public void fallbackAfterDeletingAllKeysInRealm() {
         Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType());
 
         providers = realmsResouce().realm("test").components().query(realmId, "org.keycloak.keys.KeyProvider");
-        assertProviders(providers, "fallback-RS256", "fallback-HS256");
+        assertProviders(providers, "fallback-RS256", "fallback-AES", "fallback-" + Algorithm.HS256);
     }
 
     @Test