From 7c71210e369528d281974106202af5403e071129 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Tue, 20 Oct 2015 16:18:03 -0600 Subject: [PATCH] Make MW attribue escaping methods mangle U+7F to disable strip markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you can get a strip marker in an attribute, then you can get an XSS by putting an unescaped quote into a strip marker, bypassing the html escaping routines until unstrip is called later. Ideally, user controlled attributes would have unstripBoth() run on them before escaping, but there's a couple places that is not true (e.g. transcluded special pages), and for better safety, we should be treating U+7F as a dangerous character. I escaped U+7F to '␡' instead of , as control characters are not supposed to be in html documents anyways. This also adds a helper method to the parser, that extensions can call to safely escape an html attribute. Bug: T110143 Change-Id: I88e808a57b854bb86b2dbdf93299f282c723e75e --- includes/Html.php | 8 +++++++- includes/Sanitizer.php | 1 + includes/parser/Parser.php | 24 ++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/includes/Html.php b/includes/Html.php index c61dca8..7ee7533 100644 --- a/includes/Html.php +++ b/includes/Html.php @@ -604,13 +604,19 @@ class Html { // The only difference between this transform and the one by // Sanitizer::encodeAttribute() is '<' is only encoded here if // $wgWellFormedXml is set, and ' is not encoded. + // \x7F is modified as it might be part of a strip marker. If + // a strip marker got into an attribute, and then later put + // through the parser, it might later get replaced with an + // unescaped double quote. Ideally though, any strip markers + // would be unstripped before reaching here. $map = array( '&' => '&', '"' => '"', '>' => '>', "\n" => ' ', "\r" => ' ', - "\t" => ' ' + "\t" => ' ', + "\x7F" => '␡' ); if ( $wgWellFormedXml ) { // This is allowed per spec: diff --git a/includes/Sanitizer.php b/includes/Sanitizer.php index f88dd05..5ad27c0 100644 --- a/includes/Sanitizer.php +++ b/includes/Sanitizer.php @@ -1050,6 +1050,7 @@ class Sanitizer { "\n" => ' ', "\r" => ' ', "\t" => ' ', + "\x7F" => '␡', // disable strip markers ) ); return $encValue; diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index efad151..a70c9ff 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -5097,6 +5097,10 @@ class Parser { * Transform and return $text. Use $parser for any required context, e.g. use * $parser->getTitle() and $parser->getOptions() not $wgTitle or $wgOut->mParserOptions * + * If outputting any html attributes, use $parser->escapeAttribute( $text ) + * to escape them, not htmlspecialchars! The character \x7F (Ascii delete) is used + * internally by the parser and is not safe to include in html attributes. + * * Hooks may return extended information by returning an array, of which the * first numbered element (index 0) must be the return string, and all other * entries are extracted into local variables within an internal function @@ -5782,6 +5786,26 @@ class Parser { } /** + * Escapes html for an attribute, including substituting strip markers. + * + * Tag extensions (or anything else returning half serialized parser output) + * should use this method to escape attributes. The character \x7F is not safe + * inside the attributes as it might be a strip marker that is later substituted + * for a string, which could contains an unescaped quote mark. Hence this method + * which ensures that strip markers get replaced before escaping. + * + * @note If for some reason you can't use this method, be sure to make sure \x7F + * gets replaced during your escaping for attributes. + * @param $text String Text to escape + * @return String The escaped attribue + */ + public function escapeAttribute( $text ) { + $text = $this->mStripState->unstripBoth( $text ); + $text = Sanitizer::encodeAttribute( $text ); + return $text; + } + + /** * Accessor * * @return array -- 2.0.1