Special:UserLogin?returnto=interwiki:foo will redirect to external sites
Closed, ResolvedPublic

Description

As a logged in user, https://en.wikipedia.org/w/index.php?title=Special:UserLogin&returnto=google:foo will redirect you to a Google search for "foo".

And if you're not logged in, it will redirect you to Google after you finish logging in.

Discovered by @valhallasw.

Legoktm created this task.Aug 14 2015, 9:35 PM
Legoktm updated the task description. (Show Details)
Legoktm raised the priority of this task from to Needs Triage.
Legoktm changed the visibility from "Public (No Login Required)" to "Custom Policy".
Legoktm changed the edit policy from "All Users" to "Custom Policy".
Legoktm changed Security from None to Software security bug.
Legoktm added subscribers: Legoktm, valhallasw.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 14 2015, 9:35 PM

dpatrick triaged this task as Low priority.Aug 18 2015, 9:59 PM

Is redirecting to external sites considered a security issue?

Its well known that Special:Search/google:foo will do the same thing (To the point where our old bugzilla install relied on this behaviour)

Hmm, I wasn't aware of that. I was mainly thinking in the context of T108557 - UrlShortener has a whitelist of accepted domains, but that becomes less useful if MW will just redirect you to external sites anyways (albeit a small whitelisted list of domains).

My only concern with this would be if there are any remaining browsers that preserve the HTTP method with 302 response codes. IE will only do so with non-POST/HEAD methods like DELETE and PUT, so there's nothing to worry about there.

But if a browser does treat 302 like 307, which it might since technically it's standards-compliant, it may try and re-POST the user's login data to whatever the interwiki link destination is.

My recommendation would be that we detect if the connection is HTTP/1.1, and if so explicitly use a 303 redirect to ensure login data is not leaked to the site (even if most browsers do not have this issue).

At the very least I think this should only redirect for interwikis with iw_local = true.

I have a patch primarily for T122209, but it also addresses this bug.

