SVG filter evasion using default attribute values in DTD declaration
Closed, ResolvedPublic

Description

hey,

there's a persistent cross site scripting vulnerability affecting mediawiki when SVG file is uploaded to mediawiki.

I have uploaded a POC svg file

steps to reproduce are simple

  1. log in mediawiki
  2. visit /index.php/Special:Upload
  3. upload the poc.svg.
  4. once uploaded, visit the file.
  5. see the alert message.

cheers,
Mario

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 27 2016, 11:15 PM

Tested on master, confirmed that this file uploads unmolested and displays an alert when opened.

@Bawolff Is MW supposed to do SVG sanitization? I thought it did, but I forget.

Anomie added a subscriber: Anomie.Nov 28 2016, 10:03 PM

The trick here is in the !ATTLIST specifying a strange default value for attributes that we normally prohibit, and the browser interpreting it.

Chances are the solution here is to just blacklist !ATTLIST entirely, it seems unlikely to have much valid use in SVGs. But I suppose someone should check that assumption against SVGs on Commons in case some weird optimizer is using it or something.

Bawolff renamed this task from persistent cross site scripting in svg files to SVG filter evasion using default attribute values in DTD decleration..Nov 28 2016, 10:29 PM
dpatrick triaged this task as High priority.Nov 28 2016, 10:55 PM
dpatrick added a project: Vuln-XSS.

The most obvious solution is to tell libxml to load default attribute values

(I need to check if there are any unintended consequences of doing that).

The most obvious solution is to tell libxml to load default attribute values

(I need to check if there are any unintended consequences of doing that).

Code-Review: +1. I didn't think that'd make the attributes show up to the validation callback, but I confirm it does locally with libxml version 2.9.4.

Is this going to be the final patch then?

MarkTraceur moved this task from Untriaged to Tracking on the Multimedia board.Nov 30 2016, 6:47 PM

Final version will have unit tests to prevent regressions, but probably this will be what we do (unless anyone has an alternative proposal)

Are you guys planning to release the fix for this soon?

Bawolff added a comment.EditedDec 5 2016, 4:36 AM

So upon further testing, this option appears to cause libxml to apply different (more strict) well-formedness rules. This is mostly not an issue, except that it will cause the messed up svgs from illustrator -> inkscape to be rejected, which we don't want to be rejected (T144827).

Are you guys planning to release the fix for this soon?

