Merge branch '4.11' into 4

docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md

@@ -214,39 +214,34 @@ or [sanitize](http://htmlpurifier.org/) it correctly.
 See [http://shiflett.org/articles/foiling-cross-site-attacks](http://shiflett.org/articles/foiling-cross-site-attacks)
 for in-depth information about "Cross-Site-Scripting".
 
-### What if I can't trust my editors?
+### Additional options
-
-The default configuration of Silverstripe CMS assumes some level of trust is given to your editors who have access
-to the CMS. Though the HTML WYSIWYG editor is configured to provide some control over the HTML an editor provides,
-this is not enforced server side, and so can be bypassed by a malicious editor. A editor that does so can use an
-XSS attack against an admin to perform any administrative action.
-
-If you can't trust your editors, Silverstripe CMS must be configured to filter the content so that any javascript is
-stripped out
-
-To enable filtering, set the HtmlEditorField::$sanitise_server_side [configuration](/developer_guides/configuration/configuration) property to
-true, e.g.
-
-```
-HtmlEditorField::config()->sanitise_server_side = true
-```
-
-The built in sanitiser enforces the TinyMCE whitelist rules on the server side, and is sufficient to eliminate the
-most common XSS vectors.
-
-However some subtle XSS attacks that exploit HTML parsing bugs need heavier filtering. For greater protection
-you can install the [htmlpurifier](https://github.com/silverstripe-labs/silverstripe-htmlpurifier) module which
-will replace the built in sanitiser with one that uses the [HTML Purifier](http://htmlpurifier.org/) library.
-In both cases, you must ensure that you have not configured TinyMCE to explicitly allow script elements or other
-javascript-specific attributes.
 
 For `HTMLText` database fields which aren't edited through `HtmlEditorField`, you also
 have the option to explicitly whitelist allowed tags in the field definition, e.g. `"MyField" => "HTMLText('meta','link')"`.
 The `SiteTree.ExtraMeta` property uses this to limit allowed input.
 
-##### But I also need my editors to provide javascript
+### What if I need to allow script or style tags?
 
-It is not currently possible to allow editors to provide javascript content and yet still protect other users
+The default configuration of Silverstripe CMS uses a santiser to enforce TinyMCE whitelist rules on the server side,
+and is sufficient to eliminate the most common XSS vectors. Notably, this will remove script and style tags.
+
+If your site requires script or style tags to be added via TinyMCE, Silverstripe CMS can be configured to disable the
+server side santisation. You will also need to update the TinyMCE whitelist [settings](/developer_guides/forms/field_types/htmleditorfield/#setting-options) to remove the frontend sanitisation.
+
+However, it's strongly discouraged as it opens up the possibility of malicious code being added to your site through the CMS.
+
+To disable filtering, set the `HtmlEditorField::$sanitise_server_side` [configuration](/developer_guides/configuration/configuration) property to `false`, i.e.
+
+```yml
+---
+Name: project-htmleditor
+After: htmleditor
+---
+SilverStripe\Forms\HTMLEditor\HTMLEditorField:
+  sanitise_server_side: false
+```
+
+Note it is not currently possible to allow editors to provide javascript content and yet still protect other users
 from any malicious code within that javascript.
 
 We recommend configuring [shortcodes](/developer_guides/extending/shortcodes) that can be used by editors in place of using javascript directly.
@@ -435,16 +430,6 @@ Some rules of thumb:
 *  Don't concatenate URLs in a template.  It only works in extremely simple cases that usually contain bugs.
 *  Use *Controller::join_links()* to concatenate URLs.  It deals with query strings and other such edge cases.
 
-### Filtering incoming HTML from TinyMCE
-
-In some cases you may be particularly concerned about which HTML elements are addable to Content via the CMS.
-By default, although TinyMCE is configured to restrict some dangerous tags (such as `script` tags), this restriction
-is not enforced server-side. A malicious user with write access to the CMS might create a specific request to avoid
-these restrictions.
-
-To enable server side filtering using the same whitelisting controls as TinyMCE, set the
-HtmlEditorField::$sanitise_server_side config property to true.
-
 ## Cross-Site Request Forgery (CSRF)
 
 Silverstripe CMS has built-in countermeasures against [CSRF](http://shiflett.org/articles/cross-site-request-forgeries) identity theft for all form submissions. A form object

src/Core/Convert.php

@@ -2,12 +2,11 @@
 
 namespace SilverStripe\Core;
 
+use InvalidArgumentException;
+use SimpleXMLElement;
 use SilverStripe\Dev\Deprecation;
 use SilverStripe\ORM\DB;
 use SilverStripe\View\Parsers\URLSegmentFilter;
-use InvalidArgumentException;
-use SimpleXMLElement;
-use Exception;
 
 /**
  * Library of conversion functions, implemented as static methods.
@@ -29,7 +28,6 @@ use Exception;
  */
 class Convert
 {
-
     /**
      * Convert a value to be suitable for an XML attribute.
      *
@@ -296,40 +294,34 @@ class Convert
      * @param string $val
      * @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered.
      * false by default.
-     * @param boolean $disableExternals Disables the loading of external entities. false by default. No-op in PHP 8.
+     * @param boolean $disableExternals Does nothing because xml entities are removed
+     * @deprecated 4.11.0:5.0.0
      * @return array
      * @throws Exception
      */
     public static function xml2array($val, $disableDoctypes = false, $disableExternals = false)
     {
-        // PHP 8 deprecates libxml_disable_entity_loader() as it is no longer needed
+        Deprecation::notice('4.10', 'Use a dedicated XML library instead');
-        if (\PHP_VERSION_ID >= 80000) {
-            $disableExternals = false;
-        }
 
         // Check doctype
-        if ($disableDoctypes && preg_match('/\<\!DOCTYPE.+]\>/', $val ?? '')) {
+        if ($disableDoctypes && strpos($val ?? '', '<!DOCTYPE') !== false) {
             throw new InvalidArgumentException('XML Doctype parsing disabled');
         }
 
-        // Disable external entity loading
+        // CVE-2021-41559 Ensure entities are removed due to their inherent security risk via
-        $oldVal = null;
+        // XXE attacks and quadratic blowup attacks, and also lack of consistent support
-        if ($disableExternals) {
+        $val = preg_replace('/(?s)<!ENTITY.*?>/', '', $val ?? '');
-            $oldVal = libxml_disable_entity_loader($disableExternals ?? false);
+
+        // If there's still an <!ENTITY> present, then it would be the result of a maliciously
+        // crafted XML document e.g. <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
+        if (strpos($val ?? '', '<!ENTITY') !== false) {
+            throw new InvalidArgumentException('Malicious XML entity detected');
         }
-        try {
+
-            $xml = new SimpleXMLElement($val);
+        // This will throw an exception if the XML contains references to any internal entities
-            $result = self::recursiveXMLToArray($xml);
+        // that were defined in an <!ENTITY /> before it was removed
-        } catch (Exception $ex) {
+        $xml = new SimpleXMLElement($val ?? '');
-            if ($disableExternals) {
+        return self::recursiveXMLToArray($xml);
-                libxml_disable_entity_loader($oldVal);
-            }
-            throw $ex;
-        }
-        if ($disableExternals) {
-            libxml_disable_entity_loader($oldVal ?? false);
-        }
-        return $result;
     }
 
     /**

src/Forms/HTMLEditor/HTMLEditorField.php

@@ -40,7 +40,7 @@ class HTMLEditorField extends TextareaField
      * @config
      * @var bool
      */
-    private static $sanitise_server_side = false;
+    private static $sanitise_server_side = true;
 
     /**
      * Number of rows

src/Forms/HTMLEditor/HTMLEditorSanitiser.php

@@ -345,6 +345,17 @@ class HTMLEditorSanitiser
                 foreach ($elementRule->attributesForced as $attr => $forced) {
                     $el->setAttribute($attr, $forced);
                 }
+
+                // Matches "javascript:" with any arbitrary linebreaks inbetween the characters.
+                $regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/';
+                // Strip out javascript execution in href or src attributes.
+                foreach (['src', 'href'] as $dangerAttribute) {
+                    if ($el->hasAttribute($dangerAttribute)) {
+                        if (preg_match($regex, $el->getAttribute($dangerAttribute))) {
+                            $el->removeAttribute($dangerAttribute);
+                        }
+                    }
+                }
             }
 
             if ($el->tagName === 'a' && $linkRelValue !== null) {

tests/php/Core/ConvertTest.php

@@ -403,55 +403,86 @@ PHP
      */
     public function testXML2Array()
     {
-        // Ensure an XML file at risk of entity expansion can be avoided safely
+
         $inputXML = <<<XML
 <?xml version="1.0"?>
-<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING">]>
+<!DOCTYPE results [
+  <!ENTITY long "SOME_SUPER_LONG_STRING">
+]>
+<results>
+    <result>My para</result>
+    <result>Ampersand &amp; is retained and not double encoded</result>
+</results>
+XML
+        ;
+        $expected = [
+            'result' => [
+                'My para',
+                'Ampersand & is retained and not double encoded'
+            ]
+        ];
+        $actual = Convert::xml2array($inputXML, false);
+        $this->assertEquals($expected, $actual);
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('XML Doctype parsing disabled');
+        Convert::xml2array($inputXML, true);
+    }
+
+    /**
+     * Tests {@link Convert::xml2array()} if an exception the contains a reference to a removed <!ENTITY />
+     */
+    public function testXML2ArrayEntityException()
+    {
+        $inputXML = <<<XML
+        <?xml version="1.0"?>
+        <!DOCTYPE results [
+            <!ENTITY long "SOME_SUPER_LONG_STRING">
+        ]>
         <results>
             <result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
             <result>&long;&long;&long;</result>
         </results>
-XML
+        XML;
-         ;
+        $this->expectException(Exception::class);
-        try {
+        $this->expectExceptionMessage('String could not be parsed as XML');
-            Convert::xml2array($inputXML, true);
+        Convert::xml2array($inputXML);
-        } catch (Exception $ex) {
     }
-        $this->assertTrue(
-            isset($ex)
-            && $ex instanceof InvalidArgumentException
-            && $ex->getMessage() === 'XML Doctype parsing disabled'
-        );
 
-        // Test without doctype validation
+    /**
-        $expected = [
+     * Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed <!ENTITY />
-         'result' => [
+     */
-             'Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure',
+    public function testXML2ArrayMultipleEntitiesException()
-          [
+    {
-        'long' => [
+        $inputXML = <<<XML
-         [
+        <?xml version="1.0"?>
-          'long' => 'SOME_SUPER_LONG_STRING'
+        <!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING"><!ENTITY short "SHORT_STRING">]>
-         ],
+        <results>
-         [
+            <result>Now include &long; and &short; lots of times</result>
-          'long' => 'SOME_SUPER_LONG_STRING'
+            <result>&long;&long;&long;&short;&short;&short;</result>
-         ],
+        </results>
-         [
+        XML;
-          'long' => 'SOME_SUPER_LONG_STRING'
+        $this->expectException(Exception::class);
-         ]
+        $this->expectExceptionMessage('String could not be parsed as XML');
-           ]
+        Convert::xml2array($inputXML);
-             ]
+    }
-         ]
+
-        ];
+    /**
-        $result = Convert::xml2array($inputXML, false, true);
+     * Tests {@link Convert::xml2array()} if there is a malicious <!ENTITY /> present
-        $this->assertEquals(
+     */
-            $expected,
+    public function testXML2ArrayMaliciousEntityException()
-            $result
+    {
-        );
+        $inputXML = <<<XML
-        $result = Convert::xml2array($inputXML, false, false);
+        <?xml version="1.0"?>
-        $this->assertEquals(
+        <!DOCTYPE results [
-            $expected,
+            <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
-            $result
+        ]>
-        );
+        <results>
+            <result>Evil document</result>
+        </results>
+        XML;
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('Malicious XML entity detected');
+        Convert::xml2array($inputXML);
     }
 
     /**

tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php

@@ -74,6 +74,30 @@ class HTMLEditorSanitiserTest extends FunctionalTest
                 '<a href="/test" target="_blank">Test</a>',
                 'noopener rel attribute is unchanged when link_rel_value is null'
             ],
+            [
+                'a[href|target|rel]',
+                '<a href="javascript:alert(0);">Test</a>',
+                '<a>Test</a>',
+                'Javascript in the href attribute of a link is completely removed'
+            ],
+            [
+                'a[href|target|rel]',
+                '<a href="' . implode("\n", str_split(' javascript:')) . '">Test</a>',
+                '<a>Test</a>',
+                'Javascript in the href attribute of a link is completely removed even for multiline markup'
+            ],
+            [
+                'map[name],area[href|shape|coords]',
+                '<map name="test"><area shape="rect" coords="34,44,270,350" href="javascript:alert(0);"></map>',
+                '<map name="test"><area shape="rect" coords="34,44,270,350"></map>',
+                'Javascript in the href attribute of a map\'s clickable area is completely removed'
+            ],
+            [
+                'iframe[src]',
+                '<iframe src="javascript:alert(0);"></iframe>',
+                '<iframe></iframe>',
+                'Javascript in the src attribute of an iframe is completely removed'
+            ],
         ];
 
         $config = HTMLEditorConfig::get('htmleditorsanitisertest');