From 9e8f32f1e86016733b603b50c31b97f472e8dabc Mon Sep 17 00:00:00 2001 From: Michael Kaufmann Date: Fri, 13 Oct 2023 10:18:53 +0200 Subject: [PATCH] check for symlinks when required to be within customer-homedir Signed-off-by: Michael Kaufmann --- customer_ftp.php | 2 +- lib/Froxlor/Api/Commands/DirOptions.php | 2 +- lib/Froxlor/Api/Commands/DirProtections.php | 2 +- lib/Froxlor/Api/Commands/Ftps.php | 4 +- lib/Froxlor/Api/Commands/SubDomains.php | 4 +- lib/Froxlor/Cron/Http/Apache.php | 4 +- lib/Froxlor/FileDir.php | 53 ++++++++++++++++----- tests/Ftps/FtpsTest.php | 32 +++++++++++-- 8 files changed, 78 insertions(+), 25 deletions(-) diff --git a/customer_ftp.php b/customer_ftp.php index 8ddb619929..2325579124 100644 --- a/customer_ftp.php +++ b/customer_ftp.php @@ -59,7 +59,7 @@ $actions_links = []; if (CurrentUser::canAddResource('ftps')) { - $actions_links = [ + $actions_links[] = [ 'href' => $linker->getLink(['section' => 'ftp', 'page' => 'accounts', 'action' => 'add']), 'label' => lng('ftp.account_add') ]; diff --git a/lib/Froxlor/Api/Commands/DirOptions.php b/lib/Froxlor/Api/Commands/DirOptions.php index f55ef889e7..d7d0f1fae6 100644 --- a/lib/Froxlor/Api/Commands/DirOptions.php +++ b/lib/Froxlor/Api/Commands/DirOptions.php @@ -93,7 +93,7 @@ public function add() // validation $path = FileDir::makeCorrectDir(Validate::validate($path, 'path', Validate::REGEX_DIR, '', [], true)); $userpath = $path; - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']); if (!empty($error404path)) { $error404path = $this->correctErrorDocument($error404path, true); diff --git a/lib/Froxlor/Api/Commands/DirProtections.php b/lib/Froxlor/Api/Commands/DirProtections.php index b21bdd6e62..50b06bf5e2 100644 --- a/lib/Froxlor/Api/Commands/DirProtections.php +++ b/lib/Froxlor/Api/Commands/DirProtections.php @@ -84,7 +84,7 @@ public function add() // validation $path = FileDir::makeCorrectDir(Validate::validate($path, 'path', Validate::REGEX_DIR, '', [], true)); - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']); $username = Validate::validate($username, 'username', '/^[a-zA-Z0-9][a-zA-Z0-9\-_]+\$?$/', '', [], true); $authname = Validate::validate($authname, 'directory_authname', '/^[a-zA-Z0-9][a-zA-Z0-9\-_ ]+\$?$/', '', [], true); $password = Validate::validate($password, 'password', '', '', [], true); diff --git a/lib/Froxlor/Api/Commands/Ftps.php b/lib/Froxlor/Api/Commands/Ftps.php index e29ebf9959..a3b561d76d 100644 --- a/lib/Froxlor/Api/Commands/Ftps.php +++ b/lib/Froxlor/Api/Commands/Ftps.php @@ -174,7 +174,7 @@ public function add() } elseif ($username == $password) { Response::standardError('passwordshouldnotbeusername', '', true); } else { - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']); $cryptPassword = Crypt::makeCryptPassword($password, false, true); $stmt = Database::prepare("INSERT INTO `" . TABLE_FTP_USERS . "` @@ -469,7 +469,7 @@ public function update() // path update? if ($path != '') { - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']); if ($path != $result['homedir']) { $stmt = Database::prepare("UPDATE `" . TABLE_FTP_USERS . "` diff --git a/lib/Froxlor/Api/Commands/SubDomains.php b/lib/Froxlor/Api/Commands/SubDomains.php index 1711c3ef8e..e863e8e3d1 100644 --- a/lib/Froxlor/Api/Commands/SubDomains.php +++ b/lib/Froxlor/Api/Commands/SubDomains.php @@ -564,9 +564,9 @@ private function validateDomainDocumentRoot($path = null, $url = null, $customer // If path is empty or '/' and 'Use domain name as default value for DocumentRoot path' is enabled in settings, // set default path to subdomain or domain name if ((($path == '') || ($path == '/')) && Settings::Get('system.documentroot_use_default_value') == 1) { - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $completedomain); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $completedomain, $customer['documentroot']); } else { - $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path); + $path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']); } } else { // no it's not, create a redirect diff --git a/lib/Froxlor/Cron/Http/Apache.php b/lib/Froxlor/Cron/Http/Apache.php index 8fba3764f7..7a99b49d72 100644 --- a/lib/Froxlor/Cron/Http/Apache.php +++ b/lib/Froxlor/Cron/Http/Apache.php @@ -129,7 +129,7 @@ public function createIpPort() if ($row_ipsandports['ssl'] == '0' && Settings::Get('system.le_froxlor_redirect') == '1') { $is_redirect = true; // check whether froxlor uses Let's Encrypt and not cert is being generated yet - // or a renew is ongoing - disable redirect + // or a renewal is ongoing - disable redirect if (Settings::Get('system.le_froxlor_enabled') && ($this->froxlorVhostHasLetsEncryptCert() == false || $this->froxlorVhostLetsEncryptNeedsRenew())) { $this->virtualhosts_data[$vhosts_filename] .= '# temp. disabled ssl-redirect due to Let\'s Encrypt certificate generation.' . PHP_EOL; $is_redirect = false; @@ -1255,7 +1255,7 @@ public function createFileDirOptions() // >=apache-2.4 enabled? if (Settings::Get('system.apache24') == '1') { $mypath_dir = new Directory($row_diroptions['path']); - // only create the require all granted if there is not active directory-protection + // only create the' require all granted' if there is no active directory-protection // for this path, as this would be the first require and therefore grant all access if ($mypath_dir->isUserProtected() == false) { $this->diroptions_data[$diroptions_filename] .= ' Require all granted' . "\n"; diff --git a/lib/Froxlor/FileDir.php b/lib/Froxlor/FileDir.php index 41250b1d38..141f149b8d 100644 --- a/lib/Froxlor/FileDir.php +++ b/lib/Froxlor/FileDir.php @@ -26,10 +26,10 @@ namespace Froxlor; use Exception; -use PDO; -use RecursiveCallbackFilterIterator; use Froxlor\Customer\Customer; use Froxlor\Database\Database; +use PDO; +use RecursiveCallbackFilterIterator; class FileDir { @@ -51,11 +51,12 @@ class FileDir public static function mkDirWithCorrectOwnership( string $homeDir, string $dirToCreate, - int $uid, - int $gid, - bool $placeindex = false, - bool $allow_notwithinhomedir = false - ): bool { + int $uid, + int $gid, + bool $placeindex = false, + bool $allow_notwithinhomedir = false + ): bool + { if ($homeDir != '' && $dirToCreate != '') { $homeDir = self::makeCorrectDir($homeDir); $dirToCreate = self::makeCorrectDir($dirToCreate); @@ -107,15 +108,16 @@ public static function mkDirWithCorrectOwnership( } /** - * Function which returns a correct dirname, means to add slashes at the beginning and at the end if there weren't - * some + * Returns a correct/secure dirname, means to add slashes at the beginning and at the end if there weren't + * some. If $fixes_homedir is specified, + * * * @param string $dir the path to correct * * @return string the corrected path * @throws Exception */ - public static function makeCorrectDir(string $dir): string + public static function makeCorrectDir(string $dir, string $fixed_homedir = ""): string { if (strlen($dir) > 0) { $dir = trim($dir); @@ -125,6 +127,30 @@ public static function makeCorrectDir(string $dir): string if (substr($dir, 0, 1) != '/') { $dir = '/' . $dir; } + + // if given, check that the target path is within the $fixed_homedir + // by checking each folder for being a symlink and whether it targets + // the customers homedir or points outside of it + if (!empty($fixed_homedir)) { + $to_check = explode("/", substr($dir, strlen($fixed_homedir) + 1), -1); + $check_dir = substr($fixed_homedir, 0, -1); + // Symlink check + foreach ($to_check as $sub_dir) { + $check_dir .= '/' . $sub_dir; + if (is_link($check_dir)) { + $original_target = $check_dir; + $check_dir = readlink($check_dir); + if (substr($check_dir, 0, strlen($fixed_homedir)) != $fixed_homedir) { + throw new Exception("Found symlink pointing outside of customer home directory: " . substr($original_target, strlen($fixed_homedir))); + } + } + } + // check for the path to be within the given homedir + if (substr($dir, 0, strlen($fixed_homedir)) != $fixed_homedir) { + throw new Exception("Target path not within the required customer home directory"); + } + } + return self::makeSecurePath($dir); } throw new Exception("Cannot validate directory in " . __FUNCTION__ . " which is very dangerous."); @@ -245,9 +271,10 @@ public static function safe_exec(string $exec_string, &$return_value = false, $a public static function storeDefaultIndex( string $loginname, string $destination, - $logger = null, - bool $force = false - ) { + $logger = null, + bool $force = false + ) + { if ($force || (int)Settings::Get('system.store_index_file_subs') == 1) { $result_stmt = Database::prepare(" SELECT `t`.`value`, `c`.`email` AS `customer_email`, `a`.`email` AS `admin_email`, `c`.`loginname` AS `customer_login`, `a`.`loginname` AS `admin_login` diff --git a/tests/Ftps/FtpsTest.php b/tests/Ftps/FtpsTest.php index 1ed6788d42..a4922733bf 100644 --- a/tests/Ftps/FtpsTest.php +++ b/tests/Ftps/FtpsTest.php @@ -1,9 +1,10 @@ assertEquals($customer_userdata['documentroot'], $result['homedir']); } + public function testCustomerFtpsAddSymlinkOutsideHomedir() + { + global $admin_userdata; + + // get customer + $json_result = Customers::getLocal($admin_userdata, array( + 'loginname' => 'test1' + ))->get(); + $customer_userdata = json_decode($json_result, true)['data']; // + + $customer_userdata['documentroot'] = sys_get_temp_dir() . '/'; + @unlink($customer_userdata['documentroot'] . '/frx'); + symlink(Froxlor::getInstallDir(), $customer_userdata['documentroot'] . '/frx'); + + $data = [ + 'ftp_password' => 'h4xXx0r', + 'path' => '/frx/sub', + 'ftp_description' => 'testing', + 'sendinfomail' => TRAVIS_CI == 1 ? 0 : 1 + ]; + + $this->expectExceptionMessage('Found symlink pointing outside of customer home directory: /frx'); + Ftps::getLocal($customer_userdata, $data)->add(); + } + public function testCustomerFtpsAddNoMoreResources() { global $admin_userdata; @@ -178,7 +204,7 @@ public function testCustomerFtpsAddNoMoreResources() $this->expectExceptionCode(406); $this->expectExceptionMessage('No more resources available'); - $json_result = Ftps::getLocal($customer_userdata)->add(); + Ftps::getLocal($customer_userdata)->add(); } public function testAdminFtpsAddCustomerRequired() @@ -194,7 +220,7 @@ public function testAdminFtpsAddCustomerRequired() $this->expectExceptionCode(406); $this->expectExceptionMessage('Requested parameter "loginname" is empty where it should not be for "Customers:get"'); - $json_result = Ftps::getLocal($admin_userdata, $data)->add(); + Ftps::getLocal($admin_userdata, $data)->add(); } public function testCustomerFtpsEdit()