Special:Search allows redirects to any interwiki link
Closed, ResolvedPublic

Description

This is probably a low priority issue, and probably also reported elsewhere.

I was browsing http://cwe.mitre.org/top25/ . Noticed http://cwe.mitre.org/top25/#CWE-601 (Allowing redirects to arbitrary urls) and immediately thought of things like http://en.wikipedia.org/wiki/special:Search/google:cache:https://example.com

Probably we should include a splash page about going to an external site, when someone types in a non-local interwiki directly in the search box.

Bawolff created this task.Dec 22 2015, 2:25 PM
Bawolff updated the task description. (Show Details)
Bawolff raised the priority of this task from to Needs Triage.
Bawolff added a project: Security.
Bawolff changed the visibility from "Public (No Login Required)" to "Custom Policy".
Bawolff changed the edit policy from "All Users" to "Custom Policy".
Bawolff changed Security from None to Software security bug.
Bawolff added a subscriber: Bawolff.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 22 2015, 2:25 PM
csteipp triaged this task as Low priority.Dec 22 2015, 9:07 PM
csteipp added a subscriber: csteipp.

I guess the main bad for this would be use in a phishing attack? Anyways:

This would add a splash page anytime there's a redirect to an external interwiki that could be triggered directly from a url (Normal interwiki links where the user has to click, still work like normal).

So the patch causes Special:Search/google:foo to go to Special:GotoInterwiki/google:Foo, where the user is asked if they want to continue. Similarly for returnto parameters (T109140).

The other question is: For urls like /wiki/Google:Foo, should we continue to do Special:BadTitle, or should we also use the splash page for those? This patch leaves them alone for now, but we might want to consider making those use splash page too.

Code review:

  • Title::getFullUrlForRedirect could probably just have a single $query argument. $query2 is used in some Title functions for backwards-compatibility only.
  • GotoInterwiki is pretty inconsistent as a special page name. Mostly, "goto" is not a real word ;) so I'd rather use GoToInterwiki, or RedirectToInterwiki.
  • Repeated typo in code comments: phising → phishing
Bawolff added a comment.EditedFeb 9 2016, 10:48 PM

Version 2: {F3328566}


I forgot to git add one file in version 2. Here's version 3:

Legoktm added a subscriber: Legoktm.Aug 4 2016, 7:22 AM

I will try to review the patch soon since this has come up related to UrlShortener stuff (not a blocker, but nice to have) - {T142068}.

Bugreporter added a comment.EditedAug 4 2016, 4:42 PM

Currectly typing words into the wikipedia search box and pressing Enter take users directly to the page. To keep this feature using POST instead of GET for search box may be considered.

I will try to review the patch soon since this has come up related to UrlShortener stuff (not a blocker, but nice to have) - {T142068}.

We were actually just discussing this in the securityteam meeting. Since this issue is relatively low severity, and its kind of a big change (adds a special page) we were planning to allow this to go through normal public review process

Oh, that's even better! I'll take a look once you upload it publicly then.

Oh, that's even better! I'll take a look once you upload it publicly then.

Hmm, so patch is a little bit rotted, and being annoying to rebase, I don't think I'll be able to do that tonight, so I probably won't do it for another week as I'm going to be away all next week :(

matmarex added a subscriber: Tgr.Aug 5 2016, 2:43 PM

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 from T109140 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.

Bawolff added a subscriber: MaxSem.Aug 23 2016, 9:00 PM

Can someone add me to T109140?

This is deployed on cluster

Reedy closed this task as Resolved.Mar 30 2017, 6:00 PM
Reedy assigned this task to matmarex.
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