From 364b55264cf4daafa7a5d353f9be71864307a0d6 Mon Sep 17 00:00:00 2001
From: Pierre Schmitz <pierre@archlinux.de>
Date: Thu, 25 Sep 2014 13:59:03 +0200
Subject: Update to MediaWiki 1.22.11

---
 RELEASE-NOTES-1.22             |  10 +++++
 includes/DefaultSettings.php   |   2 +-
 includes/Sanitizer.php         |  51 ++++++++++++++-------
 includes/XmlTypeCheck.php      |  57 +++++++++++++++++++++--
 includes/upload/UploadBase.php | 100 +++++++++++++++++++++++++++++++++++++----
 5 files changed, 190 insertions(+), 30 deletions(-)

diff --git a/RELEASE-NOTES-1.22 b/RELEASE-NOTES-1.22
index 3479fbca..34ced35a 100644
--- a/RELEASE-NOTES-1.22
+++ b/RELEASE-NOTES-1.22
@@ -3,6 +3,16 @@
 Security reminder: MediaWiki does not require PHP's register_globals. If you
 have it on, turn it '''off''' if you can.
 
+== MediaWiki 1.22.11 ==
+
+This is a security release of the MediaWiki 1.22 branch.
+
+=== Changes since 1.22.10 ===
+* (bug 69008) SECURITY: Enhance CSS filtering in SVG files. Filter <style>
+  elements; normalize style elements and attributes before filtering; add
+  checks for attributes that contain css; add unit tests for html5sec and
+  reported bugs.
+
 == MediaWiki 1.22.10 ==
 
 This is a maintenance release of the MediaWiki 1.22 branch.
diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index b2c12001..1ec2ea35 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -63,7 +63,7 @@ $wgConf = new SiteConfiguration;
  * MediaWiki version number
  * @since 1.2
  */
-$wgVersion = '1.22.10';
+$wgVersion = '1.22.11';
 
 /**
  * Name of the site. It must be changed in LocalSettings.php
diff --git a/includes/Sanitizer.php b/includes/Sanitizer.php
index 9e9ac38b..3ca66443 100644
--- a/includes/Sanitizer.php
+++ b/includes/Sanitizer.php
@@ -819,24 +819,16 @@ class Sanitizer {
 	}
 
 	/**
-	 * Pick apart some CSS and check it for forbidden or unsafe structures.
-	 * Returns a sanitized string. This sanitized string will have
-	 * character references and escape sequences decoded and comments
-	 * stripped (unless it is itself one valid comment, in which case the value
-	 * will be passed through). If the input is just too evil, only a comment
-	 * complaining about evilness will be returned.
-	 *
-	 * Currently URL references, 'expression', 'tps' are forbidden.
-	 *
-	 * NOTE: Despite the fact that character references are decoded, the
-	 * returned string may contain character references given certain
-	 * clever input strings. These character references must
-	 * be escaped before the return value is embedded in HTML.
-	 *
-	 * @param $value String
-	 * @return String
+	 * Normalize CSS into a format we can easily search for hostile input
+	 *  - decode character references
+	 *  - decode escape sequences
+	 *  - convert characters that IE6 interprets into ascii
+	 *  - remove comments, unless the entire value is one single comment
+	 * @param string $value the css string
+	 * @return string normalized css
 	 */
