Page MenuHomePhabricator

Error while usurping an account
Closed, ResolvedPublicSecurity

Description

When trying to perform a second rename during usurpation, I frequently get an error saying "There are unattached accounts using the requested username." even when first rename has completed successfully and Special:GlobalRenameProgress show no unattached accounts.
Although this usually gets resolved in few hours but is really getting time consuming. Earlier this was not the case and the second rename was quick—right after the first rename.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Right now it can be seen on https://en.wikipedia.org/wiki/Wikipedia:Changing_username/Usurpations (first two onhold requests).

First rename has been completed, so just click "rename user" and try to do second rename.

Right now it can be seen on https://en.wikipedia.org/wiki/Wikipedia:Changing_username/Usurpations (first two onhold requests).

First rename has been completed, so just click "rename user" and try to do second rename.

Error on the first request gone, but second still have it.

Gone on 2nd too, but this time in between is the issue.

It's been ~9 hours but still it is stuck. I think we won't be able to proceed usurp requests before this gets resolved.

Gone but it took more than 12 hours.

Debugging this a little. The rename error is centralauth-rename-unattached-intheway meaning the localusers or localnames are not updated in centralauth db. I renamed a user (وی همچنین خاطرنشان کرد one of my test socks) and everything sorta works except localnames part:

wikiadmin@10.64.48.56(centralauth)> select * from localnames where ln_name = 'وی همچنین خاطرنشان کرد';
+---------------+-------------------------------------------+
| ln_wiki       | ln_name                                   |
+---------------+-------------------------------------------+
| enwiki        | وی همچنین خاطرنشان کرد                    |
| fawiki        | وی همچنین خاطرنشان کرد                    |
| frwikisource  | وی همچنین خاطرنشان کرد                    |
| loginwiki     | وی همچنین خاطرنشان کرد                    |
| mediawikiwiki | وی همچنین خاطرنشان کرد                    |
| metawiki      | وی همچنین خاطرنشان کرد                    |
| wikidatawiki  | وی همچنین خاطرنشان کرد                    |
+---------------+-------------------------------------------+
7 rows in set (0.00 sec)

while:

wikiadmin@10.64.48.56(centralauth)> select * from localuser where lu_name = 'وی همچنین خاطرنشان کرد' limit 5;
Empty set (0.00 sec)

The weird thing is that it the home wiki (fawiki) is not among the ones remaining in the localnames. I debug a bit further.

The hook is registered so it should run it:

ladsgroup@mwmaint1002:~$ mwscript eval.php --wiki=wikidatawiki
> var_dump( $wgHooks['RenameUserComplete']);
array(3) {
  [0]=>
  string(35) "AntiSpoofHooks::asAddRenameUserHook"
  [1]=>
  string(38) "CentralAuthHooks::onRenameUserComplete"
  [2]=>
  string(46) "CentralAuthAntiSpoofHooks::asAddRenameUserHook"
}
Ladsgroup set Security to Software security bug.Feb 6 2021, 2:31 PM
Ladsgroup added projects: Security, Security-Team.
Ladsgroup changed the visibility from "Public (No Login Required)" to "Custom Policy".
Ladsgroup changed the subtype of this task from "Task" to "Security Issue".

So I found out what's going on. It happened due to migrating the hook runner to the new mediawiki hook runner system: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Renameuser/+/ac18bc2b2cbdd7738f16b940d3cc3cca12e4fe18%5E%21/includes/RenameuserHookRunner.php

The problem is very sneaky, it's the extra Rename in the hook: RenameUserRenameComplete Compare https://codesearch.wmcloud.org/search/?q=RenameUserComplete&i=nope&files=&excludeFiles=&repos= and https://codesearch.wmcloud.org/search/?q=RenameUserRenameComplete&i=nope&files=&excludeFiles=&repos=

Given that it means AntiSpoof and SpamRegex hooks are not being ran anymore, I protect it as a security issue.

This fixes it ^ needs a virtual +2 and I'll deploy it on Monday.

This fixes it ^ needs a virtual +2 and I'll deploy it on Monday.

+2, approved. Happy to help testing on Monday if needed.

Thanks! Let's try it on Monday.

Urbanecm triaged this task as High priority.Feb 6 2021, 4:45 PM
Urbanecm moved this task from Incoming to Security Patch To Deploy on the Security-Team board.
Urbanecm moved this task from Backlog to To deploy on the User-Urbanecm board.

Actually a minor nit about the patch that can be fixed after this has been made public: the hook runner class should have its methods in alphabetical order, which means that the renamed method needs to be moved.

Actually a minor nit about the patch that can be fixed after this has been made public: the hook runner class should have its methods in alphabetical order, which means that the renamed method needs to be moved.

That's...really minor IMO. Let's fix that once it's in gerrit. Secpatches should be as simple as possible, IMO.

Tested with a user and it works just fine.

so it's on wmf.29 and wmf.27 and it should work just fine(TM) but I rather get it to gerrit before wmf.30 branch cut. If Security team is fine with it.

was deployed, moving to incoming for secteam to further triage

This is deployed now.

Thanks.

I'm tracking this on the supplemental patch for now: T270466. I'll try to get to the backports (at least forwhich should only be for master). Likely no CVE since this was more of a limited production bug.

This isn't an issue on any release branch.

Yeah the issue has not been released to 3rd parties at all.

This isn't an issue on any release branch.

...

Yeah the issue has not been released to 3rd parties at all.

Yes, just the backport to master then.

Change 664015 merged by jenkins-bot:
[mediawiki/extensions/Renameuser@master] SECURITY: Bring back the old name of the hook

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