From 60d390f52e73b29f215301bcf978c56ecae5d3c5 Mon Sep 17 00:00:00 2001
From: Kijin Sung <kijin@kijinsung.com>
Date: Mon, 17 Sep 2018 00:48:47 +0900
Subject: [PATCH] Initial mitigations for #1088 #1089

---
 classes/context/Context.class.php           | 22 +++++----
 classes/security/UploadFileFilter.class.php | 54 ++++++++++++++++++++-
 modules/file/file.controller.php            |  2 +-
 modules/member/member.admin.controller.php  |  6 +--
 modules/member/member.controller.php        | 18 +++----
 5 files changed, 79 insertions(+), 23 deletions(-)

diff --git a/classes/context/Context.class.php b/classes/context/Context.class.php
index ee68ccb23..86c33c211 100644
--- a/classes/context/Context.class.php
+++ b/classes/context/Context.class.php
@@ -1261,8 +1261,9 @@ class Context
 			$tmp_name = $val['tmp_name'];
 			if(!is_array($tmp_name))
 			{
-				if(!$tmp_name || !is_uploaded_file($tmp_name) || $val['size'] <= 0)
+				if(!UploadFileFilter::check($tmp_name, $val['name']))
 				{
+					unset($_FILES[$key]);
 					continue;
 				}
 				$val['name'] = escape($val['name'], false);
@@ -1275,16 +1276,19 @@ class Context
 				$files = array();
 				foreach ($tmp_name as $i => $j)
 				{
-					if($val['size'][$i] > 0)
+					if(!UploadFileFilter::check($val['tmp_name'][$i], $val['name'][$i]))
 					{
-						$file = array();
-						$file['name'] = $val['name'][$i];
-						$file['type'] = $val['type'][$i];
-						$file['tmp_name'] = $val['tmp_name'][$i];
-						$file['error'] = $val['error'][$i];
-						$file['size'] = $val['size'][$i];
-						$files[] = $file;
+						$files = array();
+						unset($_FILES[$key]);
+						break;
 					}
+					$file = array();
+					$file['name'] = $val['name'][$i];
+					$file['type'] = $val['type'][$i];
+					$file['tmp_name'] = $val['tmp_name'][$i];
+					$file['error'] = $val['error'][$i];
+					$file['size'] = $val['size'][$i];
+					$files[] = $file;
 				}
 				if(count($files))
 				{
diff --git a/classes/security/UploadFileFilter.class.php b/classes/security/UploadFileFilter.class.php
index b19bd6889..1b3b73a34 100644
--- a/classes/security/UploadFileFilter.class.php
+++ b/classes/security/UploadFileFilter.class.php
@@ -3,8 +3,60 @@
 
 class UploadFileFilter
 {
-	public function check($file)
+	/**
+	 * Generic checker
+	 * 
+	 * @param string $file
+	 * @param string $filename
+	 * @return bool
+	 */
+	public static function check($file, $filename = null)
 	{
+		// Return error if the file is not uploaded.
+		if (!$file || !file_exists($file) || !is_uploaded_file($file))
+		{
+			return false;
+		}
+		
+		// Return error if the file size is zero.
+		if (!filesize($file))
+		{
+			return false;
+		}
+		
+		// Get the extension.
+		$ext = $filename ? strtolower(substr(strrchr($filename, '.'), 1)) : '';
+		
+		// Check SVG files.
+		if ($ext === 'svg' && !self::_checkSVG($file))
+		{
+			return false;
+		}
+		
+		// Return true if everything is OK.
+		return true;
+	}
+	
+	/**
+	 * Check SVG file for XSS or SSRF vulnerabilities (#1088, #1089)
+	 *
+	 * @param string $file
+	 * @return bool
+	 */
+	protected static function _checkSVG($file)
+	{
+		$content = file_get_contents($file);
+		
+		if (preg_match('/xlink:href\s*=\s*"(?!data:)/i', $content))
+		{
+			return false;
+		}
+		
+		if (preg_match('/<script/i', $content))
+		{
+			return false;
+		}
+		
 		return true;
 	}
 }
diff --git a/modules/file/file.controller.php b/modules/file/file.controller.php
index 0c165c264..ebf3e93ad 100644
--- a/modules/file/file.controller.php
+++ b/modules/file/file.controller.php
@@ -26,7 +26,7 @@ class fileController extends file
 	function procFileUpload()
 	{
 		Context::setRequestMethod('JSON');
-		$file_info = $_FILES['Filedata'];
+		$file_info = Context::get('Filedata');
 
 		// An error appears if not a normally uploaded file
 		if(!$file_info || !is_uploaded_file($file_info['tmp_name'])) exit();
diff --git a/modules/member/member.admin.controller.php b/modules/member/member.admin.controller.php
index c3e06f488..b6f865e5b 100644
--- a/modules/member/member.admin.controller.php
+++ b/modules/member/member.admin.controller.php
@@ -128,21 +128,21 @@ class memberAdminController extends member
 		$signature = Context::get('signature');
 		$oMemberController->putSignature($args->member_srl, $signature);
 
-		$profile_image = $_FILES['profile_image'];
+		$profile_image = Context::get('profile_image');
 		if(is_uploaded_file($profile_image['tmp_name']))
 		{
 			$output = $oMemberController->insertProfileImage($args->member_srl, $profile_image['tmp_name']);
 			if(!$output->toBool()) return $output;
 		}
 
-		$image_mark = $_FILES['image_mark'];
+		$image_mark = Context::get('image_mark');
 		if(is_uploaded_file($image_mark['tmp_name']))
 		{
 			$output = $oMemberController->insertImageMark($args->member_srl, $image_mark['tmp_name']);
 			if(!$output->toBool()) return $output;
 		}
 
-		$image_name = $_FILES['image_name'];
+		$image_name = Context::get('image_name');
 		if (is_uploaded_file($image_name['tmp_name']))
 		{
 			$output = $oMemberController->insertImageName($args->member_srl, $image_name['tmp_name']);
diff --git a/modules/member/member.controller.php b/modules/member/member.controller.php
index 6007a54e7..cceb6ce50 100644
--- a/modules/member/member.controller.php
+++ b/modules/member/member.controller.php
@@ -692,19 +692,19 @@ class memberController extends member
 		if(!$output->toBool()) return $output;
 
 		// insert ProfileImage, ImageName, ImageMark
-		$profile_image = $_FILES['profile_image'];
+		$profile_image = Context::get('profile_image');
 		if(is_uploaded_file($profile_image['tmp_name']))
 		{
 			$this->insertProfileImage($args->member_srl, $profile_image['tmp_name']);
 		}
 
-		$image_mark = $_FILES['image_mark'];
+		$image_mark = Context::get('image_mark');
 		if(is_uploaded_file($image_mark['tmp_name']))
 		{
 			$this->insertImageMark($args->member_srl, $image_mark['tmp_name']);
 		}
 
-		$image_name = $_FILES['image_name'];
+		$image_name = Context::get('image_name');
 		if(is_uploaded_file($image_name['tmp_name']))
 		{
 			$this->insertImageName($args->member_srl, $image_name['tmp_name']);
@@ -927,19 +927,19 @@ class memberController extends member
 		$output = $this->updateMember($args);
 		if(!$output->toBool()) return $output;
 
-		$profile_image = $_FILES['profile_image'];
+		$profile_image = Context::get('profile_image');
 		if(is_uploaded_file($profile_image['tmp_name']))
 		{
 			$this->insertProfileImage($args->member_srl, $profile_image['tmp_name']);
 		}
 
-		$image_mark = $_FILES['image_mark'];
+		$image_mark = Context::get('image_mark');
 		if(is_uploaded_file($image_mark['tmp_name']))
 		{
 			$this->insertImageMark($args->member_srl, $image_mark['tmp_name']);
 		}
 
-		$image_name = $_FILES['image_name'];
+		$image_name = Context::get('image_name');
 		if(is_uploaded_file($image_name['tmp_name']))
 		{
 			$this->insertImageName($args->member_srl, $image_name['tmp_name']);
@@ -1056,7 +1056,7 @@ class memberController extends member
 	function procMemberInsertProfileImage()
 	{
 		// Check if the file is successfully uploaded
-		$file = $_FILES['profile_image'];
+		$file = Context::get('profile_image');
 		if(!is_uploaded_file($file['tmp_name'])) throw new Rhymix\Framework\Exception('msg_not_uploaded_profile_image');
 		// Ignore if member_srl is invalid or doesn't exist.
 		$member_srl = Context::get('member_srl');
@@ -1161,7 +1161,7 @@ class memberController extends member
 	function procMemberInsertImageName()
 	{
 		// Check if the file is successfully uploaded
-		$file = $_FILES['image_name'];
+		$file = Context::get('image_name');
 		if(!is_uploaded_file($file['tmp_name'])) throw new Rhymix\Framework\Exception('msg_not_uploaded_image_name');
 		// Ignore if member_srl is invalid or doesn't exist.
 		$member_srl = Context::get('member_srl');
@@ -1312,7 +1312,7 @@ class memberController extends member
 	function procMemberInsertImageMark()
 	{
 		// Check if the file is successfully uploaded
-		$file = $_FILES['image_mark'];
+		$file = Context::get('image_mark');
 		if(!is_uploaded_file($file['tmp_name'])) throw new Rhymix\Framework\Exception('msg_not_uploaded_image_mark');
 		// Ignore if member_srl is invalid or doesn't exist.
 		$member_srl = Context::get('member_srl');