-	static function checkCss( $value ) {
+	public static function normalizeCss( $value ) {
+
 		// Decode character references like &#123;
 		$value = Sanitizer::decodeCharReferences( $value );
 
@@ -922,6 +914,31 @@ class Sanitizer {
 			$value
 		);
 
+		return $value;
+	}
+
+
+	/**
+	 * Pick apart some CSS and check it for forbidden or unsafe structures.
+	 * Returns a sanitized string. This sanitized string will have
+	 * character references and escape sequences decoded and comments
+	 * stripped (unless it is itself one valid comment, in which case the value
+	 * will be passed through). If the input is just too evil, only a comment
+	 * complaining about evilness will be returned.
+	 *
+	 * Currently URL references, 'expression', 'tps' are forbidden.
+	 *
+	 * NOTE: Despite the fact that character references are decoded, the
+	 * returned string may contain character references given certain
+	 * clever input strings. These character references must
+	 * be escaped before the return value is embedded in HTML.
+	 *
+	 * @param string $value
+	 * @return string
+	 */
+	static function checkCss( $value ) {
+		$value = self::normalizeCss( $value );
+
 		// Reject problematic keywords and control characters
 		if ( preg_match( '/[\000-\010\013\016-\037\177]/', $value ) ) {
 			return '/* invalid control char */';
diff --git a/includes/XmlTypeCheck.php b/includes/XmlTypeCheck.php
index eb98a4ad..aca857e9 100644
--- a/includes/XmlTypeCheck.php
+++ b/includes/XmlTypeCheck.php
@@ -39,6 +39,23 @@ class XmlTypeCheck {
 	 */
 	public $rootElement = '';
 
+	/**
+	 * A stack of strings containing the data of each xml element as it's processed. Append
+	 * data to the top string of the stack, then pop off the string and process it when the
+	 * element is closed.
+	 */
+	protected $elementData = array();
+
+	/**
+	 * A stack of element names and attributes, as we process them.
+	 */
+	protected $elementDataContext = array();
+
+	/**
+	 * Current depth of the data stack.
+	 */
+	protected $stackDepth = 0;
+
 	/**
 	 * Additional parsing options
 	 */
@@ -51,7 +68,7 @@ class XmlTypeCheck {
 	 * @param callable $filterCallback (optional)
 	 *        Function to call to do additional custom validity checks from the
 	 *        SAX element handler event. This gives you access to the element
-	 *        namespace, name, and attributes, but not to text contents.
+	 *        namespace, name, attributes, and text contents.
 	 *        Filter should return 'true' to toggle on $this->filterMatch
 	 * @param boolean $isFile (optional) indicates if the first parameter is a
 	 *        filename (default, true) or if it is a string (false)
@@ -179,7 +196,12 @@ class XmlTypeCheck {
 		$this->rootElement = $name;
 
 		if ( is_callable( $this->filterCallback ) ) {
-			xml_set_element_handler( $parser, array( $this, 'elementOpen' ), false );
+			xml_set_element_handler(
+				$parser,
+				array( $this, 'elementOpen' ),
+				array( $this, 'elementClose' )
+			);
+			xml_set_character_data_handler( $parser, array( $this, 'elementData' ) );
 			$this->elementOpen( $parser, $name, $attribs );
 		} else {
 			// We only need the first open element
@@ -193,12 +215,41 @@ class XmlTypeCheck {
 	 * @param $attribs
 	 */
 	private function elementOpen( $parser, $name, $attribs ) {
-		if ( call_user_func( $this->filterCallback, $name, $attribs ) ) {
+		$this->elementDataContext[] = array( $name, $attribs );
+		$this->elementData[] = '';
+		$this->stackDepth++;
+	}
+
+	/**
+	 * @param $parser
+	 * @param $name
+	 */
+	private function elementClose( $parser, $name ) {
+		list( $name, $attribs ) = array_pop( $this->elementDataContext );
+		$data = array_pop( $this->elementData );
+		$this->stackDepth--;
+
+		if ( call_user_func(
+			$this->filterCallback,
+			$name,
+			$attribs,
+			$data
+		) ) {
 			// Filter hit!
 			$this->filterMatch = true;
 		}
 	}
 
+	/**
+	 * @param $parser
+	 * @param $data
+	 */
+	private function elementData( $parser, $data ) {
+		// xml_set_character_data_handler breaks the data on & characters, so
+		// we collect any data here, and we'll run the callback in elementClose
+		$this->elementData[ $this->stackDepth - 1 ] .= trim( $data );
+	}
+
 	/**
 	 * @param $parser
 	 * @param $target
diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index 40b3b19a..26cc5427 100644
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -1201,7 +1201,8 @@ abstract class UploadBase {
 	 * @param $attribs array
 	 * @return bool
 	 */
-	public function checkSvgScriptCallback( $element, $attribs ) {
+	public function checkSvgScriptCallback( $element, $attribs, $data = null ) {
+
 		list( $namespace, $strippedElement ) = $this->splitXmlNamespace( $element );
 
 		static $validNamespaces = array(
@@ -1274,6 +1275,14 @@ abstract class UploadBase {
 			return true;
 		}
 
+		# Check <style> css
+		if ( $strippedElement == 'style'
+			&& self::checkCssFragment( Sanitizer::normalizeCss( $data ) )
+		) {
+			wfDebug( __METHOD__ . ": hostile css in style element.\n" );
+			return true;
+		}
+
 		foreach ( $attribs as $attrib => $value ) {
 			$stripped = $this->stripXmlNamespace( $attrib );
 			$value = strtolower( $value );
@@ -1310,6 +1319,18 @@ abstract class UploadBase {
 				return true;
 			}
 
+			# Change href with animate from (http://html5sec.org/#137). This doesn't seem
+			# possible without embedding the svg, but filter here in case.
+			if ( $stripped == 'from'
+				&& $strippedElement === 'animate'
+				&& !preg_match( '!^https?://!im', $value )
+			) {
+				wfDebug( __METHOD__ . ": Found animate that might be changing href using from "
+					. "\"<$strippedElement '$attrib'='$value'...\" in uploaded file.\n" );
+
+				return true;
+			}
+
 			# use set/animate to add event-handler attribute to parent
 			if ( ( $strippedElement == 'set' || $strippedElement == 'animate' ) && $stripped == 'attributename' && substr( $value, 0, 2 ) == 'on' ) {
 				wfDebug( __METHOD__ . ": Found svg setting event-handler attribute with \"<$strippedElement $stripped='$value'...\" in uploaded file.\n" );
@@ -1335,14 +1356,23 @@ abstract class UploadBase {
 			}
 
 			# use CSS styles to bring in remote code
-			# catch url("http:..., url('http:..., url(http:..., but not url("#..., url('#..., url(#....
-			if ( $stripped == 'style' && preg_match_all( '!((?:font|clip-path|fill|filter|marker|marker-end|marker-mid|marker-start|mask|stroke)\s*:\s*url\s*\(\s*["\']?\s*[^#]+.*?\))!sim', $value, $matches ) ) {
-				foreach ( $matches[1] as $match ) {
-					if ( !preg_match( '!(?:font|clip-path|fill|filter|marker|marker-end|marker-mid|marker-start|mask|stroke)\s*:\s*url\s*\(\s*(#|\'#|"#)!sim', $match ) ) {
-						wfDebug( __METHOD__ . ": Found svg setting a style with remote url '$attrib'='$value' in uploaded file.\n" );
-						return true;
-					}
-				}
+			if ( $stripped == 'style'
+				&& self::checkCssFragment( Sanitizer::normalizeCss( $value ) )
+			) {
+				wfDebug( __METHOD__ . ": Found svg setting a style with "
+					. "remote url '$attrib'='$value' in uploaded file.\n" );
+				return true;
+			}
+
+			# Several attributes can include css, css character escaping isn't allowed
+			$cssAttrs = array( 'font', 'clip-path', 'fill', 'filter', 'marker',
+				'marker-end', 'marker-mid', 'marker-start', 'mask', 'stroke' );
+			if ( in_array( $stripped, $cssAttrs )
+				&& self::checkCssFragment( $value )
+			) {
+				wfDebug( __METHOD__ . ": Found svg setting a style with "
+					. "remote url '$attrib'='$value' in uploaded file.\n" );
+				return true;
 			}
 
 			# image filters can pull in url, which could be svg that executes scripts
@@ -1356,6 +1386,58 @@ abstract class UploadBase {
 		return false; //No scripts detected
 	}
 
+	/**
+	 * Check a block of CSS or CSS fragment for anything that looks like
+	 * it is bringing in remote code.
+	 * @param string $value a string of CSS
+	 * @param bool $propOnly only check css properties (start regex with :)
+	 * @return bool true if the CSS contains an illegal string, false if otherwise
+	 */
+	private static function checkCssFragment( $value ) {
+
+		# Forbid external stylesheets, for both reliability and to protect viewer's privacy
+		if ( strpos( $value, '@import' ) !== false ) {
+			return true;
+		}
+
+		# We allow @font-face to embed fonts with data: urls, so we snip the string
+		# 'url' out so this case won't match when we check for urls below
+		$pattern = '!(@font-face\s*{[^}]*src:)url(\("data:;base64,)!im';
+		$value = preg_replace( $pattern, '$1$2', $value );
+
+		# Check for remote and executable CSS. Unlike in Sanitizer::checkCss, the CSS
+		# properties filter and accelerator don't seem to be useful for xss in SVG files.
+		# Expression and -o-link don't seem to work either, but filtering them here in case.
+		# Additionally, we catch remote urls like url("http:..., url('http:..., url(http:...,
+		# but not local ones such as url("#..., url('#..., url(#....
+		if ( preg_match( '!expression
+				| -o-link\s*:
+				| -o-link-source\s*:
+				| -o-replace\s*:!imx', $value ) ) {
+			return true;
+		}
+
+		if ( preg_match_all(
+				"!(\s*(url|image|image-set)\s*\(\s*[\"']?\s*[^#]+.*?\))!sim",
+				$value,
+				$matches
+			) !== 0
+		) {
+			# TODO: redo this in one regex. Until then, url("#whatever") matches the first
+			foreach ( $matches[1] as $match ) {
+				if ( !preg_match( "!\s*(url|image|image-set)\s*\(\s*(#|'#|\"#)!im", $match ) ) {
+					return true;
+				}
+			}
+		}
+
+		if ( preg_match( '/[\000-\010\013\016-\037\177]/', $value ) ) {
+			return true;
+		}
+
+		return false;
+	}
+
 	/**
 	 * Divide the element name passed by the xml parser to the callback into URI and prifix.
 	 * @param $name string
-- 
cgit v1.2.2