SECURITY: Include quote characters in strip markers so esc in attr

Strip markers get substituted for general html, which means the
substitution text general does not escape quote characters. If
someone can convince MW to put a strip marker in an attribute,
you can get around escaping requirements that way. This patch
adds the characters `"' to the strip marker text. At least one
of these characters should be escaped inside attributes (regardless
of what quote character you use for attributes), thus normal html
escaping will deactivate the strip markers, preventing the
vulnrability.

This will break any extension that escapes input with htmlspecialchars,
to add to html/half parsed html output, but assumes that strip markers
are unmangled. I don't think its very common to do this. The primary
example I found was some core usages of Xml::escapeTagsOnly(). (And
even in that case, it only affected the corner case of being called
via {{#tag:..}})

Based on MatmaRex's suggestion.

Change-Id: If887065e12026530f36e5f35dd7ab0831d313561

Signed-off-by: Chad Horohoe <chadh@wikimedia.org>

diff --git a/includes/parser/CoreTagHooks.php b/includes/parser/CoreTagHooks.php
index d4c4f6d..4541c52 100644 (file)
--- a/includes/parser/CoreTagHooks.php
+++ b/includes/parser/CoreTagHooks.php
@@ -56,9 +56,14 @@ class CoreTagHooks {
                $content = StringUtils::delimiterReplace( '<nowiki>', '</nowiki>', '$1', $text, 'i' );
 
                $attribs = Sanitizer::validateTagAttributes( $attribs, 'pre' );
-               return Xml::openElement( 'pre', $attribs ) .
-                       Xml::escapeTagsOnly( $content ) .
-                       '</pre>';
+               // We need to let both '"' and '&' through,
+               // for strip markers and entities respectively.
+               $content = str_replace(
+                       array( '>', '<' ),
+                       array( '&gt;', '&lt;' ),
+                       $content
+               );
+               return Html::rawElement( 'pre', $attribs, $content );
        }
 
        /**
@@ -98,8 +103,17 @@ class CoreTagHooks {
         * @return array
         */
        public static function nowiki( $content, $attributes, $parser ) {
-               $content = strtr( $content, [ '-{' => '-&#123;', '}-' => '&#125;-' ] );
-               return [ Xml::escapeTagsOnly( $content ), 'markerType' => 'nowiki' ];
+               $content = strtr( $content, array(
+                       // lang converter
+                       '-{' => '-&#123;',
+                       '}-' => '&#125;-',
+                       // html tags
+                       '<' => '&lt;',
+                       '>' => '&gt;'
+                       // Note: Both '"' and '&' are not converted.
+                       // This allows strip markers and entities through.
+               ) );
+               return array( $content, 'markerType' => 'nowiki' );
        }
 
        /**

diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index 96674be..a3abcad 100644 (file)
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -121,9 +121,14 @@ class Parser {
         *
         * Must not consist of all title characters, or else it will change
         * the behavior of <nowiki> in a link.
+        *
+        * Must have a character that needs escaping in attributes, otherwise
+        * someone could put a strip marker in an attribute, to get around
+        * escaping quote marks, and break out of the attribute. Thus we add
+        * `'".
         */
-       const MARKER_SUFFIX = "-QINU\x7f";
-       const MARKER_PREFIX = "\x7fUNIQ-";
+       const MARKER_SUFFIX = "-QINU`\"'\x7f";
+       const MARKER_PREFIX = "\x7f'\"`UNIQ-";
 
        # Markers used for wrapping the table of contents
        const TOC_START = '<mw:toc>';

diff --git a/tests/parser/parserTests.txt b/tests/parser/parserTests.txt
index 7051b4f..2f88f92 100644 (file)
--- a/tests/parser/parserTests.txt
+++ b/tests/parser/parserTests.txt
@@ -2265,6 +2265,15 @@ Entities inside <pre>
 
 !! end
 
+!! test
+<nowiki> inside of #tag:pre
+!! wikitext
+{{#tag:pre|Foo <nowiki>&rarr;bar</nowiki>}}
+!! html
+<pre>Foo &#8594;bar</pre>
+
+!! end
+
 !! test
 <nowiki> and <pre> preference (first one wins)
 !! wikitext