From dd53fad898e8b1de75efeda094b2f0f037d8a407 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Nov 2019 10:27:31 +0100 Subject: [PATCH 1/2] Prevent creating users with existing files Signed-off-by: Joas Schilling --- lib/private/User/Manager.php | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 29cae3da79..9bb0aff5cd 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -294,10 +294,6 @@ class Manager extends PublicEmitter implements IUserManager { * @return bool|IUser the created user or false */ public function createUser($uid, $password) { - if (!$this->verifyUid($uid)) { - return false; - } - $localBackends = []; foreach ($this->backends as $backend) { if ($backend instanceof Database) { @@ -332,22 +328,30 @@ class Manager extends PublicEmitter implements IUserManager { // Check the name for bad characters // Allowed are: "a-z", "A-Z", "0-9" and "_.@-'" - if (preg_match('/[^a-zA-Z0-9 _\.@\-\']/', $uid)) { + if (preg_match('/[^a-zA-Z0-9 _.@\-\']/', $uid)) { throw new \InvalidArgumentException($l->t('Only the following characters are allowed in a username:' . ' "a-z", "A-Z", "0-9", and "_.@-\'"')); } + // No empty username if (trim($uid) === '') { throw new \InvalidArgumentException($l->t('A valid username must be provided')); } + // No whitespace at the beginning or at the end if (trim($uid) !== $uid) { throw new \InvalidArgumentException($l->t('Username contains whitespace at the beginning or at the end')); } + // Username only consists of 1 or 2 dots (directory traversal) if ($uid === '.' || $uid === '..') { throw new \InvalidArgumentException($l->t('Username must not consist of dots only')); } + + if (!$this->verifyUid($uid)) { + throw new \InvalidArgumentException($l->t('Username is invalid because files already exist for this user')); + } + // No empty password if (trim($password) === '') { throw new \InvalidArgumentException($l->t('A valid password must be provided')); @@ -623,10 +627,18 @@ class Manager extends PublicEmitter implements IUserManager { private function verifyUid(string $uid): bool { $appdata = 'appdata_' . $this->config->getSystemValueString('instanceid'); - if ($uid === '.htaccess' || $uid === 'files_external' || $uid === '.ocdata' || $uid === 'owncloud.log' || $uid === 'nextcloud.log' || $uid === $appdata) { + if (\in_array($uid, [ + '.htaccess', + 'files_external', + '.ocdata', + 'owncloud.log', + 'nextcloud.log', + $appdata], true)) { return false; } - return true; + $dataDirectory = $this->config->getSystemValueString('datadirectory', \OC::$SERVERROOT . '/data'); + + return !file_exists(rtrim($dataDirectory, '/') . '/' . $uid); } } From 6004f6208531f9ff7e39799db39209d5a445555d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 4 Dec 2019 15:22:02 +0100 Subject: [PATCH 2/2] I love unit tests that mock unnecessary stuff Signed-off-by: Joas Schilling --- tests/lib/Cache/FileCacheTest.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/lib/Cache/FileCacheTest.php b/tests/lib/Cache/FileCacheTest.php index 26306458d8..450bdf607b 100644 --- a/tests/lib/Cache/FileCacheTest.php +++ b/tests/lib/Cache/FileCacheTest.php @@ -95,6 +95,15 @@ class FileCacheTest extends TestCache { \OC_User::setUserId($this->user); \OC::$server->getConfig()->setSystemValue('cachedirectory', $this->datadir); + if ($this->instance) { + $this->instance->clear(); + $this->instance = null; + } + + //tear down the users dir aswell + $user = \OC::$server->getUserManager()->get('test'); + $user->delete(); + // Restore the original mount point \OC\Files\Filesystem::clearMounts(); \OC\Files\Filesystem::mount($this->storage, array(), '/');