From 05a50de226edfe7a6fd5f6802e91da6c61ed0c28 Mon Sep 17 00:00:00 2001 | |
From: csteipp <[email protected]> | |
Date: Mon, 25 Apr 2016 15:25:18 -0700 | |
Subject: [PATCH] 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:..}}) | |
Bug: T110143 | |
Change-Id: If887065e12026530f36e5f35dd7ab0831d313561 | |
--- | |
includes/parser/CoreTagHooks.php | 24 +++++++++++++++++++----- | |
includes/parser/Parser.php | 5 +++-- | |
tests/parser/parserTests.txt | 9 +++++++++ | |
3 files changed, 31 insertions(+), 7 deletions(-) | |
diff --git a/includes/parser/CoreTagHooks.php b/includes/parser/CoreTagHooks.php | |
index 9755ea9..3f4f54a 100644 | |
--- 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( '>', '<' ), | |
+ $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, array( '-{' => '-{', '}-' => '}-' ) ); | |
- return array( Xml::escapeTagsOnly( $content ), 'markerType' => 'nowiki' ); | |
+ $content = strtr( $content, array( | |
+ // lang converter | |
+ '-{' => '-{', | |
+ '}-' => '}-', | |
+ // html tags | |
+ '<' => '<', | |
+ '>' => '>' | |
+ // 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 ace63a0..7e0d319 100644 | |
--- a/includes/parser/Parser.php | |
+++ b/includes/parser/Parser.php | |
@@ -115,7 +115,8 @@ class Parser { | |
const OT_PLAIN = 4; # like extractSections() - portions of the original are returned unchanged. | |
# Marker Suffix needs to be accessible staticly. | |
- const MARKER_SUFFIX = "-QINU\x7f"; | |
+ # Must have a character that needs escaping in attributes (since 1.25.6) | |
+ const MARKER_SUFFIX = "-QINU`\"'\x7f"; | |
# Markers used for wrapping the table of contents | |
const TOC_START = '<mw:toc>'; | |
@@ -346,7 +347,7 @@ class Parser { | |
* Must not consist of all title characters, or else it will change | |
* the behavior of <nowiki> in a link. | |
*/ | |
- $this->mUniqPrefix = "\x7fUNIQ" . self::getRandomString(); | |
+ $this->mUniqPrefix = "\x7f'\"`UNIQ" . self::getRandomString(); | |
$this->mStripState = new StripState( $this->mUniqPrefix ); | |
# Clear these on every parse, bug 4549 | |
diff --git a/tests/parser/parserTests.txt b/tests/parser/parserTests.txt | |
index e965352..51c597a 100644 | |
--- a/tests/parser/parserTests.txt | |
+++ b/tests/parser/parserTests.txt | |
@@ -2125,6 +2125,15 @@ Entities inside <pre> | |
!! end | |
!! test | |
+<nowiki> inside of #tag:pre | |
+!! wikitext | |
+{{#tag:pre|Foo <nowiki>→bar</nowiki>}} | |
+!! html | |
+<pre>Foo →bar</pre> | |
+ | |
+!! end | |
+ | |
+!! test | |
<nowiki> and <pre> preference (first one wins) | |
!! wikitext | |
<pre> | |
-- | |
2.6.6 | |
File Metadata
File Metadata
- Mime Type
- text/x-diff
- Storage Engine
- blob
- Storage Format
- Raw Data
- Storage Handle
- 3678985
- Default Alt Text
- T110143b-REL1_25.patch (4 KB)