From 70b5e3205a20e5d43fd6dcc2b6154543c722c07c 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 --- autoload.php | 1 + includes/Html/Html.php | 2 + includes/Html/HtmlHelperTrait.php | 4 ++ includes/Message/Message.php | 6 ++- .../DefaultOutputPipelineFactory.php | 5 +++ includes/OutputTransform/Stages/HardenNFC.php | 23 ++++++++++ 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 05426824ebc..1b91f711188 100644 --- a/autoload.php +++ b/autoload.php @@ -1873,6 +1873,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 f108b8cf9a4..964154491d6 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 a6a4e4547e0..270d84df9e1 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/Message/Message.php b/includes/Message/Message.php index d714946ad55..9e654276e2d 100644 --- a/includes/Message/Message.php +++ b/includes/Message/Message.php @@ -34,6 +34,7 @@ use MediaWiki\Page\PageReference; use MediaWiki\Page\PageReferenceValue; use MediaWiki\Parser\Parser; use MediaWiki\Parser\ParserOutput; +use MediaWiki\Parser\Sanitizer; use MediaWiki\StubObject\StubUserLang; use MediaWiki\Title\Title; use RuntimeException; @@ -1042,7 +1043,7 @@ class Message implements Stringable, 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 ( in_array( $this->getLanguage()->getCode(), [ 'qqx', 'x-xss' ] ) ) { @@ -1078,6 +1079,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 @@ -1591,7 +1593,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/OutputTransform/DefaultOutputPipelineFactory.php b/includes/OutputTransform/DefaultOutputPipelineFactory.php index beb138f6c7a..46ab7622a94 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; @@ -98,6 +99,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 @@ + '̸', # T387130 + ] ); + } + /** * Given HTML input, escape with htmlspecialchars but un-escape entities. * This allows (generally harmless) entities like   to survive. @@ -1053,7 +1059,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 54f6d2890b8..b1cd898674e 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 e8d15b6f178..af02faf9f6d 100644 --- a/tests/phpunit/includes/language/MessageTest.php +++ b/tests/phpunit/includes/language/MessageTest.php @@ -282,6 +282,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' ], ]; } @@ -584,28 +586,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', ], ]; @@ -618,7 +620,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.48.1