From 5e02e7a08ff56d59a6d225736e046ac20395dbd4 Mon Sep 17 00:00:00 2001
From: csteipp <csteipp@wikimedia.org>
Date: Wed, 28 Jan 2015 17:54:08 -0800
Subject: [PATCH] SECURITY: Whitelist mime types for data: uri's

Both iSec and Cure53 reported ways to get around our blacklist, so
move to a whitelist.

Bug: T85850
Change-Id: Ia64445eca18ddc6bc975964772d02bca21ea3f42
---
 includes/upload/UploadBase.php                   | 32 ++++++++----------------
 tests/phpunit/includes/upload/UploadBaseTest.php |  7 ++++++
 2 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index d6e7433..63aa057 100644
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -1410,28 +1410,16 @@ abstract class UploadBase {
 				}
 			}
 
-			# href with embedded svg as target
-			if ( $stripped == 'href' && preg_match( '!data:[^,]*image/svg[^,]*,!sim', $value ) ) {
-				wfDebug( __METHOD__ . ": Found href to embedded svg "
-					. "\"<$strippedElement '$attrib'='$value'...\" in uploaded file.\n" );
-
-				return true;
-			}
-
-			# href with embedded (text/xml) svg as target
-			if ( $stripped == 'href' && preg_match( '!data:[^,]*text/xml[^,]*,!sim', $value ) ) {
-				wfDebug( __METHOD__ . ": Found href to embedded svg "
-					. "\"<$strippedElement '$attrib'='$value'...\" in uploaded file.\n" );
-
-				return true;
-			}
-
-			# href with embedded (application/xml) svg as target
-			if ( $stripped == 'href' && preg_match( '!data:[^,]*application/xml[^,]*,!sim', $value ) ) {
-				wfDebug( __METHOD__ . ": Found href to embedded svg "
-					. "\"<$strippedElement '$attrib'='$value'...\" in uploaded file.\n" );
-
-				return true;
+			# only allow data: targets that should be safe. This prevents vectors like,
+			# image/svg, text/xml, application/xml, and text/html, which can contain scripts
+			if ( $stripped == 'href' && preg_match( '!^data:!i', $value ) ) {
+				// rfc2397 parameters. This is only slightly slower than (;[\w;]+)*.
+				$parameters = '(?>;[a-zA-Z0-9\!#$&\'*+.^_`{|}~-]+=(?>[a-zA-Z0-9\!#$&\'*+.^_`{|}~-]+|"(?>[\0-\x0c\x0e-\x21\x23-\x5b\x5d-\x7f]+|\\[\0-\x7f])*"))*(?:;base64)?';
+				if ( !preg_match( "!^data:\s*image/(gif|jpeg|jpg|png)$parameters,!i", $value ) ) {
+					wfDebug( __METHOD__ . ": Found href to unwhitelisted data: uri "
+						. "\"<$strippedElement '$attrib'='$value'...\" in uploaded file.\n" );
+					return true;
+				}
 			}
 
 			# Change href with animate from (http://html5sec.org/#137).
diff --git a/tests/phpunit/includes/upload/UploadBaseTest.php b/tests/phpunit/includes/upload/UploadBaseTest.php
index 4188651..4ea300c 100644
--- a/tests/phpunit/includes/upload/UploadBaseTest.php
+++ b/tests/phpunit/includes/upload/UploadBaseTest.php
@@ -356,6 +356,13 @@ class UploadBaseTest extends MediaWikiTestCase {
 				true,
 				'SVG with remote background image using image() (bug 69008)'
 			),
+			array(
+				// As reported by Cure53
+				'<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <a xlink:href="data:text/html;charset=utf-8;base64, PHNjcmlwdD5hbGVydChkb2N1bWVudC5kb21haW4pPC9zY3JpcHQ%2BDQo%3D"> <circle r="400" fill="red"></circle> </a> </svg>',
+				true,
+				true,
+				'SVG with data:text/html link target (firefox only)'
+			),
 
 			// Test good, but strange files that we want to allow
 			array(
-- 
1.8.4.5