Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions changelog/unreleased/41577
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Security: Prevent params body from overriding validated occ command

OccController validated the URL-path command against an allowlist but
then merged it with user-supplied params via array_merge, allowing a
command key in the request body to overwrite the validated value. An
authenticated caller with the updater secret could use this to execute
any occ command regardless of the allowlist. The params array is now
stripped of any command key before the merge.

https://github.com/owncloud/core/pull/41577
2 changes: 2 additions & 0 deletions core/Controller/OccController.php
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ public function execute($command, $token, $params = []) {
$this->console->setAutoExit(false);
$this->console->loadCommands(new ArrayInput([]), $output);

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

Expand Down
36 changes: 36 additions & 0 deletions tests/Core/Controller/OccControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,42 @@ public function testWrongToken() {
$this->assertEquals('updater.secret does not match the provided token', $responseData['details']);
}

/**
* Ensure that a 'command' key inside params cannot override the validated
* command (CVE allowlist-bypass via array_merge key collision).
*/
public function testParamsCommandKeyCannotOverrideValidatedCommand() {
$this->getControllerMock('localhost');

// Track which command the console is actually asked to run
$capturedInput = null;
$this->console->expects($this->once())->method('run')
->willReturnCallback(
function ($input, $output) use (&$capturedInput) {
$capturedInput = $input;
return 0;
}
);

// 'status' is an allowed command; 'user:resetpassword' is not.
// The attacker embeds 'command' => 'user:resetpassword' inside params.
$response = $this->controller->execute(
'status',
self::TEMP_SECRET,
['command' => 'user:resetpassword', '--output' => 'json']
);
$responseData = $response->getData();

// Request must succeed (the validated command ran)
$this->assertArrayHasKey('exitCode', $responseData);
$this->assertEquals(0, $responseData['exitCode']);

// The ArrayInput actually passed to the console must carry 'status',
// not the attacker-supplied 'user:resetpassword'.
$this->assertNotNull($capturedInput);
$this->assertEquals('status', $capturedInput->getFirstArgument());
}

public function testSuccess() {
$this->getControllerMock('localhost');
$this->console->expects($this->once())->method('run')
Expand Down