Skip to content

fix(occ): prevent params command key from overriding validated allowlist command#41577

Open
DeepDiver1975 wants to merge 2 commits into
masterfrom
security/fix-occ-allowlist-bypass
Open

fix(occ): prevent params command key from overriding validated allowlist command#41577
DeepDiver1975 wants to merge 2 commits into
masterfrom
security/fix-occ-allowlist-bypass

Conversation

@DeepDiver1975

Copy link
Copy Markdown
Member

Summary

  • OccController::execute() validated the URL-path $command against the allowlist, then merged it with user-supplied $params via array_merge(['command' => $command], $params)
  • PHP's array_merge with string keys causes later duplicates to overwrite earlier ones — a command key in the POST body bypassed the allowlist entirely
  • Fix: unset($params['command']) before the merge; one-line change, complete protection

Security Impact

Critical — authenticated updater callers with updater.secret + localhost access can execute any occ command (user:resetpassword, config:system:set, encryption:decrypt-all, etc.) regardless of the allowlist

Test plan

  • testParamsCommandKeyCannotOverrideValidatedCommand sends user:resetpassword in params while URL says status; asserts console receives status — fails without fix
  • Run make test TEST_PHP_SUITE=core/Controller

🤖 Generated with Claude Code

…ist command

OccController::execute() validated the URL-path $command against the
allowlist, then called array_merge(["command"=>$command], $params).
PHP's array_merge with string keys lets later values overwrite earlier
ones, so a "command" key in the POST body $params silently replaced the
validated command, allowing execution of any occ command.

Add unset($params["command"]) before the merge to strip any
attacker-supplied command key, ensuring the validated value is always
what reaches Symfony Console.

Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
@update-docs

update-docs Bot commented Jun 5, 2026

Copy link
Copy Markdown

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — fix(occ): prevent params command key from overriding validated allowlist command

Overview: OccController::execute() validated $command (from the URL path) against an allowlist, then built the ArrayInput via array_merge(['command' => $command], $params). PHP's array_merge with string keys causes later values to overwrite earlier ones, so a command key in the POST body replaced the validated command entirely. Fix: unset($params['command']) before the merge.

Correctness

The fix is correct and minimal:

unset($params['command']);
$inputArray = \array_merge(['command' => $command], $params);

After the unset, array_merge can never find a second command key in $params, so the validated value always wins. ✅

Why not use array_merge($params, ['command' => $command]) instead? Reversing the argument order would also work but would make the intent less obvious to a reader, and would still require reasoning about key collision. The explicit unset is clearer. ✅

Test

testParamsCommandKeyCannotOverrideValidatedCommand:

  • Sends 'command' => 'user:resetpassword' in params while URL command is 'status'
  • Captures the ArrayInput actually passed to console->run() via willReturnCallback
  • Asserts $capturedInput->getFirstArgument() === 'status'
  • Asserts exitCode === 0 (the allowed command ran successfully) ✅

This is a strong regression test — it directly asserts that the console receives the validated command, not the attacker-supplied one. Without the fix, getFirstArgument() would return 'user:resetpassword' and the test would fail. ✅

Summary

Aspect Assessment
Allowlist bypass ✅ Closed — unset before merge
Fix scope ✅ Minimal — one line
Test ✅ Captures actual console input, asserts correct command
Changelog ✅ Accurate

Verdict: Ready to merge. Critical security fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant