Page MenuHomePhabricator

Open Redirect in secure.wikimedia.org
Closed, ResolvedPublic

Description

The following URL will redirect to https://example.com/page.html:

https://secure.wikimedia.org/wikipedia/example.com%5C/..%5Cpage.html

This works because most major browsers interpret \ (%5C) as a forward slash in the URL. The redirect tries to visit this url:

https://example.com\.wikipedia.org/..\page.html

which the browser then interprets as:

https://example.com/page.html

The security impact of this bug is minor, but it could be used for misleading users into thinking a malicious link is genuine.

Event Timeline

Retr0id created this task.Nov 30 2016, 3:04 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 30 2016, 3:04 AM
Krenair moved this task from Backlog / Other to Operational issues on the acl*security board.
Krenair added a subscriber: Krenair.
alex@alex-laptop:~$ curl -vvv "https://secure.wikimedia.org/wikipedia/example.com%5C/..%5Cpage.html" 2>&1 | grep Location:
< Location: https://example.com\.wikipedia.org/..\page.html

From modules/mediawiki/files/apache/sites/secure.wikimedia.conf in puppet, the rewrites which allow users to determine destination domain:

  • RewriteRule ^/wikipedia/(advisory|auditcom|boardgovcom|board|chair|chapcom|checkuser|collab|commons|donate|exec|grants|incubator|internal|meta|movementroles|office|otrs-wiki|outreach|quality|searchcom|spcom|species|steward|strategy|usability|wikimania\d+|wikimaniateam)/(.*)$ https://$1.wikimedia.org/$2 [R=301,L,NE]
  • RewriteRule ^/(wikipedia|wikinews|wikisource|wikibooks|wikiquote|wikiversity|wiktionary|wikimedia)/([^@:/]+)/(.*)$ https://$2.$1.org/$3 [R=301,L,NE]

First one looks fine, second one causes this.

So [^@:/]+ could be changed to also exclude %5C perhaps?

Dzahn added a subscriber: Dzahn.Nov 30 2016, 6:02 PM

fwiw, i don't get redirected by that URL, instead i get:

Corrupted Content Error

The page you are trying to view cannot be shown because an error in the data transmission was detected.

Please contact the website owners to inform them of this problem.

firefox-esr 45.4.0esr-2 (Debian stretch)

MaxSem added a subscriber: MaxSem.Nov 30 2016, 10:04 PM

Works for me in Chrome but not Safari.

So [^@:/]+ could be changed to also exclude %5C perhaps?

Perhaps [a-z0-9]+ would be a more defensive solution, since these are the only legal subdomain characters? ("-" is also allowed, but I think it's unlikely to be needed)

Perhaps [a-z0-9]+ would be a more defensive solution, since these are the only legal subdomain characters? ("-" is also allowed, but I think it's unlikely to be needed)

We have hostnames with dashes, e.g. https://zh-min-nan.wikipedia.org or https://otrs-wiki.wikimedia.org

fgiunchedi triaged this task as High priority.Dec 1 2016, 12:09 AM
fgiunchedi added a subscriber: fgiunchedi.

Is secure.w.o even used for anything substantial these days with https everywhere?
I'd exclude . from $2 so it effectively limits to only third-level subdomains

Secure is used only to preserve old incoming links, unscientific poking indicates hundreds of requests per hour. I fixed a couple of popular Stack Overflow instances, but overall this host isn't going anywhere for a long time. Sigh.

Platonides added a subscriber: Platonides.EditedApr 26 2017, 11:18 PM

Change $2 to [a-z0-9]+

Only one subdomain level is allowed, and it becomes case-sensitive.

This patch looks ok to me. @Platonides can you push that to gerrit please?

herron changed the task status from Open to Stalled.Aug 24 2017, 3:19 PM
herron lowered the priority of this task from High to Medium.

This patch looks ok to me. @Platonides can you push that to gerrit please?

I'd be happy to push (git review -R) Platonides' patch which I have here as a local commit (with Platonides set as author).
Request: However, this task is not public while the patch in Gerrit would be public, so I am asking for an explicit OK before I will push.

We should really have someone from ops on standby ready to merge and deploy
as soon as it reaches gerrit.
Or, maybe have ops apply it directly on the puppetmasters and then update
the gerrit copy of the repo afterwards.

(that's assuming it's a puppet patch and not mediawiki-config or something,
I don't remember the details of how secure.wm.o works. I can't view that
attachment from the email chain, and I can't log into Phabricator due to my
2FA device breaking - planning on asking for that to be removed from my
account while in Barcelona)

Reedy merged a task: Restricted Task.Apr 6 2020, 7:00 PM
Reedy added a project: Traffic.
Reedy added subscribers: Colbygk, Reedy.
sbassett moved this task from Incoming to Watching on the Security-Team board.
ema moved this task from Triage to Watching on the Traffic board.May 12 2020, 7:33 AM
Colbygk added a comment.EditedJul 22 2020, 1:41 PM

Bump,

Still an issue for community curated/moderated websites such as https://scratch.mit.edu, we're going to have to ~add wikimedia.org/ to our block list~ remove wikimedia.org from our allow list.

Reedy added a comment.Jul 22 2020, 2:48 PM

Bump,

Still an issue for community curated/moderated websites such as https://scratch.mit.edu, we're going to have to add wikimedia.org/ to our block list.

Can't you just add secure.wikimedia.org?

While it's obviously not an excuse, are people actually abusing this vector?

I believe we are allowing *.wikimedia.org, as we default to an allow list vs a block list (which would be impossible to maintain with our resources). But we could potentially make an exception...

Yes, it is actively being used as a way to get around our allowlist.

CDanis added a subscriber: CDanis.Jul 23 2020, 4:22 PM

I applied @Platonides 's patch on mwdebug1002, tested it using httpbb, fixed a small error (character class was negated when it shouldn't've been), and it's now merged and being deployed.

sbassett closed this task as Resolved.Jul 23 2020, 4:52 PM
sbassett assigned this task to CDanis.
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.
sbassett added a subscriber: sbassett.

Thanks, @CDanis. Looks to be fixed. Resolving and making public.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 23 2020, 4:52 PM

Change 615799 merged by CDanis:
[operations/puppet@production] ATS: force cache revalidation on secure.wm.o

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

ArielGlenn reopened this task as Open.Jul 23 2020, 5:11 PM

Almost resolved, heh.

CDanis closed this task as Resolved.EditedJul 23 2020, 5:16 PM

Re-validation forced for ATS-BE, and also a Varnish cache ban has been put in place, so we should no longer be serving any cached 301s.

https://gerrit.wikimedia.org/r/c/operations/puppet/+/615799 can reverted after 24 hours.

Change 616493 had a related patch set uploaded (by CDanis; owner: CDanis):
[operations/puppet@production] Revert "ATS: force cache revalidation on secure.wm.o"

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

Change 616493 merged by Ema:
[operations/puppet@production] Revert "ATS: force cache revalidation on secure.wm.o"

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