Page MenuHomePhabricator

Login throttle can be tricked using non-canonicalized usernames
Closed, ResolvedPublic

Description

Well testing https://gerrit.wikimedia.org/r/#/c/270669/ (T122164), I realized that that patch actually fixes a much more serious security issue then I originally thought (One that perhaps should not have gone to gerrit. Too late now...)

Our login throttle works on the inputted username before canonicalization. That means if you try to login, all the following are considered separate usernames for the purpose of the throttle, but end up logging you in to the same account:

  • bawolff
  • Bawolff
  • _Bawolff
  • Bawolff_
  • ______________Bawolff________

etc. This allows you to bypass the throttle. The patch at https://gerrit.wikimedia.org/r/#/c/270669/ would fix this issue.

Event Timeline

Bawolff raised the priority of this task from to Needs Triage.
Bawolff updated the task description. (Show Details)
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.
csteipp triaged this task as Medium priority.Feb 16 2016, 10:14 PM
csteipp added a subscriber: csteipp.

Nice find. We'll get the public patch merged soon.

This is already merged in master. Should we maybe resolve this and add it to T124940: MediaWiki 1.26.3 security release?

This is already merged in master. Should we maybe resolve this and add it to T124940: MediaWiki 1.26.3 security release?

Yes please!

demon claimed this task.

Line $username = User::getCanonicalName( $username, 'usable' ) ?: $username; should be backported.

Line $username = User::getCanonicalName( $username, 'usable' ) ?: $username; should be backported.

I noticed actually, it wasn't using a strict comparison to false. So if someone was named User:0 people could bypass the check that's currently in master. This version also fixes that.

  • (This also needs to be applied to REL1_27)

Reopening. I'll get the updated portion of the patch deployed.

And after the SpecialUserlogin refactor with wmf2, had to patch LoginSignupSpecialPage.

This, and

were deployed today to wmf2 and wmf1 respectively.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:25 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

And after the SpecialUserlogin refactor with wmf2, had to patch LoginSignupSpecialPage.

This, and

were deployed today to wmf2 and wmf1 respectively.

What does that patch do? The username was already canonicalized before.

In any case with AuthManager enabled that code path shouldn't be used anymore.

The patch is still deployed in production but only changes wfDeprecated code paths now. Should be OK to drop.

If we don't need these anymore, we should drop them from prod

I see https://gerrit.wikimedia.org/r/#/c/270669/ on master, but it doesn't seem it was ever backported.... Do we want it backported?

T127114 says "done"...

Gerrit says

Branches	REL1_27, REL1_28, fundraising/REL1_27, master, sandbox/twentyafterfour/group0, wmf/1.29.0-wmf.10, wmf/1.29.0-wmf.11, wmf/1.29.0-wmf.12, wmf/1.29.0-wmf.13, wmf/1.29.0-wmf.14, wmf/1.29.0-wmf.15, wmf/1.29.0-wmf.16, wmf/1.29.0-wmf.2, wmf/1.29.0-wmf.3, wmf/1.29.0-wmf.4, wmf/1.29.0-wmf.5, wmf/1.29.0-wmf.6, wmf/1.29.0-wmf.7, wmf/1.29.0-wmf.8
Tags	1.27.0, 1.27.0-rc.0, 1.27.0-rc.1, 1.27.1, 1.28.0, 1.28.0-rc.0, 1.28.0-rc.1

So we just need to do REL1_23?

Has this been publicised?

The patch is still deployed in production but only changes wfDeprecated code paths now. Should be OK to drop.

@Tgr and @Anomie still agree it should go away?

Chad asked about that on IRC, I said as long as we're not seeing logspam from those wfDeprecated calls we should be good.