It bans external interwiki links in link based returnto parameters. For direct redirects, it puts a splash page before fully redirecting the user. (So its slightly different from Lego's patch).

I think people are depending on the redirect behaviour for Special:Search, hence the splash page. In the case of returnto, I think either the splash page, or just banning entirely (like Lego's patch), is reasonable.

csteipp added a subscriber: csteipp.Feb 9 2016, 8:46 PM

Doing the full splash-screen as a security patch makes me a little uncomfortable, but I think the direction generally looks good.

Should we deploy lego's original patch to stop external interwiki links from redirecting as a security patch, then do the splash redirect as a public followup?

Bawolff added a comment.EditedFeb 9 2016, 10:46 PM

Version 2 of this patch (per MatmaRex's comments):
{F3328566}

I agree that this is not something I'd prefer to be a security patch.


version 2 had a small mistake. Here is version 3:

Deployed lego's initial patch for Special:Userlogin,

19:17 csteipp: deployed initial patch for T109140

Not sure if we should wait for a tarball release before publicly doing the splash-screen patch.

Tgr added a subscriber: Tgr.Jun 10 2016, 4:48 PM

AuthManager version:

Apologies for missing this earlier. I glanced at the deployed patch but the commit summary talks about a &redirectto= parameter so I assumed it wasn't handled by the login page.

Anomie added a subscriber: Anomie.Jun 10 2016, 6:00 PM

AuthManager version:

Code-Review: +1

Bawolff lowered the priority of this task from Low to Lowest.
Bawolff raised the priority of this task from Lowest to Low.

Rebased patch:

(applies on d181ba1a9b00e7b7e418b82e0b0def3cc184e48d).

(I applied @Bawolff F3328788 on top of 8aa6c9f43e9b54e4157a5feee42229a9a48b1764, then rebased. Most conflicts looked scary but were actually simple. One interesting one was that the change in SpecialChangePassword now needed to be applied in SpecialChangeCredentials instead.)

Also included @Tgr's fix for AuthManager (slightly modified).

This needs to be tested carefully again, since I only fixed the obvious merge errors, and only checked the functionality briefly.

hashar added a subscriber: hashar.Aug 23 2016, 12:15 PM

The patch on tin /srv/patches/1.28.0-wmf.16/core/01-T109140.patch no more apply since the special page apparently got removed:

hashar@tin:/srv/mediawiki-staging/php-1.28.0-wmf.16$ git apply --check --3way /srv/patches/1.28.0-wmf.16/core/01-T109140.patch
error: includes/specials/pre-authmanager/SpecialUserlogin.php: does not exist in index

The includes/specials/pre-authmanager has been removed on August 9th via 854a462dc0aef59e1c26c057792b8a9214449af5

So I am tempted to assume that /srv/patches/1.28.0-wmf.16/core/01-T109140.patch is no more needed?

Yes, that part of the patch (for the removed old special page) is no longer needed. Everything else still is.

Hmm, the patch doesn't seem to be live in production right now, though? Visiting https://en.wikipedia.org/wiki/Special:UserLogin?returnto=google:foo redirects me to Google. And I think someone said somewhere that we want to review the patch publicly in Gerrit.

Same issue in 1.28.0-wmf.15:

$ git apply --check --3way /srv/patches/1.28.0-wmf.15/core/01-T109140.patch
error: includes/specials/pre-authmanager/SpecialUserlogin.php: does not exist in index

So I guess when we have prepared wmf.15 last week, the patch got skipped :(

wmf.16 is on group0 and group1 now. The external interwiki redirect is present on both wmf.15 (eg enwiki) and wmf.16 (eg mediawikiwiki).

https://en.wikipedia.org/wiki/Special:UserLogin?returnto=google:foo
https://www.mediawiki.org/wiki/Special:UserLogin?returnto=google:foo

Patch for wmf.16 is /srv/patches/1.28.0-wmf.16/core/01-T109140.patch and touch the no more existing file includes/specials/pre-authmanager/SpecialUserlogin.php. Looks like the check has to be moved to includes/specials/helpers/LoginHelper.php:

includes/specials/helpers/LoginHelper.php
// Allow modification of redirect behavior
Hooks::run( 'PostLoginRedirect', [ &$returnTo, &$returnToQuery, &$type ] );

$returnToTitle = Title::newFromText( $returnTo ) ?:  Title::newMainPage();

That needs the extra condition $returnToTitle->isExternal() to send back to the main page.

I do not want to mess up with login, I am long outdated on MediaWiki code development. Also looks like T122209 which is handling returnto parameter for Search could benefit from a similar fix. Ideally we would have some kind of helper function to handle titles passed to returnto parameters and automagically fallback to MainPage when the URL turns out to be external.

greg added a subscriber: greg.Aug 29 2016, 8:59 PM

@hashar you marked this as a blocker to wmf.16. You start rolling out wmf.17 tomorrow: what should happen? Should we remove this as a blocker to wmf.16? add it as a blocker to wmf.17? Actually block wmf.17 with it (which the previous choice presumes)?

Tgr added a comment.Aug 30 2016, 6:18 AM


Rebased (and fixed a typo in LoginHelper), reviewed, tested. CR+2, does what it intends to.
Some minor quibbles that are probably more convenient to handle once the patch is public:

  • should this be applied to OutputPage::addReturnTo?
  • use PROTO_HTTPS by default, safer and less trouble with caching
  • bunch of incorrect capitalizations in Title::getFullUrlForRedirect (also, getFullURL vs. getFullUrlForRedirect, although I do prefer the latter personally)
  • GoToInterwiki should probably be a hidden special page?
  • not really needed for Preferences and PageLanguage
Tgr added a comment.EditedAug 30 2016, 6:18 AM

oops, Phabricator handles network connection issues poorly. Removed duplicate.

THANK YOU @Tgr !!!

I have grabbed your attachment on tin and saved it as:

/srv/patches/1.28.0-wmf.17/core/01-T109140-rebased.patch

The old one that no more apply is kept behind as 01-T109140.patch-OLD.

That is going to land on testwiki/mediawiki tonight as I push wmf.17 to group0.

With wmf.17 https://www.mediawiki.org/wiki/Special:UserLogin?returnto=google:foo gives me the splash screen. Kudos that looks great.

@dpatrick a good candidate for making non-secret IMO.

Note to self, there is a similar issues in CentralAuth.

The patch no longer applies as of 1.29.0-wmf.9

Hey @Tgr, can you rebase this patch for 1.29.0-wmf.6?

Tgr added a comment.Dec 13 2016, 8:45 PM

Rebased version in /srv/patches/1.29.0-wmf.6/core/01-T109140.patch (it was just an i18n conflict).
FWIW I think it's not a good trade-off to kick these low-severity patches forward in secret for months (not so much because of the occasional need for rebase as for MediaWiki for third parties not getting the fix for a long time).

FWIW I think it's not a good trade-off to kick these low-severity patches forward in secret for months (not so much because of the occasional need for rebase as for MediaWiki for third parties not getting the fix for a long time).

+1

Rebased version in /srv/patches/1.29.0-wmf.9/core/01-T109140.patch. The patch mis-applied due to a refactoring of the method it changes and caused fatals. I've updated both wmf.9 on tin and synced out, along with fixing up the patch file.

Reedy closed this task as Resolved.Mar 30 2017, 5:59 PM
Reedy assigned this task to Tgr.
Reedy added a subscriber: Reedy.

Closing for ease of tracking progress. Patches attached to parent bug, due for next release

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 6 2017, 8:56 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".

Change 346839 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Do not directly redirect to interwikis, but use splash page

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

Change 346848 merged by jenkins-bot:
[mediawiki/core@REL1_27] SECURITY: Do not directly redirect to interwikis, but use splash page

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

Change 346858 merged by jenkins-bot:
[mediawiki/core@REL1_28] SECURITY: Do not directly redirect to interwikis, but use splash page

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

Change 347018 had a related patch set uploaded (by Chad; owner: Brian Wolff):
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: Do not directly redirect to interwikis, but use splash page

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

Change 347018 merged by jenkins-bot:
[mediawiki/core@wmf/1.29.0-wmf.19] SECURITY: Do not directly redirect to interwikis, but use splash page

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