From c2339636c07cf4a71312f9515f84128f5db547a8 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 12 Dec 2024 17:43:22 +1300 Subject: [PATCH] [CVE-2024-53277] Sanitise form messages against XSS attacks Includes some new additional XSS protection inspired by Symfony --- src/Core/XssSanitiser.php | 213 ++++++++ src/Forms/FormMessage.php | 8 +- src/Forms/HTMLEditor/HTMLEditorSanitiser.php | 17 +- tests/php/Core/XssSanitiserTest.php | 463 ++++++++++++++++++ tests/php/Forms/FormMessageTest.php | 100 ++++ .../Forms/FormMessageTest/TestFormMessage.php | 14 + 6 files changed, 804 insertions(+), 11 deletions(-) create mode 100644 src/Core/XssSanitiser.php create mode 100644 tests/php/Core/XssSanitiserTest.php create mode 100644 tests/php/Forms/FormMessageTest.php create mode 100644 tests/php/Forms/FormMessageTest/TestFormMessage.php diff --git a/src/Core/XssSanitiser.php b/src/Core/XssSanitiser.php new file mode 100644 index 00000000000..5ffdb224b95 --- /dev/null +++ b/src/Core/XssSanitiser.php @@ -0,0 +1,213 @@ +sanitiseHtmlValue($htmlValue); + return $htmlValue->getContent(); + } + + /** + * Remove XSS attack vectors from HTMLValue content + */ + public function sanitiseHtmlValue(HTMLValue $html): void + { + foreach ($html->query('//*') as $element) { + if (!is_a($element, DOMElement::class)) { + continue; + } + $this->sanitiseElement($element); + } + } + + /** + * Remove XSS attack vectors from a DOMElement + */ + public function sanitiseElement(DOMElement $element): void + { + // Remove elements first - if we remove the element, we don't have any attributes to check so exit early + $removed = $this->stripElement($element); + if ($removed) { + return; + } + $this->stripAttributes($element); + $this->stripAttributeContents($element); + } + + /** + * Get the names of elements which will be removed. + */ + public function getElementsToRemove(): array + { + return $this->elementsToRemove; + } + + /** + * Set the names of elements which will be removed. + * Note that allowing the elements which are included in the default list could result in XSS vulnerabilities. + */ + public function setElementsToRemove(array $elements): static + { + $this->elementsToRemove = $elements; + return $this; + } + + /** + * Get the names of attributes which will be removed from any elements that have them. + */ + public function getAttributesToRemove(): array + { + return $this->attributesToRemove; + } + + /** + * Set the names of attributes which will be removed from any elements that have them. + * Note that allowing the attributes which are included in the default list could result in XSS vulnerabilities. + */ + public function setAttributesToRemove(array $attributes): static + { + $this->attributesToRemove = $attributes; + return $this; + } + + /** + * Get whether the inner contents of an element will be kept for elements that get removed. + */ + public function getKeepInnerHtmlOnRemoveElement(): bool + { + return $this->keepInnerHtmlOnRemoveElement; + } + + /** + * Set whether to keep the inner contents of an element if it gets removed. + */ + public function setKeepInnerHtmlOnRemoveElement(bool $keep): static + { + $this->keepInnerHtmlOnRemoveElement = $keep; + return $this; + } + + /** + * If $element is one of the elements in $elementsToRemove, replace it + * with a text node. + */ + private function stripElement(DOMElement $element): bool + { + if (!in_array($element->tagName, $this->getElementsToRemove())) { + return false; + } + // Make sure we don't remove any child nodes + $parentNode = $element->parentNode; + if ($this->getKeepInnerHtmlOnRemoveElement() && $parentNode && $element->hasChildNodes()) { + // We can't just iterate through $node->childNodes because that seems to skip some children + while ($element->hasChildNodes()) { + $parentNode->insertBefore($element->firstChild, $element); + } + } + $element->remove(); + return true; + } + + /** + * Remove all attributes in $attributesToRemove from the element. + */ + private function stripAttributes(DOMElement $element): void + { + $attributesToRemove = $this->getAttributesToRemove(); + if (empty($attributesToRemove)) { + return; + } + $attributes = $element->attributes; + for ($i = count($attributes) - 1; $i >= 0; $i--) { + /** @var DOMAttr $attr */ + $attr = $attributes->item($i); + foreach ($attributesToRemove as $toRemove) { + if (str_starts_with($toRemove, '*') && str_ends_with($attr->name, str_replace('*', '', $toRemove))) { + $element->removeAttributeNode($attr); + } elseif (str_ends_with($toRemove, '*') && str_starts_with($attr->name, str_replace('*', '', $toRemove))) { + $element->removeAttributeNode($attr); + } elseif (!str_contains($toRemove, '*') && $attr->name === $toRemove) { + $element->removeAttributeNode($attr); + } + } + } + } + + /** + * Strip out attributes which have dangerous content which might otherwise execute javascript. + * This is content that we will always remove regardless of whether the attributes and elements in question + * are otherwise allowed, e.g. via WYSIWYG configuration. + */ + private function stripAttributeContents(DOMElement $element): void + { + $regex = $this->getStripAttributeContentsRegex(); + foreach (['lowsrc', 'src', 'href', 'data'] as $dangerAttribute) { + if ($element->hasAttribute($dangerAttribute)) { + $attrContent = $element->getAttribute($dangerAttribute); + if (preg_match($regex, $attrContent)) { + $element->removeAttribute($dangerAttribute); + } + } + } + } + + private function getStripAttributeContentsRegex(): string + { + $regexes = [ + $this->splitWithWhitespaceRegex('javascript:'), + $this->splitWithWhitespaceRegex('data:text/html'), + $this->splitWithWhitespaceRegex('vbscript:'), + ]; + // Regex is "starts with any of these, with optional whitespace at the start, case insensitive" + return '#^\s*(' . implode('|', $regexes) . ')#iu'; + } + + private function splitWithWhitespaceRegex(string $string): string + { + // Note that `\s` explicitly includes ALL invisible characters when used with the `u` modifier. + // That includes unicode characters like the non-breaking space. + return implode('\s*', str_split($string)); + } +} diff --git a/src/Forms/FormMessage.php b/src/Forms/FormMessage.php index ce18ee153a6..8a6d85b74a3 100644 --- a/src/Forms/FormMessage.php +++ b/src/Forms/FormMessage.php @@ -3,6 +3,7 @@ namespace SilverStripe\Forms; use InvalidArgumentException; +use SilverStripe\Core\XssSanitiser; use SilverStripe\ORM\ValidationResult; use SilverStripe\View\ViewableData; @@ -33,6 +34,7 @@ trait FormMessage /** * Returns the field message, used by form validation. + * If the current cast is ValidationResult::CAST_HTML, the message will be sanitised. * * Use {@link setError()} to set this property. * @@ -40,7 +42,11 @@ trait FormMessage */ public function getMessage() { - return $this->message; + $message = $this->message; + if ($this->getMessageCast() === ValidationResult::CAST_HTML) { + $message = XssSanitiser::create()->sanitiseString($message); + } + return $message; } /** diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php index c056bca10f3..49eda400afa 100644 --- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php +++ b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php @@ -6,6 +6,7 @@ use DOMElement; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\Core\XssSanitiser; use SilverStripe\View\Parsers\HTMLValue; use stdClass; @@ -289,6 +290,10 @@ public function sanitise(HTMLValue $html) { $linkRelValue = $this->config()->get('link_rel_value'); $doc = $html->getDocument(); + // Get a sanitiser but don't deny any specific attributes or elements, since that's + // handled as part of the element rules. + $xssSanitiser = XssSanitiser::create(); + $xssSanitiser->setElementsToRemove([])->setAttributesToRemove([]); /** @var DOMElement $el */ foreach ($html->query('//body//*') as $el) { @@ -342,16 +347,8 @@ public function sanitise(HTMLValue $html) $el->setAttribute($attr, $forced); } - // Matches "javascript:" with any arbitrary linebreaks inbetween the characters. - $regex = '#^\s*(' . implode('\s*', str_split('javascript:')) . '|' . implode('\s*', str_split('data:text/html;')) . ')#i'; - // Strip out javascript execution in href or src attributes. - foreach (['src', 'href', 'data'] as $dangerAttribute) { - if ($el->hasAttribute($dangerAttribute)) { - if (preg_match($regex, $el->getAttribute($dangerAttribute))) { - $el->removeAttribute($dangerAttribute); - } - } - } + // Explicit XSS sanitisation for anything that there's really no sensible use case for in a WYSIWYG + $xssSanitiser->sanitiseElement($el); } if ($el->tagName === 'a' && $linkRelValue !== null) { diff --git a/tests/php/Core/XssSanitiserTest.php b/tests/php/Core/XssSanitiserTest.php new file mode 100644 index 00000000000..0d1ef959447 --- /dev/null +++ b/tests/php/Core/XssSanitiserTest.php @@ -0,0 +1,463 @@ + '', + 'expected' => '', + ], + [ + 'input' => 'hello world', + 'expected' => 'hello world', + ], + [ + 'input' => '<hello world>', + 'expected' => '<hello world>', + ], + [ + 'input' => '< Hello', + 'expected' => ' Hello', + ], + [ + 'input' => 'Lorem & Ipsum', + 'expected' => 'Lorem & Ipsum', + ], + // Unknown tag + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + // Scripts + [ + 'input' => '', + 'expected' => 'alert(\'ok\');', + ], + [ + 'input' => 'javascript:/*-->', + 'expected' => 'javascript:/*-->', + ], + [ + // Not exploitable XSS + 'input' => 'ipt>alert(1)', + 'expected' => 'ipt>alert(1)', + ], + [ + // Not exploitable XSS + 'input' => 'ipt>alert(1)', + 'expected' => 'ipt>alert(1)', + ], + [ + // Not exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '
Lorem ipsum dolor sit amet, consectetur adipisicing elit.
', + 'expected' => '
Lorem ipsum dolor sit amet, consectetur adipisicing elit.alert(\'ok\');
', + ], + [ + 'input' => 'Lorem ipsum dolor sit amet, consectetur adipisicing elit.', + 'expected' => 'Lorem ipsum dolor sit amet, consectetur adipisicing elit.', + ], + [ + // Not exploitable XSS + 'input' => '<a href="javascript:evil"/>', + 'expected' => 'a href="javascript:evil"/>', + ], + [ + 'input' => 'Test', + 'expected' => 'Test', + ], + [ + 'input' => 'Test', + 'expected' => 'Test', + ], + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + // Note this includes U+200A, U+202F, U+205F, U+2000, U+2001, U+2002, U+2003, U+2004, U+2005, U+2006, U+2007, U+2008, U+2009, U+3000 + 'input' => "Lorem ipsum", + 'expected' => 'Lorem ipsum', + ], + [ + // Not exploitable XSS + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + 'input' => 'Test', + 'expected' => 'Test', + ], + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + // Not exploitable XSS + 'input' => '
', + 'expected' => '
', + ], + [ + // Not exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '<iframe src="javascript:evil"/>', + 'expected' => 'iframe src="javascript:evil"/>', + ], + [ + // Not exploitable XSS + 'input' => '<img src="javascript:evil"/>', + 'expected' => 'img src="javascript:evil"/>', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '"\>', + 'expected' => 'alert("XSS")"\>', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + // decodes to `javascript:alert('XSS')` + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + // Decodes to a SVG with `` inside it + // But that's not actually exploitable XSS + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => 'Image alternative text', + 'expected' => 'Image alternative text', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '
', + 'expected' => '
', + ], + [ + 'input' => '

', + 'expected' => '

', + ], + + [ + 'input' => '', + 'expected' => '', + ], + [ + // Decodes to a SVG with `` inside it + 'input' => '', + 'expected' => '', + ], + [ + // Not exploitable XSS + 'input' => '!!', + 'expected' => '!!', + ], + [ + 'input' => '!!', + 'expected' => '!!', + ], + [ + 'input' => '">"@x.y', + 'expected' => '">"@x.y', + ], + [ + 'input' => '
onetwothree
', + 'expected' => '
one2twothree
', + ], + // Styles + [ + 'input' => '', + 'expected' => 'body { background: red; }', + ], + [ + 'input' => '
Lorem ipsum dolor sit amet, consectetur.
', + 'expected' => '
Lorem ipsum dolor sit amet, consectetur.body { background: red; }
', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => 'Lorem ipsum dolor sit amet, consectetur.', + 'expected' => 'Lorem ipsum dolor sit amet, consectetur.', + ], + // Comments + [ + // Not exploitable XSS + 'input' => 'Lorem ipsum dolor sit amet, consectetur', + 'expected' => 'Lorem ipsum dolor sit amet, consectetur', + ], + [ + 'input' => 'Lorem ipsum ', + 'expected' => 'Lorem ipsum ', + ], + // Normal tags (just checking they don't get mangled) + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + 'input' => 'Lorem ipsum', + 'expected' => 'Lorem ipsum', + ], + [ + 'input' => 'Image alternative text', + 'expected' => 'Image alternative text', + ], + [ + 'input' => 'Image alternative text', + 'expected' => 'Image alternative text', + ], + [ + 'input' => '', + 'expected' => '', + ], + [ + 'input' => '
onetwothree
', + 'expected' => '
onetwothree
', + ], + ]; + } + + /** + * @dataProvider provideSanitise + */ + public function testSanitiseString(string $input, string $expected): void + { + $sanitiser = new XssSanitiser(); + $this->assertSame($expected, $sanitiser->sanitiseString($input)); + } + + /** + * @dataProvider provideSanitise + */ + public function testSanitiseHtmlValue(string $input, string $expected): void + { + $sanitiser = new XssSanitiser(); + $htmlValue = new HTMLValue($input); + $sanitiser->sanitiseHtmlValue($htmlValue); + $this->assertSame($expected, $htmlValue->getContent()); + } + + /** + * @dataProvider provideSanitise + */ + public function testSanitiseElement(string $input, string $expected): void + { + $sanitiser = new XssSanitiser(); + $htmlValue = new HTMLValue($input); + foreach ($htmlValue->query('//*') as $element) { + if (!is_a($element, DOMElement::class)) { + continue; + } + $element = $sanitiser->sanitiseElement($element); + } + $this->assertSame($expected, $htmlValue->getContent()); + } + + public function provideSanitiseElementsAllowed(): array + { + return [ + 'disallow these by default' => [ + 'input' => '', + 'removeElements' => null, + 'expected' => 'alert("one");', + ], + 'allow all' => [ + 'input' => '', + 'removeElements' => [], + 'expected' => '', + ], + 'disallow circle' => [ + 'input' => '', + 'removeElements' => ['circle'], + 'expected' => '', + ], + ]; + } + + /** + * @dataProvider provideSanitiseElementsAllowed + */ + public function testSanitiseElementsAllowed(string $input, ?array $removeElements, string $expected): void + { + $sanitiser = new XssSanitiser(); + if ($removeElements !== null) { + $sanitiser->setElementsToRemove($removeElements); + } + $this->assertSame($expected, $sanitiser->sanitiseString($input)); + } + + public function provideSanitiseAttributesAllowed(): array + { + return [ + 'disallow these by default' => [ + 'input' => 'abcd', + 'removeAttributes' => null, + 'expected' => 'abcd', + ], + 'allow all' => [ + 'input' => 'abcd', + 'removeAttributes' => [], + 'expected' => 'abcd', + ], + 'disallow class' => [ + 'input' => 'abcd', + 'removeAttributes' => ['class'], + 'expected' => 'abcd', + ], + 'wildcard attributes' => [ + 'input' => 'abcd', + 'removeAttributes' => [ + 'cla*', + '*tle', + // this one specifically won't do anything + 'di*ed', + ], + 'expected' => 'abcd', + ], + // Not sure why you'd do this, but this functionality is a natural consequence of how `*something` and `something*` are implemented. + 'remove all attributes' => [ + 'input' => 'abcd', + 'removeAttributes' => [ + '*', + ], + 'expected' => 'abcd', + ], + ]; + } + + /** + * @dataProvider provideSanitiseAttributesAllowed + */ + public function testSanitiseAttributesAllowed(string $input, ?array $removeAttributes, string $expected): void + { + $sanitiser = new XssSanitiser(); + if ($removeAttributes !== null) { + $sanitiser->setAttributesToRemove($removeAttributes); + } + $this->assertSame($expected, $sanitiser->sanitiseString($input)); + } + + public function provideSanitiseNoKeepInnerHtml(): array + { + return [ + 'keeps inner html' => [ + 'input' => '
something first
Keep thisand this
something last
', + 'keepInnerHtml' => true, + 'expected' => '
something firstKeep thisand thissomething last
', + ], + 'discards inner html' => [ + 'input' => '
something first
Keep thisand this
something last
', + 'keepInnerHtml' => false, + 'expected' => '
something firstsomething last
', + ], + 'multiple and nested disallowed elements (keep inner html)' => [ + 'input' => '
something
nested
nested2
last
', + 'keepInnerHtml' => true, + 'expected' => '
somethingnested nested2last
', + ], + 'multiple and nested disallowed elements (discard inner html)' => [ + 'input' => '
something
nested
nested2
last
', + 'keepInnerHtml' => false, + 'expected' => '
somethinglast
', + ], + ]; + } + + /** + * @dataProvider provideSanitiseNoKeepInnerHtml + */ + public function testSanitiseNoKeepInnerHtml(string $input, bool $keepInnerHtml, string $expected): void + { + $sanitiser = new XssSanitiser(); + $sanitiser->setElementsToRemove(['div'])->setKeepInnerHtmlOnRemoveElement($keepInnerHtml); + $this->assertSame($expected, $sanitiser->sanitiseString($input)); + } +} diff --git a/tests/php/Forms/FormMessageTest.php b/tests/php/Forms/FormMessageTest.php new file mode 100644 index 00000000000..02f689a7378 --- /dev/null +++ b/tests/php/Forms/FormMessageTest.php @@ -0,0 +1,100 @@ + [ + 'message' => '', + 'type' => '', + 'casting' => ValidationResult::CAST_HTML, + 'expected' => '', + ], + 'empty plain text' => [ + 'message' => '', + 'type' => '', + 'casting' => ValidationResult::CAST_TEXT, + 'expected' => '', + ], + 'plain HTML' => [ + 'message' => 'just some text', + 'type' => '', + 'casting' => ValidationResult::CAST_HTML, + 'expected' => 'just some text', + ], + 'plain plain text' => [ + 'message' => 'just some text', + 'type' => '', + 'casting' => ValidationResult::CAST_TEXT, + 'expected' => 'just some text', + ], + 'HTML in HTML' => [ + 'message' => '
link
', + 'type' => '', + 'casting' => ValidationResult::CAST_HTML, + 'expected' => '
link
', + ], + 'HTML in plain text' => [ + 'message' => '
link
', + 'type' => '', + 'casting' => ValidationResult::CAST_TEXT, + 'expected' => '
link
', + ], + 'Type doesnt matter HTML' => [ + 'message' => '
link
', + 'type' => 'an arbitrary string here', + 'casting' => ValidationResult::CAST_HTML, + 'expected' => '
link
', + ], + 'Type doesnt matter text' => [ + 'message' => '
link
', + 'type' => 'an arbitrary string here', + 'casting' => ValidationResult::CAST_TEXT, + 'expected' => '
link
', + ], + ]; + } + + /** + * Test that getMessage() generally works and calls the sanitiser as appropriate. + * Note we don't actually test the sanitisation here, as that is handled by the sanitiser's unit tests. + * @dataProvider provideGetMessage + */ + public function testGetMessage(string $message, string $type, string $casting, string $expected): void + { + $mockSanitiserClass = get_class(new class extends XssSanitiser { + public static int $called = 0; + public function sanitiseString(string $html): string + { + static::$called++; + return parent::sanitiseString($html); + } + }); + Injector::inst()->load([ + XssSanitiser::class => [ + 'class' => $mockSanitiserClass, + ], + ]); + $expectedSanitisationCount = $casting === ValidationResult::CAST_HTML ? 1 : 0; + + try { + $formMessage = new TestFormMessage(); + $formMessage->setMessage($message, $type, $casting); + $this->assertSame($expected, $formMessage->getMessage()); + $this->assertSame($expectedSanitisationCount, $mockSanitiserClass::$called); + } finally { + $mockSanitiserClass::$called = 0; + } + } +} diff --git a/tests/php/Forms/FormMessageTest/TestFormMessage.php b/tests/php/Forms/FormMessageTest/TestFormMessage.php new file mode 100644 index 00000000000..a77dfcd347b --- /dev/null +++ b/tests/php/Forms/FormMessageTest/TestFormMessage.php @@ -0,0 +1,14 @@ +