Page MenuHomePhabricator
Create Task
Maniphest T127114

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

Assigned To
demon
Authored By
Bawolff
Feb 16 2016, 9:05 PM
Tags
Referenced Files
F4025475: T127114-master_1.28wmf2.patch
May 18 2016, 12:16 AM
F3985249: T127114-REL1_26.patch
May 9 2016, 8:09 AM
F3985229: T127114-REL1_23.patch
May 9 2016, 8:09 AM
F3985246: T127114-REL1_25.patch
May 9 2016, 8:09 AM
F3985252: T127114-master.patch
May 9 2016, 8:09 AM
Subscribers
Aklapper
Bawolff
csteipp
demon
Malyacko
Reedy
Tgr

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.

Related Objects
Search...

Event Timeline

Bawolff created this task.Feb 16 2016, 9:05 PM
Bawolff raised the priority of this task from to Needs Triage.
Bawolff updated the task description. (Show Details)
Bawolff added projects: Security-Core, Vuln-Authn/Session, acl*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 TranscriptFeb 16 2016, 9:05 PM
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.

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?

csteipp added a comment.Apr 14 2016, 6:47 PM
In T127114#2207786, @demon wrote:

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.Apr 14 2016, 6:51 PM
demon claimed this task.
demon added a parent task: T124940: MediaWiki 1.26.3 security release.
demon mentioned this in T124940: MediaWiki 1.26.3 security release.Apr 20 2016, 3:09 PM
csteipp added a comment.May 5 2016, 9:07 PM

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

Bawolff added a comment.EditedMay 9 2016, 8:09 AM
In T127114#2268483, @csteipp wrote:

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.

  • T127114-REL1_23.patch1 KBDownload
  • T127114-REL1_25.patch1 KBDownload
  • T127114-REL1_26.patch1 KBDownload
  • T127114-master.patch1 KBDownload
    (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.

T127114-master_1.28wmf2.patch1 KBDownload

This, and

T127114-master.patch1 KBDownload
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
In T127114#2303595, @csteipp wrote:

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

T127114-master_1.28wmf2.patch1 KBDownload

This, and

T127114-master.patch1 KBDownload
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.

demon mentioned this in T140591: MediaWiki 1.28.1/1.27.2/1.23.16 security release.Jan 24 2017, 11:40 PM
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
In T127114#2581251, @Tgr wrote:

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.

• chasemp added a project: Security.Feb 10 2020, 10:59 PM
• chasemp removed a project: acl*security.Feb 20 2020, 8:17 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:42 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL