Skip to content

Commit 74904f5

Browse files
GuySartorelliGuySartorelli
authored
[CVE-2024-53277] Sanitise form messages against XSS attacks (#11555)
Includes some new additional XSS protection inspired by Symfony
1 parent 09b5052 commit 74904f5

File tree

6 files changed

+804
-11
lines changed

6 files changed

+804
-11
lines changed

‎src/Core/XssSanitiser.php

+213
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
<?php
2+
3+
namespace SilverStripe\Core;
4+
5+
use DOMAttr;
6+
use DOMElement;
7+
use SilverStripe\Core\Injector\Injectable;
8+
use SilverStripe\View\Parsers\HTMLValue;
9+
10+
/**
11+
* Sanitises HTML to prevent XSS attacks.
12+
*/
13+
class XssSanitiser
14+
{
15+
use Injectable;
16+
17+
/**
18+
* Attributes which will be removed from any element.
19+
* If an asterisk is at the start of the attribute name, all attributes ending with this name will be removed.
20+
* If an asterisk is at the end of the attribute name, all attributes starting with this name will be removed.
21+
* For example `on*` will remove `onerror`, `onmouseover`, etc
22+
*/
23+
private array $attributesToRemove = [
24+
'on*',
25+
'accesskey',
26+
];
27+
28+
private array $elementsToRemove = [
29+
'embed',
30+
'object',
31+
'script',
32+
'style',
33+
'svg',
34+
];
35+
36+
private bool $keepInnerHtmlOnRemoveElement = true;
37+
38+
private bool $removeDataSvg = true;
39+
40+
private bool $removeSvgFile = true;
41+
42+
/**
43+
* Remove XSS attack vectors from an HTML fragment string
44+
*/
45+
public function sanitiseString(string $html): string
46+
{
47+
$htmlValue = HTMLValue::create($html);
48+
$this->sanitiseHtmlValue($htmlValue);
49+
return $htmlValue->getContent();
50+
}
51+
52+
/**
53+
* Remove XSS attack vectors from HTMLValue content
54+
*/
55+
public function sanitiseHtmlValue(HTMLValue $html): void
56+
{
57+
foreach ($html->query('//*') as $element) {
58+
if (!is_a($element, DOMElement::class)) {
59+
continue;
60+
}
61+
$this->sanitiseElement($element);
62+
}
63+
}
64+
65+
/**
66+
* Remove XSS attack vectors from a DOMElement
67+
*/
68+
public function sanitiseElement(DOMElement $element): void
69+
{
70+
// Remove elements first - if we remove the element, we don't have any attributes to check so exit early
71+
$removed = $this->stripElement($element);
72+
if ($removed) {
73+
return;
74+
}
75+
$this->stripAttributes($element);
76+
$this->stripAttributeContents($element);
77+
}
78+
79+
/**
80+
* Get the names of elements which will be removed.
81+
*/
82+
public function getElementsToRemove(): array
83+
{
84+
return $this->elementsToRemove;
85+
}
86+
87+
/**
88+
* Set the names of elements which will be removed.
89+
* Note that allowing the elements which are included in the default list could result in XSS vulnerabilities.
90+
*/
91+
public function setElementsToRemove(array $elements): static
92+
{
93+
$this->elementsToRemove = $elements;
94+
return $this;
95+
}
96+
97+
/**
98+
* Get the names of attributes which will be removed from any elements that have them.
99+
*/
100+
public function getAttributesToRemove(): array
101+
{
102+
return $this->attributesToRemove;
103+
}
104+
105+
/**
106+
* Set the names of attributes which will be removed from any elements that have them.
107+
* Note that allowing the attributes which are included in the default list could result in XSS vulnerabilities.
108+
*/
109+
public function setAttributesToRemove(array $attributes): static
110+
{
111+
$this->attributesToRemove = $attributes;
112+
return $this;
113+
}
114+
115+
/**
116+
* Get whether the inner contents of an element will be kept for elements that get removed.
117+
*/
118+
public function getKeepInnerHtmlOnRemoveElement(): bool
119+
{
120+
return $this->keepInnerHtmlOnRemoveElement;
121+
}
122+
123+
/**
124+
* Set whether to keep the inner contents of an element if it gets removed.
125+
*/
126+
public function setKeepInnerHtmlOnRemoveElement(bool $keep): static
127+
{
128+
$this->keepInnerHtmlOnRemoveElement = $keep;
129+
return $this;
130+
}
131+
132+
/**
133+
* If $element is one of the elements in $elementsToRemove, replace it
134+
* with a text node.
135+
*/
136+
private function stripElement(DOMElement $element): bool
137+
{
138+
if (!in_array($element->tagName, $this->getElementsToRemove())) {
139+
return false;
140+
}
141+
// Make sure we don't remove any child nodes
142+
$parentNode = $element->parentNode;
143+
if ($this->getKeepInnerHtmlOnRemoveElement() && $parentNode && $element->hasChildNodes()) {
144+
// We can't just iterate through $node->childNodes because that seems to skip some children
145+
while ($element->hasChildNodes()) {
146+
$parentNode->insertBefore($element->firstChild, $element);
147+
}
148+
}
149+
$element->remove();
150+
return true;
151+
}
152+
153+
/**
154+
* Remove all attributes in $attributesToRemove from the element.
155+
*/
156+
private function stripAttributes(DOMElement $element): void
157+
{
158+
$attributesToRemove = $this->getAttributesToRemove();
159+
if (empty($attributesToRemove)) {
160+
return;
161+
}
162+
$attributes = $element->attributes;
163+
for ($i = count($attributes) - 1; $i >= 0; $i--) {
164+
/** @var DOMAttr $attr */
165+
$attr = $attributes->item($i);
166+
foreach ($attributesToRemove as $toRemove) {
167+
if (str_starts_with($toRemove, '*') && str_ends_with($attr->name, str_replace('*', '', $toRemove))) {
168+
$element->removeAttributeNode($attr);
169+
} elseif (str_ends_with($toRemove, '*') && str_starts_with($attr->name, str_replace('*', '', $toRemove))) {
170+
$element->removeAttributeNode($attr);
171+
} elseif (!str_contains($toRemove, '*') && $attr->name === $toRemove) {
172+
$element->removeAttributeNode($attr);
173+
}
174+
}
175+
}
176+
}
177+
178+
/**
179+
* Strip out attributes which have dangerous content which might otherwise execute javascript.
180+
* This is content that we will always remove regardless of whether the attributes and elements in question
181+
* are otherwise allowed, e.g. via WYSIWYG configuration.
182+
*/
183+
private function stripAttributeContents(DOMElement $element): void
184+
{
185+
$regex = $this->getStripAttributeContentsRegex();
186+
foreach (['lowsrc', 'src', 'href', 'data'] as $dangerAttribute) {
187+
if ($element->hasAttribute($dangerAttribute)) {
188+
$attrContent = $element->getAttribute($dangerAttribute);
189+
if (preg_match($regex, $attrContent)) {
190+
$element->removeAttribute($dangerAttribute);
191+
}
192+
}
193+
}
194+
}
195+
196+
private function getStripAttributeContentsRegex(): string
197+
{
198+
$regexes = [
199+
$this->splitWithWhitespaceRegex('javascript:'),
200+
$this->splitWithWhitespaceRegex('data:text/html'),
201+
$this->splitWithWhitespaceRegex('vbscript:'),
202+
];
203+
// Regex is "starts with any of these, with optional whitespace at the start, case insensitive"
204+
return '#^\s*(' . implode('|', $regexes) . ')#iu';
205+
}
206+
207+
private function splitWithWhitespaceRegex(string $string): string
208+
{
209+
// Note that `\s` explicitly includes ALL invisible characters when used with the `u` modifier.
210+
// That includes unicode characters like the non-breaking space.
211+
return implode('\s*', str_split($string));
212+
}
213+
}

‎src/Forms/FormMessage.php

+7-1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace SilverStripe\Forms;
44

55
use InvalidArgumentException;
6+
use SilverStripe\Core\XssSanitiser;
67
use SilverStripe\ORM\ValidationResult;
78
use SilverStripe\View\ViewableData;
89

@@ -33,14 +34,19 @@ trait FormMessage
3334

3435
/**
3536
* Returns the field message, used by form validation.
37+
* If the current cast is ValidationResult::CAST_HTML, the message will be sanitised.
3638
*
3739
* Use {@link setError()} to set this property.
3840
*
3941
* @return string
4042
*/
4143
public function getMessage()
4244
{
43-
return $this->message;
45+
$message = $this->message;
46+
if ($this->getMessageCast() === ValidationResult::CAST_HTML) {
47+
$message = XssSanitiser::create()->sanitiseString($message);
48+
}
49+
return $message;
4450
}
4551

4652
/**

‎src/Forms/HTMLEditor/HTMLEditorSanitiser.php

+7-10
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use DOMElement;
77
use SilverStripe\Core\Config\Configurable;
88
use SilverStripe\Core\Injector\Injectable;
9+
use SilverStripe\Core\XssSanitiser;
910
use SilverStripe\View\Parsers\HTMLValue;
1011
use stdClass;
1112

@@ -289,6 +290,10 @@ public function sanitise(HTMLValue $html)
289290
{
290291
$linkRelValue = $this->config()->get('link_rel_value');
291292
$doc = $html->getDocument();
293+
// Get a sanitiser but don't deny any specific attributes or elements, since that's
294+
// handled as part of the element rules.
295+
$xssSanitiser = XssSanitiser::create();
296+
$xssSanitiser->setElementsToRemove([])->setAttributesToRemove([]);
292297

293298
/** @var DOMElement $el */
294299
foreach ($html->query('//body//*') as $el) {
@@ -342,16 +347,8 @@ public function sanitise(HTMLValue $html)
342347
$el->setAttribute($attr, $forced);
343348
}
344349

345-
// Matches "javascript:" with any arbitrary linebreaks inbetween the characters.
346-
$regex = '#^\s*(' . implode('\s*', str_split('javascript:')) . '|' . implode('\s*', str_split('data:text/html;')) . ')#i';
347-
// Strip out javascript execution in href or src attributes.
348-
foreach (['src', 'href', 'data'] as $dangerAttribute) {
349-
if ($el->hasAttribute($dangerAttribute)) {
350-
if (preg_match($regex, $el->getAttribute($dangerAttribute))) {
351-
$el->removeAttribute($dangerAttribute);
352-
}
353-
}
354-
}
350+
// Explicit XSS sanitisation for anything that there's really no sensible use case for in a WYSIWYG
351+
$xssSanitiser->sanitiseElement($el);
355352
}
356353

357354
if ($el->tagName === 'a' && $linkRelValue !== null) {

0 commit comments

Comments
 (0)