From: Brian Wolff <bawolff+wn@gmail.com>
Date: Tue, 14 Mar 2017 04:01:09 +0000 (+0000)
Subject: Better path traversal prevention in TemplateParser.
X-Git-Tag: 1.31.0-rc.0~3805^2~1
X-Git-Url: http://git.cyclocoop.org//%27%40script%40/%27?a=commitdiff_plain;h=73e08353fb96f5cbd93c6a7004d91c3d0845b446;p=lhc%2Fweb%2Fwiklou.git

Better path traversal prevention in TemplateParser.

In practise this probably doesn't matter, since template names
are not user controlled, and php isn't stupid enough to fall for
tricks with nulls (afaict). Nonetheless, the code from Title is
only meant to prevent url traversal, it is not meant to prevent
file system path traversal.

Change-Id: Id690576326d03744acc8fbbe78f4b7a4b4c04d7e
---

diff --git a/includes/TemplateParser.php b/includes/TemplateParser.php
index 470a75c4b9..f581a80fbb 100644
--- a/includes/TemplateParser.php
+++ b/includes/TemplateParser.php
@@ -54,18 +54,11 @@ class TemplateParser {
 	 * @throws UnexpectedValueException If $templateName attempts upwards directory traversal
 	 */
 	protected function getTemplateFilename( $templateName ) {
-		// Prevent upwards directory traversal using same methods as Title::secureAndSplit
+		// Prevent path traversal. Based on Language::isValidCode().
+		// This is for paranoia. The $templateName should never come from
+		// untrusted input.
 		if (
-			strpos( $templateName, '.' ) !== false &&
-			(
-				$templateName === '.' || $templateName === '..' ||
-				strpos( $templateName, './' ) === 0 ||
-				strpos( $templateName, '../' ) === 0 ||
-				strpos( $templateName, '/./' ) !== false ||
-				strpos( $templateName, '/../' ) !== false ||
-				substr( $templateName, -2 ) === '/.' ||
-				substr( $templateName, -3 ) === '/..'
-			)
+			strcspn( $templateName, ":/\\\000&<>'\"%" ) !== strlen( $templateName )
 		) {
 			throw new UnexpectedValueException( "Malformed \$templateName: $templateName" );
 		}
@@ -128,6 +121,8 @@ class TemplateParser {
 			$code = $this->compileForEval( $fileContents, $filename );
 		}
 
+		echo "About to eval:\n";
+		echo $code;
 		$renderer = eval( $code );
 		if ( !is_callable( $renderer ) ) {
 			throw new RuntimeException( "Requested template, {$templateName}, is not callable" );
diff --git a/tests/phpunit/includes/TemplateParserTest.php b/tests/phpunit/includes/TemplateParserTest.php
index 469f45a5d9..2bd9086d60 100644
--- a/tests/phpunit/includes/TemplateParserTest.php
+++ b/tests/phpunit/includes/TemplateParserTest.php
@@ -51,6 +51,43 @@ class TemplateParserTest extends MediaWikiTestCase {
 				false,
 				'UnexpectedValueException'
 			],
+			[
+				"\000../foobar",
+				[],
+				false,
+				'UnexpectedValueException'
+			],
+			[
+				'/',
+				[],
+				false,
+				'UnexpectedValueException'
+			],
+			[
+				// Allegedly this can strip ext in windows.
+				'baz<',
+				[],
+				false,
+				'UnexpectedValueException'
+			],
+			[
+				'\\foo',
+				[],
+				false,
+				'UnexpectedValueException'
+			],
+			[
+				'C:\bar',
+				[],
+				false,
+				'UnexpectedValueException'
+			],
+			[
+				"foo\000bar",
+				[],
+				false,
+				'UnexpectedValueException'
+			],
 			[
 				'nonexistenttemplate',
 				[],