X-Git-Url: http://git.cyclocoop.org/?a=blobdiff_plain;f=includes%2Fupload%2FUploadBase.php;h=89ce2b3a167f545fff51c1384efd7bba51a0ac53;hb=343c01d56b8539d0d70d10841c0123b7cce3cd18;hp=e74f52083c3f3cbf51ddb5276f3ecfe8a65c66e1;hpb=ea7c8b6f8babba9aaed733524ad886efea907c02;p=lhc%2Fweb%2Fwiklou.git

diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index e74f52083c..89ce2b3a16 100644
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -171,6 +171,7 @@ abstract class UploadBase {
 			return null;
 		}
 
+		/** @var UploadBase $handler */
 		$handler = new $className;
 
 		$handler->initializeFromRequest( $request );
@@ -377,11 +378,12 @@ abstract class UploadBase {
 	}
 
 	/**
-	 * Verify the mime type.
+	 * Verify the MIME type.
 	 *
-	 * @note Only checks that it is not an evil mime. The does it have
-	 *  correct extension given its mime type check is in verifyFile.
-	 * @param string $mime Representing the mime
+	 * @note Only checks that it is not an evil MIME. The "does it have
+	 *  correct extension given its MIME type?" check is in verifyFile.
+	 *  in `verifyFile()` that MIME type and file extension correlate.
+	 * @param string $mime Representing the MIME
 	 * @return mixed True if the file is verified, an array otherwise
 	 */
 	protected function verifyMimeType( $mime ) {
@@ -396,7 +398,7 @@ abstract class UploadBase {
 				return array( 'filetype-badmime', $mime );
 			}
 
-			# Check IE type
+			# Check what Internet Explorer would detect
 			$fp = fopen( $this->mTempPath, 'rb' );
 			$chunk = fread( $fp, 256 );
 			fclose( $fp );
@@ -487,7 +489,7 @@ abstract class UploadBase {
 
 		$this->mFileProps = FSFile::getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
 
-		# check mime type, if desired
+		# check MIME type, if desired
 		$mime = $this->mFileProps['file-mime'];
 		$status = $this->verifyMimeType( $mime );
 		if ( $status !== true ) {
@@ -881,7 +883,7 @@ abstract class UploadBase {
 	/**
 	 * Return the local file and initializes if necessary.
 	 *
-	 * @return LocalFile|null
+	 * @return LocalFile|UploadStashFile|null
 	 */
 	public function getLocalFile() {
 		if ( is_null( $this->mLocalFile ) ) {
@@ -995,9 +997,9 @@ abstract class UploadBase {
 	}
 
 	/**
-	 * Checks if the mime type of the uploaded file matches the file extension.
+	 * Checks if the MIME type of the uploaded file matches the file extension.
 	 *
-	 * @param string $mime The mime type of the uploaded file
+	 * @param string $mime The MIME type of the uploaded file
 	 * @param string $extension The filename extension that the file is to be served with
 	 * @return bool
 	 */
@@ -1050,7 +1052,7 @@ abstract class UploadBase {
 	 * positives in some situations.
 	 *
 	 * @param string $file Pathname to the temporary upload file
-	 * @param string $mime The mime type of the file
+	 * @param string $mime The MIME type of the file
 	 * @param string $extension The extension of the file
 	 * @return bool True if the file contains something looking like embedded scripts
 	 */
@@ -1295,7 +1297,8 @@ abstract class UploadBase {
 	 * @param array $attribs
 	 * @return bool
 	 */
-	public function checkSvgScriptCallback( $element, $attribs ) {
+	public function checkSvgScriptCallback( $element, $attribs, $data = null ) {
+
 		list( $namespace, $strippedElement ) = $this->splitXmlNamespace( $element );
 
 		// We specifically don't include:
@@ -1379,6 +1382,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 );
@@ -1421,6 +1432,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'
@@ -1461,23 +1484,23 @@ abstract class UploadBase {
 			}
 
 			# use CSS styles to bring in remote code
-			# catch url("http:..., url('http:..., url(http:..., but not url("#..., url('#..., url(#....
-			$tagsList = "font|clip-path|fill|filter|marker|marker-end|marker-mid|marker-start|mask|stroke";
 			if ( $stripped == 'style'
-				&& preg_match_all(
-					'!((?:' . $tagsList . ')\s*:\s*url\s*\(\s*["\']?\s*[^#]+.*?\))!sim',
-					$value,
-					$matches
-				)
+				&& self::checkCssFragment( Sanitizer::normalizeCss( $value ) )
 			) {
-				foreach ( $matches[1] as $match ) {
-					if ( !preg_match( '!(?:' . $tagsList . ')\s*:\s*url\s*\(\s*(#|\'#|"#)!sim', $match ) ) {
-						wfDebug( __METHOD__ . ": Found svg setting a style with "
-							. "remote url '$attrib'='$value' in uploaded file.\n" );
+				wfDebug( __METHOD__ . ": Found svg setting a style with "
+					. "remote url '$attrib'='$value' in uploaded file.\n" );
+				return true;
+			}
 
-						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
@@ -1495,9 +1518,61 @@ 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 string $name
+	 * @param string $element
 	 * @return array Containing the namespace URI and prefix
 	 */
 	private static function splitXmlNamespace( $element ) {
@@ -1883,7 +1958,7 @@ abstract class UploadBase {
 	 * Get the current status of a chunked upload (used for polling).
 	 * The status will be read from the *current* user session.
 	 * @param string $statusKey
-	 * @return array|bool
+	 * @return Status[]|bool
 	 */
 	public static function getSessionStatus( $statusKey ) {
 		return isset( $_SESSION[self::SESSION_STATUS_KEY][$statusKey] )