sec: enforce admin check on layout assignment and role grant endpoints

[mbiuki]

Summary

Fixes a privilege-escalation chain (reported via responsible disclosure) that allowed any authenticated backend user to reach CMS Administrator and subsequently achieve remote code execution via OSGi bundle upload.

Attack chain closed by this PR:

1. PUT /api/v1/toolgroups/{id}/_addtouser — any backend user could self-assign the admin Settings layout (containing the roles portlet) with no privilege check
2. POST /dwr/call/plaincall/RoleAjax.addUserToRole.dwr — with the roles portlet now accessible, the user could grant themselves the CMS Administrator role; the only gate was portlet-access, which step 1 satisfied
3. As CMS Administrator, attacker uploads a malicious OSGi bundle → arbitrary OS command execution

Changes

ToolGroupResource.java

• _addtouser: require loggedInUser.isAdmin() for all layouts except gettingstarted (the onboarding layout — intentionally self-assignable)
• _removefromuser: same admin guard applied symmetrically

RoleAjax.java

• addUserToRole: add caller.isAdmin() check after the existing portlet-access check; log unauthorized attempts via SecurityLogger
• removeUsersFromRole: same admin guard applied symmetrically

Test plan

• Verify a low-privilege backend user receives 403 on PUT /api/v1/toolgroups/{layout_id}/_addtouser (non-gettingStarted layout)
• Verify gettingStarted layout self-assignment still works for non-admin users (onboarding flow must not break)
• Verify a low-privilege backend user with roles-portlet access receives DotSecurityException on RoleAjax.addUserToRole via DWR
• Verify a CMS Administrator can still assign layouts and manage roles normally
• Run the researcher's PoC script — it must fail at step 1 and never reach role escalation

References

• Internal tracking: dotCMS/private-issues#642
• CWE-269: Improper Privilege Management

🤖 Generated with Claude Code

[github-actions]

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

• This PR fixes dotCMS/private-issues#123 or Fixes: dotCMS/private-issues#123

• This PR closes dotCMS/private-issues#123 or Closes: dotCMS/private-issues#123

• This PR resolves dotCMS/private-issues#123 or Resolves: dotCMS/private-issues#123

