From 3cb1640a8ee595bd109acde3bcff50c3fc77da74 Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Tue, 25 Feb 2025 12:02:34 -0500 Subject: [PATCH] Ensure emitted HTML is safe against Unicode NFC normalization Ensure that Unicode NFC normalization can be applied to our HTML output safely. Even though the W3C officially recommends against normalizing HTML https://www.w3.org/International/questions/qa-html-css-normalization#converting this is still easily done inadvertently, especially when using the MediaWiki action API which normalizes parameters and results by default. See also I671648603c4635a35585c860b4857f5ea085e47f in Parsoid, and T266140 / I2e78e660ba1867744e34eda7d00ea527ec016b71 for another similar issue. The following changes are made: * The various HTML serializers (Remex/Tidy-derived, as well as the Html::* helpers) are tweaked to entity-escape U+0338 wherever it appears. * Similarly, Message::escaped() is tweaked to entity-escape U+0338. * Finally, a post-processing pass is added to the OutputTransform pipeline to catch any remaining U+0338 and entity-escape them. This catches U+0338 added during any of the previous OutputTransform stages (like TOC insertion, section edit links, etc). *When backporting* this code will likely need to be moved to ParserOutput::getText(), as the OutputTransform pipeline wasn't added until MW 1.42. Bug: T387130 Change-Id: I66564e14e730f5393f4fa5780b80f24de6075af5 --- includes/Html.php | 1 + includes/language/Message.php | 5 +++-- includes/parser/ParserOutput.php | 4 ++++ includes/parser/Sanitizer.php | 8 +++++++- includes/tidy/RemexCompatFormatter.php | 6 +++++- tests/parser/badCharacters.txt | Bin 0 -> 3568 bytes tests/phpunit/includes/HtmlTest.php | 12 ++++++++++++ tests/phpunit/includes/MessageTest.php | 14 ++++++++------ .../unit/includes/parser/SanitizerUnitTest.php | 2 ++ .../unit/includes/tidy/RemexDriverTest.php | 5 +++++ 10 files changed, 47 insertions(+), 10 deletions(-) create mode 100644 tests/parser/badCharacters.txt diff --git a/includes/Html.php b/includes/Html.php index 7fa313609c1..ffccf252206 100644 --- a/includes/Html.php +++ b/includes/Html.php @@ -217,6 +217,7 @@ class Html { // Silly XML. return substr( $start, 0, -1 ) . '/>'; } else { + $contents = Sanitizer::escapeCombiningChar( $contents ?? '' ); return $start . $contents . self::closeElement( $element ); } } diff --git a/includes/language/Message.php b/includes/language/Message.php index 359f4dd3e87..05f414da181 100644 --- a/includes/language/Message.php +++ b/includes/language/Message.php @@ -977,7 +977,7 @@ class Message implements MessageSpecifier, Serializable { // '⧼' is used instead of '<' to side-step any // double-escaping issues. // (Keep synchronised with mw.Message#toString in JS.) - return '⧼' . htmlspecialchars( $this->key ) . '⧽'; + return '⧼' . Sanitizer::escapeCombiningChar( htmlspecialchars( $this->key ) ) . '⧽'; } if ( $this->getLanguage()->getCode() === 'qqx' ) { @@ -1010,6 +1010,7 @@ class Message implements MessageSpecifier, Serializable { } elseif ( $format === self::FORMAT_ESCAPED ) { $string = $this->transformText( $string ); $string = htmlspecialchars( $string, ENT_QUOTES, 'UTF-8', false ); + $string = Sanitizer::escapeCombiningChar( $string ); } # Raw parameter replacement @@ -1509,7 +1510,7 @@ class Message implements MessageSpecifier, Serializable { case self::FORMAT_BLOCK_PARSE: case self::FORMAT_ESCAPED: default: - return htmlspecialchars( $plaintext, ENT_QUOTES ); + return Sanitizer::escapeCombiningChar( htmlspecialchars( $plaintext, ENT_QUOTES ) ); } } diff --git a/includes/parser/ParserOutput.php b/includes/parser/ParserOutput.php index cbfe434958e..c8038f51181 100644 --- a/includes/parser/ParserOutput.php +++ b/includes/parser/ParserOutput.php @@ -564,6 +564,10 @@ class ParserOutput extends CacheTime implements ContentMetadataCollector { }, $text ); + + // Hardens the output against NFC normalization (T387130). + $text = Sanitizer::escapeCombiningChar( $text ); + return $text; } diff --git a/includes/parser/Sanitizer.php b/includes/parser/Sanitizer.php index 533b6ba9087..22f3d2e8664 100644 --- a/includes/parser/Sanitizer.php +++ b/includes/parser/Sanitizer.php @@ -1110,6 +1110,12 @@ class Sanitizer { $class ), '_' ); } + public static function escapeCombiningChar( string $html ): string { + return strtr( $html, [ + "\u{0338}" => '̸', # T387130 + ] ); + } + /** * Given HTML input, escape with htmlspecialchars but un-escape entities. * This allows (generally harmless) entities like   to survive. @@ -1123,7 +1129,7 @@ class Sanitizer { # hurt. Use ENT_SUBSTITUTE so that incorrectly truncated multibyte characters # don't cause the entire string to disappear. $html = htmlspecialchars( $html, ENT_QUOTES | ENT_SUBSTITUTE ); - return $html; + return self::escapeCombiningChar( $html ); } /** diff --git a/includes/tidy/RemexCompatFormatter.php b/includes/tidy/RemexCompatFormatter.php index 98dc4e1e510..103ac1f7da2 100644 --- a/includes/tidy/RemexCompatFormatter.php +++ b/includes/tidy/RemexCompatFormatter.php @@ -22,9 +22,13 @@ class RemexCompatFormatter extends HtmlFormatter { public function __construct( $options = [] ) { parent::__construct( $options ); + // Escape non-breaking space $this->attributeEscapes["\u{00A0}"] = ' '; - unset( $this->attributeEscapes["&"] ); $this->textEscapes["\u{00A0}"] = ' '; + // Escape U+0338 (T387130) + $this->textEscapes["\u{0338}"] = '̸'; + // Disable escaping of '&', because we expect to see entities, due to 'ignoreCharRefs' + unset( $this->attributeEscapes["&"] ); unset( $this->textEscapes["&"] ); $this->textProcessor = $options['textProcessor'] ?? null; } diff --git a/tests/parser/badCharacters.txt b/tests/parser/badCharacters.txt new file mode 100644 index 0000000000000000000000000000000000000000..27ce1dac53ccc592ac3774c52c0221de85aa75c5 GIT binary patch literal 3568 zcmcgvZExE)5bnoK!2ZKk2C@cA?8aTU%(Yx}MT!nsy8=nE4_Oh=(%B+Hi2_MSF^2xH z{)qml-jR}H$J&x~Yd_iI%iZ%lcf2EeaFR<4GpP`4t`{l;U8*Idx^zO8;Lth?3oFYx z*a}lA6r{N@B6VnN)B~3b2hPX}78O`sAZ@vHfSGjK*gmZgyYNgI6xkA#wzQZXvR#;I z14|UfLY-p?1`B-;aZHB13(G9xUlu+(x}@lp6iOw_b2}nW7edMGN;!D__IG$- zIj$9EyS%ZM=aNGko#xI}qrsrA>potJqEgr=Vefg2?j3{M?JvakZlV2e z>_VP?2Dkq++TCIV;lHLBjmJ(kt{-J5rdP-KHn~g1S<=~%<+p$8GDD-N;(UQq+~MV) z5B1w09zJ?>xCc|Nzk~;br2^)pTQVc+O*1)<>iG*J<^_$vqT-vJ0#%SgJ&wVnZx1&$ zwc6=46Hc@hGx<9Lh`D0hcm%YsDVy0J`;C-4W=>m5jvs2w=r!pSQ>et?r&he`g=<3y=ICp^T~)hL2|NI}W)a&$ng zEv3eUivE_1Vv2^xq*FNJZiD8`w^KcvjSfjJYL8~XmHnCjJNo$Hi~ybOa6Q0L3&~@o z>iuNR)3C!6jdgnZaoFvO=v{Z^8GBbva~M>4#SNm7&L{RbA5O+pr^~iU6!m^i(lY26 zWR}DY@<4{o#gi>E;{lu4GQ`&r8=@Hh+vNU~ZZ_xW;>l@iNi^+g;Dga8&Y$y*%Ko+&t#OKx> zrS-wvly()o*4?!jtzrA>(hhHsc7;8jd?9I92_0^I(YOgO@M+$s6mGuvIe{O>K0qPs zLu3ZcJcT@@XXnNP-pKWUjI^gYL@8tnkN2NH@Bez}4@ZCa6A81dP}8-ls7Eh^Qrs7R z%P2zY2V`x{kuR^S6V)sWm5g1D+w=+^Z{Er2cNdPsO^$1sxW$p@ZozSN5V+uOa9GW3 zW8`Y=@5;R%yw;AM75;&fqrvYj2vAg#_%ZaC0UqassertEquals( + "

