From e98cf03d95ca44e1eab57a7791fe905fc5ab1cd2 Mon Sep 17 00:00:00 2001 From: Kijin Sung Date: Thu, 18 Oct 2018 14:34:19 +0900 Subject: [PATCH] Move upload file filter to Rhymix Framework and add proper unit tests for SVG-based attacks --- classes/security/UploadFileFilter.class.php | 128 +-------------- .../framework/filters/filecontentfilter.php | 152 ++++++++++++++++++ tests/_data/security/example.svg | 69 ++++++++ tests/_data/security/ssrf.svg | 5 + tests/_data/security/xss.svg | 8 + .../filters/FileContentFilterTest.php | 14 ++ 6 files changed, 250 insertions(+), 126 deletions(-) create mode 100644 common/framework/filters/filecontentfilter.php create mode 100644 tests/_data/security/example.svg create mode 100644 tests/_data/security/ssrf.svg create mode 100644 tests/_data/security/xss.svg create mode 100644 tests/unit/framework/filters/FileContentFilterTest.php diff --git a/classes/security/UploadFileFilter.class.php b/classes/security/UploadFileFilter.class.php index 85aae19ba..449377282 100644 --- a/classes/security/UploadFileFilter.class.php +++ b/classes/security/UploadFileFilter.class.php @@ -17,132 +17,8 @@ class UploadFileFilter 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)) : ''; - - // 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('/ 0) - { - if (preg_match($regexp, $content)) - { - return true; - } - fseek($fp, min($to, $position += $block_size)); - } - return false; + // Call Rhymix framework filter. + return Rhymix\Framework\Filters\FileContentFilter($file, $filename); } } diff --git a/common/framework/filters/filecontentfilter.php b/common/framework/filters/filecontentfilter.php new file mode 100644 index 000000000..620202245 --- /dev/null +++ b/common/framework/filters/filecontentfilter.php @@ -0,0 +1,152 @@ + 0) + { + if (preg_match($regexp, $content)) + { + return true; + } + fseek($fp, min($to, $position += $block_size)); + } + return false; + } +} diff --git a/tests/_data/security/example.svg b/tests/_data/security/example.svg new file mode 100644 index 000000000..438734c17 --- /dev/null +++ b/tests/_data/security/example.svg @@ -0,0 +1,69 @@ + + + + + + image/svg+xml + + + + + + + + + + + + diff --git a/tests/_data/security/ssrf.svg b/tests/_data/security/ssrf.svg new file mode 100644 index 000000000..bca6fc67f --- /dev/null +++ b/tests/_data/security/ssrf.svg @@ -0,0 +1,5 @@ + + + +test + \ No newline at end of file diff --git a/tests/_data/security/xss.svg b/tests/_data/security/xss.svg new file mode 100644 index 000000000..63270b5bd --- /dev/null +++ b/tests/_data/security/xss.svg @@ -0,0 +1,8 @@ + + + + + + diff --git a/tests/unit/framework/filters/FileContentFilterTest.php b/tests/unit/framework/filters/FileContentFilterTest.php new file mode 100644 index 000000000..5cacfe45c --- /dev/null +++ b/tests/unit/framework/filters/FileContentFilterTest.php @@ -0,0 +1,14 @@ +assertTrue(FileContentFilter::check(\RX_BASEDIR . 'tests/_data/security/example.svg')); + $this->assertFalse(FileContentFilter::check(\RX_BASEDIR . 'tests/_data/security/ssrf.svg')); + $this->assertFalse(FileContentFilter::check(\RX_BASEDIR . 'tests/_data/security/ssrf.svg', 'cover.jpg')); + $this->assertFalse(FileContentFilter::check(\RX_BASEDIR . 'tests/_data/security/xss.svg')); + } +}