Generally speaking, we batch security issues together and release them all at once whenever the next security release of MediaWiki is (I'm not sure when the next one is scheduled). Fixes go out to wikipedia as soon as they are tested, which should hopefully be soon.

It seems like DEFAULTATTR causes external dtd's (including http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd) and external entity references to be loaded (Depending on config, may be stopped by hhvm.libxml.ext_entity_whitelist). This is bad, so we probably can't use that approach.

So it looks like DEFAULTATTR doesn't actually cause external things to be fetched, it just causes a bunch of warnings about things that wouldn't be able to be fetched if libxml was fetching things, which is confusing. So I think that the following patch:

Is a safe (albeit hacky) patch for this issue. However I don't feel super confident in it, since what if other versions of libxml behave differently in regards to loading external dtds.

I've also attempted to do a more simple approach of looking for <!ATTLIST using a regex. Turns out you basically need to write an xml parser to do that safely (e.g. Due to parameter entities, you can invent your own escape codes, so <!ATTLIST can look like anything. Also, safely determining when the dtd ends is complicated since entity definitions can contain ]>).

Anomie added a comment.Dec 5 2016, 2:50 PM
+			foreach( $svgdtd as $dtd ) {
+				if ( $errstr === "Protocol 'http' for external XML entity '$dtd' is disabled for security reasons. This may be changed using the hhvm.libxml.ext_entity_whitelist ini setting." ) {
+					return;
+				}
+			}

Do we even care about this error if the specific DTD isn't in $svgdtd? Or could we just ignore anything matching "Protocol '[^']*' for external XML entity '[^']*' is disabled for security reasons. This may be changed using the hhvm.libxml.ext_entity_whitelist ini setting."?

I've also attempted to do a more simple approach of looking for <!ATTLIST using a regex. Turns out you basically need to write an xml parser to do that safely

Can you look for XmlReader::DOC_TYPE when processing the file and use readOuterXML() to get just the DTD? Or does that still allow screwy escapes to confuse things?

In the context of the recent discussion of supporting more HTML inside SVGs, we considered https://github.com/cure53/DOMPurify as a sanitization solution for SVGs, HTML, and MathML. It might be worth checking how they deal with this particular issue. Maybe we can steal ideas, and we would gain a data point for the quality of that library.

I think I was looking at all complicatedness of DTD, and making things more confusing then they actually are. We don't need to actually check most of the dtd options, we just need to whitelist a very small subset

Can you look for XmlReader::DOC_TYPE when processing the file and use readOuterXML() to get just the DTD? Or does that still allow screwy escapes to confuse things?

That's a good suggestion. Having libxml extract ensures that we don't accidentally think the dtd ends earlier than it does, so we can make sure we've checked the entire dtd.

Looks sane, haven't tested.

Have you tested your assumptions about long entities, different doctype URIs, and entities built from other entities against the existing set of SVGs on Commons?

 				default:
					// One of DOC, DOC_TYPE, ENTITY, END_ENTITY,

The comment needs updating, DOC_TYPE won't make it in there anymore.

+		$dtd = $reader->readOuterXML();

Do we care to flag the possible use of a too-old version of libxml, like SVGMetadataExtractor.php does? Since 2.6.20 was released in 2005 while we require a version of PHP released 8 years later, unless there's reason to think people might somehow be running a modern PHP with an ancient libxml let's say "no".

+		if ( $callbackReturn ) {
+			// Filter hit!
+			$this->filterMatch = true;
+			$this->filterMatchType = $callbackReturn;
+			$callbackReturn = false;
+		}

Do we care that the 'external_dtd_handler' result hides the 'dtd_handler' result if both hit?

Do we care that the 'external_dtd_handler' result hides the 'dtd_handler' result if both hit?

I guess it would be more technically correct to call both handlers. I'll upload a new patch.

I agree about not caring about super-old libxml.

I have not tested with real files from commons yet, however I intend to do so.

Bawolff merged a task: Restricted Task.Mar 21 2017, 11:41 PM
Bawolff added a subscriber: Jclaudius.

Is this fix targeted for any specific release of mediawiki?

If not, is there anything I can do to help advance this landing?

Do we care that the 'external_dtd_handler' result hides the 'dtd_handler' result if both hit?

I guess it would be more technically correct to call both handlers. I'll upload a new patch.

Actually, I think its fine to hide the other result, since the rest of the library basically behaves that way.

New version (rebased on to master):

Bawolff added a comment.EditedMar 26 2017, 9:48 AM

I am now running this new code over all the svgs uploaded to commons in the last three months (I'm not sure how long that will take, so if its going on and on, I might cut the test short), to see if this has any likely false positives.

For reference, I'm using the script at https://www.mediawiki.org/wiki/Wikimedia_Security_Team/SVG_filter_changes

Is this fix targeted for any specific release of mediawiki?

I'm hoping that it will be in the next release of MediaWiki (1.28.1, 1.27.2, 1.23.15) which should be soonish.

Bawolff added a comment.EditedMar 27 2017, 10:45 AM

So, my script checked the most recent 59,500 files (until it died for some unknown reason, possibly some sort of memory leak, but this seems like a good enough sample size. This covers all svg files less than 10 mb uploaded between yesterday and December 11, 2016).

It found the following files did not meet validation with the new checks. I haven't investigated why yet, so its possible there's an unrelated error:

Bawolff added a comment.EditedMar 27 2017, 1:11 PM

Ok, revised version of patch:

This version allows:

  • Entities to be aliases of other entities (e.g. <!ENTITY foo "&bar;"> where the replacement text solely consists of another entitity ref)
  • Comments in the internal dtd subset
  • The precise text <!ATTLIST svg xmlns:xlink CDATA #FIXED "http://www.w3.org/1999/xlink"> for compat with graphviz

I'm re-running my testing script with the new version.

Backports of this version of the patch:



Reedy renamed this task from SVG filter evasion using default attribute values in DTD decleration. to SVG filter evasion using default attribute values in DTD declaration.Mar 27 2017, 4:20 PM

Ok, my script completed checking it. It ran against all SVGS < 10 mb uploaded between now and October 1, 2016 to commos.wikimedia.org (91646 files in total). None of them triggered the filter, so I call this a success.

Patch was deployed to Wikimedia March 27 2017 22:00

Bawolff moved this task from Backlog to Pending release on the Security board.Mar 27 2017, 10:03 PM
Reedy closed this task as Resolved.Mar 30 2017, 6:03 PM
Reedy assigned this task to Bawolff.
Reedy added a subscriber: Reedy.

Closing for ease of tracking progress. Patches attached to parent bug, due for next release

Reedy added a comment.Mar 30 2017, 8:48 PM

Can someone give me a TLDR for the CVE bug over at T160876

What the Flaw is, and how it can be exploited. Thanks

Can someone give me a TLDR for the CVE bug over at T160876

What the Flaw is, and how it can be exploited. Thanks

Done

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 6 2017, 8:57 PM

Change 346844 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Whitelist DTD declaration in SVG

https://gerrit.wikimedia.org/r/346844

Change 346863 merged by jenkins-bot:
[mediawiki/core@REL1_28] SECURITY: Whitelist DTD declaration in SVG

https://gerrit.wikimedia.org/r/346863

Change 346853 merged by jenkins-bot:
[mediawiki/core@REL1_27] SECURITY: Whitelist DTD declaration in SVG

https://gerrit.wikimedia.org/r/346853

Change 347041 had a related patch set uploaded (by Chad; owner: Brian Wolff):
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: Whitelist DTD declaration in SVG

https://gerrit.wikimedia.org/r/347041

Change 347041 merged by jenkins-bot:
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: Whitelist DTD declaration in SVG

https://gerrit.wikimedia.org/r/347041

Glrx added a subscriber: Glrx.Sep 13 2017, 10:07 PM