Blocking the account after creation would probably solve several problems, but would require a major change... In the task I just closed as duplicate, I had proposed to use the block ID like for autoblocks, but I guess that wouldn't be easy, and could possibly leave some info leak. Any solution to this is likely related to T213982#4888012.

So, I tried to think about possible solutions. We could certainly do something like T152394#4888162, i.e. implement a custom system resembling autoblocks. However, that would (probably) require a major effort, and would end up duplicating some DatabaseBlock logic. Hence, the alternative is to use DatabaseBlock's logic. The idea being to call doAutoblock on the IP of the user. This brings us another problem: we don't have a "parent" block object referring to the user, because the account still has to be created. Which, in turn, would leave us with two alternatives:

1. As proposed above, we allow the account creation and issue the block afterwards; this way we can set an autoblock cleanly. This would definitely be saner and future-proof, BUT it has three disadvantages:
   1. We'd have to change the entry point, shifting away from the AuthenticationProvider. We'd probably have to use something like the LocalUserCreated hook.
   2. The account will be created anyway, and this is something that sometimes should not be allowed. For instance, if the username contains blasphemy, or is grossly insulting in any other way. And I believe that most of the account creation filters do that. Creating the account means more work for sysops who have to delete log entries.
   3. If the creation succeeds, we miss the chance to block further attempts. This is usually not important, but it could be in some edge cases. For example, suppose there's a SUL account still not attached to your wiki. For some reason, you want to forbid that specific person from attaching the account, and blocking any attempt. If the account is created, you cannot do that anymore. The example itself doesn't make much sense, but I hope it can at least give an idea of what I'm talking about.
