diff options
Diffstat (limited to 'includes')
| -rw-r--r-- | includes/DefaultSettings.php | 2 | ||||
| -rw-r--r-- | includes/Sanitizer.php | 51 | ||||
| -rw-r--r-- | includes/XmlTypeCheck.php | 57 | ||||
| -rw-r--r-- | includes/upload/UploadBase.php | 100 |
4 files changed, 180 insertions, 30 deletions
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 { $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 @@ -40,6 +40,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 */ private $parserOptions = array( @@ -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,7 +215,26 @@ 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; } @@ -201,6 +242,16 @@ class XmlTypeCheck { /** * @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 * @param $data */ 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 @@ -1357,6 +1387,58 @@ abstract class UploadBase { } /** + * 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 * @return array containing the namespace URI and prefix |