Page MenuHomePhabricator

Global blocks: if an IP is within two ranges and one is locally disabled, GlobalBlock won't listen to the other one (CVE-2020-10534)
Closed, ResolvedPublic

Description

I've noticed that 82.102.16.0/20 is globally blocked by @kolbert on Aug. 2.

82.102.24.251 is within that range but managed to edit on eswiki today: https://es.wikipedia.org/wiki/Especial:Contribuciones/82.102.24.251.

82.102.26.202 is also within the range but has also managed to edit: https://es.wikipedia.org/wiki/Especial:Contribuciones/82.102.26.202

Given that the range is globally blocked no one should've been allowed to edit.

Please investigate whether this is affecting this specific global block, or just some or all of them (which would be quite catastrophic).

Thank you.

Updated status
After checking https://es.wikipedia.org/wiki/Especial:Lista_de_bloqueos_globales?ip=82.102.24.251 we see:

  • 82.102.0.0/19 is globally blocked, but the global block is locally disabled.
  • However 82.102.16.0/20 is globally blocked but not locally disabled.
  • Given that 82.102.24.251 is both in the /19 and the /20 it looks like GlobalBlock just ignores the /20 global block and lets the entire /19 range edit, making the /20 global block useless.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 3 2019, 5:45 PM
kolbert added a subscriber: kolbert.
jrbs added a subscriber: jrbs.Aug 3 2019, 6:23 PM

It looks this was an API-based edit.

[urbanecm@mwlog1001 /srv/mw-log]$ grep 82.102.26.202 api.log
[snip]
2019-08-03 15:43:08 [XUWrjApAAEsAAJ55r7kAAACH] mw1280 eswiki 1.34.0-wmf.16 api INFO: API POST 82.102.26.202 82.102.26.202 T=833ms action=edit format=json title=Rosal%C3%ADa%20Vila section=0 text=%7B%7BFicha%20de%20artista%20musical%0A%7Cnombre%20%20%20%20%20%20%20%20%20%20%20%20%3D%20Rosal%C3%ADa%0A%7Cimagen%20%20%20%20%20%20%20%20%20%20%20%20%3D%20Rosalia%202019-portrait.jpg%0A%7Csubt%C3%ADtulo%20%20%20%20%20%20%20%20%20%3D%20Rosal%C3%ADa%20en%20los%20%5B%5BPremios%20Goya%5D%5D%20de%202019.%0A%7Ctama%C3%B1o%20%20%20%20%20%20%20%20%20%20%20%20%3D%20250px%0A%7Cfondo%20%20%20%20%20%20%20%20%20%20%20%20%20%3D%20solista%0A%7Cnombre%20real%20%20%20%20%20%20%20%3D%20Rosal%C3%ADa%20V[...] summary=Eliminaci%C3%B3n%20de%20carga%20ideol%C3%B3gica. basetimestamp=2019-07-31T18:50:43Z starttimestamp=2019-07-31T18:50:43Z token=[redacted]
[snip]
MarcoAurelio added a comment.EditedAug 3 2019, 6:42 PM

I've been discussing with @Urbanecm on IRC with this.

My guess is that this might be some sort of caching issue. 82.102.16.0/20 was globally blocked as part of a batch of global blocks which were all issued using a Toolforge tool. Maybe the database, given that those were issued at high speed, failed to load them properly or something like that?

Edit: this is not the reason. See updated description for details.

NOTE: Updated task details.
MarcoAurelio renamed this task from GlobalBlocks not working to Global blocks: if an IP is within two ranges and one is locally disabled, GlobalBlock won't listen to the other one.Aug 3 2019, 6:55 PM

Do I understand GlobalBlocking::getGlobalBlockingBlock()'s $block = $dbr->selectRow( 'globalblocks', self::selectFields(), $conds, __METHOD__ ); correctly? I feel it means "select the first block in the database", which would usually mean the oldest one. Maybe we should add a method like DatabaseBlock::chooseMostSpecificBlock() to GlobalBlocking::getGlobalBlockingBlock() and select the most specific block. That would solve this issue, since the smaller block would be used, instead of the /19 one.

Urbanecm claimed this task.Aug 3 2019, 10:27 PM
Urbanecm triaged this task as Low priority.
Urbanecm added a project: Patch-For-Review.
Urbanecm added a subscriber: Catrope.

This should fix it. The patch basically makes sure the block displayed on Special:Contributions and the block used displayed in "You are blocked" message is a) the same b) the most specific block. It should work the same way like local blocks after applying this patch.

