fix: prevent appconfig path traversal to RCE via public_/remote_ keys (OC10-146)

changelog/unreleased/41803

@@ -0,0 +1,10 @@
+Security: Prevent path traversal via appconfig public_/remote_ keys
+
+We've fixed a path traversal in the appconfig `public_`/`remote_` service
+handlers. An authenticated admin could set such a key on the `core` app to a
+traversal value which was later included by `public.php`, leading to remote
+code execution. The included path is now validated against traversal and the
+app-id guard can no longer be bypassed by mangled spellings such as a trailing
+space.
+
+https://github.com/owncloud/core/pull/41802

core/ajax/appconfig.php

@@ -35,7 +35,11 @@
 // on its own. This should only be possible programmatically.
 // This change is due the fact that an admin may not be expected
 // to execute arbitrary code in every environment.
-if ($app === 'core' && isset($_POST['key']) &&(\substr((string)$_POST['key'], 0, 7) === 'remote_' || \substr((string)$_POST['key'], 0, 7) === 'public_')) {
+// The app id is normalized (cleanAppId does not strip whitespace/case) so that
+// mangled spellings which the database folds back to the "core" row
+// (e.g. "core " with a trailing space) cannot slip past the guard. See OC10-146.
+$normalizedApp = isset($app) ? \strtolower(\trim((string)$app)) : $app;
+if ($normalizedApp === 'core' && isset($_POST['key']) &&(\substr((string)$_POST['key'], 0, 7) === 'remote_' || \substr((string)$_POST['key'], 0, 7) === 'public_')) {
 	OC_JSON::error(['data' => ['message' => 'Unexpected error!']]);
 	return;
 }

public.php

@@ -57,6 +57,15 @@
 		exit;
 	}
 
+	// The stored handler path is later require_once'd relative to the app
+	// directory, so reject any path traversal to prevent inclusion (and thus
+	// execution) of files outside the app tree. This mirrors the guard in
+	// remote.php and defends against a poisoned "core" public_* appconfig
+	// value (see OC10-146 / OC10-5).
+	if (\strpos($file, '../') !== false || \strpos($file, '/..') !== false) {
+		throw new Exception('Path not allowed');
+	}
+
 	$parts = \explode('/', $file, 2);
 	$app = $parts[0];
 

settings/Controller/AppConfigController.php

