Page MenuHomePhabricator

Very long usernames can cause an infinite loop when loading Special:GlobalRenameRequest (CVE-2021-36125)
Closed, ResolvedPublicSecurity

Description

Special:GlobalRenameRequest helpfully "suggests" a new username, which is the user's current username followed by three random characters:

		do {
			$rand = $this->getUser()->getName() . rand( 123, 999 );
		} while ( !GlobalRenameRequest::isNameAvailable( $rand )->isOK() );

If the user's current username is constructed such that none of those usernames can possibly pass isNameAvailable, this will loop forever. Presumably, this consumes 100% of a CPU core until the request times out.

I considered registering all of User:ST47123 .. User:ST47999, but I'm lazy and there is an easier way:

The new username must pass all of the tests in UserNameUtils.php, including that the new username must not be longer than MaxNameChars (which is 85 on WMF). If the current username is at least 83 characters long, this will loop forever. With sufficient threads, this could very easily DoS the site.

I would suggest removing this "suggested username" feature from GlobalRenameRequest entirely, as adding three digits to the end of the username isn't a very good suggestion anyway.

If a traceback would be helpful, you can find one here: [fc0657b2-77c2-41eb-9996-ce38f062394a] 2020-08-20 02:27:10: Fatal exception of type "WMFTimeoutException"

Event Timeline

Legoktm added a subscriber: Legoktm.

I think the easiest fix is to just add a counter and stop after say 5 tries.

...as adding three digits to the end of the username isn't a very good suggestion anyway.

Why not?

Legoktm added a subscriber: bd808.

Here's a quick patch:

...as adding three digits to the end of the username isn't a very good suggestion anyway.

Why not?

It doesn't really matter, but if someone is going to request a global rename, their intent is probably not to just add a few digits to the end of their current name. I think most global renames are done either to correct problems with a project's username policy (such as usernames that represent a company), or to remove someone's real name from their username, which "just stick 123 on the end" doesn't really help with.

Here's a quick patch:

LGTM. I should be trouted for writing that original loop.

It doesn't really matter, but if someone is going to request a global rename, their intent is probably not to just add a few digits to the end of their current name.

This logic is from the time of SUL unification and the "add a random suffix" bit was mostly about making things slightly easier for folks who "lost" their local username due to unification. I think I would agree that it is of low value in any wiki farm that did not have a unification step to pass through. And it should be mostly useless today in the Wikimedia farm. Which I guess is a long way of saying I would not be personally sad for the fix to be a simpler version of @Legoktm's patch that just drops the suggestion part entirely.

I would not be personally sad for the fix to be a simpler version of @Legoktm's patch that just drops the suggestion part entirely.

Any opinions whether to simplify the patch, and/or anyone willing to get that patch merged?

Rebased and deployed the above security patch (T260865#6398950) to wmf.5. Logs seems fine. Will also track at T279733.

sbassett lowered the priority of this task from High to Low.Jul 1 2021, 9:51 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett removed a project: Patch-For-Review.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

Change 702757 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/extensions/CentralAuth@master] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

Change 702715 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/extensions/CentralAuth@REL1_36] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

Change 702716 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/extensions/CentralAuth@REL1_35] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

Change 702757 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

Change 702716 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@REL1_35] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

Change 702715 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@REL1_36] SECURITY: GlobalRename: Avoid DoS/infinite loop in suggested username feature

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

sbassett renamed this task from Very long usernames can cause an infinite loop when loading Special:GlobalRenameRequest to Very long usernames can cause an infinite loop when loading Special:GlobalRenameRequest (CVE-2021-36125).Jul 2 2021, 8:01 PM
sbassett assigned this task to Legoktm.
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.