2. A partly crazy idea: create a DatabaseBlock object referring to the yet-not-created account, and call doAutoblock on it without then inserting the main block. This is hackier, but if it works (and I still don't know if it does) it'd be a cheap solution.

For (1), I find (B) to be especially unacceptable. So, (2). Is it a hack? Sort of. Is it future-proof? Probably not. I think it's worth trying, at least to mitigate this very serious info leak.

Also, a side note. AbuseFilter tables still have to be switched to the actor style. While switching the abuse_filter_log table (patch), it was discovered that the task won't be easy: currently, there's a hack in place to hide IPs of account creators in the AbuseLog (code). That could be avoided with solution 1 (although old entries will remain a problem).

I tried to hack the AF code a bit. What I did is: if the user is anon, add the following to the block created in doAbuseFilterBlock:

	$block->setHideName( true );
	$block->mAuto = true;
	$block->mParentBlockId = ????;//FIXME

and don't create the log entry below. The IP is blocked, the log says nothing about it, and the AbuseLog still says that the user was blocked. Nothing seems to explode, too. But there's a problem with the mParentBlockId: without setting it, the (auto)block won't be reported in Special:AutoblockList (hence, you cannot unblock); setting it to a bogus value will show the block there, but you won't be able to change it.

And that's expected, of course, given that we don't have a parent block.

OK, so I went for an alternative approach. If the user is anon (and this is an account creation), block the account being created (even if it doesn't exist), then autoblock the IP. This works: the IP is blocked, it appears in Special:AutoblockList, and not in public logs. But that's still a bit hacky.

Hence, a question for @Anomie, @Tchanders, @dmaza: we're trying to put a block on a non-existing account. Especially given the new actor schema and the ongoing block refactoring, is that going to crash badly, or can we live with that?

Tentatively proposing a patch with the solution above:

Aside from blocking a non-existing user, two remarks:

• Maybe we should make it easier for checkusers to find the autoblock. Maybe writing it somewhere in the log entry, or next to the "block" checkbox, or something like that. Otherwise, an IP (or even a range) might be blocked without any log pointing to the block.
• The method is currently hacky: it swaps the target and builds an autoblock in place. That should probably be factored out, having a different method for autoblock, maybe one for rangeblocks, and accepting a different format as the target (and maybe all methods could use the same internal method to build a block object). I've left this as @fixme to keep the security patch short.

I think it's still a bit early to start a review without an answer to my question above, but CC @Huji and @matej_suchanek for a preliminary look.

In T152394#5513776, @Daimona wrote:

Hence, a question for @Anomie, @Tchanders, @dmaza: we're trying to put a block on a non-existing account. Especially given the new actor schema and the ongoing block refactoring, is that going to crash badly, or can we live with that?

The actor change specifically didn't change ipb_address to use the actor table, since the target of a block need not be an actor. An IP range was the specific non-actor target considered when doing that, but a not[-yet]-registered user name would also be a non-actor. I can't speak to the rest of the block refactoring, beyond noting that T228077 was declined.

Somewhat related T234164?

In T152394#5532569, @MarcoAurelio wrote:

Somewhat related T234164?

Meh, yes. This bug is pretty simple to discover, and yet very serious. I know that it's been sitting there for 3+ years, but I'm tempted to set UBN. Even a hack (blocking a non-existing account) would be fine for now.

The patch above is not perfect, so I sent https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/539848/ to make it possible to build a new version on top of it.

Thanks for the details @Daimona - here are some thoughts about those two solutions.

Set a block against a non-existent account

I'd advise against allowing blocks to be set against targets that aren't considered valid (in this case a string that doesn't map to an existing account name or valid IP address/range). Blocks with invalid targets would behave unpredictably - e.g. once the block has been made against the non-existent account, it won't be possible to alter it via the UI or API, since they will reject the updated block for having an invalid target.

It seems odd to me that it is even possible to make such a block via DatabaseBlock::insert; it seems that that method should either check the target validity (and the validity of other fields), or only be callable from somewhere that has already checked validity.

This part of the blocking code is still rather disorganised - the target validation is done via SpecialBlock::validateTarget, but should really be somewhere more generic than the special page.

Let the account be created and block it

From the blocking perspective this is more "correct", since it works within the existing rules - but, as mentioned above, it depends whether the downsides of allowing the account to be created are acceptable.

We could certainly do something like T152394#4888162, i.e. implement a custom system resembling autoblocks. However, that would (probably) require a major effort, and would end up duplicating some DatabaseBlock logic. Hence, the alternative is to use DatabaseBlock's logic.

I'd just note that the autoblocking functionality in DatabaseBlock doesn't really belong in that class. Ideally, DatabaseBlock should represent the block stored in the database, whereas inserting, deleting, autoblocking, etc. should really be done via a separate service (T221075), to be reused by different types of block that need that behaviour.

In T152394#5533650, @Tchanders wrote:

Thanks for the details @Daimona - here are some thoughts about those two solutions.

Thanks, very appreciated!

Set a block against a non-existent account

I'd advise against allowing blocks to be set against targets that aren't considered valid

Heh, I admit it's not pretty. Of note, we're already doing that for account autocreations (not just creations), according to T234164. But I do recognize that it's very hacky. My question, for now, is whether we can tolerate this hackiness in the short term, to stop leaking PI, then think about a longer-term solution in a public task. Noting that such a solution is probably not immediate, and could require some time.

e.g. once the block has been made against the non-existent account, it won't be possible to alter it via the UI or API, since they will reject the updated block for having an invalid target.

In theory, there's no need to do that, the idea being that people are only supposed to remove the autoblock (which I tested and it works). Of course, we need to tell people about this, and smashing an uncaught exception or a mysterious error in their face is not a pretty way to do that.

It seems odd to me that it is even possible to make such a block via DatabaseBlock::insert; it seems that that method should either check the target validity (and the validity of other fields), or only be callable from somewhere that has already checked validity.

Yes, it's indeed possible, and I agree that it should check the validity of the target. I can open a separate task for that if you want. But again, note that it won't work out of the box, especially as long as AF keeps blocking non-existent people for account autocreation.

Let the account be created and block it

From the blocking perspective this is more "correct", since it works within the existing rules - but, as mentioned above, it depends whether the downsides of allowing the account to be created are acceptable.

Yeah, and I honestly think it's not acceptable. IMHO, if a filter is set to block the person creating an account, that's because an account with a given name should not exist. And even if this may not have been the original intent of previous AF maintainers, I think this is now a de facto standard, since we got people used to the fact that an AF block on account creation means the account is not even created.

I'd just note that the autoblocking functionality in DatabaseBlock doesn't really belong in that class. Ideally, DatabaseBlock should represent the block stored in the database, whereas inserting, deleting, autoblocking, etc. should really be done via a separate service (T221075), to be reused by different types of block that need that behaviour.

Interesting. Yes, that would help.

So... Aside from the temporary hack, let me recap the acceptance criteria for a longer-term solution and see if you or anyone else may have a better idea.

Notes: This applies to account (auto)creations; relevant code for AF blocks.

• The IP of the account creator should not be disclosed; this applies to:
  • Special:Log/block
  • Special:Contribution for the IP
  • Special:Blocklist
  • Querying the ipblocks table

-> AFAIK, the Block logic should handle all of them at the same time.

• Nevertheless, the IP address should be blocked with the specified duration
• The block should be removable, still without disclosing the target (e.g. from Special:AutoblockList)
• The account creation should not succeed

New version of the hacky patch using autoblocks, note that it depends on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/539848/.

At this point, given that we're already (unintendedly) blocking a non-existing target for autocreations, and the seriousness of this bug vs how easy it is to spot, I'd suggest to go with this hacky solution, and then discuss a proper one publicly.

In T152394#5533726, @Daimona wrote:

T152394.patch4 KBDownload

Uh, it has an undeclared variable. Here is the updated version, but it still fails. When trying to publish an RC entry for the block, it will try to create an actor for it, and that will fail :-[

Adding for investigation, unsure if we'll be able to implement a proper solution within the grant.

Huh, I spoke too soon... It wouldn't be a one-liner; there are two main issues to be resolved first:

• The AF code for blocks doesn't know whether the current action is an account creation. This can be easily resolved by passing a parameter to BlockingConsequence::__construct.
• Core's BlockUser doesn't allow altering the log entry. Also, I don't know whether it's possible to easily retrieve the entry after it's been published and oversight it later, but I don't want to do that because it would still be published unhidden. CC @Tchanders and @Urbanecm for the recent work on BlockUser -- how do you think a caller should be allowed to modify a log entry before it's published? Or, for the limited use case outlined here, do you think we might add a setter to BlockUser, e.g. setLogEntryDeleted( int $flags )?

I would suggest making the block action not work when action is related to account creation. That would solve the immediate security issue, without also affecting a lot of usecases -- personally I don't think this is actually used anywhere, and if it is, it's probably not a large amount of projects.

To be honest, I don't think BlockUser should be responsible for suppressing the log entries out of sudden, I don't see such a concept elsewhere in MW. If we want to take that path, I would suggest oversighting the log entry using some other service (which can be created if there's not one already).

However, a nicer solution might be imitating core's autoblock functionality -- not sure, but it sounds core's block classes might be extended, and altered from extensions providing alternative blocking mechanisms? We could make the action work more like an autoblock, which isn't logged, and the IP is not exposed (MW exposes block ID instead).

In T152394#6717773, @Urbanecm wrote:

I would suggest making the block action not work when action is related to account creation. That would solve the immediate security issue, without also affecting a lot of usecases -- personally I don't think this is actually used anywhere, and if it is, it's probably not a large amount of projects.

I don't have hard data, but it is certainly used. I can count 7 such filters on itwiki, and I believe there are others on other wikis. Disabling the feature would effectively introduce a loss of functionality.

To be honest, I don't think BlockUser should be responsible for suppressing the log entries out of sudden, I don't see such a concept elsewhere in MW. If we want to take that path, I would suggest oversighting the log entry using some other service (which can be created if there's not one already).

The problem is, this isn't possible. BlockUser::log creates a ManualLogEntry, sets data, inserts the entry, and publishes it. Deletion should happen in between creation and insertion (at least for our use case), but BlockUser doesn't offer any hook inside log(). Note, however, that it wouldn't be complicated to delete the entry. I might commit a POC on gerrit, but the base idea is:

private $logDeletionFlags;

/**
 * @param int $flags One of LogPage::* constants
 */
public function setLogDeletionFlags( int $flags ) : void { // Alternatively, it might be a constructor option inside $blockOptions instead of a setter, perhaps even better
    $this->logDeletionFlags = $flags;
}

private function log( DatabaseBlock $block, bool $isReblock ) {
     // ...
    if ( $this->logDeletionFlags !== null ) {
        $logEntry->setDeleted( $this->logDeletionFlags );
    }
    // ...

However, a nicer solution might be imitating core's autoblock functionality -- not sure, but it sounds core's block classes might be extended, and altered from extensions providing alternative blocking mechanisms? We could make the action work more like an autoblock, which isn't logged, and the IP is not exposed (MW exposes block ID instead).

This would indeed be a great idea. However, my previous investigation (see T152394#5496287 and following comments, might be outdated in theory, but it's probably not) revealed that MW core doesn't support "stand-alone autoblocks", that is, autoblocks without a "parent" block. This is a feature that might be implemented, but it wouldn't be as easy as deleting the log entry. Perhaps implement the log-deletion solution, mark it as @internal, and then refactor autoblocks after this task is resolved?

Hmm, good points, @Daimona. We can add a hook to modify the log object I guess. What about changing BlockUser to allow suppression in Gerrit (I can review quickly), and then using it (as a security patch) in AF?

In T152394#6717844, @Urbanecm wrote:

Hmm, good points, @Daimona. We can add a hook to modify the log object I guess. What about changing BlockUser to allow suppression in Gerrit (I can review quickly), and then using it (as a security patch) in AF?

The problem with a hook is that it would be more like a long-term solution. A setter might be marked as @internal until a better solution is implemented. That said, review should certainly happen on gerrit, and I'd like to also hear the opinion by @Tchanders or anybody else on the AHT team per Developers/Maintainers. Anyway, both solutions work for me.

Requires https://gerrit.wikimedia.org/r/c/mediawiki/core/+/656612

Whooops, I just realized that this wasn't deployed. The patch above still applies cleanly on master. Anybody who would like to take a look & deploy? @Urbanecm? @sbassett?

In T152394#6911820, @Daimona wrote:

Whooops, I just realized that this wasn't deployed. The patch above still applies cleanly on master. Anybody who would like to take a look & deploy? @Urbanecm? @sbassett?

Happy to pair with you on Monday if you’re free :).

In T152394#6911880, @Urbanecm wrote:

In T152394#6911820, @Daimona wrote:

Whooops, I just realized that this wasn't deployed. The patch above still applies cleanly on master. Anybody who would like to take a look & deploy? @Urbanecm? @sbassett?

Happy to pair with you on Monday if you’re free :).

Sure, thank you:) Just ping me whenever you're comfortable

Cool, Will do.

Fixed patch:

In T152394#6913645, @Daimona wrote:

Fixed patch:

T152394-fixed.patch1 KBDownload

Deployed as of this patch: F34161556.

14:25 <@Urbanecm> !log Deploy security patch for T152394

moving to incoming for secteam to do the backports, etc.

@Daimona @Urbanecm - there's also this one that we should probably deploy soon: T272244. Should be simple to deploy, but it'd be nice to have someone available who could quickly help test it out.

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

[mediawiki/extensions/AbuseFilter@master] SECURITY: Don't leak IPs when blocking anon account creations

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

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

[mediawiki/extensions/AbuseFilter@REL1_36] SECURITY: Don't leak IPs when blocking anon account creations

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

Change 680410 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] SECURITY: Don't leak IPs when blocking anon account creations

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

The fix is not perfect for a reason I mentioned in the security task above.

Change 680352 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_36] SECURITY: Don't leak IPs when blocking anon account creations

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