Skip to content

[security] GHSA-x3pq-323g-f95v: enforce privilege-escalation guards on permission/role updates and login tokens - #2157

Draft
pimcore-deployments wants to merge 2 commits into
2026.3from
security/ghsa-x3pq-323g-f95v-f07fc55a20bab857
Draft

pimcore-deployments wants to merge 2 commits into
2026.3from
security/ghsa-x3pq-323g-f95v-f07fc55a20bab857

Conversation

@pimcore-deployments

Copy link
Copy Markdown
Contributor

Advisory: GHSA-x3pq-323g-f95v

Security-Advisory: pimcore/platform-version/GHSA-x3pq-323g-f95v

Vulnerability / root cause

The advisory describes a broken-access-control (OWASP A01) privilege-escalation chain. The endpoints that update user/role permissions and roles are gated only by #[IsGranted(UserPermissions::USER_MANAGEMENT->value)], which checks that the caller holds the generic users permission — not that the caller already holds the specific permissions or roles being granted. Concretely, verified against the current code:

  1. UpdateService::updatePermissions() wrote the requested permission list onto the target user/role with no check against the caller's own effective permissions.
  2. RoleService::updateRoleById() routes a role's permission list through the same UpdateService::updatePermissions(), so it was exposed the same way.
  3. No guard prevented a users-permission holder from reaching admin-equivalent power via permission/role grants, since (1)/(2) left that path open.
  4. UserLoginService::getLoginTokenUrl() additionally accepted an unused, client-controlled tokenLoginUrl field via the TokenLink request schema, and did not check whether the target user's effective permission set exceeded the caller's own before minting a login token for them.

Any authenticated user holding only the users permission could therefore grant themselves (or another account) arbitrary additional permissions/roles, and could mint a valid login-token URL for a more privileged account.

Fix

  • Added a centralized guard, UpdateService::denyEscalationBeyondCallersOwnPermissions(), called from both updatePermissions() and updateRoles(). It resolves the current caller via the existing SecurityServiceInterface::getCurrentUser() primitive, bypasses the check for superusers (isAdmin()), and otherwise rejects (via ForbiddenException) any permission — direct or granted through a role — that the caller does not already hold themselves (UserInterface::isAllowed()). This single change protects both the user-permission path and the role-permission path (RoleService::updateRoleById() calls through the same method), and closes the admin-equivalent-power gap as a direct consequence.
  • UserLoginService::getLoginTokenUrl() no longer accepts a client-supplied URL at all — the TokenLink parameter was removed from the method, the controller, and the OpenAPI request body; the TokenLink schema class and TokenLinkRequestBody attribute were deleted outright as dead/vulnerable code. The method now also rejects (reusing the existing LOGIN_TOKEN_NON_ADMIN error key) any request where the target user's effective permission set (via the existing UserPermissionServiceInterface::getAllowedUserPermissions()) is not a subset of the caller's own, with the same superuser bypass.
  • UserCloneService/RoleCloneService also write permissions/roles directly, but they are not named in the advisory's root-cause list and are left untouched to keep this fix minimal and scoped to the reported issue.

Backward compatibility

All touched classes/interfaces are annotated @internal (UpdateService, UpdateServiceInterface, RoleServiceInterface, UpdateRoleController, UserLoginService, UserLoginServiceInterface, TokenLoginLinkController, TokenLink, TokenLinkRequestBody), so none of this is covered by the BC promise:

  • UpdateServiceInterface::updatePermissions()/updateRoles(): public method signatures are unchanged; only the @throws docblock gained ForbiddenException for the new, narrower rejection case. No call site needed to change (the new dependency was added to UpdateService's constructor, not its public methods).
  • UserLoginServiceInterface::getLoginTokenUrl(): signature changed (removed the TokenLink $parameter argument) — BC-safe only because the interface is @internal.
  • TokenLink (schema) and TokenLinkRequestBody (OpenAPI attribute) were deleted — both @internal, with no external contract.

No public, non-@internal API was touched.

Tests added

  • tests/Unit/User/Service/UpdateServiceTest.php: testNonAdminCannotGrantPermissionTheyDoNotHoldThemselves, testNonAdminCanGrantPermissionsTheyAlreadyHoldThemselves, testAdminCanGrantAnyPermission, testNonAdminCannotAssignRoleGrantingPermissionTheyDoNotHoldThemselves, testNonAdminCanAssignRoleWithPermissionsTheyAlreadyHoldThemselves (existing tests' helper methods were updated to pass the new SecurityServiceInterface constructor dependency, defaulting to an admin caller so prior test behavior is preserved).
  • tests/Unit/User/Service/UserLoginServiceTest.php: testGetLoginTokenUrlDoesNotAcceptAnyUrlParameter, testGetLoginTokenUrlRejectsTargetWithPermissionCallerDoesNotHold, testGetLoginTokenUrlAllowsTargetWithPermissionsCallerAlsoHolds, testGetLoginTokenUrlAllowsAdminCallerRegardlessOfTargetPermissions (existing helper updated to pass the new UserPermissionServiceInterface constructor dependency).

Verification status

php -l was run against every touched file (all pass). The bundle ships no local vendor/ in this environment and the Codeception suite normally runs inside Docker (docker compose -f docker-compose.yml exec php-studio-backend-bundle vendor/bin/codecept run Unit), which is not available here — the added/updated tests have not been executed locally; CI is the validator for this PR.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • api.anthropic.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "api.anthropic.com"

See Network Configuration for more information.

Generated by Draft a security-advisory fix · claude · sonnet50 · 635.8 AIC · ⌖ 27.6 AIC · ⊞ 7.7K · ◷

…n permission/role updates and login tokens

UpdateService::updatePermissions()/updateRoles() previously let any caller holding the
generic "users" permission grant arbitrary permissions and roles - including ones the
caller did not hold themselves - to any user or role, and getLoginTokenUrl() accepted a
client-supplied URL and did not bound the target user's privileges to the caller's.

Centralize a "no privilege escalation beyond the caller's own effective permissions"
guard in UpdateService (reused by both the user and role update paths) and in
UserLoginService::getLoginTokenUrl(), which also drops the client-supplied URL
parameter and schema entirely, generating the login URL server-side.

Co-Authored-By: Claude <noreply@anthropic.com>
@pimcore-deployments pimcore-deployments added ai-generated This content was generated using AI. Security labels Oct 27, 2026
…kens

2 new UserLoginServiceTest cases failed with a TypeError: AuthenticationResolverInterface::generateTokenByUser() requires a concrete Pimcore\Model\User, but the tests passed a UserInterface mock as $targetUser. Production code (unchanged by this PR) always passes the repository's User instance here. Fixed by constructing a real User() (it is final, so it cannot be mocked) via a new createTargetUser() helper, matching the pattern already used in UpdateServiceTest. No production code or security-check assertions were changed.

Co-Authored-By: Claude <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@mcop1 mcop1 added this to the 2026.3.4 milestone Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-generated This content was generated using AI. Security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants