[SECURITY] XSS issues in Fluid view helpers 37/59537/2
authorAndreas Wolf <dev@a-w.io>
Tue, 22 Jan 2019 08:43:08 +0000 (09:43 +0100)
committerOliver Hader <oliver.hader@typo3.org>
Tue, 22 Jan 2019 08:43:11 +0000 (09:43 +0100)
* HtmlentitiesViewHelper
* UrlencodeViewHelper
* StripTagsViewHelper

Resolves: #85764
Releases: master, 9.5, 8.7
Security-Commit: 37bc147e634d67d521b716f83ca8d925ec57d531
Security-Bulletin: TYPO3-CORE-SA-2019-005
Change-Id: I1d5473b20378217a68e06c792be7f1cf096859fe
Reviewed-on: https://review.typo3.org/59537
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Tested-by: Oliver Hader <oliver.hader@typo3.org>
typo3/sysext/fluid/Classes/ViewHelpers/Format/HtmlentitiesViewHelper.php
typo3/sysext/fluid/Classes/ViewHelpers/Format/StripTagsViewHelper.php
typo3/sysext/fluid/Classes/ViewHelpers/Format/UrlencodeViewHelper.php
typo3/sysext/fluid/Tests/Unit/ViewHelpers/Format/HtmlentitiesViewHelperTest.php
typo3/sysext/fluid/Tests/Unit/ViewHelpers/Format/StripTagsViewHelperTest.php
typo3/sysext/fluid/Tests/Unit/ViewHelpers/Format/UrlencodeViewHelperTest.php

index 7778514..84477cd 100644 (file)
@@ -82,13 +82,13 @@ class HtmlentitiesViewHelper extends AbstractEncodingViewHelper
         $keepQuotes = $arguments['keepQuotes'];
         $doubleEncode = $arguments['doubleEncode'];
 
-        if (!is_string($value)) {
+        if (!is_string($value) && !(is_object($value) && method_exists($value, '__toString'))) {
             return $value;
         }
         if ($encoding === null) {
             $encoding = self::resolveDefaultEncoding();
         }
-        $flags = $keepQuotes ? ENT_NOQUOTES : ENT_COMPAT;
-        return htmlentities($value, $flags, $encoding, $doubleEncode);
+        $flags = $keepQuotes ? ENT_NOQUOTES : ENT_QUOTES;
+        return htmlentities((string)$value, $flags, $encoding, $doubleEncode);
     }
 }
index 51eaf59..8436fbd 100644 (file)
@@ -99,9 +99,9 @@ unction');
     ) {
         $value = $renderChildrenClosure();
         $allowedTags = $arguments['allowedTags'];
-        if (!is_string($value)) {
+        if (!is_string($value) && !(is_object($value) && method_exists($value, '__toString'))) {
             return $value;
         }
-        return strip_tags($value, $allowedTags);
+        return strip_tags((string)$value, $allowedTags);
     }
 }
index f41599f..ddc3551 100644 (file)
@@ -74,10 +74,9 @@ class UrlencodeViewHelper extends AbstractViewHelper
     public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, RenderingContextInterface $renderingContext)
     {
         $value = $renderChildrenClosure();
-
-        if (!is_string($value)) {
+        if (!is_string($value) && !(is_object($value) && method_exists($value, '__toString'))) {
             return $value;
         }
-        return rawurlencode($value);
+        return rawurlencode((string)$value);
     }
 }
index 1c5a63a..a5e2410 100644 (file)
@@ -91,7 +91,7 @@ class HtmlentitiesViewHelperTest extends ViewHelperBaseTestcase
     /**
      * @test
      */
-    public function renderDecodesSimpleString()
+    public function renderEncodesSimpleString()
     {
         $source = 'Some special characters: &©"\'';
         $this->setArgumentsUnderTest(
@@ -100,7 +100,7 @@ class HtmlentitiesViewHelperTest extends ViewHelperBaseTestcase
                 'value' => $source
             ]
         );
-        $expectedResult = 'Some special characters: &amp;&copy;&quot;\'';
+        $expectedResult = 'Some special characters: &amp;&copy;&quot;&#039;';
         $actualResult = $this->viewHelper->initializeArgumentsAndRender();
         $this->assertEquals($expectedResult, $actualResult);
     }
@@ -136,7 +136,7 @@ class HtmlentitiesViewHelperTest extends ViewHelperBaseTestcase
                 'encoding' => 'ISO-8859-1',
             ]
         );
-        $expectedResult = 'Some special characters: &amp;&copy;&quot;\'';
+        $expectedResult = 'Some special characters: &amp;&copy;&quot;&#039;';
         $actualResult = $this->viewHelper->initializeArgumentsAndRender();
         $this->assertEquals($expectedResult, $actualResult);
     }
@@ -194,4 +194,45 @@ class HtmlentitiesViewHelperTest extends ViewHelperBaseTestcase
         $actualResult = $this->viewHelper->render();
         $this->assertSame($source, $actualResult);
     }
+
+    /**
+     * Ensures that obejcts are handled properly:
+     * + class not having __toString() method as given
+     * + class having __toString() method gets encoded
+     *
+     * @param object $source
+     * @param mixed $expectation
+     * @test
+     * @dataProvider renderEscapesObjectIfPossibleDataProvider
+     */
+    public function renderEscapesObjectIfPossible($source, $expectation)
+    {
+        $this->setArgumentsUnderTest(
+            $this->viewHelper,
+            [
+                'value' => $source
+            ]
+        );
+        $actualResult = $this->viewHelper->render();
+        $this->assertSame($expectation, $actualResult);
+    }
+
+    /**
+     * @return array
+     */
+    public function renderEscapesObjectIfPossibleDataProvider(): array
+    {
+        $stdClass = new \stdClass();
+        $toStringClass = new class() {
+            public function __toString(): string
+            {
+                return '<script>alert(\'"&xss"\')</script>';
+            }
+        };
+
+        return [
+            'plain object' => [$stdClass, $stdClass],
+            'object with __toString()' => [$toStringClass, '&lt;script&gt;alert(&#039;&quot;&amp;xss&quot;&#039;)&lt;/script&gt;'],
+        ];
+    }
 }
index 2425481..446ef88 100644 (file)
@@ -100,11 +100,17 @@ class StripTagsViewHelperTest extends ViewHelperBaseTestcase
     }
 
     /**
+     * Ensures that obejcts are handled properly:
+     * + class not having __toString() method as given
+     * + class having __toString() method gets tags stripped off
+     *
+     * @param $source
+     * @param $expectation
      * @test
+     * @dataProvider renderEscapesObjectIfPossibleDataProvider
      */
-    public function renderReturnsUnmodifiedSourceIfItIsNoString()
+    public function renderEscapesObjectIfPossible($source, $expectation)
     {
-        $source = new \stdClass();
         $this->setArgumentsUnderTest(
             $this->viewHelper,
             [
@@ -112,6 +118,25 @@ class StripTagsViewHelperTest extends ViewHelperBaseTestcase
             ]
         );
         $actualResult = $this->viewHelper->render();
-        $this->assertSame($source, $actualResult);
+        $this->assertSame($expectation, $actualResult);
+    }
+
+    /**
+     * @return array
+     */
+    public function renderEscapesObjectIfPossibleDataProvider(): array
+    {
+        $stdClass = new \stdClass();
+        $toStringClass = new class() {
+            public function __toString(): string
+            {
+                return '<script>alert(\'"xss"\')</script>';
+            }
+        };
+
+        return [
+            'plain object' => [$stdClass, $stdClass],
+            'object with __toString()' => [$toStringClass, 'alert(\'"xss"\')'],
+        ];
     }
 }
index 3fd90d2..08bd01e 100644 (file)
@@ -102,20 +102,43 @@ class UrlencodeViewHelperTest extends ViewHelperBaseTestcase
     }
 
     /**
+     * Ensures that obejcts are handled properly:
+     * + class not having __toString() method as given
+     * + class having __toString() method gets rawurlencoded
+     *
+     * @param $source
+     * @param $expectation
      * @test
+     * @dataProvider renderEscapesObjectIfPossibleDataProvider
      */
-    public function renderReturnsUnmodifiedSourceIfItIsNoString()
+    public function renderEscapesObjectIfPossible($source, $expectation)
     {
-        $source = new \stdClass();
-
         $this->setArgumentsUnderTest(
             $this->viewHelper,
             [
                 'value' => $source
             ]
         );
-
         $actualResult = $this->viewHelper->render();
-        $this->assertSame($source, $actualResult);
+        $this->assertSame($expectation, $actualResult);
+    }
+
+    /**
+     * @return array
+     */
+    public function renderEscapesObjectIfPossibleDataProvider(): array
+    {
+        $stdClass = new \stdClass();
+        $toStringClass = new class() {
+            public function __toString(): string
+            {
+                return '<script>alert(\'"xss"\')</script>';
+            }
+        };
+
+        return [
+            'plain object' => [$stdClass, $stdClass],
+            'object with __toString()' => [$toStringClass, '%3Cscript%3Ealert%28%27%22xss%22%27%29%3C%2Fscript%3E'],
+        ];
     }
 }