Page MenuHomePhabricator

Echo Notification Mute (Block List) can be bypassed by changing username
Closed, ResolvedPublic5 Estimated Story Points

Assigned To
Authored By
dbarratt
Aug 16 2017, 11:35 PM
Referenced Files
F9227023: T173475-6.patch
Aug 30 2017, 4:50 PM
F9205105: T173475-5.patch
Aug 29 2017, 2:56 PM
F9194843: T173475-4.patch
Aug 28 2017, 3:56 PM
F9173537: T173475-3.patch
Aug 25 2017, 1:32 PM
F9167327: T173475-2.patch
Aug 24 2017, 3:08 PM
F9157027: T173475.patch
Aug 23 2017, 2:50 PM
Tokens
"Orange Medal" token, awarded by dpatrick.

Description

The Echo Notification Mute (Block List) (introduced in T150419) can by bypassed by changing your username. This is a security issue because it is a form of access bypass: User Apples removes access of User Bananas to send notifications to User Apples. User Bananas then changes their username to User Oranges. User Oranges can now send notifications to User Apples and thereby bypass the access restriction imposed by User Apples.

This is because the block list is stored as a new line delimited list of usernames in the user_properties table. The Renameuser extension does not update the username in the preference(s).

This could be resolved by updating Renameuser to update the preference for every user. But this would be an expensive operation as it would have to go through every user and update the preference (if the username is in their list) or it would have to do a fulltext search (and exclude instances where the is part of a different username). Regardless, that is also expensive.

Alternatively, the Mute list should be updated to use the centralauth id retrieved from CentralIdLookup.

Another solution could be to use a separate table rather than a user preference. But ideally should still use a centralauth id, so perhaps that is outside of the scope of this issue.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@MaxSem: Oh that's handy. In that case I guess there's no reason not to switch it. @dbarratt: I have no strong opinion on the storage format. The existing format was new line separated. Comma separated isn't ideal since commas are allowed in usernames so then you have to quote them as well.

@dbarratt: We'll also need a maintenance script for migrating all the existing entries in the preferences. See https://www.mediawiki.org/wiki/Manual:Maintenance_scripts and the existing maintenance scripts in Echo for examples. Usernames are not allowed to be numbers, so you should be able to assume that any non-numbers in the prefs need to be replaced with global IDs.

@dbarratt: We'll also need a maintenance script for migrating all the existing entries in the preferences. See https://www.mediawiki.org/wiki/Manual:Maintenance_scripts and the existing maintenance scripts in Echo for examples. Usernames are not allowed to be numbers, so you should be able to assume that any non-numbers in the prefs need to be replaced with global IDs.

Doh! I knew that, I just completely forgot about it. :) I'll take care of it.

@dbarratt: This sounds like a script that will be run once rather than regularly, so I don't think you'll need to register it.

I discovered that sometimes it's already an array.

Why is this not on gerrit...?

@Niharika because it's not public, but maybe it ought to be?

Given...

Considering that less than 1 rename happens per day across all WMF wikis, this is definitely an edge case. I would favor declining.

I'm not sure how big an abuse threat this is. Not anyone can rename themselves, IIRC. It's a job for the stewards or someone to approve renames.

Why is this not on gerrit...?

@Niharika because it's not public, but maybe it ought to be?

Yeah, since it's an abuse vector I'd personally prefer it be patched on the server before it goes into gerrit and becomes public but since it's a moderately difficult one (you have to ask to be globally named and have that done which is a moderately slow process and difficult to do frequently) if it's a huge pain to go through the security patch process then that's ok.

Also, given that this feature did not exist a week ago, I am not convinced that it's worth the hassles of getting this in as a security patch. We should put this on gerrit.

dbarratt changed the visibility from "Custom Policy" to "Public (No Login Required)".

Change 374871 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/Echo@master] Use User Ids instead of User Names for Echo Mute

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

dbarratt changed the task status from Stalled to Open.Sep 14 2017, 8:36 AM
dbarratt moved this task from Ready to Code Review on the Anti-Harassment (AHT Sprint 5) board.

Change 381890 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/Echo@master] Fix PHPDoc Documentation

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

Change 381890 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/Echo@master] Fix PHPDoc Documentation

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

@MaxSem Here's a patch with only the documentation fixes. Once that is merged then I'll rebase the main patch which will fix the tests. :)

Change 381890 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Fix PHPDoc Documentation

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

Here's the feature patch that needs to be reviewed/merged:
https://gerrit.wikimedia.org/r/#/c/374871/

