From 2bb3784882bb95db73db1f9846b7c02972b424cb Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Tue, 25 Feb 2025 12:02:34 -0500 Subject: [PATCH 1/5] 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 --- autoload.php | 1 + includes/Html/Html.php | 2 + includes/Html/HtmlHelperTrait.php | 4 ++ .../DefaultOutputPipelineFactory.php | 5 +++ includes/OutputTransform/Stages/HardenNFC.php | 23 ++++++++++ includes/language/Message/Message.php | 6 ++- includes/parser/Sanitizer.php | 8 +++- includes/tidy/RemexCompatFormatter.php | 2 + tests/parser/badCharacters.txt | Bin 1892 -> 3623 bytes tests/phpunit/includes/Html/HtmlTest.php | 12 +++++ .../OutputTransform/Stages/HardenNFCTest.php | 41 ++++++++++++++++++ .../phpunit/includes/language/MessageTest.php | 14 +++--- .../includes/parser/SanitizerUnitTest.php | 2 + .../unit/includes/tidy/RemexDriverTest.php | 5 +++ 14 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 includes/OutputTransform/Stages/HardenNFC.php create mode 100644 tests/phpunit/includes/OutputTransform/Stages/HardenNFCTest.php diff --git a/autoload.php b/autoload.php index 59ac3f318e5..1c90eb72b9e 100644 --- a/autoload.php +++ b/autoload.php @@ -1945,6 +1945,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\OutputTransform\\Stages\\HandleParsoidSectionLinks' => __DIR__ . '/includes/OutputTransform/Stages/HandleParsoidSectionLinks.php', 'MediaWiki\\OutputTransform\\Stages\\HandleSectionLinks' => __DIR__ . '/includes/OutputTransform/Stages/HandleSectionLinks.php', 'MediaWiki\\OutputTransform\\Stages\\HandleTOCMarkers' => __DIR__ . '/includes/OutputTransform/Stages/HandleTOCMarkers.php', + 'MediaWiki\\OutputTransform\\Stages\\HardenNFC' => __DIR__ . '/includes/OutputTransform/Stages/HardenNFC.php', 'MediaWiki\\OutputTransform\\Stages\\HydrateHeaderPlaceholders' => __DIR__ . '/includes/OutputTransform/Stages/HydrateHeaderPlaceholders.php', 'MediaWiki\\OutputTransform\\Stages\\ParsoidLocalization' => __DIR__ . '/includes/OutputTransform/Stages/ParsoidLocalization.php', 'MediaWiki\\OutputTransform\\Stages\\RenderDebugInfo' => __DIR__ . '/includes/OutputTransform/Stages/RenderDebugInfo.php', diff --git a/includes/Html/Html.php b/includes/Html/Html.php index e383fd18051..0b80411adce 100644 --- a/includes/Html/Html.php +++ b/includes/Html/Html.php @@ -28,6 +28,7 @@ namespace MediaWiki\Html; use MediaWiki\Json\FormatJson; use MediaWiki\MainConfigNames; use MediaWiki\MediaWikiServices; +use MediaWiki\Parser\Sanitizer; use MediaWiki\Request\ContentSecurityPolicy; use UnexpectedValueException; @@ -193,6 +194,7 @@ class Html { if ( isset( self::$voidElements[$element] ) ) { return $start; } else { + $contents = Sanitizer::escapeCombiningChar( $contents ?? '' ); return $start . $contents . self::closeElement( $element ); } } diff --git a/includes/Html/HtmlHelperTrait.php b/includes/Html/HtmlHelperTrait.php index 3d61c696501..1a938d8825e 100644 --- a/includes/Html/HtmlHelperTrait.php +++ b/includes/Html/HtmlHelperTrait.php @@ -3,6 +3,7 @@ namespace MediaWiki\Html; use Wikimedia\Assert\Assert; +use Wikimedia\RemexHtml\Serializer\HtmlFormatter; use Wikimedia\RemexHtml\Serializer\SerializerNode; /** @@ -23,6 +24,9 @@ trait HtmlHelperTrait { parent::__construct( $options ); $this->shouldModifyCallback = $shouldModifyCallback; $this->modifyCallback = $modifyCallback; + // Escape U+0338 (T387130) + '@phan-var HtmlFormatter $this'; + $this->textEscapes["\u{0338}"] = '̸'; } public function element( SerializerNode $parent, SerializerNode $node, $contents ) { diff --git a/includes/OutputTransform/DefaultOutputPipelineFactory.php b/includes/OutputTransform/DefaultOutputPipelineFactory.php index e1d393cf5c8..63e60e53d9b 100644 --- a/includes/OutputTransform/DefaultOutputPipelineFactory.php +++ b/includes/OutputTransform/DefaultOutputPipelineFactory.php @@ -14,6 +14,7 @@ use MediaWiki\OutputTransform\Stages\ExtractBody; use MediaWiki\OutputTransform\Stages\HandleParsoidSectionLinks; use MediaWiki\OutputTransform\Stages\HandleSectionLinks; use MediaWiki\OutputTransform\Stages\HandleTOCMarkers; +use MediaWiki\OutputTransform\Stages\HardenNFC; use MediaWiki\OutputTransform\Stages\HydrateHeaderPlaceholders; use MediaWiki\OutputTransform\Stages\ParsoidLocalization; use MediaWiki\OutputTransform\Stages\RenderDebugInfo; @@ -101,6 +102,10 @@ class DefaultOutputPipelineFactory { 'HydrateHeaderPlaceholders' => [ 'class' => HydrateHeaderPlaceholders::class, ], + # This should be last, in order to ensure final output is hardened + 'HardenNFC' => [ + 'class' => HardenNFC::class, + ], ]; public function __construct( diff --git a/includes/OutputTransform/Stages/HardenNFC.php b/includes/OutputTransform/Stages/HardenNFC.php new file mode 100644 index 00000000000..66108268ea0 --- /dev/null +++ b/includes/OutputTransform/Stages/HardenNFC.php @@ -0,0 +1,23 @@ +key ) . '⧽'; + return '⧼' . Sanitizer::escapeCombiningChar( htmlspecialchars( $this->key ) ) . '⧽'; } if ( in_array( $this->getLanguage()->getCode(), [ 'qqx', 'x-xss' ] ) ) { @@ -1056,6 +1057,7 @@ class Message implements Stringable, 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 @@ -1535,7 +1537,7 @@ class Message implements Stringable, 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/Sanitizer.php b/includes/parser/Sanitizer.php index 570e26326a3..5ef67e07c5c 100644 --- a/includes/parser/Sanitizer.php +++ b/includes/parser/Sanitizer.php @@ -1041,6 +1041,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. @@ -1056,7 +1062,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 caedc2828b7..8323986c0e6 100644 --- a/includes/tidy/RemexCompatFormatter.php +++ b/includes/tidy/RemexCompatFormatter.php @@ -28,6 +28,8 @@ class RemexCompatFormatter extends HtmlFormatter { // Escape non-breaking space $this->attributeEscapes["\u{00A0}"] = ' '; $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["&"] ); diff --git a/tests/parser/badCharacters.txt b/tests/parser/badCharacters.txt index 10e5fd3b5f80c9ea2fd5d7bff3c8d9b5acd0fd7d..680ab05055ad28b8d7f58694810f8d465c26fd5e 100644 GIT binary patch literal 3623 zcmd5xBrfu@b=W7%xHQ{ zutZ@jGzC^*u+(SB4uIJk{Pg2fIy~8iqpM2E0X;_+a|H(1^99+}{{Mpu|DqmgI3}5z zY-pM9X1c6}lM@1~YbQy=1eI=p9P+3QDm)XF3;r|+!XTJMRS|qr#cI&BoDey(ayT=> zkh5gO?Vco%H-1F@|cG9YJ_Hkgwd4|a_O=AHz|&Ls_`9;_2pE|jj&Tj)m*AGV&` z=sKvhaf8B@YKMLs7oAar2xB_nQ&{B@|1$B>!8t{*qEISX&FzpZoe3rL3+CX>``7Tu za$IZ7xA|Zr=aNGkofOX1!z5{%rjO^MtQGc&*au$q2Nq=?qES5_35uyo<4RApBA#Q& zl(069NlmcN3S%e?PSbd^+IY+!j}kGCMoB%s2b77(=C&3Y!lT+C1(0&gBMPx0A`R9? zXK1Z9>AST*r>}%9ZiD@+>)20j20t~3H-X}1KYnD?agEa;{QuBT8kUmxLV3iOfJg!Hn~g9SyFAt>g(Tim7`JAalSwmZ?N+D zqJIC~<0nt{cVNQ(m;3=?sew7^mW+sI(^Sr)X2z`+%|Ruop})qN$d<2Y%uuiX_3dFg z70MzBy)@P7(B$T1JLAaoX&FLMAuf+KO=yc+q#X1KHnkY(B*y^1VxM%Q|n)()`Y1Azob9ZHlPaom9 z(P5Lnd7W^V=!AE%Gc}B$7*OPLJRk1S{L5&1p`y=fQBKhCnsf?>fu~bHbMp@3cvnq%#NJdpL6W+t zd5W~u`RpI%@~j&oWwTp^aKpvtARdpHlRS-E3;`A^j>p$vjgn$89?^8D*0l27#fE5- zw$E?wPUspmM;DJzMhT1Yzzef1@j>Ymf}cGx=~@W3*cZA#@n~jK{(oyP-HQX0X?#Fi z&@K-zCrtpuXSsxK<>80U!`9J+h@^>HuPE_Jc5jY-JTG&+is%21(Uy9>Z ziigW$G-?A4T;IEt&h7tQPUOe2k5s6G5Tc~rr{IS~-FR@154P3N)prO|$Qf4NfAOOK z%f2s?{@@2D-5fh>ppUS_E+p)58h};uMz()D$wA!76hoaQ~hCp z*ZekZRLIv(VV3?a#LpRjKPP4W_sAJRZ!uwv6f=5>pj#MzcF=AZ3Eco|-vZqO;L_;% crjUi^6`p)uQw@ur=+#b_i}l#=cAE_AZ&W_L6951J delta 11 ScmZ23^Mr3hAKTassertEquals( + "