@Catrope, could you review?

Restricted Application added a project: User-Urbanecm. · View Herald TranscriptAug 3 2019, 10:27 PM
Urbanecm moved this task from Backlog to Waiting on review on the User-Urbanecm board.
Reedy added a subscriber: Reedy.Aug 5 2019, 9:32 AM

Do I understand GlobalBlocking::getGlobalBlockingBlock()'s $block = $dbr->selectRow( 'globalblocks', self::selectFields(), $conds, __METHOD__ ); correctly? I feel it means "select the first block in the database", which would usually mean the oldest one. Maybe we should add a method like DatabaseBlock::chooseMostSpecificBlock() to GlobalBlocking::getGlobalBlockingBlock() and select the most specific block. That would solve this issue, since the smaller block would be used, instead of the /19 one.

selectRow will indeed only return one row. Whether it's the oldest is indeterminate when a sort order isn't provided

Rebased. Anyone to review?

Second attempt...

sbassett moved this task from Incoming to In Progress on the Security-Team board.
Jcross added a subscriber: Jcross.Jan 16 2020, 8:04 PM

Hi @Urbanecm

Regretfully, this ticket was outside of the team workflow thus your requests for review were not seen or acted upon by the Security Team in a timely fashion. We apologize for the confusion, as we are working hard to improve and evolve our processes. For future reference, tagging Security is not considered a request for service from the Security Team and following our SOP will ensure our prompt attention to your needs. Thank you - and again, we apologize for the confusion. We are adding our Security-Team tag and will take a look during our next clinic meeting.

Cheers

Third time's the charm.

@Jcross Thanks for the explanation - was there any update with this patch?

@Urbanecm - Sorry for the delay on this. We should hopefully be able to get it reviewed and deployed sometime next week.

@Urbanecm - I've reviewed the patch and can +1 for now as it seems sane, though I didn't get to test it as completely as I'd like. I'm thinking it can probably go out during this coming Monday's sec deployment window. Would you be around to help test?

@Urbanecm - I've reviewed the patch and can +1 for now as it seems sane, though I didn't get to test it as completely as I'd like. I'm thinking it can probably go out during this coming Monday's sec deployment window. Would you be around to help test?

Unfortunately, I'm not a steward and I cannot place global blocks. To fully test, we'd need some ranges with IPs I'm able to access globally blocked. Maybe @MarcoAurelio could help us place the blocks? Or, we can also wait for the end of SE2020 :-).

Unfortunately, I'm not a steward and I cannot place global blocks. To fully test, we'd need some ranges with IPs I'm able to access globally blocked. Maybe @MarcoAurelio could help us place the blocks? Or, we can also wait for the end of SE2020 :-).

It depends on the urgency. I could probably coordinate with both of you to attempt to place some global blocks and see if this works or you can wait until SE is over and do it yourself (and therefore save my fees :-P ). As you wish :-)

Given this waited in the queue for months, a week shouldn't hurt us much. @sbassett Would Mar 2 sec window be okay with you?

Given this waited in the queue for months, a week shouldn't hurt us much. @sbassett Would Mar 2 sec window be okay with you?

@Urbanecm @MarcoAurelio -

Should be fine. I know the weekly security deployment window (UTC 22:00–00:00) isn't very Euro-friendly - would you prefer to try to add this to the European SWAT on March 2nd? I'm happy to help test/deploy at either time.

Should be fine. I know the weekly security deployment window (UTC 22:00–00:00) isn't very Euro-friendly - would you prefer to try to add this to the European SWAT on March 2nd? I'm happy to help test/deploy at either time.

@Urbanecm @MarcoAurelio - does the above work for Monday, March 2nd?

sbassett moved this task from Backlog to In Progress on the user-sbassett board.

EU SWAT would be better, but I can make it if needed.

@Urbanecm - do we need to get this on the deployment schedule or no?

@MarcoAurelio - does 12:00–13:00 UTC March 2nd work for you?

@MarcoAurelio - does 12:00–13:00 UTC March 2nd work for you?

I can't make any promises. However @Urbanecm is now a steward and can access the production global blocking interface to test if needed.

recent changes made me do one other rebase...

13:58 <Urbanecm> !log Deploy security fix for T229731

This is now live, and per my test.wikipedia testing, it works.

@Urbanecm - Thanks for patching and deploying this! I've added this to the tracking bug for the next security release: T240393. So let's keep this task private and hold off on the backports/CVE for a bit.

