From 21cc1752578c1e9cb1a7fd48b3a36c65ca81515e Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Tue, 25 Feb 2025 12:02:34 -0500 Subject: [PATCH] SECURITY: Ensure emitted HTML is safe against Unicode NFC normalization CVE-2025-32699 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 8154dac2f65..d8989441e61 100644 --- a/autoload.php +++ b/autoload.php @@ -2068,6 +2068,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 8017f7ba310..94887c11bb5 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->getLanguageCode(), [ '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 @@ -1557,7 +1559,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 b286358b891..3b06a2ccdd6 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 ae38674c389..9a97396daa9 100644 --- a/tests/phpunit/includes/language/MessageTest.php +++ b/tests/phpunit/includes/language/MessageTest.php @@ -279,6 +279,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' ], ]; } @@ -566,28 +568,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', ], ]; @@ -600,7 +602,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