Page MenuHomePhabricator

SVG validation should allow relative urls in <a> links in uploaded SVG files
Open, LowPublic

Description

https://github.com/wikimedia/mediawiki/blob/master/includes/upload/UploadBase.php#L1402
The bug with HREF validation in svg.
as described "# href with non-local target (don't allow http://, javascript:, etc)" it should filter out all non-local links, but it allows it.

How to fix: remove negation

if ($strippedElement === 'a'
&& preg_match( '!^https?://!im', $value ) )
){
...

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Dec 30 2015, 3:26 PM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 30 2015, 3:26 PM
Nox86 created this task.Dec 30 2015, 3:26 PM
Nox86 renamed this task from Wrong 'uploaded-href-attribute-svg' rule to Wrong 'uploaded-href-attribute-svg' rule, allow non-local links in svg.
Nox86 triaged this task as Unbreak Now! priority.
Nox86 updated the task description. (Show Details)
Nox86 added projects: Wikimedia-Blog, Security.
Nox86 changed Security from None to Software security bug.
Nox86 edited subscribers, added: Nox86; removed: Aklapper.
Nox86 renamed this task from Wrong 'uploaded-href-attribute-svg' rule, allow non-local links in svg to Wrong 'uploaded-href-attribute-svg' rule, non-local links in svg are allowed.Dec 30 2015, 3:35 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptDec 30 2015, 4:03 PM
Bawolff lowered the priority of this task from Unbreak Now! to High.Dec 30 2015, 7:58 PM
Bawolff added a subscriber: Bawolff.

Thank you for taking the time to report this.

Im not really near a real computer right now, so I just quickly looked at code on my phone (so i might be missing something).

I think the code is correct but the comment is misleading. I assume we would want to allow non local clickable links. We just want to ban loading non local sub resources. (e.g. xlink:href is ok on <a>. Its not ok on <image>)

That said, the /m flag looks wrong on the regex. What if someone starts with javascript:, adds a newline in the attribute, and then starts a line with http://?

e

Correct, external (clickable) links are allowed. The sanitization should prevent any automatic loading of resources from external domains.

@Bawolff, I think you're correct about the m. That looks like a bug to me too.

Nox86 added a comment.EditedDec 30 2015, 9:32 PM

sorry I put incomplete info, here the example:
The svg below is uploadable, but it shouldn't (?) as well as javascript shouldn't be allowed.

<svg>
  <a href="http://example.com"><circle cx="50" cy="50" r="40"/></a>
</svg>

The good case with relative link to the wiki (it is hightly useful to have for exported block diagrams)

<svg>
  <a href="my_local_link"><circle cx="50" cy="50" r="40"/></a>
</svg>

Please leave the ability to have local links in the svg. We use it for documents exported from Visio. It is the only way we can integrate those diagrams with wikimedia!
Thanks

Nox86 added a comment.Jan 7 2016, 6:29 PM

don't know where I should comment (here or T123071), so sorry if it is wrong

It still don't allow local target:

let`s assume $strippedElement is a and (href) $value is some_local_target

$strippedElement === 'a' is true and preg_match( '!^https?://!i', $value ) is false, then

if not (true and false) then show error 'uploaded-href-attribute-svg'

Is it so hard guys?

From my experience with similar issues elsewhere in the codebase, yes, it is pretty damn hard. Basically impossible. It's very hard to tell whether, say, <a href="ja\tva\nSCRIPT : foo"> is a perfectly fine innocent local link, or a security issue waiting to happen. Various browsers have slightly differing rules for deciding whether a protocol is valid or not.

I think it's very sensible here that we do not allow local links. It's just too difficult to filter out the bad ones. External links to known protocols (like http and https) are generally fine (or at least, not a security issue).

We should, however, be able to allow local links starting with / or ./.

I think there's something of a philosophical debate that we would need to have publicly around local paths on WMF projects too, since you're making it very hard to reuse the image on another domain if you're referencing local paths.

That said, let's get the followup xss addressed, and then we can make this bug public and decide if this feature is wanted.

Nox86 added a comment.Jan 8 2016, 10:34 AM

Would it be better if wikimedia on upload just generate a warning "svg contains <a> tag" + an option to enable custom filter?
I think svg upload is responsibility of wikimedia admin\owner, not wikimedia developer.

Please remembe, that svg is the only way we can integrate Visio diagrams with wikimedia.

As for now even simple case doesn't work:

<svg>
  <a href="my_local_link"><circle cx="50" cy="50" r="40"/></a>
</svg>

@Nox86, it is the responsibility of the developers to make sure scripts can't be uploaded in svg files, and your initial recommendation (removing the negation) would allow uploading svg's with scripts (<svg><a href='javascript:alert(1)'><circle cx="50" cy="50" r="40"/></a></svg>).

The comment at https://github.com/wikimedia/mediawiki/blob/master/includes/upload/UploadBase.php#L1397 should be changed, so that it doesn't say http:// isn't allowed. Those are uploaded regularly on commons, and removing that would require community consensus.

Adding local links could happen with community consensus, although there would need to be more strict rules than what you've proposed. The url would have to start with a / or ./, to prevent browsers from interpreting the link as a harmful protocol, just as "javascript:". But like I said before, that's also a philosophical debate, because then an svg that works on commons won't work the same if it's shared on another site.

If you're proposing this change for a non-WMF site, I'd be happy to work with you on a patch to control this behavior based on a config flag.

I submitted https://gerrit.wikimedia.org/r/267545 to correct the wrong localisation message which claims the opposite of the truth here.

Bawolff lowered the priority of this task from High to Low.Apr 17 2016, 10:44 AM

https://gerrit.wikimedia.org/r/296195 for the code comment.

I don't really think this is a security issue, more a political one (Should we allow local links in svgs). I'm inclined to say no (SVGs uploaded to MW are generally assumed to be self-contained). Thus I'm moving this to the media handling component, and making it not security.

I'm not opposed to making this configurable if people really want this.

Bawolff renamed this task from Wrong 'uploaded-href-attribute-svg' rule, non-local links in svg are allowed to SVG validation should allow relative urls in <a> links in uploaded SVG files.Jun 27 2016, 9:52 AM
Bawolff removed a project: Security.
Bawolff changed Security from Software security bug to None.
Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".
Bawolff changed the edit policy from "Custom Policy" to "All Users".
Restricted Application added subscribers: Steinsplitter, Matanya. · View Herald TranscriptJun 27 2016, 9:52 AM
MarkTraceur moved this task from Untriaged to Triaged on the Multimedia board.Dec 6 2016, 4:16 PM