Page MenuHomePhabricator
Files F5024188

T151735.patch
Bawolff (Brian Wolff)
Actions

Authored By
Bawolff
Dec 10 2016, 12:41 PM
Size
17 KB
Referenced Files
None
Subscribers
None

T151735.patch
View Options

From 70a7211199fe7d21c077aa990e427639534e36fb Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
Date: Mon, 28 Nov 2016 23:34:24 +0000
Subject: [PATCH] Security: Whitelist DTD declaration in SVG
Only allow ENTITY declarations inside the doctype internal
subset. Do not allow parameter entities, recursive entity
references are entity values longer than 255 bytes, or
external entity references. Filter external doctype subset
to only allow the standard svg doctypes.
This prevents someone bypassing filter by using default
attribute values in internal dtd subset. No browser loads
the external dtd subset that I could find, but whitelist
just to be safe anyways.
Issue reported by Cassiogomes11.
Bug: T151735
Change-Id: I7cb4690f759ad97e70e06e560978b6207d84c446
---
includes/libs/mime/XmlTypeCheck.php | 140 +++++++++++++++++++++++
includes/upload/UploadBase.php | 33 +++++-
languages/i18n/en.json | 1 +
languages/i18n/qqq.json | 1 +
tests/phpunit/includes/upload/UploadBaseTest.php | 83 +++++++++++++-
5 files changed, 252 insertions(+), 6 deletions(-)
diff --git a/includes/libs/mime/XmlTypeCheck.php b/includes/libs/mime/XmlTypeCheck.php
index 3958f8c..8a9671f 100644
--- a/includes/libs/mime/XmlTypeCheck.php
+++ b/includes/libs/mime/XmlTypeCheck.php
@@ -73,6 +73,9 @@ class XmlTypeCheck {
*/
private $parserOptions = [
'processing_instruction_handler' => '',
+ 'external_dtd_handler' => '',
+ 'dtd_handler' => '',
+ 'require_safe_dtd' => true
];
/**
@@ -86,6 +89,9 @@ class XmlTypeCheck {
* filename (default, true) or if it is a string (false)
* @param array $options list of additional parsing options:
* processing_instruction_handler: Callback for xml_set_processing_instruction_handler
+ * external_dtd_handler: Callback for the url of external dtd subset
+ * dtd_handler: Callback given the full text of the <!DOCTYPE declaration.
+ * require_safe_dtd: Only allow non-recursive entities in internal dtd (default true)
*/
function __construct( $input, $filterCallback = null, $isFile = true, $options = [] ) {
$this->filterCallback = $filterCallback;
@@ -186,6 +192,9 @@ class XmlTypeCheck {
if ( $reader->nodeType === XMLReader::PI ) {
$this->processingInstructionHandler( $reader->name, $reader->value );
}
+ if ( $reader->nodeType === XMLReader::DOC_TYPE ) {
+ $this->DTDHandler( $reader );
+ }
} while ( $reader->nodeType != XMLReader::ELEMENT );
// Process the rest of the document
@@ -234,6 +243,11 @@ class XmlTypeCheck {
$reader->value
);
break;
+ case XMLReader::DOC_TYPE:
+ // We should never see a doctype after first
+ // element.
+ $this->wellFormed = false;
+ break;
default:
// One of DOC, DOC_TYPE, ENTITY, END_ENTITY,
// NOTATION, or XML_DECLARATION
@@ -341,4 +355,130 @@ class XmlTypeCheck {
$this->filterMatchType = $callbackReturn;
}
}
+ /**
+ * Handle coming across a <!DOCTYPE declaration.
+ *
+ * @param XMLReader $reader Reader currently pointing at DOCTYPE node.
+ */
+ private function DTDHandler( XMLReader $reader ) {
+ $externalCallback = $this->parserOptions['external_dtd_handler'];
+ $generalCallback = $this->parserOptions['dtd_handler'];
+ $checkIfSafe = $this->parserOptions['require_safe_dtd'];
+ if ( !$externalCallback && !$generalCallback && !$checkIfSafe ) {
+ return;
+ }
+ $dtd = $reader->readOuterXML();
+ $callbackReturn = false;
+
+ if ( $generalCallback ) {
+ $callbackReturn = call_user_func( $generalCallback, $dtd );
+ }
+ if ( $callbackReturn ) {
+ // Filter hit!
+ $this->filterMatch = true;
+ $this->filterMatchType = $callbackReturn;
+ $callbackReturn = false;
+ }
+
+ $parsedDTD = $this->parseDTD( $dtd );
+ if ( $externalCallback && isset( $parsedDTD['type'] ) ) {
+ $callbackReturn = call_user_func(
+ $externalCallback,
+ $parsedDTD['type'],
+ isset( $parsedDTD['publicid'] ) ? $parsedDTD['publicid'] : null,
+ isset( $parsedDTD['systemid'] ) ? $parsedDTD['systemid'] : null
+ );
+ }
+ if ( $callbackReturn ) {
+ // Filter hit!
+ $this->filterMatch = true;
+ $this->filterMatchType = $callbackReturn;
+ $callbackReturn = false;
+ }
+
+ if ( $checkIfSafe && isset( $parsedDTD['internal'] ) ) {
+ if ( !$this->checkDTDIsSafe( $parsedDTD['internal'] ) ) {
+ $this->wellFormed = false;
+ }
+ }
+ }
+
+ /**
+ * Check if the internal subset of the DTD is safe.
+ *
+ * We whitelist an extremely restricted subset of DTD features.
+ *
+ * Safe is defined as:
+ * * Only contains entity defintions (e.g. No <!ATLIST )
+ * * Entity definitions are not "system" entities
+ * * Entity definitions are not "parameter" (i.e. %) entities
+ * * Entity definitions do not reference other entites except &amp; and quotes
+ * * Entity references aren't overly long (>255 bytes).
+ *
+ * @param string $internalSubset The internal subset of the DTD
+ * @return bool true if safe.
+ */
+ private function checkDTDIsSafe( $internalSubset ) {
+ $offset = 0;
+ $res = preg_match(
+ '/^(?:\s*<!ENTITY\s+\S+\s+(?:"(?:[^"%&]|&amp;|&quot;){0,255}"|\'(?:[^\'%&]|&amp;|&apos;){0,255}\')\s*>)*\s*$/',
+ $internalSubset
+ );
+
+ return (bool)$res;
+ }
+
+ /**
+ * Parse DTD into parts.
+ *
+ * If there is an error parsing the dtd, sets wellFormed to false.
+ *
+ * @param $dtd string
+ * @return array Possibly containing keys publicid, systemid, type and internal.
+ */
+ private function parseDTD( $dtd ) {
+ $m = [];
+ $res = preg_match(
+ '/^<!DOCTYPE\s*\S+\s*' .
+ '(?:(?P<typepublic>PUBLIC)\s*' .
+ '(?:"(?P<pubquote>[^"]*)"|\'(?P<pubapos>[^\']*)\')' . // public identifer
+ '\s*"(?P<pubsysquote>[^"]*)"|\'(?P<pubsysapos>[^\']*)\'' . // system identifier
+ '|(?P<typesystem>SYSTEM)\s*' .
+ '(?:"(?P<sysquote>[^"]*)"|\'(?P<sysapos>[^\']*)\')' .
+ ')?\s*' .
+ '(?:\[\s*(?P<internal>.*)\])?\s*>$/s',
+ $dtd,
+ $m
+ );
+ if ( !$res ) {
+ $this->wellFormed = false;
+ return [];
+ }
+ $parsed = [];
+ foreach ( $m as $field => $value ) {
+ if ( $value === '' || is_numeric( $field ) ) {
+ continue;
+ }
+ switch ( $field ) {
+ case 'typepublic':
+ case 'typesystem':
+ $parsed['type'] = $value;
+ break;
+ case 'pubquote':
+ case 'pubapos':
+ $parsed['publicid'] = $value;
+ break;
+ case 'pubsysquote':
+ case 'pubsysapos':
+ case 'sysquote':
+ case 'sysapos':
+ $parsed['systemid'] = $value;
+ break;
+ case 'internal':
+ $parsed['internal'] = $value;
+ break;
+ }
+ }
+ return $parsed;
+ }
}
diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index ea6ef30..21e730a 100644
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -1356,7 +1356,10 @@ abstract class UploadBase {
$filename,
[ $this, 'checkSvgScriptCallback' ],
true,
- [ 'processing_instruction_handler' => 'UploadBase::checkSvgPICallback' ]
+ [
+ 'processing_instruction_handler' => 'UploadBase::checkSvgPICallback',
+ 'external_dtd_handler' => 'UploadBase::checkSvgExternalDTD',
+ ]
);
if ( $check->wellFormed !== true ) {
// Invalid xml (bug 58553)
@@ -1389,6 +1392,34 @@ abstract class UploadBase {
}
/**
+ * Verify that DTD urls referenced are only the standard dtds
+ *
+ * Browsers seem to ignore external dtds. However just to be on the
+ * safe side, only allow dtds from the svg standard.
+ *
+ * @param string $type PUBLIC or SYSTEM
+ * @param string $publicId The well-known public identifier for the dtd
+ * @param string $systemId The url for the external dtd
+ */
+ public static function checkSvgExternalDTD( $type, $publicId, $systemId ) {
+ // This doesn't include the XHTML+MathML+SVG doctype since we don't
+ // allow XHTML anyways.
+ $allowedDTDs = [
+ 'http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd',
+ 'http://www.w3.org/TR/2001/REC-SVG-20010904/DTD/svg10.dtd',
+ 'http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-basic.dtd',
+ 'http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd'
+ ];
+ if ( $type !== 'PUBLIC'
+ || !in_array( $systemId, $allowedDTDs )
+ || strpos( $publicId, "-//W3C//" ) !== 0
+ ) {
+ return [ 'upload-scripted-dtd' ];
+ }
+ return false;
+ }
+
+ /**
* @todo Replace this with a whitelist filter!
* @param string $element
* @param array $attribs
diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 9d16ea1..e5c0376 100644
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -1484,6 +1484,7 @@
"php-uploaddisabledtext": "File uploads are disabled in PHP.\nPlease check the file_uploads setting.",
"uploadscripted": "This file contains HTML or script code that may be erroneously interpreted by a web browser.",
"upload-scripted-pi-callback": "Cannot upload a file that contains XML-stylesheet processing instruction.",
+ "upload-scripted-dtd": "Cannot upload SVG files that contain a non-standard DTD declaration.",
"uploaded-script-svg": "Found scriptable element \"$1\" in the uploaded SVG file.",
"uploaded-hostile-svg": "Found unsafe CSS in the style element of uploaded SVG file.",
"uploaded-event-handler-on-svg": "Setting event-handler attributes <code>$1=\"$2\"</code> is not allowed in SVG files.",
diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index c477fce..6c1a758 100644
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -1668,6 +1668,7 @@
"php-uploaddisabledtext": "This means that file uploading is disabled in PHP, not upload of PHP-files.",
"uploadscripted": "Used as error message when uploading a file.\n\nSee also:\n* {{msg-mw|zip-wrong-format}}\n* {{msg-mw|uploadjava}}\n* {{msg-mw|uploadvirus}}",
"upload-scripted-pi-callback": "Used as error message when uploading an SVG file that contains xml-stylesheet processing instruction.",
+ "upload-scripted-dtd": "Used as an error message when uploading an svg file that contains a DTD declaration where the system identifier of the DTD is for something other than the standard SVG DTDS, or it is a SYSTEM DTD, or the public identifier does not start with -//W3C//. Note that errors related to the internal dtd subset do not use this error message.",
"uploaded-script-svg": "Used as error message when uploading an SVG file that contains scriptable tags (script, handler, stylesheet, iframe).\n\nParameters:\n* $1 - The scriptable tag that blocked the SVG file from uploading.",
"uploaded-hostile-svg": "Used as error message when uploading an SVG file that contains unsafe CSS.",
"uploaded-event-handler-on-svg": "Used as error message when uploading an SVG file that contains event-handler attributes.\n\nParameters:\n* $1 - The event-handler attribute that is being modified in the SVG file.\n* $2 - The value that is given to the event-handler attribute.",
diff --git a/tests/phpunit/includes/upload/UploadBaseTest.php b/tests/phpunit/includes/upload/UploadBaseTest.php
index 6be272f..dd4b728 100644
--- a/tests/phpunit/includes/upload/UploadBaseTest.php
+++ b/tests/phpunit/includes/upload/UploadBaseTest.php
@@ -130,8 +130,8 @@ class UploadBaseTest extends MediaWikiTestCase {
*/
public function testCheckSvgScriptCallback( $svg, $wellFormed, $filterMatch, $message ) {
list( $formed, $match ) = $this->upload->checkSvgString( $svg );
- $this->assertSame( $wellFormed, $formed, $message );
- $this->assertSame( $filterMatch, $match, $message );
+ $this->assertSame( $wellFormed, $formed, $message . " (well-formed)" );
+ $this->assertSame( $filterMatch, $match, $message . " (filter match)" );
}
public static function provideCheckSvgScriptCallback() {
@@ -254,11 +254,17 @@ class UploadBaseTest extends MediaWikiTestCase {
],
[
'<?xml version="1.0"?> <?xml-stylesheet type="text/xml" href="#stylesheet"?> <!DOCTYPE doc [ <!ATTLIST xsl:stylesheet id ID #REQUIRED>]> <svg xmlns="http://www.w3.org/2000/svg"> <xsl:stylesheet id="stylesheet" version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <iframe xmlns="http://www.w3.org/1999/xhtml" src="javascript:alert(1)"></iframe> </xsl:template> </xsl:stylesheet> <circle fill="red" r="40"></circle> </svg>',
- true,
+ false,
true,
'SVG with embedded stylesheet (http://html5sec.org/#125)'
],
[
+ '<?xml version="1.0"?> <?xml-stylesheet type="text/xml" href="#stylesheet"?> <svg xmlns="http://www.w3.org/2000/svg"> <xsl:stylesheet id="stylesheet" version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform"> <xsl:template match="/"> <iframe xmlns="http://www.w3.org/1999/xhtml" src="javascript:alert(1)"></iframe> </xsl:template> </xsl:stylesheet> <circle fill="red" r="40"></circle> </svg>',
+ true,
+ true,
+ 'SVG with embedded stylesheet no doctype'
+ ],
+ [
'<svg xmlns="http://www.w3.org/2000/svg" id="x"> <listener event="load" handler="#y" xmlns="http://www.w3.org/2001/xml-events" observer="x"/> <handler id="y">alert(1)</handler> </svg>',
true,
true,
@@ -364,7 +370,7 @@ class UploadBaseTest extends MediaWikiTestCase {
],
[
'<?xml version="1.0" encoding="UTF-8" standalone="no"?> <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" [ <!ENTITY lol "lol"> <!ENTITY lol2 "&#x3C;&#x73;&#x63;&#x72;&#x69;&#x70;&#x74;&#x3E;&#x61;&#x6C;&#x65;&#x72;&#x74;&#x28;&#x27;&#x58;&#x53;&#x53;&#x45;&#x44;&#x20;&#x3D;&#x3E;&#x20;&#x27;&#x2B;&#x64;&#x6F;&#x63;&#x75;&#x6D;&#x65;&#x6E;&#x74;&#x2E;&#x64;&#x6F;&#x6D;&#x61;&#x69;&#x6E;&#x29;&#x3B;&#x3C;&#x2F;&#x73;&#x63;&#x72;&#x69;&#x70;&#x74;&#x3E;"> ]> <svg xmlns="http://www.w3.org/2000/svg" width="68" height="68" viewBox="-34 -34 68 68" version="1.1"> <circle cx="0" cy="0" r="24" fill="#c8c8c8"/> <text x="0" y="0" fill="black">&lol2;</text> </svg>',
- true,
+ false,
true,
'SVG with encoded script tag in internal entity (reported by Beyond Security)'
],
@@ -375,6 +381,16 @@ class UploadBaseTest extends MediaWikiTestCase {
'SVG with external entity'
],
[
+ // The base64 = <script>alert(1)</script>. If for some reason
+ // entities actually do get loaded, this should trigger
+ // filterMatch to be true. So this test verifies that we
+ // are not loading external entities.
+ '<?xml version="1.0"?> <!DOCTYPE svg [ <!ENTITY foo SYSTEM "data:text/plain;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pgo="> ]> <svg xmlns="http://www.w3.org/2000/svg" version="1.1"> <desc>&foo;</desc> <rect width="300" height="100" style="fill:rgb(0,0,255);stroke-width:1;stroke:rgb(0,0,2)" /> </svg>',
+ false,
+ false, /* False verifies entities aren't getting loaded */
+ 'SVG with data: uri external entity'
+ ],
+ [
"<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\"> <g> <a xlink:href=\"javascript:alert('1&#10;https://google.com')\"> <rect width=\"300\" height=\"100\" style=\"fill:rgb(0,0,255);stroke-width:1;stroke:rgb(0,0,2)\" /> </a> </g> </svg>",
true,
true,
@@ -393,6 +409,60 @@ class UploadBaseTest extends MediaWikiTestCase {
false,
'SVG with local urls, including filter: in style'
],
+ [
+ '<?xml version="1.0" encoding="UTF-8" standalone="no"?><!DOCTYPE x [<!ATTLIST image x:href CDATA "data:image/png,foo" onerror CDATA "alert(\'XSSED = \'+document.domain)" onload CDATA "alert(\'XSSED = \'+document.domain)"> ]> <svg xmlns:h="http://www.w3.org/1999/xhtml" xmlns:x="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg"> <image /> </svg>',
+ false,
+ false,
+ 'SVG with evil default attribute values'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8" standalone="no"?><!DOCTYPE svg SYSTEM "data:application/xml-dtd;base64,PCFET0NUWVBFIHN2ZyBbPCFBVFRMSVNUIGltYWdlIHg6aHJlZiBDREFUQSAiZGF0YTppbWFnZS9wbmcsZm9vIiBvbmVycm9yIENEQVRBICJhbGVydCgnWFNTRUQgPSAnK2RvY3VtZW50LmRvbWFpbikiIG9ubG9hZCBDREFUQSAiYWxlcnQoJ1hTU0VEID0gJytkb2N1bWVudC5kb21haW4pIj4gXT4K"><svg xmlns:x="http://www.w3.org/1999/xlink" xmlns="http://www.w3.org/2000/svg"> <image /> </svg>',
+ true,
+ true,
+ 'SVG with an evil external dtd'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg PUBLIC "-//FOO/bar" "http://example.com"><svg></svg>',
+ true,
+ true,
+ 'SVG with random public doctype'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg SYSTEM \'http://example.com/evil.dtd\' ><svg></svg>',
+ true,
+ true,
+ 'SVG with random SYSTEM doctype'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg [<!ENTITY % foo "bar" >] ><svg></svg>',
+ false,
+ false,
+ 'SVG with parameter entity'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg [<!ENTITY foo "bar%a;" ] ><svg></svg>',
+ false,
+ false,
+ 'SVG with entity referencing parameter entity'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg [<!ENTITY foo "bar0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"> ] ><svg></svg>',
+ false,
+ false,
+ 'SVG with long entity'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg [<!ENTITY foo \'"Hi", said bob\'> ] ><svg><g>&foo;</g></svg>',
+ true,
+ false,
+ 'SVG with apostrophe quote entity'
+ ],
+ [
+ '<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE svg [<!ENTITY name "Bob"><!ENTITY foo \'"Hi", said &name;.\'> ] ><svg><g>&foo;</g></svg>',
+ false,
+ false,
+ 'SVG with recursive entity',
+ ]
];
// @codingStandardsIgnoreEnd
}
@@ -478,7 +548,10 @@ class UploadTestHandler extends UploadBase {
$svg,
[ $this, 'checkSvgScriptCallback' ],
false,
- [ 'processing_instruction_handler' => 'UploadBase::checkSvgPICallback' ]
+ [
+ 'processing_instruction_handler' => 'UploadBase::checkSvgPICallback',
+ 'external_dtd_handler' => 'UploadBase::checkSvgExternalDTD'
+ ]
);
return [ $check->wellFormed, $check->filterMatch ];
}
--
1.9.5 (Apple Git-50.3)

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
4202049
Default Alt Text
T151735.patch (17 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits
  NODES
3d 1
coding 11
HOME 1
html5 1
Intern 16
Javascript 3
languages 10
Note 2
os 13
server 1
text 10
todo 1
Verify 1
web 1