From a1a5c030ddc00fcf66e6e536155bb0c2fd344075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:53:36 +0200 Subject: [PATCH 1/5] fix(user): confine backend provided user homes to the data directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user backend can supply a per user home directory. SyncService::syncHome() accepted whatever the backend returned after nothing more than a check for a leading slash, and persisted it on the account. Such a value is not necessarily under the control of the ownCloud administrator - the LDAP backend for instance can be configured to read the home from a per user attribute. A home pointing at the ownCloud code directory turns the user's file listing into read and write access to the application's own PHP files; writing a PHP file into a web reachable location, or modifying one of the shipped ones, results in remote code execution. Storage\Local::getSourcePath() looks like it would catch this, but it anchors its containment check to a data directory it derives from the very home it is validating, so it validates the path against itself. A backend provided home is now refused unless it resolves inside the configured `datadirectory`. Installations that legitimately keep user homes elsewhere, for instance on a separate NFS mount, can list the permitted base directories in the new `user.home_base_dirs` config option. The refusal is logged with the backend class, the offending path and the name of the config option. Symlinks are resolved on the longest leading part of the path that exists on disk before the comparison, because a home directory is created lazily on first login and usually does not exist yet when it is validated. That keeps a symlinked data directory working while preventing a symlink inside it from being used to escape. The containment check compares against the base directory plus a separator, so a sibling directory sharing the prefix (/data-evil vs /data) is not mistaken for being contained. syncHome() is the single point every backend's home passes through, so this covers all of them. The LDAP backend carries the matching check at its own entry point in owncloud/user_ldap#849. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- config/config.sample.php | 21 ++++ lib/private/User/SyncService.php | 127 +++++++++++++++++++- tests/lib/User/SyncServiceTest.php | 187 +++++++++++++++++++++++++++++ 3 files changed, 334 insertions(+), 1 deletion(-) diff --git a/config/config.sample.php b/config/config.sample.php index fe603c2c3795..e0fc2af9de25 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -392,6 +392,27 @@ */ 'user.search_min_length' => 2, +/** + * Additional base directories a user home may be located in + * + * User backends can supply a per user home directory - the LDAP backend for + * instance can be configured to read it from a user attribute. Such a value is + * not necessarily controlled by the ownCloud administrator, so a home is only + * accepted when it lies inside the configured `datadirectory`. A home pointing + * at the ownCloud code directory would otherwise expose - and allow overwriting + * - the application's own files. + * + * If user homes legitimately live outside of the data directory, for example on + * a separate NFS mount, list the permitted base directories here. Any home + * inside one of them is accepted. Keep this list as narrow as possible and never + * include the ownCloud code directory. + * + * Defaults to `[]`, i.e. only the data directory is permitted. + * + * The LDAP backend honours the analogous `user_ldap.home_base_dirs` option. + */ +'user.home_base_dirs' => [], + /** * Mail Parameters * diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 640c1cce957f..28324bd9c0d0 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -244,6 +244,7 @@ private function syncQuota(Account $a, UserInterface $backend) { /** * @param Account $a * @param UserInterface $backend + * @throws \OutOfBoundsException if the backend provides a home outside of the permitted base directories */ private function syncHome(Account $a, UserInterface $backend) { // Fallback for backends that dont yet use the new interfaces @@ -265,12 +266,21 @@ private function syncHome(Account $a, UserInterface $backend) { if ($providesHome) { $home = $backend->getHome($uid); } + $dataDir = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data'); if (!\is_string($home) || $home[0] !== '/') { - $home = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . "/$uid"; + $home = $dataDir . "/$uid"; $this->logger->debug( 'User backend ' .\get_class($backend)." provided no home for <$uid>", ['app' => self::class] ); + } else { + // A backend provided home is not necessarily under the control of + // the administrator - an LDAP directory can carry a per user home + // attribute. Accepting one that points at the ownCloud code root + // would turn the user's file listing into read/write access to the + // application's own PHP files, so confine it to the data directory + // and to any additional base directory the admin opted into. + $this->verifyHomeLocation($home, $dataDir, $uid, \get_class($backend)); } // This will set the home if not provided by the backend $a->setHome($home); @@ -283,6 +293,121 @@ private function syncHome(Account $a, UserInterface $backend) { } } + /** + * Ensure a backend provided home lies within the data directory or within one + * of the base directories the administrator explicitly permitted. + * + * @param string $home + * @param string $dataDir + * @param string $uid + * @param string $backendClass + * @throws \OutOfBoundsException if the home is not contained in a permitted base directory + */ + private function verifyHomeLocation($home, $dataDir, $uid, $backendClass) { + $normalizedHome = self::normalizePath($home); + $baseDirs = $this->config->getSystemValue('user.home_base_dirs', []); + if (!\is_array($baseDirs)) { + $baseDirs = []; + } + \array_unshift($baseDirs, $dataDir); + + foreach ($baseDirs as $baseDir) { + if (!\is_string($baseDir) || $baseDir === '') { + continue; + } + if (self::isContainedIn($normalizedHome, self::normalizePath($baseDir))) { + return; + } + } + + $this->logger->error( + "User backend $backendClass returned home <$normalizedHome> for user <$uid> which is outside" . + " of the data directory <$dataDir> and of any directory listed in the 'user.home_base_dirs'" . + " config option - refusing it", + ['app' => self::class] + ); + throw new \OutOfBoundsException( + "Home provided by user backend $backendClass is not inside a permitted base directory for uid: $uid" + ); + } + + /** + * Resolve a path to a canonical form for comparison: symlinks are resolved on + * the longest leading part that exists on disk, the remainder is normalized + * lexically. realpath() alone cannot be used here because a home directory is + * created lazily on first login, so it usually does not exist yet when it is + * validated - but resolving the existing prefix keeps a symlink from pointing + * an apparently contained home somewhere else, and lets an admin have a + * symlinked data directory. + * + * @param string $path an absolute path + * @return string the resolved path, without a trailing slash + */ + private static function normalizePath($path) { + $lexical = self::normalizePathLexically($path); + + $suffix = []; + $candidate = $lexical; + while ($candidate !== '' && $candidate !== '/') { + $real = \realpath($candidate); + if ($real !== false) { + return \rtrim($real, '/') . (empty($suffix) ? '' : '/' . \implode('/', $suffix)); + } + \array_unshift($suffix, \basename($candidate)); + $parent = \dirname($candidate); + if ($parent === $candidate) { + break; + } + $candidate = $parent; + } + + return $lexical; + } + + /** + * Resolve '.', '..' and duplicate slashes without touching the filesystem. + * + * @param string $path + * @return string the normalized path, without a trailing slash + */ + private static function normalizePathLexically($path) { + $isAbsolute = isset($path[0]) && $path[0] === '/'; + $parts = []; + foreach (\explode('/', $path) as $part) { + if ($part === '' || $part === '.') { + continue; + } + if ($part === '..') { + // a leading '..' cannot escape the root, mirroring the kernel + if (\end($parts) === '..') { + $parts[] = $part; + } elseif (\array_pop($parts) === null && !$isAbsolute) { + $parts[] = $part; + } + continue; + } + $parts[] = $part; + } + return ($isAbsolute ? '/' : '') . \implode('/', $parts); + } + + /** + * Whether $path is $baseDir itself or lies underneath it. Both arguments are + * expected to be normalized already. + * + * @param string $path + * @param string $baseDir + * @return bool + */ + private static function isContainedIn($path, $baseDir) { + if ($path === $baseDir) { + return true; + } + // compare against the separator as well, so that a sibling directory + // sharing the base dir's prefix (/data-evil vs /data) is not accepted + return \strpos($path, \rtrim($baseDir, '/') . '/') === 0; + } + /** * @param Account $a * @param UserInterface $backend diff --git a/tests/lib/User/SyncServiceTest.php b/tests/lib/User/SyncServiceTest.php index 6cc4f907f695..501c931070c6 100644 --- a/tests/lib/User/SyncServiceTest.php +++ b/tests/lib/User/SyncServiceTest.php @@ -45,6 +45,8 @@ interface IUserInterfaceWithQuotaBackendTest extends UserInterface, IProvidesQuo } interface IUserInterfaceWithUserNameBackendTest extends UserInterface, IProvidesUserNameBackend { } +interface IUserInterfaceWithHomeBackendTest extends UserInterface, IProvidesHomeBackend { +} class SyncServiceTest extends TestCase { /** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */ @@ -166,6 +168,191 @@ public function testSyncHomeLogsWhenBackendDiffersFromExisting() { self::invokePrivate($s, 'syncHome', [$a, $backend]); } + /** + * A backend-supplied home is not necessarily under the control of the + * ownCloud administrator (an LDAP directory can set it per user). Accepting + * one that points at the ownCloud code root would hand out read/write access + * to the application's own PHP files, so it must be refused before it is + * persisted on the account. + * + * @dataProvider providesHomeOutsideDataDir + */ + public function testSyncHomeRefusesHomeOutsideDataDir($home) { + $this->expectException(\OutOfBoundsException::class); + + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn($home); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user.home_base_dirs') { + return []; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + // the malicious home must never reach the account + $a->expects($this->never())->method('setHome'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } + + public function providesHomeOutsideDataDir() { + return [ + 'application root' => ['/var/www/owncloud'], + 'apps directory' => ['/var/www/owncloud/apps'], + 'system directory' => ['/etc'], + 'traversal' => ['/var/www/owncloud/data/../../../etc'], + 'prefix sibling' => ['/var/www/owncloud/data-evil'], + ]; + } + + public function testSyncHomeAcceptsHomeInsideDataDir() { + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn('/var/www/owncloud/data/alice'); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user.home_base_dirs') { + return []; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome', 'getUpdatedFields'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + $a->expects($this->any())->method('getUpdatedFields')->willReturn([]); + $a->expects($this->once())->method('setHome')->with('/var/www/owncloud/data/alice'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } + + /** + * A symlink inside the data directory must not be usable to point a home that + * looks contained at a location outside of it. + */ + public function testSyncHomeRefusesHomeEscapingViaSymlink() { + $this->expectException(\OutOfBoundsException::class); + + $tmp = \realpath(\sys_get_temp_dir()) . '/oc-synchome-' . \uniqid(); + \mkdir($tmp . '/data', 0777, true); + \mkdir($tmp . '/apps', 0777, true); + \symlink($tmp . '/apps', $tmp . '/data/escape'); + + try { + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn($tmp . '/data/escape'); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($tmp) { + if ($key === 'datadirectory') { + return $tmp . '/data'; + } + if ($key === 'user.home_base_dirs') { + return []; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + $a->expects($this->never())->method('setHome'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } finally { + \unlink($tmp . '/data/escape'); + \rmdir($tmp . '/apps'); + \rmdir($tmp . '/data'); + \rmdir($tmp); + } + } + + /** + * A symlinked data directory is a legitimate setup and must keep working: the + * home is compared after resolving both sides. + */ + public function testSyncHomeAcceptsHomeUnderSymlinkedDataDir() { + $tmp = \realpath(\sys_get_temp_dir()) . '/oc-synchome-' . \uniqid(); + \mkdir($tmp . '/real-data', 0777, true); + \symlink($tmp . '/real-data', $tmp . '/data'); + + try { + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn($tmp . '/data/alice'); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($tmp) { + if ($key === 'datadirectory') { + return $tmp . '/data'; + } + if ($key === 'user.home_base_dirs') { + return []; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome', 'getUpdatedFields'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + $a->expects($this->any())->method('getUpdatedFields')->willReturn([]); + $a->expects($this->once())->method('setHome')->with($tmp . '/data/alice'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } finally { + \unlink($tmp . '/data'); + \rmdir($tmp . '/real-data'); + \rmdir($tmp); + } + } + + /** + * Admins with homes on a separate mount can opt back in explicitly. + */ + public function testSyncHomeAcceptsHomeInConfiguredBaseDir() { + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn('/mnt/nfs/homes/alice'); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user.home_base_dirs') { + return ['/mnt/nfs/homes']; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome', 'getUpdatedFields'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + $a->expects($this->any())->method('getUpdatedFields')->willReturn([]); + $a->expects($this->once())->method('setHome')->with('/mnt/nfs/homes/alice'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } + /** */ public function testTrySyncExistingUserWithOtherBackend() { From 3058f6fe2426ab6af4fbb3c3c9e2a86262271ae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:54:55 +0200 Subject: [PATCH 2/5] chore: add changelog entry for #41752 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41752 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 changelog/unreleased/41752 diff --git a/changelog/unreleased/41752 b/changelog/unreleased/41752 new file mode 100644 index 000000000000..0c824d8ffba1 --- /dev/null +++ b/changelog/unreleased/41752 @@ -0,0 +1,24 @@ +Security: Confine backend provided user homes to the data directory + +A user backend can supply a per user home directory - the LDAP backend for +instance can be configured to read it from a user attribute such as +homeDirectory. The account sync accepted that value after nothing more than a +check for a leading slash, so a home pointing at the ownCloud code directory +turned the user's file listing into read and write access to the application's +own PHP files. Writing a PHP file into a web reachable location, or modifying +one of the shipped ones, results in remote code execution. The relative form +was concatenated onto the data directory without normalization, so a value +containing ".." escaped it as well. + +A backend provided home is now rejected unless it resolves inside the +configured datadirectory. Installations that legitimately keep user homes +elsewhere, for example on a separate NFS mount, can list the permitted base +directories in the new "user.home_base_dirs" config option. Symlinks are +resolved before the comparison, so a symlinked data directory keeps working +while a symlink inside it cannot be used to escape. + +Note that a home is only set when an account has none yet, so accounts that were +provisioned before this change keep the home already stored for them. + +https://github.com/owncloud/core/pull/41752 +https://github.com/owncloud/user_ldap/pull/849 From 540592f4b72252f375443f8a01599811b262f3fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:03:40 +0200 Subject: [PATCH 3/5] docs(config): reword the user.home_base_dirs sample comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sentence wrapped so that a line began with "- the application's own files.", which AsciiDoc renders as a list item once config-to-docs converts config.sample.php into the admin manual's config parameters page. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- config/config.sample.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index e0fc2af9de25..5a3633251594 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -399,8 +399,8 @@ * instance can be configured to read it from a user attribute. Such a value is * not necessarily controlled by the ownCloud administrator, so a home is only * accepted when it lies inside the configured `datadirectory`. A home pointing - * at the ownCloud code directory would otherwise expose - and allow overwriting - * - the application's own files. + * at the ownCloud code directory would otherwise expose the application's own + * files, and allow overwriting them. * * If user homes legitimately live outside of the data directory, for example on * a separate NFS mount, list the permitted base directories here. Any home From b2dd5c6a45502f929666ba6c06bba249eb3ce0b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:32:51 +0200 Subject: [PATCH 4/5] fix(user): refuse a base directory that is not an absolute path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Carries the fix for the review feedback on owncloud/user_ldap#849 over to the core side of the containment check, which had the identical defect. verifyHomeLocation() skipped '' before normalizing, but '.', './' and './.' normalize *to* the empty string and so slipped through. isContainedIn() then compared against rtrim('', '/') . '/' == '/', i.e. strpos($path, '/') === 0, which is true for every absolute path - a single stray '.' in user.home_base_dirs silently disabled the containment check completely. The value is now required to be an absolute path before it is normalized. That covers '', '.', './', './.' and '..' in one condition, and additionally rules out a relative entry such as 'data', which realpath() would have resolved against the working directory of whichever process happened to run the check - for a home check that runs from both the web server and occ, that is not a stable answer. isContainedIn() also refuses an empty base dir in its own right, so the predicate cannot fail open if a future caller forgets to guard, and the normalizePath() docblock now states that passing an absolute path is the caller's responsibility. config.sample.php documents that entries must be absolute and that others are ignored. Verified with a negative control: reverting only the guard reproduces exactly the three '.'-shaped failures, restoring it turns them green. Also adds absolute-path traversal cases (/var/www/../../opt, /var/www/./owncloud/./apps, /.., ...). Those already passed - normalizePath() resolves dot segments before comparing - so they are coverage for behaviour that was already correct. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- config/config.sample.php | 4 +++ lib/private/User/SyncService.php | 17 ++++++++- tests/lib/User/SyncServiceTest.php | 58 ++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/config/config.sample.php b/config/config.sample.php index 5a3633251594..7427d763d8ab 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -407,6 +407,10 @@ * inside one of them is accepted. Keep this list as narrow as possible and never * include the ownCloud code directory. * + * Every entry must be an absolute path. Entries that are not are ignored, because + * a relative one would be resolved against the working directory of whichever + * process happens to run the check. + * * Defaults to `[]`, i.e. only the data directory is permitted. * * The LDAP backend honours the analogous `user_ldap.home_base_dirs` option. diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 28324bd9c0d0..73a14b4e448f 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -312,7 +312,13 @@ private function verifyHomeLocation($home, $dataDir, $uid, $backendClass) { \array_unshift($baseDirs, $dataDir); foreach ($baseDirs as $baseDir) { - if (!\is_string($baseDir) || $baseDir === '') { + // A base directory is only meaningful as an absolute path. Anything + // else is a misconfiguration and must not widen the comparison: '.' + // and './' would normalize to the empty string, which makes the + // containment check below accept every absolute path, and a relative + // value would resolve against the working directory of whichever + // process happens to run this check. + if (!\is_string($baseDir) || !isset($baseDir[0]) || $baseDir[0] !== '/') { continue; } if (self::isContainedIn($normalizedHome, self::normalizePath($baseDir))) { @@ -340,6 +346,10 @@ private function verifyHomeLocation($home, $dataDir, $uid, $backendClass) { * an apparently contained home somewhere else, and lets an admin have a * symlinked data directory. * + * Callers are responsible for passing an absolute path: a relative one would be + * resolved against the working directory of whichever process happens to run + * this, and '' and '.' normalize to the empty string. + * * @param string $path an absolute path * @return string the resolved path, without a trailing slash */ @@ -400,6 +410,11 @@ private static function normalizePathLexically($path) { * @return bool */ private static function isContainedIn($path, $baseDir) { + // an empty base dir contains nothing - without this the comparison below + // degenerates into strpos($path, '/') === 0, accepting every absolute path + if ($baseDir === '') { + return false; + } if ($path === $baseDir) { return true; } diff --git a/tests/lib/User/SyncServiceTest.php b/tests/lib/User/SyncServiceTest.php index 501c931070c6..eb1d867a6db8 100644 --- a/tests/lib/User/SyncServiceTest.php +++ b/tests/lib/User/SyncServiceTest.php @@ -211,6 +211,12 @@ public function providesHomeOutsideDataDir() { 'apps directory' => ['/var/www/owncloud/apps'], 'system directory' => ['/etc'], 'traversal' => ['/var/www/owncloud/data/../../../etc'], + // an absolute path can carry dot segments anywhere, not just trailing + 'traversal mid-path' => ['/var/www/../../opt'], + 'traversal out of data dir' => ['/var/www/owncloud/data/../../apps'], + 'dot segments' => ['/var/www/./owncloud/./apps'], + 'traversal to root' => ['/..'], + 'dot segments to code root' => ['/var/www/owncloud/data/../.'], 'prefix sibling' => ['/var/www/owncloud/data-evil'], ]; } @@ -327,6 +333,58 @@ public function testSyncHomeAcceptsHomeUnderSymlinkedDataDir() { /** * Admins with homes on a separate mount can opt back in explicitly. */ + /** + * A base directory that is not usable as one must not widen the check. Values + * such as '.' or a relative path normalize to something unrelated to the + * intended directory - notably '.' normalizes to the empty string, which used + * to make the containment check accept every absolute path. + * + * @dataProvider providesUnusableBaseDirs + */ + public function testSyncHomeRefusesUnusableBaseDir($baseDir) { + $this->expectException(\OutOfBoundsException::class); + + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ + $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); + $backend->expects($this->any())->method('getHome')->willReturn('/var/www/owncloud/apps'); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($baseDir) { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user.home_base_dirs') { + return [$baseDir]; + } + return $default; + }); + + $a = $this->getMockBuilder(Account::class)->setMethods(['getHome', 'setHome', 'getUpdatedFields'])->getMock(); + $a->expects($this->any())->method('getHome')->willReturn(''); + $a->expects($this->any())->method('getUpdatedFields')->willReturn([]); + $a->expects($this->never())->method('setHome'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); + self::invokePrivate($s, 'syncHome', [$a, $backend]); + } + + public function providesUnusableBaseDirs() { + return [ + 'empty' => [''], + // these normalize to the empty string + 'current directory' => ['.'], + 'current directory with slash' => ['./'], + 'dot segments only' => ['./.'], + // a relative base dir would resolve against the working directory of + // whichever process happens to run the check + 'relative' => ['data'], + 'relative with dot' => ['./data'], + 'parent' => ['..'], + 'not a string' => [null], + ]; + } + public function testSyncHomeAcceptsHomeInConfiguredBaseDir() { /** @var UserInterface | IProvidesHomeBackend | \PHPUnit\Framework\MockObject\MockObject $backend */ $backend = $this->createMock(IUserInterfaceWithHomeBackendTest::class); From 6c51534ef1267aa5cad60db9f39d6bc9327bba74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:35:03 +0200 Subject: [PATCH 5/5] chore: note the absolute-path requirement in the changelog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41752 | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/changelog/unreleased/41752 b/changelog/unreleased/41752 index 0c824d8ffba1..bdeba48696e9 100644 --- a/changelog/unreleased/41752 +++ b/changelog/unreleased/41752 @@ -15,7 +15,10 @@ configured datadirectory. Installations that legitimately keep user homes elsewhere, for example on a separate NFS mount, can list the permitted base directories in the new "user.home_base_dirs" config option. Symlinks are resolved before the comparison, so a symlinked data directory keeps working -while a symlink inside it cannot be used to escape. +while a symlink inside it cannot be used to escape. Every entry in the option +has to be an absolute path; entries that are not are ignored, because a +relative one would be resolved against the working directory of whichever +process happens to run the check. Note that a home is only set when an account has none yet, so accounts that were provisioned before this change keep the home already stored for them.