From d2c58f3bbc03846c460acddd38203387cd06416c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 19 Apr 2022 15:11:30 +1200 Subject: [PATCH 1/3] [CVE-2022-28803] Block XSS in links and iframes. --- src/Forms/HTMLEditor/HTMLEditorSanitiser.php | 11 +++++++++ .../HTMLEditor/HTMLEditorSanitiserTest.php | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php index 9cc194de1..1caff953e 100644 --- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php +++ b/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) { diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php index 97da89c97..6c1ba3b0d 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php @@ -74,6 +74,30 @@ class HTMLEditorSanitiserTest extends FunctionalTest 'Test', 'noopener rel attribute is unchanged when link_rel_value is null' ], + [ + 'a[href|target|rel]', + 'Test', + 'Test', + 'Javascript in the href attribute of a link is completely removed' + ], + [ + 'a[href|target|rel]', + 'Test', + 'Test', + 'Javascript in the href attribute of a link is completely removed even for multiline markup' + ], + [ + 'map[name],area[href|shape|coords]', + '', + '', + 'Javascript in the href attribute of a map\'s clickable area is completely removed' + ], + [ + 'iframe[src]', + '', + '', + 'Javascript in the src attribute of an iframe is completely removed' + ], ]; $config = HTMLEditorConfig::get('htmleditorsanitisertest'); From 991aedf017a529e0fcd1c1ade3d7da58546f7159 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 15 Feb 2022 17:49:51 +1300 Subject: [PATCH 2/3] [CVE-2022-25238] Sanitise htmlfields serverside by default --- .../09_Security/04_Secure_Coding.md | 59 +++++++------------ src/Forms/HTMLEditor/HTMLEditorField.php | 2 +- 2 files changed, 23 insertions(+), 38 deletions(-) diff --git a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md index 8d3bc5693..1a4032424 100644 --- a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md +++ b/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? - -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. +### Additional options 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 diff --git a/src/Forms/HTMLEditor/HTMLEditorField.php b/src/Forms/HTMLEditor/HTMLEditorField.php index 12279c3a8..9075a5895 100644 --- a/src/Forms/HTMLEditor/HTMLEditorField.php +++ b/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 From b5abc3845581ee922ae9ef50e5caecb21f5a4ec7 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Mon, 14 Feb 2022 18:25:51 +1300 Subject: [PATCH 3/3] CVE-2021-41559 Disable xml entities --- src/Core/Convert.php | 46 ++++++------- tests/php/Core/ConvertTest.php | 115 +++++++++++++++++++++------------ 2 files changed, 92 insertions(+), 69 deletions(-) diff --git a/src/Core/Convert.php b/src/Core/Convert.php index 872c3fe5e..6d1af179e 100644 --- a/src/Core/Convert.php +++ b/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 - if (\PHP_VERSION_ID >= 80000) { - $disableExternals = false; - } + Deprecation::notice('4.10', 'Use a dedicated XML library instead'); // Check doctype - if ($disableDoctypes && preg_match('/\<\!DOCTYPE.+]\>/', $val)) { + if ($disableDoctypes && strpos($val ?? '', '/', '', $val ?? ''); + + // If there's still an present, then it would be the result of a maliciously + // crafted XML document e.g. ENTITY ext SYSTEM "http://evil.com"> + if (strpos($val ?? '', ' before it was removed + $xml = new SimpleXMLElement($val ?? ''); + return self::recursiveXMLToArray($xml); } /** diff --git a/tests/php/Core/ConvertTest.php b/tests/php/Core/ConvertTest.php index 76aec076f..9b724d3cb 100644 --- a/tests/php/Core/ConvertTest.php +++ b/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 = << -]> + +]> - Now include &long; lots of times to expand the in-memory size of this XML structure - &long;&long;&long; + My para + Ampersand & is retained and not double encoded XML - ; - try { - Convert::xml2array($inputXML, true); - } catch (Exception $ex) { - } - $this->assertTrue( - isset($ex) - && $ex instanceof InvalidArgumentException - && $ex->getMessage() === 'XML Doctype parsing disabled' - ); - - // Test without doctype validation + ; $expected = [ - 'result' => [ - 'Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure', - [ - 'long' => [ - [ - 'long' => 'SOME_SUPER_LONG_STRING' - ], - [ - 'long' => 'SOME_SUPER_LONG_STRING' - ], - [ - 'long' => 'SOME_SUPER_LONG_STRING' - ] - ] - ] - ] + 'result' => [ + 'My para', + 'Ampersand & is retained and not double encoded' + ] ]; - $result = Convert::xml2array($inputXML, false, true); - $this->assertEquals( - $expected, - $result - ); - $result = Convert::xml2array($inputXML, false, false); - $this->assertEquals( - $expected, - $result - ); + $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 + */ + public function testXML2ArrayEntityException() + { + $inputXML = << + + ]> + + Now include &long; lots of times to expand the in-memory size of this XML structure + &long;&long;&long; + + XML; + $this->expectException(Exception::class); + $this->expectExceptionMessage('String could not be parsed as XML'); + Convert::xml2array($inputXML); + } + + /** + * Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed + */ + public function testXML2ArrayMultipleEntitiesException() + { + $inputXML = << + ]> + + Now include &long; and &short; lots of times + &long;&long;&long;&short;&short;&short; + + XML; + $this->expectException(Exception::class); + $this->expectExceptionMessage('String could not be parsed as XML'); + Convert::xml2array($inputXML); + } + + /** + * Tests {@link Convert::xml2array()} if there is a malicious present + */ + public function testXML2ArrayMaliciousEntityException() + { + $inputXML = << + ENTITY ext SYSTEM "http://evil.com"> + ]> + + Evil document + + XML; + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Malicious XML entity detected'); + Convert::xml2array($inputXML); } /**