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).

See also T122209: Special:Search allows redirects to any interwiki link.

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.

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?

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.

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.

In T109140#2372245, @Tgr wrote:

AuthManager version:

0001-SECURITY-Don-t-redirect-to-external-sites-after-logi.patch1 KBDownload

Code-Review: +1

Rebased patch:

(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.

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:

// 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.

@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)?

• 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

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?

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).

In T109140#2870462, @Tgr wrote:

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.

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