Skip to content

Commit

Permalink
#2305 위장한 파일 업로드를 이용한 XSS, SSRF 취약점 고침
Browse files Browse the repository at this point in the history
- 취약점 제보자
  - Provensec (https://www.provensec.com)
  - Author: Subodh Kumar
- 취약점 패치 제공 : @kijin
- 이 패치는 Rhymix에서 문제를 고친 코드를 기반으로하여 XE1에 맞게 일부 변경되어 적용되었습니다
  • Loading branch information
bnu committed Sep 20, 2018
1 parent 92c79f9 commit 8d7d0e9
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 60 deletions.
41 changes: 20 additions & 21 deletions classes/context/Context.class.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function init()
if(!isset($GLOBALS['HTTP_RAW_POST_DATA']) && version_compare(PHP_VERSION, '5.6.0', '>=') === TRUE)
{
$GLOBALS['HTTP_RAW_POST_DATA'] = file_get_contents("php://input");

// If content is not XML JSON, unset
if(!preg_match('/^[\<\{\[]/', $GLOBALS['HTTP_RAW_POST_DATA']) && strpos($_SERVER['CONTENT_TYPE'], 'json') === FALSE && strpos($_SERVER['HTTP_CONTENT_TYPE'], 'json') === FALSE)
{
Expand Down Expand Up @@ -711,7 +711,7 @@ function checkSSO()
echo self::get('lang')->msg_invalid_request;
return false;
}

setcookie(session_name(), $session_name);

$url = preg_replace('/[\?\&]SSOID=.+$/', '', self::getRequestUrl());
Expand Down Expand Up @@ -1413,18 +1413,13 @@ function _filterRequestVar($key, $val, $do_stripslashes = true, $remove_hack = f
{
$result[$k] = !preg_match('/^[0-9,]+$/', $v) ? (int) $v : $v;
}
elseif($key === 'mid' || $key === 'search_keyword')
{
elseif(in_array($key, array('mid','search_keyword','search_target','xe_validator_id'))) {
$result[$k] = escape($v, false);
}
elseif($key === 'vid')
{
$result[$k] = urlencode($v);
}
elseif($key === 'xe_validator_id')
{
$result[$k] = escape($v, false);
}
elseif(stripos($key, 'XE_VALIDATOR', 0) === 0)
{
unset($result[$k]);
Expand Down Expand Up @@ -1492,7 +1487,7 @@ function _setUploadedArgument()
$tmp_name = $val['tmp_name'];
if(!is_array($tmp_name))
{
if(!$tmp_name || !is_uploaded_file($tmp_name))
if(!UploadFileFilter::check($tmp_name, $val['name']))
{
continue;
}
Expand All @@ -1503,22 +1498,26 @@ function _setUploadedArgument()
else
{
$files = array();
$count_files = count($tmp_name);

for($i = 0; $i < $count_files; $i++)
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))
{
self::set($key, $files, true);
}
if($files) $this->set($key, $files, TRUE);
}
}
}
Expand Down
154 changes: 129 additions & 25 deletions classes/security/UploadFileFilter.class.php
Original file line number Diff line number Diff line change
@@ -1,44 +1,148 @@
<?php
/* Copyright (C) NAVER <http://www.navercorp.com> */

class UploadFileFilter
{
private static $_block_list = array ('exec', 'system', 'passthru', 'show_source', 'phpinfo', 'fopen', 'file_get_contents', 'file_put_contents', 'fwrite', 'proc_open', 'popen');
/**
* 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 = filesize($file)) == 0)
{
return false;
}

// Get the extension.
$ext = $filename ? strtolower(substr(strrchr($filename, '.'), 1)) : '';

public function check($file)
// Check the first 4KB of the file for possible XML content.
$fp = fopen($file, 'rb');
$first4kb = fread($fp, 4096);
$is_xml = preg_match('/<(?:\?xml|!DOCTYPE|html|head|body|meta|script|svg)\b/i', $first4kb);

// Check SVG files.
if (($ext === 'svg' || $is_xml) && !self::_checkSVG($fp, 0, $filesize))
{
fclose($fp);
return false;
}

// Check XML files.
if (($ext === 'xml' || $is_xml) && !self::_checkXML($fp, 0, $filesize))
{
fclose($fp);
return false;
}

// Check HTML files.
if (($ext === 'html' || $ext === 'shtml' || $ext === 'xhtml' || $ext === 'phtml' || $is_xml) && !self::_checkHTML($fp, 0, $filesize))
{
fclose($fp);
return false;
}

// Return true if everything is OK.
fclose($fp);
return true;
}

/**
* Check SVG file for XSS or SSRF vulnerabilities (#1088, #1089)
*
* @param resource $fp
* @param int $from
* @param int $to
* @return bool
*/
protected static function _checkSVG($fp, $from, $to)
{
if (self::_matchStream('/<script|<handler\b|xlink:href\s*=\s*"(?!data:)/i', $fp, $from, $to))
{
return false;
}
if (self::_matchStream('/\b(?:ev:(?:event|listener|observer)|on[a-z]+)\s*=/i', $fp, $from, $to))
{
return false;
}

return true;
}

/**
* Check XML file for external entity inclusion.
*
* @param resource $fp
* @param int $from
* @param int $to
* @return bool
*/
protected static function _checkXML($fp, $from, $to)
{
// TODO: 기능개선후 enable
if (self::_matchStream('/<!ENTITY/i', $fp, $from, $to))
{
return false;
}

return TRUE; // disable
if (! $file || ! FileHandler::exists($file)) return TRUE;
return self::_check ( $file );
return true;
}

private function _check($file)
/**
* Check HTML file for PHP code, server-side includes, and other nastiness.
*
* @param resource $fp
* @param int $from
* @param int $to
* @return bool
*/
protected static function _checkHTML($fp, $from, $to)
{
if (! ($fp = fopen ( $file, 'r' ))) return FALSE;
if (self::_matchStream('/<\?(?!xml\b)|<!--#(?:include|exec|echo|config|fsize|flastmod|printenv)\b/i', $fp, $from, $to))
{
return false;
}

$has_php_tag = FALSE;
return true;
}

while ( ! feof ( $fp ) )
/**
* Match a stream against a regular expression.
*
* This method is useful when dealing with large files,
* because we don't need to load the entire file into memory.
* We allow a generous overlap in case the matching string
* occurs across a block boundary.
*
* @param string $regexp
* @param resource $fp
* @param int $from
* @param int $to
* @param int $block_size (optional)
* @param int $overlap_size (optional)
* @return bool
*/
protected static function _matchStream($regexp, $fp, $from, $to, $block_size = 16384, $overlap_size = 1024)
{
fseek($fp, $position = $from);
while (strlen($content = fread($fp, $block_size + $overlap_size)) > 0)
{
$content = fread ( $fp, 8192 );
if (FALSE === $has_php_tag) $has_php_tag = strpos ( $content, '<?' );
foreach ( self::$_block_list as $v )
if (preg_match($regexp, $content))
{
if (FALSE !== $has_php_tag && FALSE !== strpos ( $content, $v ))
{
fclose ( $fp );
return FALSE;
}
return true;
}
fseek($fp, min($to, $position += $block_size));
}

fclose ( $fp );

return TRUE;
return false;
}
}

/* End of file : UploadFileFilter.class.php */
/* Location: ./classes/security/UploadFileFilter.class.php */
2 changes: 1 addition & 1 deletion modules/admin/admin.admin.controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ function procAdminEnviromentGatheringAgreement()
function procAdminUpdateConfig()
{
$adminTitle = Context::get('adminTitle');
$file = $_FILES['adminLogo'];
$file = Context::get('adminLogo');

$oModuleModel = getModel('module');
$oAdminConfig = $oModuleModel->getModuleConfig('admin');
Expand Down
2 changes: 1 addition & 1 deletion modules/file/file.controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function init()
function procFileUpload()
{
Context::setRequestMethod('JSON');
$file_info = $_FILES['Filedata'];
$file_info = Context::get('Filedata');

// An error appears if not a normally uploaded file
if(!is_uploaded_file($file_info['tmp_name'])) exit();
Expand Down
6 changes: 3 additions & 3 deletions modules/member/member.admin.controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,21 +114,21 @@ function procMemberAdminInsert()
$this->add('member_srl', $args->member_srl);
$this->setMessage($msg_code);

$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']);
Expand Down
18 changes: 9 additions & 9 deletions modules/member/member.controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -387,19 +387,19 @@ function procMemberInsert()
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']);
Expand Down Expand Up @@ -605,19 +605,19 @@ function procMemberModifyInfo()
$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']);
Expand Down Expand Up @@ -731,7 +731,7 @@ function procMemberLeave()
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'])) return $this->stop('msg_not_uploaded_profile_image');
// Ignore if member_srl is invalid or doesn't exist.
$member_srl = Context::get('member_srl');
Expand Down Expand Up @@ -840,7 +840,7 @@ function insertProfileImage($member_srl, $target_file)
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'])) return $this->stop('msg_not_uploaded_image_name');
// Ignore if member_srl is invalid or doesn't exist.
$member_srl = Context::get('member_srl');
Expand Down Expand Up @@ -986,7 +986,7 @@ function procMemberDeleteImageName($_memberSrl = 0)
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'])) return $this->stop('msg_not_uploaded_image_mark');
// Ignore if member_srl is invalid or doesn't exist.
$member_srl = Context::get('member_srl');
Expand Down

1 comment on commit 8d7d0e9

@kijin
Copy link
Contributor

@kijin kijin commented on 8d7d0e9 Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UploadFileFilter는 클래스 이름만 똑같고 제가 라이믹스에서 완전히 새로 작성한 것이므로, func.inc.php에 새로 추가된 함수들처럼 Copyright를 붙여주시면 감사하겠습니다^^

Please sign in to comment.