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 ) )
){
...
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T122653 SVG validation should allow relative urls in <a> links in uploaded SVG files | |||
| Resolved | • csteipp | T123071 Incorrectly identifying http link in a's href attributes, due to m modifier in regex |
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.
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
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.
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.
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.
Shouldn't we handle internal links the same way as in wikipedia?
Lets upload svg containing <text>[[Wikipedia]]</text>
Wikimedia Commons should deliver compiled version containing <text><tspan xlink:href="https://de.wikipedia.org/wiki/Wikipedia"><a xlink:href="https://de.wikipedia.org/wiki/Wikipedia">Wikipedia</a></tspan></text> per default and offer source for download in addition.
Next step, let's say in 10 years time, would be to enable wikipedia to show graphical html5 content.