Reedy added a comment.Mar 10 2020, 1:51 PM

@Urbanecm - Thanks for patching and deploying this! I've added this to the tracking bug for the next security release: T240393. So let's keep this task private and hold off on the backports/CVE for a bit.

Noting GlobalBlocking is not bundled, and it's not a core patch...

Noting GlobalBlocking is not bundled, and it's not a core patch...

Ah, yes. So... I guess if this patch seems stable in prod, we can make this task public and start the backports.

Reedy added a comment.Mar 10 2020, 2:31 PM

Noting GlobalBlocking is not bundled, and it's not a core patch...

Ah, yes. So... I guess if this patch seems stable in prod, we can make this task public and start the backports.

Yeah, WFM. If we get the patch into master now ish, it can go out with the train in a few hours

We still want a CVE for this one though, right?

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

Change 578566 had a related patch set uploaded (by SBassett; owner: Urbanecm):
[mediawiki/extensions/GlobalBlocking@master] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Change 578566 merged by SBassett:
[mediawiki/extensions/GlobalBlocking@master] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

sbassett added a comment.EditedMar 12 2020, 8:49 PM

I've submitted the CVE request for this issue - I'll update this task and T240400 once I have the ID. Gerrit couldn't seem to handle simple picks for the backports to 1.31, 1.33 and 1.34, so those will likely involve a bit more work, if they're even feasible.

I've submitted the CVE request for this issue - I'll update this task and T240400 once I have the ID. Gerrit couldn't seem to handle simple picks for the backports to 1.31, 1.33 and 1.34, so those will likely involve a bit more work, if they're even feasible.

Basically if there's any conflicts, gerrit says no. jgit sucks

Change 579427 had a related patch set uploaded (by Reedy; owner: Urbanecm):
[mediawiki/extensions/GlobalBlocking@REL1_34] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Change 579428 had a related patch set uploaded (by Reedy; owner: Urbanecm):
[mediawiki/extensions/GlobalBlocking@REL1_33] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Change 579429 had a related patch set uploaded (by Reedy; owner: Urbanecm):
[mediawiki/extensions/GlobalBlocking@REL1_31] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Change 579431 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/GlobalBlocking@master] Use IPUtils instead of deprecated IP class

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

Change 579427 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@REL1_34] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Change 579431 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@master] Use IPUtils instead of deprecated IP class

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

sbassett renamed this task from Global blocks: if an IP is within two ranges and one is locally disabled, GlobalBlock won't listen to the other one to Global blocks: if an IP is within two ranges and one is locally disabled, GlobalBlock won't listen to the other one (CVE-2020-10534).EditedMar 13 2020, 7:49 PM
sbassett added a subscriber: Jdforrester-WMF.

CVE here. Thanks, @Reedy, @DannyS712, @Jdforrester-WMF for getting the backports done!

sbassett closed this task as Resolved.Mar 13 2020, 7:52 PM
sbassett moved this task from In Progress to Our Part Is Done on the Security-Team board.
sbassett moved this task from Waiting to Done on the user-sbassett board.

CVE here. Thanks, @Reedy, @DannyS712, @Jdforrester-WMF for getting the backports done!

Not all the backports are merged yet - should this still be open?

Change 579428 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@REL1_33] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

sbassett reopened this task as Open.Mar 13 2020, 8:41 PM

Not all the backports are merged yet - should this still be open?

I just +2'd 1.33, so that should merge soon. Looking at the issue for 1.31, it's complaining about an actual phpcs file, nothing related to GlobalBlocking, from what I can tell. In that case, we can likely just manually submit and file a bug regarding the phpcs issue in quibble, yes?

Reedy added a comment.EditedMar 13 2020, 8:46 PM

Not all the backports are merged yet - should this still be open?

I just +2'd 1.33, so that should merge soon. Looking at the issue for 1.31, it's complaining about an actual phpcs file, nothing related to GlobalBlocking, from what I can tell. In that case, we can likely just manually submit and file a bug regarding the phpcs issue in quibble, yes?

It's basically an instance of T233759: Update mediawiki/mediawiki-codesniffer for php7.3 support in release branches (or rather, would be fixed by it)

Change 579429 merged by Reedy:
[mediawiki/extensions/GlobalBlocking@REL1_31] SECURITY: Apply most specific global block and make sure applied block matches block showed on Special:Contributions

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

Reedy closed this task as Resolved.Mar 24 2020, 5:41 PM