fix(command): restrict allowed_classes in CommandJob unserialize() to prevent RCE

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

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 {

tests/lib/Command/CommandJobTest.php

@@ -0,0 +1,130 @@
+<?php
+/**
+ * @author Thomas Müller <thomas.mueller@tmit.eu>
+ *
+ * @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 <http://www.gnu.org/licenses/>
+ *
+ */
+
+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'));
+	}
+}