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.

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.

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).

In T151735#2829370, @Bawolff wrote:

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

T151735 - Load default attribute values when validating svgs1 KBDownload

(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?

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?

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).

In T151735#2838492, @Cassiogomes11 wrote:

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 ]>).

+			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.

In T151735#2861607, @Bawolff wrote:

T151735.patch17 KBDownload

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.

Is this fix _targeted for any specific release of mediawiki?

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

In T151735#2871001, @Bawolff wrote:

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):

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

In T151735#3120617, @Jclaudius wrote:

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.

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:

• https://commons.wikimedia.org/wiki/File:Max-flow_min-cut_example.svg (<!ATTLIST svg xmlns:xlink CDATA #FIXED "http://www.w3.org/1999/xlink"> . has the xlink namespace decleration but doesn't actually ever use it)
• https://commons.wikimedia.org/wiki/File:Excluded_nymap.svg (Uses recursive entity expansion to map locations to an entity, which are then mapped to category entities which maps to colour)
• https://commons.wikimedia.org/wiki/File:Polygonum_ramosissimum_ssp_ramosissimum_nymap.svg (ditto recursive entities)
• https://commons.wikimedia.org/wiki/File:Polygonum_glaucum_nymap.svg (ditto recursive entities)
• https://commons.wikimedia.org/wiki/File:Polygonum_aviculare_ssp._buxiforme_nymap.svg
• https://commons.wikimedia.org/wiki/File:Polygonum_erectum_nymap.svg
• https://commons.wikimedia.org/wiki/File:Polygonum_ramosissimum_ssp_prolificum_nymap.svg
• https://commons.wikimedia.org/wiki/File:Graph_of_meanings_for_Stamp,_Seal,_Philately.svg (Graphviz <!ATTLIST svg xmlns:xlink CDATA #FIXED "http://www.w3.org/1999/xlink">)
• https://commons.wikimedia.org/wiki/File:First_Ionization_Energy.svg (Comment in dtd internal subset)
• https://commons.wikimedia.org/wiki/File:Syringa_vulgaris_nymap.svg
• https://commons.wikimedia.org/wiki/File:Syringa_reticulata_nymap.svg
• https://commons.wikimedia.org/wiki/File:Anemone_multifida_var_multifida_nymap.svg
• https://commons.wikimedia.org/wiki/File:Anemone_virginiana_nymap.svg

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:

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

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

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

In T151735#3145267, @Reedy wrote:

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

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