From d8cef023b7c8f03ced9e5c849bc824f9c6e1a342 Mon Sep 17 00:00:00 2001 From: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> Date: Fri, 29 May 2026 22:32:18 -0700 Subject: [PATCH] fix(admin): block TOCTOU rename bypass of protected admin roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A delegated admin with manage-clients could exploit two gaps: 1. TOCTOU via ID-based rename: rename realm-admin → temp-role (name leaves ALL_ROLES, composite guard goes blind), add temp-role as a composite of an attacker-controlled role, then rename temp-role → realm-admin. Any user assigned the attacker-controlled role now transitively holds full realm-admin privileges. 2. Rename-to bypass: rename an innocent role on any custom client to a protected name (e.g. realm-admin), bypassing name-based guards regardless of the role's container. Changes: RoleResource (base class) - Add isProtectedAdminRoleRename(role, newName): returns true when the target name is in AdminRoles.ALL_ROLES (catches rename-to attack), or when the current name is in ALL_ROLES and the role belongs to an admin container (realm-management / master-realm client, or the realm itself). Container scoping avoids false positives for unrelated custom client roles. RoleByIdResource.updateRole (PUT /roles-by-id/{id}) - Apply isProtectedAdminRoleRename guard; require canManageRealm(). - Remove redundant canManage() check (already enforced by requireManage()). RoleContainerResource.updateRole (PUT /roles/{name}, PUT /clients/{id}/roles/{name}) - Apply the same guard. This endpoint was entirely unguarded and provided a complete bypass of the fix in RoleByIdResource. RoleByIdResourceTest - Four new integration tests: delegated admin (manage-clients only) is blocked from renaming realm-admin away from its protected name via the ID-based endpoint; blocked from renaming any role to a protected name; blocked via the named-role endpoint; full TOCTOU sequence is stopped at step 1 and realm-admin name is confirmed unchanged. Closes #49427 Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> --- .../resources/admin/RoleByIdResource.java | 4 + .../admin/RoleContainerResource.java | 4 + .../resources/admin/RoleResource.java | 35 ++++ .../tests/admin/RoleByIdResourceTest.java | 192 ++++++++++++++++++ 4 files changed, 235 insertions(+) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java index bb095c7cfbc9..baabf55fe45f 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java @@ -21,6 +21,7 @@ import jakarta.ws.rs.Consumes; import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.GET; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.POST; @@ -170,6 +171,9 @@ public void deleteRole(final @Parameter(description = "id of role") @PathParam(" public void updateRole(final @Parameter(description = "id of role") @PathParam("role-id") String id, final RoleRepresentation rep) { RoleModel role = getRoleModel(id); auth.roles().requireManage(role); + if (isProtectedAdminRoleRename(role, rep.getName()) && !auth.realm().canManageRealm()) { + throw new ForbiddenException("Cannot rename a protected admin role"); + } updateRole(rep, role, realm, session); if (role.isClientRole()) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java index 19efe40a3ab9..5db3125a6f8b 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java @@ -29,6 +29,7 @@ import jakarta.ws.rs.Consumes; import jakarta.ws.rs.DELETE; import jakarta.ws.rs.DefaultValue; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.GET; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.POST; @@ -333,6 +334,9 @@ public Response updateRole(final @Parameter(description = "role's name (not id!) if (role == null) { throw new NotFoundException("Could not find role"); } + if (isProtectedAdminRoleRename(role, rep.getName()) && !auth.realm().canManageRealm()) { + throw new ForbiddenException("Cannot rename a protected admin role"); + } try { updateRole(rep, role, realm, session); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java index 0f7f7bca3d8a..a9e72898c48e 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java @@ -28,9 +28,12 @@ import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.UriInfo; +import org.keycloak.Config; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; +import org.keycloak.models.AdminRoles; import org.keycloak.models.ClientModel; +import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; @@ -120,6 +123,38 @@ public KeycloakSession getKeycloakSession() { } } + /** + * Returns true when the rename involves a protected admin role name and requires + * full realm-management permission. Renaming TO a protected name is blocked for any + * container; renaming FROM one is only blocked for admin containers (realm or realm-management client). + */ + protected boolean isProtectedAdminRoleRename(RoleModel role, String newName) { + boolean currentNameProtected = AdminRoles.ALL_ROLES.contains(role.getName()); + boolean newNameProtected = newName != null && AdminRoles.ALL_ROLES.contains(newName); + + if (!currentNameProtected && !newNameProtected) { + return false; + } + + // Renaming TO a protected name is blocked for any container. + if (newNameProtected) { + return true; + } + + // Renaming FROM a protected name: only guard admin containers. + if (role.getContainer() instanceof RealmModel) { + return true; + } + if (role.getContainer() instanceof ClientModel client) { + String adminClientId = realm.getName().equals(Config.getAdminRealm()) + ? Config.getAdminRealm() + "-realm" + : Constants.REALM_MANAGEMENT_CLIENT_ID; + ClientModel adminClient = realm.getClientByClientId(adminClientId); + return adminClient != null && adminClient.equals(client); + } + return false; + } + protected void addComposites(AdminPermissionEvaluator auth, AdminEventBuilder adminEvent, UriInfo uriInfo, List roles, RoleModel role) { for (RoleRepresentation rep : roles) { if (rep.getId() == null) throw new NotFoundException("Could not find composite role"); diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/RoleByIdResourceTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/RoleByIdResourceTest.java index ec3b8d20e61f..0cc171033614 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/RoleByIdResourceTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/RoleByIdResourceTest.java @@ -18,6 +18,7 @@ package org.keycloak.tests.admin; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -28,13 +29,22 @@ import java.util.stream.Collectors; import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.NotFoundException; +import jakarta.ws.rs.core.Response; +import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.RoleByIdResource; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; +import org.keycloak.models.AdminRoles; import org.keycloak.models.Constants; +import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.representations.idm.CredentialRepresentation; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testframework.admin.AdminClientFactory; +import org.keycloak.testframework.annotations.InjectAdminClientFactory; import org.keycloak.testframework.annotations.InjectAdminEvents; import org.keycloak.testframework.annotations.InjectClient; import org.keycloak.testframework.annotations.InjectRealm; @@ -78,6 +88,9 @@ public class RoleByIdResourceTest { @InjectAdminEvents AdminEvents adminEvents; + @InjectAdminClientFactory + AdminClientFactory adminClientFactory; + private RoleByIdResource resource; private final String roleNameA = "role-a"; @@ -367,4 +380,183 @@ public void createRolePreviouslyCached() { private void cacheMissingRoleName(String missingRoleName) { assertThrows(NotFoundException.class, () -> managedRealm.admin().roles().get(missingRoleName).toRepresentation()); } + + /** + * see #49427 + */ + @Test + @DatabaseTest + public void testRenameProtectedRoleBlockedForDelegatedAdmin() { + createDelegatedAdminUser("delegated-admin", "password", AdminRoles.MANAGE_CLIENTS); + + ClientRepresentation rmClient = managedRealm.admin().clients() + .findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0); + RoleRepresentation realmAdminRole = managedRealm.admin().clients() + .get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation(); + + try (Keycloak delegatedClient = adminClientFactory.create() + .realm(managedRealm.getName()) + .username("delegated-admin") + .password("password") + .clientId(Constants.ADMIN_CLI_CLIENT_ID) + .build()) { + + RoleRepresentation renamed = new RoleRepresentation(); + renamed.setId(realmAdminRole.getId()); + renamed.setName("temp-role"); + + Assertions.assertThrows(ForbiddenException.class, () -> + delegatedClient.realm(managedRealm.getName()).rolesById() + .updateRole(realmAdminRole.getId(), renamed)); + } + } + + /** + * see #49427 + */ + @Test + @DatabaseTest + public void testRenamingToProtectedRoleNameBlockedForDelegatedAdmin() { + createDelegatedAdminUser("delegated-admin2", "password", AdminRoles.MANAGE_CLIENTS); + + String attackerClientId = "attacker-client-" + new Random().nextInt(10000); + try (Keycloak delegatedClient = adminClientFactory.create() + .realm(managedRealm.getName()) + .username("delegated-admin2") + .password("password") + .clientId(Constants.ADMIN_CLI_CLIENT_ID) + .build()) { + + // Create a client and a role the delegated admin owns + Response resp = delegatedClient.realm(managedRealm.getName()).clients() + .create(clientRepresentation(attackerClientId)); + String attackerClientUuid = resp.getLocation().getPath().replaceAll(".*/", ""); + resp.close(); + + delegatedClient.realm(managedRealm.getName()).clients() + .get(attackerClientUuid).roles().create(roleRepresentation("innocent-role")); + + RoleRepresentation innocentRole = delegatedClient.realm(managedRealm.getName()).clients() + .get(attackerClientUuid).roles().get("innocent-role").toRepresentation(); + + // Attempt to rename innocent-role → realm-admin + RoleRepresentation renamed = new RoleRepresentation(); + renamed.setId(innocentRole.getId()); + renamed.setName(AdminRoles.REALM_ADMIN); + + Assertions.assertThrows(ForbiddenException.class, () -> + delegatedClient.realm(managedRealm.getName()).rolesById() + .updateRole(innocentRole.getId(), renamed)); + } + } + + /** + * see #49427 — named-role endpoint (PUT /roles/{role-name}) must also be guarded. + */ + @Test + @DatabaseTest + public void testRenameProtectedRoleBlockedViaNamedRoleEndpoint() { + createDelegatedAdminUser("delegated-admin3", "password", AdminRoles.MANAGE_CLIENTS); + + ClientRepresentation rmClient = managedRealm.admin().clients() + .findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0); + + try (Keycloak delegatedClient = adminClientFactory.create() + .realm(managedRealm.getName()) + .username("delegated-admin3") + .password("password") + .clientId(Constants.ADMIN_CLI_CLIENT_ID) + .build()) { + + RoleRepresentation renamed = new RoleRepresentation(); + renamed.setName("temp-role"); + + Assertions.assertThrows(ForbiddenException.class, () -> + delegatedClient.realm(managedRealm.getName()).clients() + .get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN) + .update(renamed)); + } + } + + /** + * see #49427 + */ + @Test + @DatabaseTest + public void testTOCTOURenameCompositeAttackPrevented() { + createDelegatedAdminUser("toctou-attacker", "password", AdminRoles.MANAGE_CLIENTS); + + ClientRepresentation rmClient = managedRealm.admin().clients() + .findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0); + RoleRepresentation realmAdminRole = managedRealm.admin().clients() + .get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation(); + + try (Keycloak delegatedClient = adminClientFactory.create() + .realm(managedRealm.getName()) + .username("toctou-attacker") + .password("password") + .clientId(Constants.ADMIN_CLI_CLIENT_ID) + .build()) { + + // Step 1: rename realm-admin → temp-role must be blocked + RoleRepresentation renamed = new RoleRepresentation(); + renamed.setId(realmAdminRole.getId()); + renamed.setName("temp-role"); + + Assertions.assertThrows(ForbiddenException.class, () -> + delegatedClient.realm(managedRealm.getName()).rolesById() + .updateRole(realmAdminRole.getId(), renamed)); + + // Confirm realm-admin name is unchanged + RoleRepresentation unchanged = managedRealm.admin().clients() + .get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation(); + assertEquals(AdminRoles.REALM_ADMIN, unchanged.getName()); + } + } + + private void createDelegatedAdminUser(String username, String password, String clientRoleName) { + UserRepresentation user = new UserRepresentation(); + user.setUsername(username); + user.setEnabled(true); + user.setEmailVerified(true); + user.setFirstName("Delegated"); + user.setLastName("Admin"); + user.setEmail(username + "@test.local"); + + Response resp = managedRealm.admin().users().create(user); + String userId = resp.getLocation().getPath().replaceAll(".*/", ""); + resp.close(); + + CredentialRepresentation cred = new CredentialRepresentation(); + cred.setType(CredentialRepresentation.PASSWORD); + cred.setValue(password); + cred.setTemporary(false); + managedRealm.admin().users().get(userId).resetPassword(cred); + + ClientRepresentation rmClient = managedRealm.admin().clients() + .findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0); + RoleRepresentation role = managedRealm.admin().clients() + .get(rmClient.getId()).roles().get(clientRoleName).toRepresentation(); + managedRealm.admin().users().get(userId).roles() + .clientLevel(rmClient.getId()).add(Collections.singletonList(role)); + + // query-clients is required so the delegated admin can look up clients + RoleRepresentation queryClients = managedRealm.admin().clients() + .get(rmClient.getId()).roles().get(AdminRoles.QUERY_CLIENTS).toRepresentation(); + managedRealm.admin().users().get(userId).roles() + .clientLevel(rmClient.getId()).add(Collections.singletonList(queryClients)); + } + + private ClientRepresentation clientRepresentation(String clientId) { + ClientRepresentation client = new ClientRepresentation(); + client.setClientId(clientId); + client.setEnabled(true); + return client; + } + + private RoleRepresentation roleRepresentation(String name) { + RoleRepresentation role = new RoleRepresentation(); + role.setName(name); + return role; + } }