CVE-2023-6069: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: Improper Link Resolution Before File Access in GitHub repository froxlor/froxlor prior to 2.1.0.


CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 8.8
Published: 2023-11-10T01:15Z
Repository: https://github.com/froxlor/froxlor
Language: PHP
Files changed: customer_ftp.php, lib/Froxlor/Api/Commands/DirOptions.php, lib/Froxlor/Api/Commands/DirProtections.php, lib/Froxlor/Api/Commands/Ftps.php, lib/Froxlor/Api/Commands/SubDomains.php, lib/Froxlor/Cron/Http/Apache.php, lib/Froxlor/FileDir.php, tests/Ftps/FtpsTest.php
Commit date: 2023-10-13 10:18:53 +0200
Commit message: check for symlinks when required to be within customer-homedir

Signed-off-by: Michael Kaufmann <d00p@froxlor.org>

Diff: diff --git a/customer_ftp.php b/customer_ftp.php
index 8ddb619929..2325579124 100644
--- a/customer_ftp.php
+++ b/customer_ftp.php
@@ -57,11 +57,11 @@ if ($page == 'overview' || $page == 'accounts') {
 			Response::dynamicError($e->getMessage());
 		}
 
 		$actions_links = [];
 		if (CurrentUser::canAddResource('ftps')) {
-			$actions_links = [
+			$actions_links[] = [
 				'href' => $linker->getLink(['section' => 'ftp', 'page' => 'accounts', 'action' => 'add']),
 				'label' => lng('ftp.account_add')
 			];
 		}
 		$actions_links[] = [
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
@@ -91,11 +91,11 @@ class DirOptions extends ApiCommand implements ResourceEntity
 		$error500path = $this->getParam('error500path', true, '');
 
 		// 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
@@ -82,11 +82,11 @@ class DirProtections extends ApiCommand implements ResourceEntity
 		// parameters
 		$authname = $this->getParam('directory_authname', true, '');
 
 		// 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);
 		$password = Crypt::validatePassword($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
@@ -172,11 +172,11 @@ class Ftps extends ApiCommand implements ResourceEntity
 			if (!empty($username_check) && $username_check['username'] = $username) {
 				Response::standardError('usernamealreadyexists', $username, true);
 			} 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 . "`
 						(`customerid`, `username`, `description`, `password`, `homedir`, `login_enabled`, `uid`, `gid`, `shell`)
 						VALUES (:customerid, :username, :description, :password, :homedir, :loginenabled, :guid, :guid, :shell)");
@@ -467,11 +467,11 @@ class Ftps extends ApiCommand implements ResourceEntity
 			$this->logger()->logAction($this->isAdmin() ? FroxlorLogger::ADM_ACTION : FroxlorLogger::USR_ACTION, LOG_NOTICE, "[API] updated ftp-account password for '" . $result['username'] . "'");
 		}
 
 		// 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 . "`
 					SET `homedir` = :homedir
 					WHERE `customerid` = :customerid
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
@@ -562,13 +562,13 @@ class SubDomains extends ApiCommand implements ResourceEntity
 				Response::standardError('pathmaynotcontaincolon', '', true);
 			}
 			// 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
 			$_doredirect = true;
 		}
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
@@ -127,11 +127,11 @@ class Apache extends HttpConfigBase
 				$is_redirect = false;
 				// check for SSL redirect
 				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;
 					} else {
 						$_sslport = $this->checkAlternativeSslPort();
@@ -1253,11 +1253,11 @@ class Apache extends HttpConfigBase
 					$this->diroptions_data[$diroptions_filename] .= '  AllowOverride None' . "\n";
 					$this->diroptions_data[$diroptions_filename] .= '  AddHandler cgi-script .cgi .pl' . "\n";
 					// >=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";
 							// $this->diroptions_data[$diroptions_filename] .= ' AllowOverride All' . "\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
@@ -24,14 +24,14 @@
  */
 
 namespace Froxlor;
 
 use Exception;
-use PDO;
-use RecursiveCallbackFilterIterator;
 use Froxlor\Customer\Customer;
 use Froxlor\Database\Database;
+use PDO;
+use RecursiveCallbackFilterIterator;
 
 class FileDir
 {
 	/**
 	 * Creates a directory below a users homedir and sets all directories,
@@ -49,15 +49,16 @@ class FileDir
 	 * @throws Exception
 	 */
 	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);
 
 			if (substr($dirToCreate, 0, strlen($homeDir)) == $homeDir) {
@@ -105,28 +106,53 @@ class FileDir
 		}
 		return false;
 	}
 
 	/**
-	 * 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);
 			if (substr($dir, -1, 1) != '/') {
 				$dir .= '/';
 			}
 			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.");
 	}
 
@@ -243,13 +269,14 @@ class FileDir
 	 * @throws Exception
 	 */
 	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`
 			FROM `" . TABLE_PANEL_CUSTOMERS . "` AS `c` INNER JOIN `" . TABLE_PANEL_ADMINS . "` AS `a`
 			ON `c`.`adminid` = `a`.`adminid`
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,11 +1,12 @@
 <?php
-use PHPUnit\Framework\TestCase;
 
 use Froxlor\Api\Commands\Admins;
 use Froxlor\Api\Commands\Customers;
 use Froxlor\Api\Commands\Ftps;
+use Froxlor\Froxlor;
+use PHPUnit\Framework\TestCase;
 
 /**
  *
  * @covers \Froxlor\Api\ApiCommand
  * @covers \Froxlor\Api\ApiParameter
@@ -162,10 +163,35 @@ class FtpsTest extends TestCase
 		$json_result = Ftps::getLocal($customer_userdata, $data)->add();
 		$result = json_decode($json_result, true)['data'];
 		$this->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;
 
 		// get customer
@@ -176,11 +202,11 @@ class FtpsTest extends TestCase
 
 		$customer_userdata['ftps_used'] = 100;
 
 		$this->expectExceptionCode(406);
 		$this->expectExceptionMessage('No more resources available');
-		$json_result = Ftps::getLocal($customer_userdata)->add();
+		Ftps::getLocal($customer_userdata)->add();
 	}
 
 	public function testAdminFtpsAddCustomerRequired()
 	{
 		global $admin_userdata;
@@ -192,11 +218,11 @@ class FtpsTest extends TestCase
 			'sendinfomail' => 1
 		];
 
 		$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()
 	{
 		global $admin_userdata;

Vulnerable code: 			$actions_links = [
		$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path);
		$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path);
			$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $completedomain);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path);
					// or a renew is ongoing - disable redirect
						// only create the require all granted if there is not active directory-protection
use PDO;
use RecursiveCallbackFilterIterator;
		int $uid,
		int $gid,
		bool $placeindex = false,
		bool $allow_notwithinhomedir = false
	): bool {
	 * Function which returns a correct dirname, means to add slashes at the beginning and at the end if there weren't
	 * some
	public static function makeCorrectDir(string $dir): string
		$logger = null,
		bool $force = false
	) {
use PHPUnit\Framework\TestCase;
		$json_result = Ftps::getLocal($customer_userdata)->add();
		$json_result = Ftps::getLocal($admin_userdata, $data)->add();
Fixed code: 			$actions_links[] = [
		$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']);
		$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']);
			$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $completedomain, $customer['documentroot']);
				$path = FileDir::makeCorrectDir($customer['documentroot'] . '/' . $path, $customer['documentroot']);
					// or a renewal is ongoing - disable redirect
						// only create the' require all granted' if there is no active directory-protection
use PDO;
use RecursiveCallbackFilterIterator;
		int    $uid,
		int    $gid,
		bool   $placeindex = false,
		bool   $allow_notwithinhomedir = false
	): bool
	{
	 * 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,
	 *
	public static function makeCorrectDir(string $dir, string $fixed_homedir = ""): string

			// 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");
				}
			}

			   $logger = null,
		bool   $force = false
	)
	{
use Froxlor\Froxlor;
use PHPUnit\Framework\TestCase;
	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();
	}

		Ftps::getLocal($customer_userdata)->add();
		Ftps::getLocal($admin_userdata, $data)->add();