Repository navigation
fix(admin): block TOCTOU rename bypass of protected admin roles - #49516
ThreeMangoTrees wants to merge 1 commit into
Conversation
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 keycloak#49427
Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
|
@abstractj can you review this PR? |
|
That's for @keycloak/core-iam maintainers. |
| if (role == null) { | ||
| throw new NotFoundException("Could not find role"); | ||
| } | ||
| if (isProtectedAdminRoleRename(role, rep.getName()) && !auth.realm().canManageRealm()) { |
There was a problem hiding this comment.
Also related to this: Looking at the resources (both RoleByIdResource and RoleContainerResource, the admin roles are still vulnerable to deletes - shouldn't we prevent deletion of protected roles as well?
There was a problem hiding this comment.
+1, IMO we should not allow deletion as well.
| * 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) { |
There was a problem hiding this comment.
If I'm getting this right, it would block other updates for protected roles even if we're not renaming the role. For example, if one changes the role description this method would block the update even if no rename is happening.
There was a problem hiding this comment.
Thanks, @ThreeMangoTrees.
It would be better if we introduce the "rename" check to org.keycloak.services.resources.admin.fgap.RolePermissions so that we just call this method from both endpoints you are updating here.
It will also make it easier to implement isProtectedAdminRoleRename as you will be able to reuse some code from there that handles the "master realm-management clients" (e.g.: master-realm, foo-realm).
| if (role.getContainer() instanceof RealmModel) { | ||
| return true; | ||
| } | ||
| if (role.getContainer() instanceof ClientModel client) { |
There was a problem hiding this comment.
We also need to check the client roles from master realm clients such as foo-realm and master-realm. See AdminRoles.APP_SUFFIX.
This is the "realm-management" client, but in the master realm, which allows cross-realm admin delegation.
|
@ThreeMangoTrees Can you please review #49624? It addresses the same problem, but also considers other aspects such as:
If you don't mind, I'll be closing this PR in favor of the above. |
| * see #49427 | ||
| */ | ||
| @Test | ||
| @DatabaseTest |
There was a problem hiding this comment.
It seems unnecessary to test this in database testsuite. Regular integration test should suffice,
| * see #49427 | ||
| */ | ||
| @Test | ||
| @DatabaseTest |
There was a problem hiding this comment.
Same here, and other tests. No need to test it for DBs.
Your change is solving the problem at the root so makes more sense. Thank you for your change. |
|
@ThreeMangoTrees thanks, closing now in favor of #49624 |
Summary
Closes #49427
A delegated admin holding only
manage-clientscould exploit two gaps in role-rename validation to escalate to fullrealm-adminprivileges.Attack 1 — TOCTOU via rename-away
The
checkAdminRolescomposite guard is name-based: it checksrole.getName()againstAdminRoles.ALL_ROLES. Before this fix, no guard ran during a rename, enabling this sequence:realm-admin→temp-role— the name leavesALL_ROLES; the composite guard goes blind.temp-roleas a composite child of an attacker-controlled role.temp-role→realm-admin— composites are stored by role ID, so the link persists.Any user assigned the attacker-controlled role now transitively holds full
realm-adminprivileges.Attack 2 — Rename-to bypass
A delegated admin creates an innocent role on their own client and renames it to
"realm-admin". That role now passes name-based guards even though it was never a real admin role.Changes
RoleResource— newisProtectedAdminRoleRename(role, newName)Core helper shared by both update endpoints. Two-tier logic:
newName ∈ ALL_ROLES): blocked unconditionally for any container. Stops Attack 2.role.getName() ∈ ALL_ROLES): blocked only when the role belongs to an admin container (the realm itself, or therealm-management/master-realmclient). Container scoping avoids false positives for custom client roles whose names coincidentally collide with an admin role string.RoleByIdResource—PUT /roles-by-id/{id}Guard applied after
requireManage()and beforeupdateRole(). A delegated admin that passesrequireManagebut lackscanManageRealm()is rejected with403 Forbidden.RoleContainerResource—PUT /roles/{name}andPUT /clients/{id}/roles/{name}(additional fix)This endpoint was entirely unguarded. Patching only
RoleByIdResourcewould leave the vulnerability trivially bypassable via the named-role URL. The same guard is applied here to deliver a complete remediation.RoleByIdResourceTest— 4 new integration testsEach test creates a delegated admin with only
manage-clientsand verifies a specific attack vector is blocked:testRenameProtectedRoleBlockedForDelegatedAdminrealm-admin→temp-rolevia ID endpointtestRenamingToProtectedRoleNameBlockedForDelegatedAdminrealm-adminvia ID endpointtestRenameProtectedRoleBlockedViaNamedRoleEndpointrealm-admin→temp-rolevia named-role endpointtestTOCTOURenameCompositeAttackPreventedrealm-adminname confirmed unchangedTest plan
RoleByIdResourceTestpassRoleByIdResourceTestandRoleContainerResourceTestare unaffectedmanage-realm) can still rename admin roles normally