@kaldari or @MaxSem
How do we have this script run on deployment?

@kaldari or @MaxSem
How do we have this script run on deployment?

You ask the deployer to run it for you. Or poke MaxSem to run it. He has a special bond with maintenance scripts.

Change 374871 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use User Ids instead of User Names for Echo Mute

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

You ask the deployer to run it for you. Or poke MaxSem to run it. He has a special bond with maintenance scripts.

but wont this go out with the weekly train? How do I find out who the deployer is?

You ask the deployer to run it for you. Or poke MaxSem to run it. He has a special bond with maintenance scripts.

but wont this go out with the weekly train? How do I find out who the deployer is?

Ah, I imagined you'd SWAT this. The train left before your change was merged this week so you should SWAT it and ask the deployer to run it for you. Otherwise this change won't be live on wikis until Thursday next week.

@dbarratt: There's basically two different ways to do this:

  • SWAT deployment: You schedule the deployment in a SWAT window and once the code is deployed, you ask the deployer to run it for you.
  • Riding the train: The maintenance script rides the deployment train and ends up on all the wikis a week after merging. Then you, or someone with access to terbium, runs the maintenance script.

If you want to run the maintenance script yourself, there are instructions on wikitech, although they don't seem to mention foreachwiki <command> which is basically a shortcut for mwscriptwikiset <command> all.dblist.

Added documentation for foreachwiki :)

Change 382508 had a related patch set uploaded (by Thcipriani; owner: Dbarratt):
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] Use User Ids instead of User Names for Echo Mute

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

Change 382511 had a related patch set uploaded (by Thcipriani; owner: Dbarratt):
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.1] Use User Ids instead of User Names for Echo Mute

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

Change 382508 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] Use User Ids instead of User Names for Echo Mute

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

Change 382511 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.1] Use User Ids instead of User Names for Echo Mute

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

Mentioned in SAL (#wikimedia-operations) [2017-10-05T18:58:44Z] <thcipriani@tin> Synchronized php-1.31.0-wmf.2/extensions/Echo: SWAT: [[gerrit:382508|Use User Ids instead of User Names for Echo Mute]] T173475 (duration: 00m 56s)

Mentioned in SAL (#wikimedia-operations) [2017-10-05T19:05:14Z] <thcipriani@tin> Synchronized php-1.31.0-wmf.1/extensions/Echo: SWAT: [[gerrit:382511|Use User Ids instead of User Names for Echo Mute]] T173475 (duration: 00m 55s)

Change 382561 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/Echo@master] Use main Database Connection for Maintenance Script

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

Change 382561 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use main Database Connection for Maintenance Script

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

Change 383173 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] Reapply "Use User Ids instead of User Names for Echo Mute""

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

Change 383173 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] Reapply "Use User Ids instead of User Names for Echo Mute""

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

Mentioned in SAL (#wikimedia-operations) [2017-10-09T19:17:52Z] <hashar@tin> Synchronized php-1.31.0-wmf.2/extensions/Echo: Reapply "Use User Ids instead of User Names for Echo Mute"" - T173475 (duration: 00m 52s)

Change 383182 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] updatePerUserBlacklist wfWaitForSlaves()

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

hashar subscribed.

The updatePerUserBlacklist script required a live hack. I did a commit for the wmf branch but that needs to be ported to the master branch https://gerrit.wikimedia.org/r/383182 updatePerUserBlacklist wfWaitForSlaves()

Mentioned in SAL (#wikimedia-operations) [2017-10-09T19:50:42Z] <hashar@tin> Synchronized php-1.31.0-wmf.2/extensions/Echo/maintenance/updatePerUserBlacklist.php: Sync a live hack in Echo updatePerUserBlacklist https://gerrit.wikimedia.org/r/#/c/383182/ - T173475 (duration: 00m 47s)

Change 383182 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.31.0-wmf.2] updatePerUserBlacklist wfWaitForSlaves()

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

Change 383183 had a related patch set uploaded (by Dbarratt; owner: Hashar):
[mediawiki/extensions/Echo@master] updatePerUserBlacklist wfWaitForSlaves()

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

The updatePerUserBlacklist script required a live hack. I did a commit for the wmf branch but that needs to be ported to the master branch https://gerrit.wikimedia.org/r/383182 updatePerUserBlacklist wfWaitForSlaves()

Tracked by T177437 and the live hack patch is proposed to master branch. The script ran fine once applied :)

Change 383183 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] updatePerUserBlacklist wfWaitForSlaves()

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