Skip to content

Commit 5733fcb

Browse files
fix(occ): prevent params command key from overriding validated allowlist command (#41577)
* fix(occ): prevent params command key from overriding validated allowlist 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> * chore: add changelog entry for #41577 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
1 parent bfdf515 commit 5733fcb

3 files changed

Lines changed: 48 additions & 0 deletions

File tree

changelog/unreleased/41577

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Security: Prevent params body from overriding validated occ command
2+
3+
OccController validated the URL-path command against an allowlist but
4+
then merged it with user-supplied params via array_merge, allowing a
5+
command key in the request body to overwrite the validated value. An
6+
authenticated caller with the updater secret could use this to execute
7+
any occ command regardless of the allowlist. The params array is now
8+
stripped of any command key before the merge.
9+
10+
https://github.com/owncloud/core/pull/41577

core/Controller/OccController.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ public function execute($command, $token, $params = []) {
106106
$this->console->setAutoExit(false);
107107
$this->console->loadCommands(new ArrayInput([]), $output);
108108

109+
// Prevent user-supplied params from overriding the validated command
110+
unset($params['command']);
109111
$inputArray = \array_merge(['command' => $command], $params);
110112
$input = new ArrayInput($inputArray);
111113

tests/Core/Controller/OccControllerTest.php

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,42 @@ public function testWrongToken() {
8484
$this->assertEquals('updater.secret does not match the provided token', $responseData['details']);
8585
}
8686

87+
/**
88+
* Ensure that a 'command' key inside params cannot override the validated
89+
* command (CVE allowlist-bypass via array_merge key collision).
90+
*/
91+
public function testParamsCommandKeyCannotOverrideValidatedCommand() {
92+
$this->getControllerMock('localhost');
93+
94+
// Track which command the console is actually asked to run
95+
$capturedInput = null;
96+
$this->console->expects($this->once())->method('run')
97+
->willReturnCallback(
98+
function ($input, $output) use (&$capturedInput) {
99+
$capturedInput = $input;
100+
return 0;
101+
}
102+
);
103+
104+
// 'status' is an allowed command; 'user:resetpassword' is not.
105+
// The attacker embeds 'command' => 'user:resetpassword' inside params.
106+
$response = $this->controller->execute(
107+
'status',
108+
self::TEMP_SECRET,
109+
['command' => 'user:resetpassword', '--output' => 'json']
110+
);
111+
$responseData = $response->getData();
112+
113+
// Request must succeed (the validated command ran)
114+
$this->assertArrayHasKey('exitCode', $responseData);
115+
$this->assertEquals(0, $responseData['exitCode']);
116+
117+
// The ArrayInput actually passed to the console must carry 'status',
118+
// not the attacker-supplied 'user:resetpassword'.
119+
$this->assertNotNull($capturedInput);
120+
$this->assertEquals('status', $capturedInput->getFirstArgument());
121+
}
122+
87123
public function testSuccess() {
88124
$this->getControllerMock('localhost');
89125
$this->console->expects($this->once())->method('run')

0 commit comments

Comments
 (0)