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 470a75c..f581a80 100644 (file)
--- 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 469f45a..2bd9086 100644 (file)
--- 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',
                                [],