Careless use of $wgExternalLinkTarget is insecure
Closed, ResolvedPublic

Description

Introduced in r41333, $wgExternalLinkTarget allows MediaWiki operators to set the target attribute on external links. The documented use-case is to set $wgExternalLinkTarget = '_blank';, so links open in a new window or tab:

includes/DefaultSettings.php L4205-4208
/**
 * Set a default target for external links, e.g. _blank to pop up a new window
 */
$wgExternalLinkTarget = false;

The problem is that when you click on a target="_blank" link, JavaScript code on the destination page has full control of the window object of the source page, via window.opener. In the event the page is cross-origin, the new window is allowed to set window.opener.location to a new value.

window.opener.document is protected by CORS, but window.opener.location is not, allowing the target page to surreptitiously redirect the tab that opened it to a phishing page.

There is a good explanation of this issue, with working examples, at https://mathiasbynens.github.io/rel-noopener/

This is a good reason never to use target="_blank" with user-generated links.

Should we prevent users from shooting themselves in the foot by deprecating and refusing to honor $wgExternalLinkTarget when it is set to "_blank"? (This article suggests most people who think they want to use _blank shouldn't.)

At minimum, I think we should:

  • Update the comment to make it clear that this is risky.
  • Emit a warning when the configured value is unsafe.
ori created this task.Apr 24 2016, 8:57 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 24 2016, 8:57 PM
ori changed the title from "Careless use of $wgExternalLinkTarget can make wiki vulnerable to XSS" to "Careless use of $wgExternalLinkTarget is insecure".Apr 24 2016, 8:57 PM

JavaScript code on the destination page has full control of the window object of the source page

Where "full control" means that it can navigate it, and that's basically all. It can't even read the location (so not a leak for private wikis), only set it.

Also, this is not specific to target="_blank", it works the same for any value of target (except _self and maybe some other magic ones, I suppose).

I dunno, I think we could just set rel="noopener" on external links and document this.

It can't even read the location (so not a leak for private wikis)

Well generally speaking all pages can read the location by looking at the referrer headers, but that's nothing new.

Its is a bizarre feature of the web browser security model though... At least javascript: urls do not seem to be allowed cross-origin.

I dunno, I think we could just set rel="noopener" on external links and document this.

That sounds like a reasonable response to me.

Grunny added a subscriber: Grunny.Apr 25 2016, 9:24 AM

I dunno, I think we could just set rel="noopener" on external links and document this.

That sounds like a reasonable response to me.

Just something to keep in mind, rel="noopener" only works on Chrome and Opera, and does not work in Firefox, Safari, IE, and Edge.

This would also probably affect anyone with the exlinks Gadget enabled on Wikimedia wikis: https://en.wikipedia.org/wiki/MediaWiki:Gadget-exlinks.js

So if we still want to allow taregt="_blank" generally, and try to mitigate via rel="noopener noreferrer", here's a patch

[To clarify, this shouldn't be taken as me trying to force that approach. I think which approach to take (ie whether to allow _blank at all) is still something that should be considered "up in the air"]

See also T98013 where we set target to be _blank on votewiki

Bawolff edited the task description. (Show Details)Apr 26 2016, 9:10 PM
csteipp triaged this task as "Normal" priority.Apr 26 2016, 9:25 PM

New version based on csteipp's CR:

Also, I know this is a little late, but I was kind of hoping this could maybe get into security release, since the window.opener thing is trending in the blogosphere, and was at the top of HN the other day. With that in mind, here are backports:

csteipp added a subscriber: csteipp.May 5 2016, 6:43 PM

New version based on csteipp's CR:
T133507-master

LGTM, I'm going to deploy this

csteipp closed this task as "Resolved".May 5 2016, 6:48 PM
csteipp claimed this task.

Deployed

18:47 csteipp: deployed patch for T133507

New version with [SECURITY] prefixed to the subject line:

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:28 PM
demon changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:28 PM