̸ & < >

", + 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/OutputTransform/Stages/HardenNFCTest.php b/tests/phpunit/includes/OutputTransform/Stages/HardenNFCTest.php new file mode 100644 index 00000000000..20282ef89ff --- /dev/null +++ b/tests/phpunit/includes/OutputTransform/Stages/HardenNFCTest.php @@ -0,0 +1,41 @@ +markTestSkipped( 'HydrateHeaderPlaceHolders should always run' ); + } + + public function provideTransform(): array { + $text = "

\u{0338}

"; + $expectedText = "

̸

"; + return [ + [ new ParserOutput( $text ), null, [], new ParserOutput( $expectedText ) ], + ]; + } +} diff --git a/tests/phpunit/includes/language/MessageTest.php b/tests/phpunit/includes/language/MessageTest.php index b8cea329dd7..8a34c7d87d0 100644 --- a/tests/phpunit/includes/language/MessageTest.php +++ b/tests/phpunit/includes/language/MessageTest.php @@ -280,6 +280,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' ], ]; } @@ -567,28 +569,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', ], ]; @@ -601,7 +603,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 8a54f074f8a..210449370c5 100644 --- a/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php +++ b/tests/phpunit/unit/includes/parser/SanitizerUnitTest.php @@ -204,6 +204,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 390e80f0341..ad9424fca81 100644 --- a/tests/phpunit/unit/includes/tidy/RemexDriverTest.php +++ b/tests/phpunit/unit/includes/tidy/RemexDriverTest.php @@ -309,6 +309,11 @@ class RemexDriverTest extends MediaWikiUnitTestCase { 'foo', "

foo

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

\u{0338} \u{0338}

", + '

̸ ̸

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