@@ -74,12 +74,48 @@ public function getKeys($app) {
 	 * @param string $default
 	 */
 	public function getValue($app, $key, $default = null) {
-		if ($app === 'core' && (\strpos((string)$key, 'remote_') === 0 || \strpos((string)$key, 'public_') === 0)) {
+		if ($this->isProtectedCoreServiceKey($app, $key)) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
 		return new JSONResponse($this->appConfig->getValue($app, $key, $default));
 	}
 
+	/**
+	 * Whether the given app/key pair targets a protected "core" remote_/public_
+	 * service handler. These handlers are require_once'd by remote.php/public.php
+	 * and may only be registered programmatically (from an app's info.xml), never
+	 * through this admin-facing controller, otherwise an admin can point them at
+	 * an arbitrary file and achieve code execution.
+	 *
+	 * The app name is normalized before the comparison so that mangled spellings
+	 * which the database folds back to the "core" row (e.g. "core " with a
+	 * trailing space, "CORE", "core/") cannot slip past the guard. See OC10-146
+	 * and the related OC10-5. Note this is best-effort defense-in-depth: the
+	 * authoritative protection against traversal is the containment check at the
+	 * include sites in public.php/remote.php.
+	 *
+	 * @param string $app
+	 * @param string $key
+	 * @return bool
+	 */
+	private function isProtectedCoreServiceKey($app, $key) {
+		return $this->isCoreApp($app)
+			&& (\strpos((string)$key, 'remote_') === 0 || \strpos((string)$key, 'public_') === 0);
+	}
+
+	/**
+	 * Whether the given (possibly mangled) app id resolves to the "core" app.
+	 * The name is stripped and normalized so that spellings which the database
+	 * folds back to the "core" row (e.g. "core " with a trailing space, "CORE",
+	 * "core/") are all recognised. See OC10-146.
+	 *
+	 * @param string $app
+	 * @return bool
+	 */
+	private function isCoreApp($app) {
+		return \strtolower(\trim(\OC_App::cleanAppId((string)$app))) === 'core';
+	}
+
 	/**
 	 * Set the value for the target key in the app. If no value is provided,
 	 * the request will fail.
@@ -96,7 +132,7 @@ public function setValue($app, $key, $value) {
 		// on its own. This should only be possible programmatically.
 		// This change is due the fact that an admin may not be expected
 		// to execute arbitrary code in every environment.
-		if ($app === 'core' && (\strpos((string)$key, 'remote_') === 0 || \strpos((string)$key, 'public_') === 0)) {
+		if ($this->isProtectedCoreServiceKey($app, $key)) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
 
@@ -112,7 +148,7 @@ public function deleteKey($app, $key) {
 		if (!isset($app, $key)) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
-		if ($app === 'core' && (\strpos((string)$key, 'remote_') === 0 || \strpos((string)$key, 'public_') === 0)) {
+		if ($this->isProtectedCoreServiceKey($app, $key)) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
 
@@ -129,6 +165,13 @@ public function deleteApp($app) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
 
+		// Deleting the whole "core" appconfig would drop the programmatically
+		// managed remote_/public_ service handlers (and all other core config),
+		// so it must never be possible through this admin-facing controller.
+		if ($this->isCoreApp($app)) {
+			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
+		}
+
 		return new JSONResponse($this->appConfig->deleteApp($app));
 	}
 }

tests/Settings/Controller/AppConfigControllerTest.php

@@ -90,6 +90,14 @@ public function setValueProvider() {
 			['appId1', 'key1', null],
 			['core', 'remote_key1', 'foo'],
 			['core', 'public_key1', 'foo'],
+			// mangled "core" app ids that the database folds back to the
+			// "core" row must not slip past the guard (OC10-146 / OC10-5)
+			['core ', 'public_webdav', 'files/../../../poc.php'],
+			[' core', 'public_key1', 'foo'],
+			['CORE', 'public_key1', 'foo'],
+			['Core', 'remote_key1', 'foo'],
+			['core/', 'public_key1', 'foo'],
+			['core..', 'remote_key1', 'foo'],
 		];
 	}
 
@@ -105,6 +113,44 @@ public function testSetValueWrong($app, $key, $value): void {
 		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
 	}
 
+	public function getValueWrongProvider(): array {
+		return [
+			['core', 'remote_key1'],
+			['core', 'public_key1'],
+			['core ', 'public_webdav'],
+			['CORE', 'public_key1'],
+			['core/', 'remote_key1'],
+		];
+	}
+
+	/**
+	 * @dataProvider getValueWrongProvider
+	 */
+	public function testGetValueWrong($app, $key): void {
+		$this->appConfig->expects($this->never())
+			->method('getValue');
+
+		$response = $this->appConfigController->getValue($app, $key, null);
+		$this->assertEquals([], $response->getData());
+		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
+	}
+
+	/**
+	 * The guard must only block the "core" app; unrelated apps whose id merely
+	 * contains "core", and non-service keys on core, must still work.
+	 */
+	public function testSetValueAllowedNearMisses(): void {
+		$this->appConfig->expects($this->exactly(2))
+			->method('setValue')
+			->willReturn(true);
+
+		$response = $this->appConfigController->setValue('encore', 'public_key1', 'foo');
+		$this->assertSame(Http::STATUS_OK, $response->getStatus());
+
+		$response = $this->appConfigController->setValue('core', 'some_other_key', 'foo');
+		$this->assertSame(Http::STATUS_OK, $response->getStatus());
+	}
+
 	public function testDeleteKey(): void {
 		$this->appConfig->method('deleteKey')
 			->with('appId003', 'key3')
@@ -122,6 +168,11 @@ public function deleteKeyProvider(): array {
 			['appId1', null, null],
 			['core', 'remote_key1', 'foo'],
 			['core', 'public_key1', 'foo'],
+			// mangled "core" app ids must be rejected here too (OC10-146)
+			['core ', 'public_webdav'],
+			[' core', 'public_key1'],
+			['CORE', 'remote_key1'],
+			['core/', 'public_key1'],
 		];
 	}
 
@@ -148,11 +199,27 @@ public function testDeleteApp(): void {
 		$this->assertSame(Http::STATUS_OK, $response->getStatus());
 	}
 
-	public function testDeleteAppWrong(): void {
+	public function deleteAppWrongProvider(): array {
+		return [
+			[null],
+			// deleting the "core" appconfig (or a mangled spelling that folds
+			// back to it) must be rejected (OC10-146)
+			['core'],
+			['core '],
+			[' core'],
+			['CORE'],
+			['core/'],
+		];
+	}
+
+	/**
+	 * @dataProvider deleteAppWrongProvider
+	 */
+	public function testDeleteAppWrong($app): void {
 		$this->appConfig->expects($this->never())
 			->method('deleteApp');
 
-		$response = $this->appConfigController->deleteApp(null);
+		$response = $this->appConfigController->deleteApp($app);
 		$this->assertEquals([], $response->getData());
 		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
 	}