From c7df5d40670859a0eb911d373fbfa2a1253b03fb 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:52:04 +0200 Subject: [PATCH 1/3] fix(user): confine the home folder naming rule to the data directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "User Home Folder Naming Rule" setting can point the per user home at an LDAP attribute (`attr:homeDirectory`). UserEntry::getHome() returned that attribute value almost verbatim: an absolute path was accepted after nothing more than a check for a leading slash, and a relative one was concatenated onto the data directory without normalization, so a value containing ".." escaped it. The LDAP directory is not necessarily under the control of the ownCloud administrator. 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. The home read from LDAP is now rejected 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_ldap.home_base_dirs` config option. The refusal is logged with both 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. ownCloud core carries the matching defense-in-depth check in SyncService::syncHome(), which covers every user backend that supplies a home. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- CHANGELOG.md | 4 + lib/User/UserEntry.php | 120 +++++++++++++++++- tests/unit/User/UserEntryTest.php | 199 +++++++++++++++++++++++++++++- 3 files changed, 316 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d710795e..da46877a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) ## [Unreleased] - XXXX-XX-XX +### Fixed + +- [#848](https://github.com/owncloud/user_ldap/pull/848) - Confine the LDAP home folder naming rule to the data directory + ## [0.20.2] - 2026-07-27 diff --git a/lib/User/UserEntry.php b/lib/User/UserEntry.php index 333f1a74..48915cdc 100644 --- a/lib/User/UserEntry.php +++ b/lib/User/UserEntry.php @@ -261,6 +261,7 @@ public function getEMailAddress() { * returns the home directory of the user if specified by LDAP settings * @return string|null * @throws \Exception if a naming rule attribute is enforced, but it doesn't exist for that LDAP user + * @throws \OutOfBoundsException if the configured attribute points outside of the permitted base directories */ public function getHome() { $path = ''; @@ -274,15 +275,47 @@ public function getHome() { } if ($path !== '') { + $dataDir = $this->config->getSystemValue( + 'datadirectory', + \OC::$SERVERROOT.'/data' + ); //if attribute's value is an absolute path take this, otherwise append it to data dir //check for / at the beginning if ($path[0] !== '/') { - $path = $this->config->getSystemValue( - 'datadirectory', - \OC::$SERVERROOT.'/data' - ) . '/' . $path; + $path = $dataDir . '/' . $path; } - return $path; + + // The value comes from the LDAP directory, which is not necessarily + // under the control of the ownCloud administrator. Left unchecked, a + // home pointing at the ownCloud code root turns the user's file + // listing into read/write access to the application's own PHP files, + // i.e. remote code execution. Confine it to the data directory, and + // to any additional base directories the admin opted into. + $path = self::normalizePath($path); + $baseDirs = $this->config->getSystemValue('user_ldap.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($path, self::normalizePath($baseDir))) { + return $path; + } + } + + $this->logger->error( + "Home dir <$path> for uid <{$this->getUserId()}> is outside of the data directory" . + " <$dataDir> and of any directory listed in the 'user_ldap.home_base_dirs' config" . + " option - refusing it", + ['app' => 'user_ldap'] + ); + throw new \OutOfBoundsException( + 'Home dir read from LDAP is not inside a permitted base directory for uid: ' . $this->getUserId() + ); } // TODO use OutOfBoundsException and https://github.com/owncloud/core/pull/28805 @@ -298,6 +331,83 @@ public function getHome() { return null; } + /** + * 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 at the time + * 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; + } + /** * @brief reads the image from LDAP that shall be used as Avatar * @return string|null image binary data (provided by LDAP) diff --git a/tests/unit/User/UserEntryTest.php b/tests/unit/User/UserEntryTest.php index 7bcaf82b..70536c9c 100644 --- a/tests/unit/User/UserEntryTest.php +++ b/tests/unit/User/UserEntryTest.php @@ -483,6 +483,14 @@ public function testGetEmailAddressUnset() { } public function testGetHomeAttributeWithAbsolutePath() { + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/absolute/path'; + } + return $default; + }); $this->connection->expects($this->once()) ->method('__get') ->with($this->equalTo('homeFolderNamingRule')) @@ -499,10 +507,197 @@ public function testGetHomeAttributeWithAbsolutePath() { self::assertEquals('/absolute/path/to/home', $userEntry->getHome()); } + /** + * The home attribute is controlled by the LDAP directory, not by the + * ownCloud admin. An absolute path outside of the data directory must be + * refused: pointing it at the ownCloud code root would expose - and allow + * overwriting - the application's own PHP files, which is remote code + * execution. + * + * @dataProvider providesHomeOutsideDataDir + */ + public function testGetHomeOutsideDataDirIsRefused($home) { + $this->expectException(\OutOfBoundsException::class); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return []; + } + return $default; + }); + // the error path resolves the uid for the log message, which reads + // further config options + $this->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($key) { + return $key === 'homeFolderNamingRule' ? 'attr:home' : 'mail'; + }); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'mail' => [0 => 'a@b.c'], + 'home' => [0 => $home] + ] + ); + $userEntry->getHome(); + } + + public function providesHomeOutsideDataDir() { + return [ + // the reported vector: the ownCloud code root itself + 'application root' => ['/var/www/owncloud'], + 'apps directory' => ['/var/www/owncloud/apps'], + 'system directory' => ['/etc'], + // traversal through the relative branch, which is concatenated + // onto the data directory without any normalization + 'relative traversal' => ['../../../../etc'], + 'relative traversal to root' => ['..'], + 'traversal mid-path' => ['foo/../../../etc'], + // a sibling directory that merely shares the data dir's prefix + // must not pass a naive string comparison + 'prefix sibling' => ['/var/www/owncloud/data-evil'], + ]; + } + + /** + * 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 testGetHomeEscapingViaSymlinkIsRefused() { + $this->expectException(\OutOfBoundsException::class); + + $tmp = \realpath(\sys_get_temp_dir()) . '/oc-ldaphome-' . \uniqid(); + \mkdir($tmp . '/data', 0777, true); + \mkdir($tmp . '/apps', 0777, true); + \symlink($tmp . '/apps', $tmp . '/data/escape'); + + try { + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($tmp) { + if ($key === 'datadirectory') { + return $tmp . '/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return []; + } + return $default; + }); + $this->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($key) { + return $key === 'homeFolderNamingRule' ? 'attr:home' : 'mail'; + }); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'mail' => [0 => 'a@b.c'], + 'home' => [0 => $tmp . '/data/escape'] + ] + ); + $userEntry->getHome(); + } 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 testGetHomeUnderSymlinkedDataDirIsAccepted() { + $tmp = \realpath(\sys_get_temp_dir()) . '/oc-ldaphome-' . \uniqid(); + \mkdir($tmp . '/real-data', 0777, true); + \symlink($tmp . '/real-data', $tmp . '/data'); + + try { + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($tmp) { + if ($key === 'datadirectory') { + return $tmp . '/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return []; + } + return $default; + }); + $this->connection->expects($this->once()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue('attr:home')); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'home' => [0 => $tmp . '/data/alice'] + ] + ); + self::assertEquals($tmp . '/real-data/alice', $userEntry->getHome()); + } finally { + \unlink($tmp . '/data'); + \rmdir($tmp . '/real-data'); + \rmdir($tmp); + } + } + + /** + * Admins with home directories on a separate mount can opt back in by + * listing the permitted base directories explicitly. + */ + public function testGetHomeOutsideDataDirAllowedByConfig() { + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return ['/mnt/nfs/homes']; + } + return $default; + }); + $this->connection->expects($this->once()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue('attr:home')); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'home' => [0 => '/mnt/nfs/homes/alice'] + ] + ); + self::assertEquals('/mnt/nfs/homes/alice', $userEntry->getHome()); + } + public function testGetHomeAttributeWithRelativePath() { - $this->config->expects($this->once()) + $this->config->expects($this->any()) ->method('getSystemValue') - ->will($this->returnValue('/path/to/data')); + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/path/to/data'; + } + return $default; + }); $this->connection->expects($this->once()) ->method('__get') ->with($this->equalTo('homeFolderNamingRule')) From e2091e7e13c6c5e8fea08726620ff77d5f064b86 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:07 +0200 Subject: [PATCH 2/3] chore: correct the changelog PR reference to #849 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.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da46877a..b1b8c14f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) ### Fixed -- [#848](https://github.com/owncloud/user_ldap/pull/848) - Confine the LDAP home folder naming rule to the data directory +- [#849](https://github.com/owncloud/user_ldap/pull/849) - Confine the LDAP home folder naming rule to the data directory ## [0.20.2] - 2026-07-27 From 1f7034b9cd9cda6f5d4fbed5e21c2d76840ab6d0 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:36 +0200 Subject: [PATCH 3/3] 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 Addresses review feedback on #849 about the empty path contract. The base dir loop 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_ldap.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. 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 both normalizePath() docblocks now state that passing an absolute path is the caller's responsibility. Verified with a negative control: reverting only the guard reproduces exactly the three '.'-shaped failures, restoring it turns them green. Also adds the absolute-path traversal cases the review asked for (/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, alongside a companion provider asserting that dot segments landing back inside the data directory are still accepted. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- lib/User/UserEntry.php | 17 ++++- tests/unit/User/UserEntryTest.php | 110 ++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/lib/User/UserEntry.php b/lib/User/UserEntry.php index 48915cdc..a6395f91 100644 --- a/lib/User/UserEntry.php +++ b/lib/User/UserEntry.php @@ -299,7 +299,13 @@ public function getHome() { \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($path, self::normalizePath($baseDir))) { @@ -340,6 +346,10 @@ public function getHome() { * pointing 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/unit/User/UserEntryTest.php b/tests/unit/User/UserEntryTest.php index 70536c9c..dc413fae 100644 --- a/tests/unit/User/UserEntryTest.php +++ b/tests/unit/User/UserEntryTest.php @@ -561,6 +561,13 @@ public function providesHomeOutsideDataDir() { 'relative traversal' => ['../../../../etc'], 'relative traversal to root' => ['..'], 'traversal mid-path' => ['foo/../../../etc'], + // an absolute path can carry dot segments too - these resolve to a + // location outside of the data directory + 'absolute traversal mid-path' => ['/var/www/../../opt'], + 'absolute traversal out of data dir' => ['/var/www/owncloud/data/../../apps'], + 'absolute traversal with dot segments' => ['/var/www/./owncloud/./apps'], + 'absolute traversal to root' => ['/..'], + 'absolute dot segments to code root' => ['/var/www/owncloud/data/../.'], // a sibling directory that merely shares the data dir's prefix // must not pass a naive string comparison 'prefix sibling' => ['/var/www/owncloud/data-evil'], @@ -689,6 +696,109 @@ public function testGetHomeOutsideDataDirAllowedByConfig() { self::assertEquals('/mnt/nfs/homes/alice', $userEntry->getHome()); } + /** + * Dot segments that resolve back inside the data directory are fine - it is + * where the path lands that matters, not how it is spelled. + * + * @dataProvider providesHomeWithDotSegmentsInsideDataDir + */ + public function testGetHomeWithDotSegmentsInsideDataDirIsAccepted($home, $expected) { + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return []; + } + return $default; + }); + $this->connection->expects($this->once()) + ->method('__get') + ->with($this->equalTo('homeFolderNamingRule')) + ->will($this->returnValue('attr:home')); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'home' => [0 => $home] + ] + ); + self::assertEquals($expected, $userEntry->getHome()); + } + + public function providesHomeWithDotSegmentsInsideDataDir() { + return [ + 'dot segment' => ['/var/www/owncloud/data/./alice', '/var/www/owncloud/data/alice'], + 'traversal that returns' => ['/var/www/owncloud/data/foo/../alice', '/var/www/owncloud/data/alice'], + 'duplicate slashes' => ['/var/www/owncloud//data//alice', '/var/www/owncloud/data/alice'], + 'trailing slash' => ['/var/www/owncloud/data/alice/', '/var/www/owncloud/data/alice'], + 'the data directory itself' => ['/var/www/owncloud/data', '/var/www/owncloud/data'], + 'relative with dot segments' => ['./alice', '/var/www/owncloud/data/alice'], + ]; + } + + /** + * 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 testGetHomeIsRefusedForUnusableBaseDir($baseDir) { + $this->expectException(\OutOfBoundsException::class); + + $this->config->expects($this->any()) + ->method('getSystemValue') + ->willReturnCallback(function ($key, $default = '') use ($baseDir) { + if ($key === 'datadirectory') { + return '/var/www/owncloud/data'; + } + if ($key === 'user_ldap.home_base_dirs') { + return [$baseDir]; + } + return $default; + }); + // the error path resolves the uid for the log message, which reads + // further config options + $this->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($key) { + return $key === 'homeFolderNamingRule' ? 'attr:home' : 'mail'; + }); + $userEntry = new UserEntry( + $this->config, + $this->logger, + $this->connection, + [ + 'dn' => [0 => 'cn=foo,dc=foobar,dc=bar'], + 'mail' => [0 => 'a@b.c'], + 'home' => [0 => '/var/www/owncloud/apps'] + ] + ); + $userEntry->getHome(); + } + + 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 testGetHomeAttributeWithRelativePath() { $this->config->expects($this->any()) ->method('getSystemValue')