From 0737fa549ff4ac7a8f1a3a91d7b573d9dceb365d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 14:26:19 +0200 Subject: [PATCH 1/2] fix(command): restrict allowed_classes in CommandJob unserialize() to prevent RCE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CommandJob::run() called unserialize() without the allowed_classes option, enabling PHP Object Injection via POP gadget chains (Guzzle, Symfony, Doctrine) if an attacker can write to the oc_jobs table. Replace bare unserialize() with a two-pass strategy: Pass 1 uses allowed_classes=false to safely extract the stored class name without instantiating anything. After verifying the class implements ICommand, Pass 2 permits only that specific class, blocking all gadget chains. Signed-off-by: Thomas Müller Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- lib/private/Command/CommandJob.php | 29 +++++- tests/lib/Command/CommandJobTest.php | 130 +++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 tests/lib/Command/CommandJobTest.php diff --git a/lib/private/Command/CommandJob.php b/lib/private/Command/CommandJob.php index a935300303b8..ac2ae9ec2327 100644 --- a/lib/private/Command/CommandJob.php +++ b/lib/private/Command/CommandJob.php @@ -29,7 +29,34 @@ */ class CommandJob extends QueuedJob { protected function run($serializedCommand) { - $command = \unserialize($serializedCommand); + // First pass: deserialize without instantiating any objects so that no + // __wakeup() / __destruct() magic methods are triggered on untrusted data. + // This lets us safely extract the stored class name. + $incomplete = \unserialize($serializedCommand, ['allowed_classes' => false]); + + // Retrieve the class name encoded in the serialized string. When + // allowed_classes is false PHP returns an __PHP_Incomplete_Class whose + // ::class name is exposed via the "__PHP_Incomplete_Class_Name" property. + if (!($incomplete instanceof \__PHP_Incomplete_Class)) { + throw new \InvalidArgumentException('Invalid serialized command: expected a serialized object'); + } + $classNameKey = '__PHP_Incomplete_Class_Name'; + $className = ((array) $incomplete)[$classNameKey] ?? null; + if (!\is_string($className) || $className === '') { + throw new \InvalidArgumentException('Invalid serialized command: could not determine class name'); + } + + // Verify that the stored class name is a known, loaded class that + // actually implements ICommand before we allow it to be instantiated. + if (!\class_exists($className) || !\is_a($className, ICommand::class, true)) { + throw new \InvalidArgumentException( + 'Invalid serialized command: class "' . $className . '" does not implement ICommand' + ); + } + + // Second pass: now safe to deserialize – only the verified class is + // permitted, so no gadget chains from other classes can be triggered. + $command = \unserialize($serializedCommand, ['allowed_classes' => [$className]]); if ($command instanceof ICommand) { $command->handle(); } else { diff --git a/tests/lib/Command/CommandJobTest.php b/tests/lib/Command/CommandJobTest.php new file mode 100644 index 000000000000..e1b0a8df2751 --- /dev/null +++ b/tests/lib/Command/CommandJobTest.php @@ -0,0 +1,130 @@ + + * + * @copyright Copyright (c) 2024, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Command; + +use OC\Command\CommandJob; +use OCP\Command\ICommand; +use PHPUnit\Framework\TestCase; + +/** + * A legitimate ICommand used to verify the happy path still works. + */ +class DummyICommand implements ICommand { + /** @var bool */ + public static $handled = false; + + public function handle() { + self::$handled = true; + } +} + +/** + * A class that does NOT implement ICommand – used to prove the gadget-chain + * scenario is blocked: if an attacker serializes an arbitrary object and + * stores it in oc_jobs.argument, CommandJob::run() must refuse it. + * + * The __wakeup() method sets a flag so we can assert it was never called. + */ +class MaliciousGadget { + /** @var bool */ + public static $wakeupCalled = false; + + public function __wakeup() { + self::$wakeupCalled = true; + } +} + +/** + * Thin subclass that exposes the protected run() method for unit testing + * without needing a full background-job infrastructure. + */ +class TestableCommandJob extends CommandJob { + public function runPublic(string $serialized): void { + $this->run($serialized); + } +} + +/** + * @package Test\Command + */ +class CommandJobTest extends TestCase { + protected function setUp(): void { + parent::setUp(); + DummyICommand::$handled = false; + MaliciousGadget::$wakeupCalled = false; + } + + // ------------------------------------------------------------------ + // Happy-path: a properly serialized ICommand must be executed + // ------------------------------------------------------------------ + + public function testRunExecutesLegitimateICommand(): void { + $serialized = \serialize(new DummyICommand()); + $job = new TestableCommandJob(); + $job->runPublic($serialized); + $this->assertTrue(DummyICommand::$handled, 'ICommand::handle() should have been called'); + } + + // ------------------------------------------------------------------ + // Security: a serialized non-ICommand object must be rejected AND + // no magic methods (__wakeup / __destruct) may be triggered on it. + // + // BEFORE the fix: unserialize() was called unconditionally, so + // __wakeup() on the gadget fires → $wakeupCalled becomes true → test + // fails on assertFalse below. + // + // AFTER the fix: the first-pass unserialize uses allowed_classes=>false + // (no magic methods triggered), the class-name check throws before any + // real instantiation, so $wakeupCalled stays false. + // ------------------------------------------------------------------ + + public function testMagicMethodsNotTriggeredOnUntrustedPayload(): void { + $serialized = \serialize(new MaliciousGadget()); + $job = new TestableCommandJob(); + + try { + $job->runPublic($serialized); + $this->fail('An InvalidArgumentException must be thrown for non-ICommand payloads'); + } catch (\InvalidArgumentException $e) { + // expected – fall through to the critical assertion below + } + + // The key security assertion: __wakeup() must NOT have fired, proving + // that no PHP object injection gadget chain could be triggered. + $this->assertFalse( + MaliciousGadget::$wakeupCalled, + '__wakeup() must not be called on untrusted serialized objects (PHP Object Injection guard)' + ); + } + + // ------------------------------------------------------------------ + // Security: a plain scalar stored in the argument column must be + // rejected (not an object at all). + // ------------------------------------------------------------------ + + public function testRunRejectsNonObjectPayload(): void { + $job = new TestableCommandJob(); + + $this->expectException(\InvalidArgumentException::class); + $job->runPublic(\serialize('just a string')); + } +} From b8569b14effa55153ea43f57ee7ae7cc7593cc1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 15:58:57 +0200 Subject: [PATCH 2/2] chore: add changelog entry for #41582 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41582 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/41582 diff --git a/changelog/unreleased/41582 b/changelog/unreleased/41582 new file mode 100644 index 000000000000..54604b8a823b --- /dev/null +++ b/changelog/unreleased/41582 @@ -0,0 +1,10 @@ +Security: Restrict unserialize() allowed classes in CommandJob + +CommandJob::run() called unserialize() without the allowed_classes +option on data sourced from the oc_jobs database table. An attacker +with database write access could inject a crafted PHP object payload +to trigger gadget chains from bundled libraries and achieve remote +code execution. Deserialization is now restricted to verified ICommand +implementations only. + +https://github.com/owncloud/core/pull/41582