̸ & < >

", + Html::element( 'p', [ 'test' => "\u{0338}\"&" ], "\u{0338} & < >" ), + 'Attribute and content escaping' + ); + + $this->assertEquals( + '

̸ &

', + Html::rawElement( 'p', [], "\u{0338} &" ), + "Combining characters escaped even in raw contents (T387130)" + ); } public function dataXmlMimeType() { diff --git a/tests/phpunit/includes/MessageTest.php b/tests/phpunit/includes/MessageTest.php index 512ebc5d989..7322f016609 100644 --- a/tests/phpunit/includes/MessageTest.php +++ b/tests/phpunit/includes/MessageTest.php @@ -303,6 +303,7 @@ class MessageTest extends MediaWikiLangTestCase { '⧼script>alert(1)</script⧽' ], [ 'script>alert(1)alert(1)', 'plain', '', '<script>alert(1)</script>' ], + [ "\u{0338}isolated combining char", 'escaped', '̸isolated combining char', '̸isolated combining char' ], ]; } @@ -610,28 +612,28 @@ class MessageTest extends MediaWikiLangTestCase { public static function providePlaintextParams() { return [ [ - 'one $2
foo
[[Bar]] {{Baz}} <', + "one $2
\u{0338}foo
[[Bar]] {{Baz}} <", 'plain', ], [ // expect - 'one $2
foo
[[Bar]] {{Baz}} <', + "one $2
\u{0338}foo
[[Bar]] {{Baz}} <", // format 'text', ], [ - 'one $2 <div>foo</div> [[Bar]] {{Baz}} &lt;', + 'one $2 <div≯foo</div> [[Bar]] {{Baz}} &lt;', 'escaped', ], [ - 'one $2 <div>foo</div> [[Bar]] {{Baz}} &lt;', + 'one $2 <div≯foo</div> [[Bar]] {{Baz}} &lt;', 'parse', ], [ - "

one $2 <div>foo</div> [[Bar]] {{Baz}} &lt;\n

", + "

one $2 <div≯foo</div> [[Bar]] {{Baz}} &lt;\n

", 'parseAsBlock', ], ]; @@ -650,7 +652,7 @@ class MessageTest extends MediaWikiLangTestCase { $msg = new RawMessage( '$1 $2' ); $params = [ 'one $2', - '
foo
[[Bar]] {{Baz}} <', + "
\u{0338}foo
[[Bar]] {{Baz}} <", ]; $this->assertSame( $expect, diff --git a/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php b/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php index dee4e1f3c6b..5758ef74bae 100644 --- a/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php +++ b/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php @@ -194,6 +194,8 @@ class SanitizerUnitTest extends MediaWikiUnitTestCase { [ 'a¡b', 'a¡b' ], [ 'foo'bar', "foo'bar" ], [ '<script>foo</script>', '' ], + [ '̸', "\u{0338}" ], + [ '̸', '̸' ], ]; } diff --git a/tests/phpunit/unit/includes/tidy/RemexDriverTest.php b/tests/phpunit/unit/includes/tidy/RemexDriverTest.php index 0b2a2a615ca..7b8373386ad 100644 --- a/tests/phpunit/unit/includes/tidy/RemexDriverTest.php +++ b/tests/phpunit/unit/includes/tidy/RemexDriverTest.php @@ -314,6 +314,11 @@ class RemexDriverTest extends MediaWikiUnitTestCase { 'foo', "

foo

", ], + [ + 'Unicode combining characters (T387130)', + "

\u{0338} \u{0338}

", + '

̸ ̸

', + ], ]; public function provider() { -- 2.48.1