[SECURITY] Disallow invalid encoding in GeneralUtility::validPathStr 40/50740/2
authorBenni Mack <benni@typo3.org>
Tue, 22 Nov 2016 10:09:16 +0000 (11:09 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 22 Nov 2016 10:09:18 +0000 (11:09 +0100)
Directory names, which have an invalid UTF encoding,
cause the preg_match() to return false.
To avoid that the complete statement in GeneralUtility::validPathStr()
returns true in this case, a strict comparison against 0 is added,
so that we ensure that strings with invalid encodings are rejected
by this API method.

As a consequence UTF-16 encoded path names are rejected as well, if the
system / file system does not support them.

Resolves: #73453
Releases: master, 8.4, 7.6, 6.2
Security-Commit: 2a05bec1cfd6fdafdaba8de51369f1d86ca60db0
Security-Bulletins: TYPO3-CORE-SA-2016-023, 024
Change-Id: I875d45005b4a8b8d027fba078c9be399bb13a782
Reviewed-on: https://review.typo3.org/50740
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
composer.json
composer.lock
typo3/sysext/core/Classes/Utility/GeneralUtility.php
typo3/sysext/core/Tests/Unit/Utility/GeneralUtilityTest.php

index de0dac4..21744ac 100644 (file)
@@ -47,7 +47,8 @@
        },
        "require-dev": {
                "phpunit/phpunit": "~4.8.0",
-               "mikey179/vfsStream": "1.6.0"
+               "mikey179/vfsStream": "1.6.0",
+               "symfony/polyfill-mbstring": "~1.0"
        },
        "suggest": {
                "ext-gd": "GDlib/Freetype is required for building images with text (GIFBUILDER) and can also be used to scale images",
index f5fe1b6..a2701a2 100644 (file)
@@ -4,8 +4,8 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",
         "This file is @generated automatically"
     ],
