Page MenuHomePhabricator

AbuseFilter blocks not working for account autocreations
Open, LowPublicSecurity

Description

I've noticed this on itwiki, where a filter repeatedly failed to block an LTA trying to autocreate an account. Debugging locally revealed that, for autocreateaccount actions, the performer of the action is a User object identical to the one for the account being created. But the account doesn't exist yet, so it cannot be blocked.

For normal account creations this is not an issue, because the creator is correctly provided as an anonymous user (which BTW leads to T152394).

I'm not sure for how long this has been broken, but even if the relevant code was recently refactored, the block target seems to always have been the same. It's actually possible that the previous code was reporting the block as successful, but I don't think this has ever been the case.

Event Timeline

Daimona triaged this task as High priority.Jan 17 2021, 4:01 PM
Daimona added a project: AbuseFilter.

I've verified that in REL1_35, the block was performed on a non-existing user and appeared as successful (logged in the AbuseLog, Special:Log/block and visible on Special:IPBlockList). It didn't have any effect though. So the new code (and probably also BlockUser, which validates the target) made the problem more obvious, but it's always been like this.

Daimona added a project: Patch-For-Review.
Daimona added a subscriber: matej_suchanek.

A bit hacky, but it works and it's very tiny.

A bit hacky, but it works and it's very tiny.

AbuseFilter requires 1.36+, where User is not newable and rather than new User the ideal way to create an anonymous user is with UserFactory::newAnonymous with the user factory retrieved via MediaWikiServices. Is it possible to use that here? Not a big deal if not

A bit hacky, but it works and it's very tiny.

AbuseFilter requires 1.36+, where User is not newable and rather than new User the ideal way to create an anonymous user is with UserFactory::newAnonymous with the user factory retrieved via MediaWikiServices. Is it possible to use that here? Not a big deal if not

It can be done on gerrit once this task is made public. Being a security patch, I want to make it as short as possible. new User works and it's short, and thus IMHO suitable in this context.

A bit hacky, but it works and it's very tiny.

AbuseFilter requires 1.36+, where User is not newable and rather than new User the ideal way to create an anonymous user is with UserFactory::newAnonymous with the user factory retrieved via MediaWikiServices. Is it possible to use that here? Not a big deal if not

It can be done on gerrit once this task is made public. Being a security patch, I want to make it as short as possible. new User works and it's short, and thus IMHO suitable in this context.

ack. Makes sense, +1 from me, untested

+1 to the patch for now, @Daimona - is there a time before or after the trains this week where you'd be available to test with a filter and some autocreated accounts? If so, I can help get this deployed.

Ping @Daimona @DannyS712 - we can probably deploy this today during the security deployment window. If one of you would be around to help test, that would be extremely helpful. Thanks.

It's actually possible that the previous code was reporting the block as successful, but I don't think this has ever been the case.

This specific issue was fixed, so this bug should at least be less noticeable. I should check whether the patch above is still needed.

@sbassett I've verified that this issue still exists, although the interface now ignores the block result and disallows the action anyway (since T272330 was resolved with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/657092/). I've also verified that the patch above fixes the issue, and it can be deployed whenever you wish. A few memos:

  • Once the patch is merged on gerrit, new User should be replaced with UserFactory
  • I kinda liked the idea that the account to be blocked should be the creator's one (and not their IP), but this isn't doable because the account doesn't exist. Blocking the account on another wiki, where it exists, would have the following problems:
    • Which wiki to choose?
    • How to retrieve the external account?
    • The ethical implication make this a no-go. I can easily imagine user Alice, sysop on wiki X, who goes to wiki Y where a misconfigured filters prevents her account autocreation and blocks here on wiki X. This is a no-go.
  • This fix is also affected by T152394 (and would be resolved by the same patch). There shouldn't be any problem for WMF prod (since the other patch is already applied), but writing this here in case you want to test this locally.
  • Once the patch is merged on gerrit, new User should be replaced with UserFactory

Ok.

  • I kinda liked the idea that the account to be blocked should be the creator's one (and not their IP), but this isn't doable because the account doesn't exist. Blocking the account on another wiki, where it exists, would have the following problems:
    • Which wiki to choose?
    • How to retrieve the external account?
    • The ethical implication make this a no-go. I can easily imagine user Alice, sysop on wiki X, who goes to wiki Y where a misconfigured filters prevents her account autocreation and blocks here on wiki X. This is a no-go.

Ok.

  • This fix is also affected by T152394 (and would be resolved by the same patch). There shouldn't be any problem for WMF prod (since the other patch is already applied), but writing this here in case you want to test this locally.

Ok, so T152394 resolves this issue? If so, I believe it's been deployed (T152394#6913651). Or does the patch here further resolve that issue? Or are they just dependent upon each other in some way (e.g. I should apply both if I want to test locally?)

  • This fix is also affected by T152394 (and would be resolved by the same patch). There shouldn't be any problem for WMF prod (since the other patch is already applied), but writing this here in case you want to test this locally.

Ok, so T152394 resolves this issue? If so, I believe it's been deployed (T152394#6913651). Or does the patch here further resolve that issue? Or are they just dependent upon each other in some way (e.g. I should apply both if I want to test locally?)

What I mean is that the patch for this task will add another code path that causes the issue at T152394 (since it'll block the creator's IP), but the patch already sent for that task will fix the issue. So if you're testing this locally and notice that the block reveals the IP of the creator, keep in mind that it was already fixed in prod.

What I mean is that the patch for this task will add another code path that causes the issue at T152394 (since it'll block the creator's IP), but the patch already sent for that task will fix the issue. So if you're testing this locally and notice that the block reveals the IP of the creator, keep in mind that it was already fixed in prod.

Ok, sounds good. I'll most likely try to deploy this to production during this Monday's (2021-03-22) security window.

sbassett moved this task from Security Patch To Deploy to Watching on the Security-Team board.

Deployed to wmf.35. Logstash seems fine. Also tracked at T276237 and T270466.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Fri, Apr 16, 7:34 PM
sbassett changed the edit policy from "Custom Policy" to "All Users".

Change 680353 had a related patch set uploaded (by SBassett; author: Daimona Eaytoy):

[mediawiki/extensions/AbuseFilter@REL1_36] SECURITY: Use an anonymous user as creator for autocreations

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

Change 680354 had a related patch set uploaded (by SBassett; author: Daimona Eaytoy):

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Use an anonymous user as creator for autocreations

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

Change 680355 had a related patch set uploaded (by SBassett; author: Daimona Eaytoy):

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Use an anonymous user as creator for autocreations

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

Bugreporter closed this task as a duplicate of Restricted Task.Fri, Apr 16, 9:33 PM

It might have been a duplicate, but that other task should probably be merged here since this is where everything is. Plus, there are open patches for this task

sbassett lowered the priority of this task from High to Low.Sat, Apr 17, 12:51 AM
sbassett removed a project: Patch-For-Review.

Change 680354 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Use an anonymous user as creator for autocreations

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

Change 680355 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Use an anonymous user as creator for autocreations

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

Change 680353 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_36] SECURITY: Use an anonymous user as creator for autocreations

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

Change 680412 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] SECURITY: Use an anonymous user as creator for autocreations

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