diff --git a/changelog/unreleased/41577 b/changelog/unreleased/41577 new file mode 100644 index 000000000000..6738a8b089a4 --- /dev/null +++ b/changelog/unreleased/41577 @@ -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 diff --git a/core/Controller/OccController.php b/core/Controller/OccController.php index 2e0e7e29716f..5a1d18d7f1b0 100644 --- a/core/Controller/OccController.php +++ b/core/Controller/OccController.php @@ -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); diff --git a/tests/Core/Controller/OccControllerTest.php b/tests/Core/Controller/OccControllerTest.php index c71ec344d812..c4fd02e86c43 100644 --- a/tests/Core/Controller/OccControllerTest.php +++ b/tests/Core/Controller/OccControllerTest.php @@ -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')