Page MenuHomePhabricator

Forbid blocking IP ranges as big as /1 and /2, as done on ruwikiquote using the API
Closed, ResolvedPublic

Description

Coincidently I found out that an adminbot blocked IP ranges on ruwikiquote as big as /1 and /2. That's almost everyone...

ru.wikiquote.org
2018-Jun-20 — 2023-Jun-20: 128.0.0.0/1 blocked by QBA-bot ({{BlockedHosting}})
2018-Jun-20 — 2023-Jun-20: 128.0.0.0/2 blocked by QBA-bot ({{BlockedHosting}})
https://tools.wmflabs.org/meta/stalktoy/183.198.0.0%2F16

It shouldn't be possible on Wikimedia wikis to block such huge ranges. Probably a bug, so please correct it.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 13 2018, 2:14 PM
Samtar added a subscriber: Samtar.

This task doesn't only affect Russian Sites, nor is it a consequence of the site being Russian

Legoktm set Security to Software security bug.Jul 13 2018, 2:24 PM
Legoktm added a project: Security.
Legoktm changed the visibility from "Public (No Login Required)" to "Custom Policy".
Legoktm added a subscriber: Legoktm.
$wgBlockCIDRLimit = [
	'IPv4' => 16, # Blocks larger than a /16 (64k addresses) will not be allowed
	'IPv6' => 19,
];

Somehow the limits are being bypassed?

Legoktm claimed this task.Jul 13 2018, 2:42 PM
Legoktm added a project: MediaWiki-API.

Looks like action=block in the API is no longer checking against $wgBlockCIDRLimit.

Anomie added a subscriber: Anomie.Jul 13 2018, 2:53 PM

It looks like it was broken by rMWb11b0c798d6f: * Moved Target field validation into a field validation callback to show erros… back in 2011, when the validation was moved out of SpecialBlock::processForm().

Anomie moved this task from Unsorted to Needs Code on the MediaWiki-API board.Jul 13 2018, 2:54 PM

I didn't git blame/bisect to see when this was introduced.

Code-Review+1

As a minor thing, SpecialBlock::checkUnblockSelf() is now called twice by the API, but that can probably be cleaned up at another time.

The call in ApiBlock results in an error message with the block info included, which the call from SpecialBlock::validateTarget() wouldn't. We want to keep that block info.

$this->mUser = User::newFromName( '128.0.0.0/16', false );

Eew. But I guess ok for a security patch to avoid further rewriting of the test class.

revi added a subscriber: revi.Jul 25 2018, 7:46 PM

Ping? Should I go ahead and deploy this?

I think it's ready to deploy. I rebased it on to master since it didn't apply against the current deployment branch.

I think it's ready to deploy. I rebased it on to master since it didn't apply against the current deployment branch.

Code-Review+1. Tests pass locally, let's get this deployed.

Deployed and verified on test.wikipedia.org. Final patch I used (just added SECURITY: to the commit message) is attached.

Legoktm renamed this task from Forbid blocking IP ranges as big as /1 and /2, as done on ruwikiquote to Forbid blocking IP ranges as big as /1 and /2, as done on ruwikiquote using the API.Dec 20 2018, 9:45 AM
Legoktm closed this task as Resolved.
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 6 2019, 4:01 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJun 6 2019, 4:01 PM

Change 514765 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_27] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514765 merged by Reedy:
[mediawiki/core@REL1_27] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514776 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_30] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514776 merged by Reedy:
[mediawiki/core@REL1_30] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514756 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514852 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_31] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514952 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_32] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514852 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514976 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_33] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514952 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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

Change 514976 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: API: Respect $wgBlockCIDRLimit in action=block

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