Page MenuHomePhabricator

svg external image not placed in image tag
Closed, DeclinedPublic

Description

Author: dodgy

Description:
when adding a link to an external image from a domain that is allowed ( via http://www.mediawiki.org/wiki/Manual:$wgAllowExternalImages or http://www.mediawiki.org/wiki/Manual:$wgAllowExternalImagesFrom or http://www.mediawiki.org/wiki/Manual:$wgEnableImageWhitelist ), files with extension gif|png|jpg|jpeg result in the image being inlined, but this is not the case for SVG files.

The following fixes this:

in \includes\parser\Parser.php, line 91, 92:
const EXT_IMAGE_REGEX = '/^(http:\/\/|https:\/\/)([^][<>"\\x00-\\x20\\x7F\p{Zs}]+)

		\\/([A-Za-z0-9_.,~%\\-+&;#*?!=()@\\x80-\\xFF]+)\\.((?i)gif|png|jpg|jpeg)$/Sxu';

change that to :
const EXT_IMAGE_REGEX = '/^(http:\/\/|https:\/\/)([^][<>"\\x00-\\x20\\x7F\p{Zs}]+)

		\\/([A-Za-z0-9_.,~%\\-+&;#*?!=()@\\x80-\\xFF]+)\\.((?i)gif|png|jpg|jpeg|svg)$/Sxu';

Note the simple addition of svg.

I have done so myself in mediawiki 1.22.5, and this allows SVGs to be visible on the page, in Chrome 33.0.1750.154 m


Version: 1.22.5
Severity: normal

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:15 AM
bzimport set Reference to bz63806.
bzimport added a subscriber: Unknown Object (MLST).

Hi! Thanks for the report and investigating a fix!
You are welcome to use Developer access

https://www.mediawiki.org/wiki/Developer_access

to submit the code change as a Git branch directly into Gerrit:

https://www.mediawiki.org/wiki/Git/Tutorial

If you don't want to set up Git/Gerrit, you can also use https://tools.wmflabs.org/gerrit-patch-uploader/

Change 139680 had a related patch set uploaded by ImPacific:
fix bug 63806 (svg external image placed)

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

Change 139680 had a related patch set uploaded by TheDJ:
Recognize SVGs for External Images

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

Copy-and-paste some discussion from IRC:

cscott-free: gwicke: https://bugzilla.wikimedia.org/show_bug.cgi?id=63806 allows $wgAllowExternalImages to include .svgs as well as png/jpg/gif/etc.
cscott-free: gwicke: the patch itself looks trivial. i'm a little concerned about the possible security implications... but presumably added an appropriate RELEASE NOTES entry would be sufficient.
cscott-free: gwicke: if you've got some extra cycles to double check that and make sure i'm not missing any obvious ops/security problems i'd appreciate it.
gwicke: I'm not terribly familiar with SVG security issues
gwicke: not sure if SVGs can embed JS for example
cscott-free: i believe that they can
cscott-free: further, they can use external URLs for resources, which is enough to do port scanning, etc
gwicke: that could be an invitation to steal cookies by external-linking to a prepared SVG image original hosted on the same wiki
cscott-free: right. but the owner of the wiki presumably has added an explicit whitelist, so trusts the site.
cscott-free: which mitigates the threat to some degree.
cscott-free: the only new security issue is that the owner of the wiki might have some vulnerable svgs sitting around, which they didn't audit because they weren't previously linkable. or something like that.
cscott-free: but if you've enabled $wgAllowExternalImages you're asking for trouble.
gwicke: normally that's fairly harmless
gwicke: it's different with SVGs though
gwicke: could be at least
cscott-free: yeah. what's your intuition there? just release-note it? or is this different-enough trouble that there should be an explicit $wgAllowExternalImagesYesSVGsToo opt-in?
gwicke: so SVG does support onmouseover etc
cscott-free: yeah, SVG supports full HTML embedding as well, although i don't think support is present in all browsers. so basically anything you can do in raw HTML embedded in the page you can do in an embedded SVG.
gwicke: I don't think that just adding them without investigating the security implications more deeply would be a good idea
cscott-free: i'm going to suggest to the author that they add an opt-in variable, then.
cscott-free: it makes the patch more complicated than just adding '|svg' to a regexp, but better safe than sorry.
gwicke: yeah, that sounds like a better approach
cscott-free: thanks

To recap: we think that, because there are security implications, we shouldn't allow .svgs as external images unless the wiki owner has explicitly set a configuration variable to opt-in to this behavior.

The new variable should also be mentioned in the RELEASE NOTES.

Hopefully these changes won't be too difficult to implement. Thanks for your contribution so far!

(In reply to C. Scott Ananian from comment #4)

To recap: we think that, because there are security implications, we
shouldn't allow .svgs as external images unless the wiki owner has
explicitly set a configuration variable to opt-in to this behavior.

The new variable should also be mentioned in the RELEASE NOTES.

My initial thought is to agree with gwicke, and this should have it's own flag. The SOP of javascript in svg files is a little murky, and each browser has their own way of implementing controls around it.

I'd rather be safe and realize a year from now we can combine the flags than suddenly put everyone using the existing functionality at risk.

thanks all for the feedback here and at gerrit. I was not aware of the potential risks involved with SVGs. I Will try my hand at implementing the configuration variable if possible :)

Change 184337 had a related patch set uploaded (by ImPacific):
Introducing a new variable $wgAllowExternalSVG

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

Patch-For-Review

Change 139680 had a related patch set uploaded (by Paladox):
Allow external SVGs to be included from whitelisted sites

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

Change 472757 had a related patch set uploaded (by Mustaq Ahammad; owner: Mustaq Ahammad):
[mediawiki/core@master] T65806 FIXED

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

Change 139680 abandoned by Thiemo Kreuz (WMDE):
Allow external SVGs to be included from whitelisted sites

Reason:
This is not going to happen, at least not the way this patch proposes. See discussion in T65806.

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

Change 184337 abandoned by Thiemo Kreuz (WMDE):
Introducing a new variable $wgAllowExternalSVG

Reason:
No action for about 5 years. This patch is not only incomplete, it breaks existing functionality. If needed, the patch can still be found from the Phabricator ticket T65806.

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

thiemowmde subscribed.

Not going to happen because of the security risk, not even as a separate configuration flag.

thiemowmde raised the priority of this task from Medium to Needs Triage.