fix possible privilege escalation from customer to root when specifying custom error documents in directory-options

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

lib/Froxlor/Api/Commands/DirOptions.php

@@ -157,16 +157,15 @@ class DirOptions extends ApiCommand implements ResourceEntity
 	 * this functions validates a given value as ErrorDocument
 	 * refs #267
 	 *
-	 * @param
+	 * @param string $errdoc
-	 *            string error-document-string
 	 * @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
@@ -176,14 +175,14 @@ class DirOptions extends ApiCommand implements ResourceEntity
 					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') {
@@ -191,7 +190,7 @@ class DirOptions extends ApiCommand implements ResourceEntity
 				}
 			}
 		}
-		return $errdoc;
+		return trim($errdoc);
 	}
 
 	/**

lib/Froxlor/FileDir.php

@@ -147,9 +147,9 @@ class FileDir
 	 */
 	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 = [
 			':',
 			';',
@@ -161,7 +161,11 @@ class FileDir
 			'$',
 			'~',
 			'?',
-			"\0"
+			"\0",
+			"\n",
+			"\r",
+			"\t",
+			"\f"
 		];
 		foreach ($badchars as $bc) {
 			$path = str_replace($bc, "", $path);
@@ -606,7 +610,7 @@ class FileDir
 	}
 
 	/**
-	 * 
+	 *
 	 * @return array|false
 	 */
 	public static function getFilesystemQuota()

lng/de.lng.php

@@ -837,6 +837,7 @@ return [
 		'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',

lng/en.lng.php

@@ -905,6 +905,7 @@ return [
 		'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',

tests/Extras/DirOptionsTest.php

@@ -191,4 +191,49 @@ class DirOptionsTest extends TestCase
 		$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();
+	}
 }