CVE-2023-0671: Improper Control of Generation of Code ('Code Injection') (CWE-94)
value: Code Injection in GitHub repository froxlor/froxlor prior to 2.0.10.
CWE description: The product constructs all or part of a code segment using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the syntax or behavior of the intended code segment.
CVSS v3 base score: 8.8
Published: 2023-02-04T01:15Z
Repository: https://github.com/froxlor/froxlor
Language: PHP
Files changed: lib/Froxlor/Api/Commands/DirOptions.php, lib/Froxlor/FileDir.php, lng/de.lng.php, lng/en.lng.php, tests/Extras/DirOptionsTest.php
Commit date: 2023-01-28 20:00:24 +0100
Commit message: fix possible privilege escalation from customer to root when specifying custom error documents in directory-options

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

Diff: diff --git a/lib/Froxlor/Api/Commands/DirOptions.php b/lib/Froxlor/Api/Commands/DirOptions.php
index c39507ec40..96a892658a 100644
--- a/lib/Froxlor/Api/Commands/DirOptions.php
+++ b/lib/Froxlor/Api/Commands/DirOptions.php
@@ -155,45 +155,44 @@ class DirOptions extends ApiCommand implements ResourceEntity
 
 	/**
 	 * this functions validates a given value as ErrorDocument
 	 * refs #267
 	 *
-	 * @param
-	 *            string error-document-string
+	 * @param string $errdoc
 	 * @param bool $throw_exception
 	 *
 	 * @return string error-document-string
 	 *
 	 */
