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.

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

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

demon added a subscriber: demon.Apr 14 2016, 6:12 PM

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 closed this task as Resolved.
demon claimed this task.

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

Bawolff added a comment.EditedMay 9 2016, 8:09 AM

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)
csteipp reopened this task as Open.May 17 2016, 12:46 AM

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

csteipp closed this task as Resolved.May 18 2016, 12:16 AM

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.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:25 PM
Tgr added a subscriber: Tgr.Jun 10 2016, 11:23 AM

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.

Tgr added a comment.Aug 25 2016, 3:39 AM

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

Reedy added a subscriber: Reedy.Mar 19 2017, 11:52 PM

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?

Reedy added a subscriber: Anomie.Apr 7 2017, 7:22 PM

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?

Anomie added a comment.Apr 7 2017, 9:11 PM

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