-    "hash": "eefa09bcf4e772af6f855ca0f2df0510",
-    "content-hash": "26df927d26dda8dbb6aa07ec809a53a9",
+    "hash": "dd1162b906da731fc7f4de62ec09eb05",
+    "content-hash": "5286db76845b82466c2f426e621d88c2",
     "packages": [
         {
             "name": "cogpowered/finediff",
             "time": "2015-06-21 13:59:46"
         },
         {
+            "name": "symfony/polyfill-mbstring",
+            "version": "v1.3.0",
+            "source": {
+                "type": "git",
+                "url": "https://github.com/symfony/polyfill-mbstring.git",
+                "reference": "e79d363049d1c2128f133a2667e4f4190904f7f4"
+            },
+            "dist": {
+                "type": "zip",
+                "url": "https://api.github.com/repos/symfony/polyfill-mbstring/zipball/e79d363049d1c2128f133a2667e4f4190904f7f4",
+                "reference": "e79d363049d1c2128f133a2667e4f4190904f7f4",
+                "shasum": ""
+            },
+            "require": {
+                "php": ">=5.3.3"
+            },
+            "suggest": {
+                "ext-mbstring": "For best performance"
+            },
+            "type": "library",
+            "extra": {
+                "branch-alias": {
+                    "dev-master": "1.3-dev"
+                }
+            },
+            "autoload": {
+                "psr-4": {
+                    "Symfony\\Polyfill\\Mbstring\\": ""
+                },
+                "files": [
+                    "bootstrap.php"
+                ]
+            },
+            "notification-url": "https://packagist.org/downloads/",
+            "license": [
+                "MIT"
+            ],
+            "authors": [
+                {
+                    "name": "Nicolas Grekas",
+                    "email": "p@tchwork.com"
+                },
+                {
+                    "name": "Symfony Community",
+                    "homepage": "https://symfony.com/contributors"
+                }
+            ],
+            "description": "Symfony polyfill for the Mbstring extension",
+            "homepage": "https://symfony.com",
+            "keywords": [
+                "compatibility",
+                "mbstring",
+                "polyfill",
+                "portable",
+                "shim"
+            ],
+            "time": "2016-11-14 01:06:16"
+        },
+        {
             "name": "symfony/yaml",
             "version": "v2.7.4",
             "source": {
index dbda2ab..5172a48 100644 (file)
@@ -3916,12 +3916,11 @@ class GeneralUtility
      * @param string $theFile File path to evaluate
      * @return bool TRUE, $theFile is allowed path string, FALSE otherwise
      * @see http://php.net/manual/en/security.filesystem.nullbytes.php
-     * @todo Possible improvement: Should it rawurldecode the string first to check if any of these characters is encoded?
      */
     public static function validPathStr($theFile)
     {
         return strpos($theFile, '//') === false && strpos($theFile, '\\') === false
-            && !preg_match('#(?:^\\.\\.|/\\.\\./|[[:cntrl:]])#u', $theFile);
+            && preg_match('#(?:^\\.\\.|/\\.\\./|[[:cntrl:]])#u', $theFile) === 0;
     }
 
     /**
index 02b0154..4af9090 100644 (file)
@@ -4272,14 +4272,34 @@ class GeneralUtilityTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
      */
     public function validPathStrInvalidCharactersDataProvider()
     {
-        return [
+        $data = [
             'double slash in path' => ['path//path'],
             'backslash in path' => ['path\\path'],
             'directory up in path' => ['path/../path'],
             'directory up at the beginning' => ['../path'],
             'NUL character in path' => ['path' . chr(0) . 'path'],
-            'BS character in path' => ['path' . chr(8) . 'path']
+            'BS character in path' => ['path' . chr(8) . 'path'],
+            'invalid UTF-8-sequence' => ["\xc0" . 'path/path'],
+            'Could be overlong NUL in some UTF-8 implementations, invalid in RFC3629' => ["\xc0\x80" . 'path/path'],
         ];
+
+        // Mixing with regular utf-8
+        $utf8Characters = 'Ссылка/';
+        foreach ($data as $key => $value) {
+            $data[$key . ' with UTF-8 characters prepended'] = [$utf8Characters . $value[0]];
+            $data[$key . ' with UTF-8 characters appended'] = [$value[0] . $utf8Characters];
+        }
+
+        // Encoding with UTF-16
+        foreach ($data as $key => $value) {
+            $data[$key . ' encoded with UTF-16'] = [mb_convert_encoding($value[0], 'UTF-16')];
+        }
+
+        // Valid paths encoded with UTF-16
+        $data['Valid path but UTF-16 encoded'] = [mb_convert_encoding('fileadmin/foo.txt', 'UTF-16')];
+        $data['UTF-8 characters with UTF-16 encoded'] = [mb_convert_encoding('fileadmin/templates/Ссылка (fce).xml', 'UTF-16')];
+
+        return $data;
     }
 
     /**
@@ -4295,13 +4315,27 @@ class GeneralUtilityTest extends \TYPO3\CMS\Core\Tests\UnitTestCase
     }
 
     /**
+     * Data provider for positive values within validPathStr()
+     */
+    public function validPathStrDataProvider()
+    {
+        $data = [
+            'normal ascii path' => ['fileadmin/templates/myfile..xml'],
+            'special character' => ['fileadmin/templates/Ссылка (fce).xml']
+        ];
+
+        return $data;
+    }
+
+    /**
      * Tests whether Unicode characters are recognized as valid file name characters.
      *
+     * @dataProvider validPathStrDataProvider
      * @test
      */
-    public function validPathStrWorksWithUnicodeFileNames()
+    public function validPathStrWorksWithUnicodeFileNames($path)
     {
-        $this->assertTrue(GeneralUtility::validPathStr('fileadmin/templates/Ссылка (fce).xml'));
+        $this->assertTrue(GeneralUtility::validPathStr($path));
     }
 
     /**