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

[oc-tmueller]

Summary

An authenticated admin could set the core appconfig key public_webdav (or any public_/remote_ key) to a path-traversal value and have it require_once'd by public.php on the next GET /public.php/webdav, achieving remote code execution.

Two independent defects made this possible; both are fixed here (defense in depth).

1. Sink — public.php had no traversal check (essential fix)

public.php included the stored handler path relative to the app directory with no traversal guard, unlike remote.php which already rejects ../. Added the same guard so a traversal path can never be included — this is the DB- and endpoint-independent gate.

2. Guard bypass — strict $app === 'core' compare

AppConfigController blocked admins from setting public_/remote_ keys on core with a strict $app === 'core' compare. A mangled app id such as "core " (trailing space) is not equal to "core" in PHP, yet the database folds it back to the core row — defeating the guard. This is the same defeat mechanism as the earlier core%81 truncation bypass. The app id is now normalized (cleanAppId + trim + strtolower) before the check, in:

• AppConfigController::getValue/setValue/deleteKey
• the legacy core/ajax/appconfig.php endpoint
• and deleteApp now refuses to wipe the whole core appconfig.

Tests

Adds regression tests covering the mangled core spellings ("core ", " core", "CORE", "core/", "core..") and allowed near-misses (encore, non-service keys on core).

Verified in the ownCloud CI toolchain (PHP 7.4, PHPUnit 9.6, sqlite): OK — 40 tests, 105 assertions. Reverting the guard to the strict compare makes 13 of the new cases fail, confirming they catch the bypass.

---

Resubmission of #41802, re-authored and SSH-signed under @oc-tmueller.

[CLAassistant]

All committers have signed the CLA.

[phil-davis]

What do we do about this?

The PR number is off-by-one. The master PR #41804 has the correct numbers.

[phil-davis]

@oc-tmueller you can decide if you update the PR number in the changelog entry.

[kw-fscheuer]

Thanks for the two-layer approach — the reported PoC is definitely dead: "core " no longer slips past isCoreApp(), and files/../…/poc.php is rejected before the include. I'm requesting changes on one blocking point: the new guard in public.php isn't yet a containment check, so the bug class isn't closed.

1. public.php doesn't contain the include to the app directory (blocking)

public.php:65-67 rejects ../ and /.., but the include at public.php:85 is still

require_once OC_App::getAppPath($app) . '/' . $parts[1];

and getAppPath() returns false for an app id that has no directory on disk (lib/private/App/AppManager.php:682-690), while OC_App::loadApp() returns silently in that case rather than failing (lib/private/legacy/app.php:174-179). false . '/' is '/', so the include path becomes absolute — and an absolute path contains no ../, so the new guard passes it.

The isInstalled($app) check at public.php:77 doesn't stop that, because it isn't a filesystem check: AppManager::isInstalled() is isset($installedApps[$appId]) over getInstalledAppsValues(), which is built purely from appconfig enabled rows (AppManager.php:136-151, :256-259). A row for an app id that doesn't exist on disk can be created through this same admin endpoint ({"app":"<any-id>","key":"enabled","value":"yes"} — no guard applies, the app id isn't core). With such a row in place, a stored handler value of <that-id>/<absolute-path-to-a-php-file> is included from /, with no traversal sequence anywhere in the value.

Suggested containment check at the include site (keep the cheap substring check in front of it as a fast path if you like):

$appPath = OC_App::getAppPath($app);
if ($appPath === false || !isset($parts[1]) || $parts[1] === '') {
    throw new Exception('Path not allowed');
}
$base   = \realpath($appPath);
$target = \realpath($appPath . '/' . $parts[1]);
if ($base === false || $target === false
    || \strpos($target, $base . '/') !== 0
    || \substr($target, -4) !== '.php') {
    throw new Exception('Path not allowed');
}
require_once $target;

That is DB-independent — the property the commit message wants from this layer — and it also covers a symlink out of the app tree. Legitimate values are unaffected: every programmatic writer stores <appId>/<path-from-info.xml> (lib/private/legacy/app.php:989,992, lib/private/Installer.php:160,163 and :557,560), which always resolves inside the app directory.

remote.php builds its include path the same way in the default: branch of switch ($app), so it needs the same treatment (the core branch there is fine — hardcoded services + OC::$SERVERROOT).

2. The app-id normalization can't enumerate database foldings

isCoreApp() (settings/Controller/AppConfigController.php:115-117) catches whitespace, case, / and ... But the only requirement for a bypass is that the written app id be collation-equal to core, which is an open set. Two spellings that survive strtolower(trim(cleanAppId(...))):

• a trailing invalid byte — i.e. the core%81 truncation this PR cites from OC10-5: cleanAppId() doesn't strip it and neither does trim();
• an accent-insensitive collation (utf8mb4_0900_ai_ci, the MySQL 8 default) makes e.g. córe compare equal to core.

Either way AppConfig::setValue() reconciles onto the existing row via UPDATE … WHERE appid = :app (lib/private/AppConfig.php:157-184). Rather than enumerating spellings, validating the charset and rejecting anything that isn't a canonical app id closes the class:

private function isCoreApp($app) {
    $app = (string)$app;
    // any spelling the database could fold onto "core" is outside this charset
    if (\preg_match('/^[a-z0-9_.-]{1,32}$/', $app) !== 1) {
        return true;   // not a canonical app id → refuse the service-key write
    }
    return $app === 'core';
}

(Returning 400 for a non-canonical app id in setValue/deleteKey/deleteApp would be equivalent and clearer. A key-prefix-only block would over-reach — files_sharing legitimately owns public_share_sharers_groups_allowlist*, apps/files_sharing/lib/SharingAllowlist.php:72,80.)

3. core/ajax/appconfig.php didn't get the deleteApp half

The legacy endpoint's guard (core/ajax/appconfig.php:41-42) only fires when $_POST['key'] is set, so action=deleteApp&app=core still reaches $appConfig->deleteApp($app) at :69 and drops every core row — including the programmatically registered public_*/remote_* handlers that the new block at AppConfigController.php:171 exists to protect. The route is live (core/routes.php:90-91).

if ($normalizedApp === 'core' && $action === 'deleteApp') {
    OC_JSON::error(['data' => ['message' => 'Unexpected error!']]);
    return;
}

4. Smaller notes

• The sink guard — the layer the commit message calls essential — is the one with no test; all the new assertions are on the controller. An acceptance scenario (occ config:app:set core public_zzz --value "files/../../../etc/passwd", then GET /public.php/zzz) would pin it, plus the app-id-without-a-directory case once the containment check is in.
• changelog/unreleased/41803 still links …/pull/41802 (as already noted above).

All of the above applies unchanged to #41804, which is byte-identical apart from the changelog.