diff --git a/src/Html/HtmlBuilder.php b/src/Html/HtmlBuilder.php index 662adf8ef..36bee0777 100644 --- a/src/Html/HtmlBuilder.php +++ b/src/Html/HtmlBuilder.php @@ -482,6 +482,18 @@ protected static function getReadableLength($str) return strlen(preg_replace('/\s+/', ' ', $str)); } + /** + * Matches one HTML attribute: the separators before its name (1), the name (2), and the value + * with its equals sign, if any (3). A quoted value is consumed whole, including any ">" inside + * it. The separators include "=" so that a stray one cannot strand the rest of the scan. + */ + protected const ATTRIBUTE_PATTERN = '([\x00-\x20\/=]*+)([^\x00-\x20=\/>]++)((?:[\x00-\x20]*+=[\x00-\x20]*+(?:"[^"]*+"|\'[^\']*+\'|[^\x00-\x20>]*+))?)'; + + /** + * Attribute names that are never safe to keep. + */ + protected const UNSAFE_ATTRIBUTE_NAME_PATTERN = '#^(?:on\w*|xmlns(?::[\w\-]+)?|style)$#i'; + /** * Cleans HTML to prevent most XSS attacks. * @param string $html HTML @@ -498,8 +510,13 @@ public static function clean($html) $html = preg_replace('#(&\#x*)([0-9A-F]+);*#iu', "$1$2;", $html); $html = html_entity_decode($html, ENT_COMPAT|ENT_HTML5, 'UTF-8'); - // Remove any attribute starting with "on" or xmlns - $html = preg_replace('#(<[^>]+[\x00-\x20\"\'\/])(on|xmlns)[^>]*>#iUu', "$1>", $html); + // Remove event handler, xmlns and style attributes from every tag. Styles are + // included for IE's expression() support. + $html = preg_replace_callback( + '#(<[a-z][^\x00-\x20\/>]*+)((?:' . self::ATTRIBUTE_PATTERN . ')*+)#iu', + fn (array $tag): string => $tag[1] . static::stripUnsafeAttributes($tag[2]), + $html + ); // Remove javascript: and vbscript: protocols $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*j[\x00-\x20]*a[\x00-\x20]*v[\x00-\x20]*a[\x00-\x20]*s[\x00-\x20]*c[\x00-\x20]*r[\x00-\x20]*i[\x00-\x20]*p[\x00-\x20]*t[\x00-\x20]*:#iUu', '$1=$2nojavascript...', $html); @@ -507,9 +524,6 @@ public static function clean($html) $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*-moz-binding[\x00-\x20]*:#Uu', '$1=$2nomozbinding...', $html); $html = preg_replace('#([a-z]*)[\x00-\x20\/]*=[\x00-\x20\/]*([\`\'\"]*)[\x00-\x20\/|(&\#\d+;)]*data[\x00-\x20]*:#Uu', '$1=$2nodata...', $html); - // Only works in IE: - $html = preg_replace('#(<[^>]+[\x00-\x20\"\'\/])style[^>]*>#iUu', "$1>", $html); - // Remove namespaced elements (we do not need them) $html = preg_replace('#]*>#i', "", $html); @@ -520,4 +534,19 @@ public static function clean($html) return $html; } + + /** + * Removes unsafe attributes from a tag's attribute section, keeping each separator so the + * attributes either side of a removed one do not run together. + */ + protected static function stripUnsafeAttributes($attributes) + { + return preg_replace_callback( + '#' . self::ATTRIBUTE_PATTERN . '#iu', + fn (array $attribute): string => preg_match(self::UNSAFE_ATTRIBUTE_NAME_PATTERN, $attribute[2]) + ? $attribute[1] + : $attribute[0], + $attributes + ); + } } diff --git a/tests/Html/HtmlBuilderTest.php b/tests/Html/HtmlBuilderTest.php index 1c4ffb54a..5ea4b572a 100644 --- a/tests/Html/HtmlBuilderTest.php +++ b/tests/Html/HtmlBuilderTest.php @@ -80,4 +80,91 @@ public function testClean() $result = with(new HtmlBuilder)->clean('Test'); $this->assertEquals('Test', $result); } + + public function unsafeAttributeProvider() + { + return [ + 'unquoted value' => ['', 'onerror'], + 'double quoted value' => ['', 'onerror'], + 'greater than in an earlier value' => ['>', 'onerror'], + 'greater than, unquoted handler' => ['', 'onerror'], + 'greater than, single quotes' => [">", 'onerror'], + 'greater than, no separator' => ['', 'onerror'], + 'no separator' => ['', 'onerror'], + 'slash separator' => ['', 'onerror'], + 'quote inside an unquoted value' => ['a"b', 'onerror'], + 'unterminated tag' => [' ['> ['', 'onerror'], + 'newline separator' => ["\"\" src=x\nonerror=alert(1)>", 'onerror'], + 'spaces around the equals sign' => ['>', 'onerror'], + 'greater than in a later value' => ['', 'onerror'], + 'two handlers' => ['>', 'onerror'], + 'handler before other attributes' => ['>', 'onerror'], + 'nested quotes' => ['\'"\' src=x onerror=alert(1)>', 'onerror'], + 'svg element' => ['', 'onload'], + 'details element' => ['
', 'ontoggle'], + 'xmlns declaration' => ['>', 'xmlns'], + 'namespaced xmlns declaration' => ['>', 'xmlns'], + 'inline style' => ['', 'style'], + 'equals sign before a name' => ['', 'onerror'], + 'bare equals sign' => ['', 'onerror'], + 'slash then equals sign' => ['', 'onerror'], + 'repeated equals signs' => ['', 'onerror'], + ]; + } + + /** + * @dataProvider unsafeAttributeProvider + */ + public function testCleanRemovesUnsafeAttributes($html, $attribute) + { + $result = with(new HtmlBuilder)->clean($html); + + $this->assertStringNotContainsStringIgnoringCase($attribute, $result); + } + + public function safeMarkupProvider() + { + return [ + 'heading' => ["# Hello\n"], + 'emphasis' => ["**bold** and _italic_\n"], + 'link with an ampersand' => ["[x](https://example.com/a?b=1&c=2)\n"], + 'table' => ["| a | b |\n|---|---|\n| 1 | 2 |\n"], + 'list' => ["- one\n- two\n"], + 'block quote' => ["> quoted text\n"], + 'code fence' => ["```php\n ['use `onerror=alert(1)` carefully'], + 'prose mentioning an attribute' => ['Set onboarding=true in the config.'], + 'prose mentioning a handler' => ['The onclick= attribute is removed.'], + 'inline html' => ['some bold and italic text'], + 'greater than in an attribute' => ['a > b'], + 'greater than in a title' => ['x'], + 'path beginning with on' => ['x'], + 'attribute text inside a value' => ['x onerror=alert(1)'], + 'data attributes' => ['
x
'], + 'framework bound attributes' => [''], + 'several attributes' => ['a'], + 'self closing tags' => ['

'], + 'html entities' => ['a & b < c'], + ]; + } + + /** + * @dataProvider safeMarkupProvider + */ + public function testCleanPreservesSafeMarkup($html) + { + $this->assertEquals($html, with(new HtmlBuilder)->clean($html)); + } + + public function testCleanKeepsTheAttributesSurroundingARemovedOne() + { + $result = with(new HtmlBuilder)->clean('>'); + + $this->assertStringNotContainsString('onerror', $result); + $this->assertStringContainsString('alt=">"', $result); + $this->assertStringContainsString('src="y.png"', $result); + $this->assertStringContainsString('title="t"', $result); + } }