check for symlinks when required to be within customer-homedir

Signed-off-by: Michael Kaufmann <d00p@froxlor.org>
froxlor · Oct 13, 2023 · 9e8f32f · 9e8f32f
1 parent a7b6622
commit 9e8f32f
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 25 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,9 +1,10 @@
 <?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;
 
 /**
  *
@@ -164,6 +165,31 @@ public function testCustomerFtpsAdd()
 		$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;
@@ -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()