-	private function correctErrorDocument($errdoc = null, $throw_exception = false)
+	private function correctErrorDocument(string $errdoc, $throw_exception = false)
 	{
-		if ($errdoc !== null && $errdoc != '') {
+		if (trim($errdoc) != '') {
 			// not a URL
 			if ((strtoupper(substr($errdoc, 0, 5)) != 'HTTP:' && strtoupper(substr($errdoc, 0, 6)) != 'HTTPS:') || !Validate::validateUrl($errdoc)) {
 				// a file
 				if (substr($errdoc, 0, 1) != '"') {
 					$errdoc = FileDir::makeCorrectFile($errdoc);
 					// apache needs a starting-slash (starting at the domains-docroot)
 					if (!substr($errdoc, 0, 1) == '/') {
 						$errdoc = '/' . $errdoc;
 					}
-				} else {
+				} elseif (preg_match('/^"([^\r\n\t\f\0"]+)"$/', $errdoc)) {
 					// a string (check for ending ")
 					// string won't work for lighty
 					if (Settings::Get('system.webserver') == 'lighttpd') {
 						Response::standardError('stringerrordocumentnotvalidforlighty', '', $throw_exception);
-					} elseif (substr($errdoc, -1) != '"') {
-						$errdoc .= '"';
 					}
+				} else {
+					Response::standardError('invaliderrordocumentvalue', '', $throw_exception);
 				}
 			} else {
 				if (Settings::Get('system.webserver') == 'lighttpd') {
 					Response::standardError('urlerrordocumentnotvalidforlighty', '', $throw_exception);
 				}
 			}
 		}
-		return $errdoc;
+		return trim($errdoc);
 	}
 
 	/**
 	 * return a directory-protection entry by id
 	 *
diff --git a/lib/Froxlor/FileDir.php b/lib/Froxlor/FileDir.php
index 49c649c1bb..37f2af2c9e 100644
--- a/lib/Froxlor/FileDir.php
+++ b/lib/Froxlor/FileDir.php
@@ -145,13 +145,13 @@ class FileDir
 	 *
 	 * @return string the corrected path
 	 */
 	public static function makeSecurePath($path)
 	{
-		// check for bad characters, some are allowed with escaping
+		// check for bad characters, some are allowed with escaping,
 		// but we generally don't want them in our directory-names,
-		// thx to aaronmueller for this snipped
+		// thx to aaronmueller for this snippet
 		$badchars = [
 			':',
 			';',
 			'|',
 			'&',
@@ -159,11 +159,15 @@ class FileDir
 			'<',
 			'`',
 			'$',
 			'~',
 			'?',
-			"\0"
+			"\0",
+			"\n",
+			"\r",
+			"\t",
+			"\f"
 		];
 		foreach ($badchars as $bc) {
 			$path = str_replace($bc, "", $path);
 		}
 
@@ -604,11 +608,11 @@ class FileDir
 	{
 		FileDir::safe_exec(self::getImmutableFunction(true) . escapeshellarg($filename));
 	}
 
 	/**
-	 * 
+	 *
 	 * @return array|false
 	 */
 	public static function getFilesystemQuota()
 	{
 		// enabled at all?
diff --git a/lng/de.lng.php b/lng/de.lng.php
index f5ebea66b9..5e5a0a61ba 100644
--- a/lng/de.lng.php
+++ b/lng/de.lng.php
@@ -835,10 +835,11 @@ return [
 		'errorwhensaving' => 'Bei dem Speichern des Feldes "%s" trat ein Fehler auf',
 		'pathmaynotcontaincolon' => 'Der eingegebene Pfad sollte keinen Doppelpunkt (":") enthalten. Bitte geben Sie einen korrekten Wert für den Pfad ein.',
 		'notrequiredpasswordcomplexity' => 'Die vorgegebene Passwort-Komplexität wurde nicht erfüllt.<br />Bitte kontaktieren Sie Ihren Administrator, wenn Sie Fragen zur Komplexitäts-Vorgabe haben.',
 		'stringerrordocumentnotvalidforlighty' => 'Ein Text als Fehlerdokument funktioniert leider in LigHTTPd nicht, bitte geben Sie einen Pfad zu einer Datei an',
 		'urlerrordocumentnotvalidforlighty' => 'Eine URL als Fehlerdokument funktioniert leider in LigHTTPd nicht, bitte geben Sie einen Pfad zu einer Datei an',
+		'invaliderrordocumentvalue' => 'Der angegebene Wert für das Fehlederdokument ist keine gültige Datei, URL oder Text-Zeile.',
 		'intvaluetoolow' => 'Die angegebene Zahl ist zu klein (Feld "%s")',
 		'intvaluetoohigh' => 'Die angegebene Zahl ist zu groß (Feld "%s")',
 		'phpfpmstillenabled' => 'PHP-FPM ist derzeit aktiviert. Bitte deaktivieren Sie es, um FCGID zu aktivieren',
 		'fcgidstillenabled' => 'FCGID ist derzeit aktiviert. Bitte deaktivieren Sie es, um PHP-FPM zu aktivieren',
 		'domains_cantdeletedomainwithaliases' => 'Sie können keine Domain löschen, die noch von Alias-Domains verwendet wird. Löschen Sie zuerst alle Alias-Domains dieser Domain.',
diff --git a/lng/en.lng.php b/lng/en.lng.php
index 69cdf753f4..400da4b52d 100644
--- a/lng/en.lng.php
+++ b/lng/en.lng.php
@@ -903,10 +903,11 @@ return [
 		'pathmaynotcontaincolon' => 'The path you have entered should not contain a colon (":"). Please enter a correct path value.',
 		'exception' => '%s',
 		'notrequiredpasswordcomplexity' => 'The specified password-complexity was not satisfied.<br />Please contact your administrator if you have any questions about the complexity-specification',
 		'stringerrordocumentnotvalidforlighty' => 'A string as ErrorDocument does not work in lighttpd, please specify a path to a file',
 		'urlerrordocumentnotvalidforlighty' => 'An URL as ErrorDocument does not work in lighttpd, please specify a path to a file',
+		'invaliderrordocumentvalue' => 'The value given as ErrorDocument does not seem to be a valid file, URL or string.',
 		'intvaluetoolow' => 'The given number is too low (field %s)',
 		'intvaluetoohigh' => 'The given number is too high (field %s)',
 		'phpfpmstillenabled' => 'PHP-FPM is currently active. Please deactivate it before activating FCGID',
 		'fcgidstillenabled' => 'FCGID is currently active. Please deactivate it before activating PHP-FPM',
 		'domains_cantdeletedomainwithaliases' => 'You cannot delete a domain which is used for alias-domains. You have to delete the aliases first.',
diff --git a/tests/Extras/DirOptionsTest.php b/tests/Extras/DirOptionsTest.php
index ec2a3f0b38..a33c9b8e64 100644
--- a/tests/Extras/DirOptionsTest.php
+++ b/tests/Extras/DirOptionsTest.php
@@ -189,6 +189,51 @@ class DirOptionsTest extends TestCase
 			'id' => 1
 		];
 		$this->expectExceptionMessage("Directory option with id #1 could not be found");
 		DirOptions::getLocal($admin_userdata, $data)->get();
 	}
+
+	public function testCustomerDirOptionsAddMalformed()
+	{
+		global $admin_userdata;
+
+		// get customer
+		$json_result = Customers::getLocal($admin_userdata, array(
+			'loginname' => 'test1'
+		))->get();
+		$customer_userdata = json_decode($json_result, true)['data'];
+
+		$data = [
+			'path' => '/testmalformed',
+			'error404path' => '/"'.PHP_EOL.'something/../../../../weird 404.html'.PHP_EOL.'#'
+		];
+		$json_result = DirOptions::getLocal($customer_userdata, $data)->add();
+		$result = json_decode($json_result, true)['data'];
+		$expected = '/"something/././././weird\ 404.html#';
+		$this->assertEquals($expected, $result['error404path']);
+	}
+
+	public function testCustomerDirOptionsAddMalformedInvalid()
+	{
+		global $admin_userdata;
+
+		// get customer
+		$json_result = Customers::getLocal($admin_userdata, array(
+			'loginname' => 'test1'
+		))->get();
+		$customer_userdata = json_decode($json_result, true)['data'];
+
+		$data = [
+			'path' => '/testmalformed',
+			'error404path' => '"'.PHP_EOL.'IncludeOptional /something/else/'.PHP_EOL.'#'
+		];
+		$this->expectExceptionMessage("The value given as ErrorDocument does not seem to be a valid file, URL or string.");
+		DirOptions::getLocal($customer_userdata, $data)->add();
+
+		$data = [
+			'path' => '/testmalformed',
+			'error404path' => '"something"oh no a quote within the string"'
+		];
+		$this->expectExceptionMessage("The value given as ErrorDocument does not seem to be a valid file, URL or string.");
+		DirOptions::getLocal($customer_userdata, $data)->add();
+	}
 }

Vulnerable code: 	 * @param
	 *            string error-document-string
	private function correctErrorDocument($errdoc = null, $throw_exception = false)
		if ($errdoc !== null && $errdoc != '') {
				} else {
					} elseif (substr($errdoc, -1) != '"') {
						$errdoc .= '"';
		return $errdoc;
		// check for bad characters, some are allowed with escaping
		// thx to aaronmueller for this snipped
			"\0"
	 * 
Fixed code: 	 * @param string $errdoc
	private function correctErrorDocument(string $errdoc, $throw_exception = false)
		if (trim($errdoc) != '') {
				} elseif (preg_match('/^"([^\r\n\t\f\0"]+)"$/', $errdoc)) {
				} else {
					Response::standardError('invaliderrordocumentvalue', '', $throw_exception);
		return trim($errdoc);
		// check for bad characters, some are allowed with escaping,
		// thx to aaronmueller for this snippet
			"\0",
			"\n",
			"\r",
			"\t",
			"\f"
	 *
		'invaliderrordocumentvalue' => 'Der angegebene Wert für das Fehlederdokument ist keine gültige Datei, URL oder Text-Zeile.',
		'invaliderrordocumentvalue' => 'The value given as ErrorDocument does not seem to be a valid file, URL or string.',

	public function testCustomerDirOptionsAddMalformed()
	{
		global $admin_userdata;

		// get customer
		$json_result = Customers::getLocal($admin_userdata, array(
			'loginname' => 'test1'
		))->get();
		$customer_userdata = json_decode($json_result, true)['data'];

		$data = [
			'path' => '/testmalformed',
			'error404path' => '/"'.PHP_EOL.'something/../../../../weird 404.html'.PHP_EOL.'#'
		];
		$json_result = DirOptions::getLocal($customer_userdata, $data)->add();
		$result = json_decode($json_result, true)['data'];
		$expected = '/"something/././././weird\ 404.html#';
		$this->assertEquals($expected, $result['error404path']);
	}

	public function testCustomerDirOptionsAddMalformedInvalid()
	{
		global $admin_userdata;

		// get customer
		$json_result = Customers::getLocal($admin_userdata, array(
			'loginname' => 'test1'
		))->get();
		$customer_userdata = json_decode($json_result, true)['data'];

		$data = [
			'path' => '/testmalformed',
			'error404path' => '"'.PHP_EOL.'IncludeOptional /something/else/'.PHP_EOL.'#'
		];
		$this->expectExceptionMessage("The value given as ErrorDocument does not seem to be a valid file, URL or string.");
		DirOptions::getLocal($customer_userdata, $data)->add();

		$data = [
			'path' => '/testmalformed',
			'error404path' => '"something"oh no a quote within the string"'
		];
		$this->expectExceptionMessage("The value given as ErrorDocument does not seem to be a valid file, URL or string.");
		DirOptions::getLocal($customer_userdata, $data)->add();
	}