diff --git a/changelog/unreleased/41752 b/changelog/unreleased/41752 new file mode 100644 index 00000000000..bdeba48696e --- /dev/null +++ b/changelog/unreleased/41752 @@ -0,0 +1,27 @@ +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. 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. + +https://github.com/owncloud/core/pull/41752 +https://github.com/owncloud/user_ldap/pull/849 diff --git a/config/config.sample.php b/config/config.sample.php index fe603c2c379..7427d763d8a 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -392,6 +392,31 @@ */ '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 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 + * 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. + */ +'user.home_base_dirs' => [], + /** * Mail Parameters * diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 640c1cce957..73a14b4e448 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,136 @@ 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) { + // 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))) { + 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. + * + * 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 + */ + 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) { + // 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; + } + // 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 6cc4f907f69..eb1d867a6db 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,249 @@ 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'], + // 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'], + ]; + } + + 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. + */ + /** + * 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); + $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() {