• Other supported keywords: fix, fixed, close, closed, resolve, resolved
  Option 2: Link via GitHub UI (Note: won't clear the failed check)

1. Go to the PR → Development section (right sidebar)

2. Click "Link issue" and select an existing issue

3. Push a new commit or re-run the workflow to clear the failed check
   Option 3: Use branch naming
   Create a new branch with one of these patterns:

• 123-feature-description (number at start)

• issue-123-feature-description (issue-number at start)

• feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

[claude]

Claude finished @mbiuki's task in 55s —— View job

---

I'll analyze this and get back to you.

[github-actions]

🤖 dotBot Review (Bedrock)

Reviewed 2 file(s); 1 candidate(s) → 1 confirmed, 0 uncertain (unverified, kept for review).

Confirmed findings

• 🟡 Medium dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java:124 — Missing user ID validation in gettingstarted layout assignment
  The code allows non-admin users to assign the 'gettingstarted' layout to arbitrary user IDs via the userId parameter. While the admin check is bypassed for this layout type, there's no validation that the target userId matches the logged-in user's ID, enabling potential privilege escalation if the layout contains sensitive portlets.

---

_{us.deepseek.r1-v1:0 · Run: #28671877655 · tokens: in: 8448 · out: 2919 · total: 11367 · calls: 4 · est. ~$0.027}

[mbiuki]

This PR fixes https://github.com/dotCMS/private-issues/issues/642

[github-actions]

PRs linked to this issue

• sec: harden portlet-only gated methods in RoleAjax by @mbiuki

[mbiuki]

Versions Affected

Exploitable range: v21.02 → v26.06.22-03 (current latest)

Gap	Introduced	First vulnerable release
RoleAjax.addUserToRole (portlet-only gate)	March 2012	v3.0
ToolGroupResource._addtouser (enabling step)	December 2020	v21.02

No patched release has been cut yet — this PR is the fix.

[mbiuki]

Vulnerability Introduction Timeline

ToolGroupResource.addToolGroupToUser — admin check was never present

Date	Commit	Author	Event
2020-12-16	8aa87f7b87	Will Ezell	Method created without any admin check (PR #19680, issue #19581) — only requiredBackendUser(true) enforced
2021-03-22	890feffa94	Nollymar Longa	?userid= parameter added — widened exploit surface from self-assignment to assigning layouts to any user
2025-11-04	3babaf0d17	—	Partial portlet-level mitigation added (private-issues#482) — insufficient alone
PR #36344	—	—	Full fix: admin-only gate added — not yet in any release

Affected Versions

• First vulnerable release: 21.02 (December 2020 commit shipped in the Feb 2021 release cut)
• Widened attack surface (other-user assignment): ~21.03–21.04 (March 2021 commit)
• All affected: 21.02 → v26.06.22-03 (current latest), including all LTS lines:
  • 23.10 LTS ✗
  • 24.04 LTS ✗
  • 24.12 LTS ✗
  • 25.07 LTS ✗
• Partial mitigation from: v25.11.07-1 (still exploitable via chain with sec: enforce admin check on layout assignment and role grant endpoints #36344)
• Fully fixed in: not yet released

[mbiuki]

🔴 Red Team Review — PR #36344

Three independent reviewers examined this PR across security, performance, and test coverage.

---

Verdicts

Dimension	Verdict
🔐 Security	NEEDS_CHANGES
⚡ Performance	MINOR_IMPROVEMENT
🧪 Test Coverage	NEEDS_IMPROVEMENT

---

🔐 Security

The _addtouser admin guard is correctly implemented. However, three issues require remediation before merge.

🚨 Critical — _removefromuser has no admin guard

ToolGroupResource.java:66–92

The PR description states _removefromuser received the same admin guard symmetrically — this is false in current HEAD. The endpoint only requires requiredBackendUser(true). Any authenticated backend user can call PUT /api/v1/toolgroups/{layoutId}/_removefromuser?userid={anyUserId} to strip any layout from any user's role, including CMS Administrators.

Fix: Apply .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) to _removefromuser identically to _addtouser.

⚠️ High — addUserToRole / removeUsersFromRole use portlet-based check, not role-based

RoleAjax.java:282–345

getAdminUser() checks whether the user has a layout containing the "users" portlet — not whether they hold the CMS Administrator role. An attacker who obtains the "users" portlet via any other mechanism could bypass this guard. The correct check is isAdmin() → doesUserHaveRole(user, loadCMSAdminRole()), which is a direct role membership check. requiredRoles(CMS_ADMINISTRATOR_ROLE) (used correctly in _addtouser) is stricter than getAdminUser().

Additionally, both methods do DB reads (loadRoleById, loadUserById) before the admin guard fires — wasted work for unauthorized callers, inconsistent with the fail-fast pattern applied in saveRoleLayouts (PR #36345).

Fix: Replace getAdminUser() in both methods with an explicit isAdmin() check at the top of the method body, before any DB reads.

⚠️ High — SecurityLogger coverage removed

RoleAjax.java:282–345

Explicit SecurityLogger.logInfo() calls on unauthorized role-assignment attempts were present in an earlier commit but dropped in the current HEAD. The getAdminUser() fallback logs "unauthorized attempt to call getUserById" — a misleading message that won't surface in forensic searches for role-escalation incidents.

Fix: Add explicit log entries before throwing:

SecurityLogger.logInfo(getClass(),
    "Unauthorized attempt to assign role [roleId=" + roleId + "] by user " + caller.getUserId());

🔵 Low — gettingStarted self-assignment exception removed

The original commit allowed non-admin users to self-assign the onboarding gettingStarted layout. The current HEAD applies requiredRoles(CMS_ADMINISTRATOR_ROLE) to all layouts uniformly, breaking the documented onboarding flow. This is an over-restriction, not a security regression — but needs product validation.

ℹ️ Info (pre-existing) — getUsersByRole has no auth guard

RoleAjax.java:211 — any session-authenticated user can enumerate all users in any role including CMS Admin. Not introduced by this PR.

---

⚡ Performance

Net positive change.

• _addtouser guard fires inside InitBuilder.init() before any layout/user DB reads — correct fail-fast ordering
• isAdmin() and getAdminUser() both resolve from RoleCache on the warm path — negligible overhead
• Pre-existing anti-pattern in addUserToRole: loadRoleById + loadUserById fire before the admin check — not introduced by this PR
• Minor redundancy: WebResource.init() calls both checkAdminPermissions and checkRolePermissions for the same CMS Admin role check — both are cache hits, pre-existing issue

---

🧪 Test Coverage

Scenario	Status
Non-admin blocked on _addtouser	✅
Admin succeeds on _addtouser	✅
Non-admin blocked on addUserToRole (with portlet access)	✅
gettingStarted non-admin self-assignment allowed	❌ Missing
Non-admin blocked on _removefromuser	❌ Missing
Admin succeeds on _removefromuser	❌ Missing
Non-admin blocked on removeUsersFromRole	❌ Missing
Admin succeeds on addUserToRole / removeUsersFromRole	❌ Missing
Cross-user ?userid= assignment blocked for non-admin	❌ Missing
SecurityLogger invoked on unauthorized attempts	❌ Missing

Recommended additions:

// ToolGroupResourceTest
test_removeToolGroupFromUser_lowPrivilegeUser_throwsSecurityException()
test_removeToolGroupFromUser_adminUser_succeeds()
test_addToolGroupToUser_gettingStartedLayout_nonAdmin_succeeds()  // if exception is restored
test_addToolGroupToUser_crossUserAssignment_lowPrivilegeUser_throwsSecurityException()

// RoleAjaxSecurityTest
test_removeUsersFromRole_lowPrivilegeUser_throwsSecurityException()
test_addUserToRole_adminUser_succeeds()
test_removeUsersFromRole_adminUser_succeeds()

---

Summary

The _addtouser fix is correct and closes Step 1 of the escalation chain. Three blockers must be resolved before merge:

1. 🚨 Add admin guard to _removefromuser
2. ⚠️ Replace getAdminUser() with isAdmin() in addUserToRole + removeUsersFromRole
3. ⚠️ Restore SecurityLogger with accurate action descriptions on unauthorized role-assignment attempts

[mbiuki]

Correction to the Red Team Review above

After reading the actual PR commit (4d0d4977ff), the three blockers I flagged are already addressed:

1. ✅ _removefromuser — isAdmin() check is present (added by this PR)
2. ✅ addUserToRole / removeUsersFromRole — isAdmin() check + SecurityLogger are both present (added by this PR)
3. ✅ SecurityLogger — accurate log messages on unauthorized attempts are already in place

The review was based on pre-PR baseline code rather than the PR branch — apologies for the noise.

One real issue was found and fixed: both _removefromuser and _addtouser were calling loadUserById() before the isAdmin() check, so a non-admin caller with a ?userid= param triggered a needless DB read before being rejected. Commit ff86ac22f7 reorders those two blocks so the admin check fires first.