Page MenuHomePhabricator

CVE-2022-28203: Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down
Closed, ResolvedPublicSecurity

Description

I have been seeing a lot of slow queries like this:

wikiadmin@10.64.16.175(commonswiki)> explain SELECT  /*! STRAIGHT_JOIN */ img_name,img_timestamp,actor_user,actor_name  FROM `image` JOIN `actor` ON ((actor_id=img_actor)) LEFT JOIN `user_groups` ON (ug_group = 'bot' AND (ug_user = actor_user) AND (ug_expiry IS NULL OR ug_expiry >= '20211214173620'))   WHERE actor_name = 'Gov.mm' AND (ug_group IS NULL) AND (((img_timestamp<'20211214000000')))  ORDER BY img_timestamp DESC LIMIT 51  ;
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
| id   | select_type | table       | type   | possible_keys                     | key           | key_len | ref                                | rows     | Extra                   |
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
|    1 | SIMPLE      | image       | range  | img_timestamp,img_actor_timestamp | img_timestamp | 14      | NULL                               | 29263285 | Using where             |
|    1 | SIMPLE      | actor       | const  | PRIMARY,actor_name                | actor_name    | 257     | const                              | 1        | Using where             |
|    1 | SIMPLE      | user_groups | eq_ref | PRIMARY,ug_group,ug_expiry        | PRIMARY       | 261     | commonswiki.actor.actor_user,const | 1        | Using where; Not exists |
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
3 rows in set (0.004 sec)

The straight join is actually problematic, if you remove it, MySQL realizes the user doesn't exists and return with empty result right away:

wikiadmin@10.64.16.175(commonswiki)> explain SELECT  img_name,img_timestamp,actor_user,actor_name  FROM `image` JOIN `actor` ON ((actor_id=img_actor)) LEFT JOIN `user_groups` ON (ug_group = 'bot' AND (ug_user = actor_user) AND (ug_expiry IS NULL OR ug_expiry >= '20211214173620'))   WHERE actor_name = 'Gov.mm' AND (ug_group IS NULL) AND (((img_timestamp<'20211214000000')))  ORDER BY img_timestamp DESC LIMIT 51  ;
+------+-------------+-------+------+---------------+------+---------+------+------+-----------------------------------------------------+
| id   | select_type | table | type | possible_keys | key  | key_len | ref  | rows | Extra                                               |
+------+-------------+-------+------+---------------+------+---------+------+------+-----------------------------------------------------+
|    1 | SIMPLE      | NULL  | NULL | NULL          | NULL | NULL    | NULL | NULL | Impossible WHERE noticed after reading const tables |
+------+-------------+-------+------+---------------+------+---------+------+------+-----------------------------------------------------+
1 row in set (0.001 sec)

wikiadmin@10.64.16.175(commonswiki)> SELECT  img_name,img_timestamp,actor_user,actor_name  FROM `image` JOIN `actor` ON ((actor_id=img_actor)) LEFT JOIN `user_groups` ON (ug_group = 'bot' AND (ug_user = actor_user) AND (ug_expiry IS NULL OR ug_expiry >= '20211214173620'))   WHERE actor_name = 'Gov.mm' AND (ug_group IS NULL) AND (((img_timestamp<'20211214000000')))  ORDER BY img_timestamp DESC LIMIT 51  ;
Empty set (0.001 sec)

You can replace "Gov.mm" with any gibberish and it would still goes in direction of reading 29M rows. Suggestion: Just run a check if the actor exists before making the query.

Event Timeline

It is the same with existing actors too:

wikiadmin@10.64.16.175(commonswiki)> explain SELECT  /*! STRAIGHT_JOIN */ img_name,img_timestamp,actor_user,actor_name  FROM `image` JOIN `actor` ON ((actor_id=img_actor)) LEFT JOIN `user_groups` ON (ug_group = 'bot' AND (ug_user = actor_user) AND (ug_expiry IS NULL OR ug_expiry >= '20211214173620'))   WHERE actor_name = 'Ladsgroup' AND (ug_group IS NULL) AND (((img_timestamp<'20211214000000')))  ORDER BY img_timestamp DESC LIMIT 51  ;
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
| id   | select_type | table       | type   | possible_keys                     | key           | key_len | ref                                | rows     | Extra                   |
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
|    1 | SIMPLE      | image       | range  | img_timestamp,img_actor_timestamp | img_timestamp | 14      | NULL                               | 29263338 | Using where             |
|    1 | SIMPLE      | actor       | const  | PRIMARY,actor_name                | actor_name    | 257     | const                              | 1        | Using where             |
|    1 | SIMPLE      | user_groups | eq_ref | PRIMARY,ug_group,ug_expiry        | PRIMARY       | 261     | commonswiki.actor.actor_user,const | 1        | Using where; Not exists |
+------+-------------+-------------+--------+-----------------------------------+---------------+---------+------------------------------------+----------+-------------------------+
3 rows in set (0.001 sec)

While without STRAIGHT_JOIN it's much faster:

wikiadmin@10.64.16.175(commonswiki)> explain SELECT img_name,img_timestamp,actor_user,actor_name  FROM `image` JOIN `actor` ON ((actor_id=img_actor)) LEFT JOIN `user_groups` ON (ug_group = 'bot' AND (ug_user = actor_user) AND (ug_expiry IS NULL OR ug_expiry >= '20211214173620'))   WHERE actor_name = 'Ladsgroup' AND (ug_group IS NULL) AND (((img_timestamp<'20211214000000')))  ORDER BY img_timestamp DESC LIMIT 51  ;
+------+-------------+-------------+-------+-----------------------------------+---------------------+---------+-------------+------+--------------------------+
| id   | select_type | table       | type  | possible_keys                     | key                 | key_len | ref         | rows | Extra                    |
+------+-------------+-------------+-------+-----------------------------------+---------------------+---------+-------------+------+--------------------------+
|    1 | SIMPLE      | actor       | const | PRIMARY,actor_name                | actor_name          | 257     | const       | 1    |                          |
|    1 | SIMPLE      | user_groups | const | PRIMARY,ug_group,ug_expiry        | PRIMARY             | 261     | const,const | 0    | Unique row not found     |
|    1 | SIMPLE      | image       | range | img_timestamp,img_actor_timestamp | img_actor_timestamp | 22      | NULL        | 395  | Using where; Using index |
+------+-------------+-------------+-------+-----------------------------------+---------------------+---------+-------------+------+--------------------------+
3 rows in set (0.003 sec)
Ladsgroup triaged this task as High priority.
Ladsgroup added a project: DBA.

It needs a simple join decomposition. I will never understand the fascination of mediawiki with join. Each purge makes around 200 queries with warm cache and 1k with cold one and we are worried about one more really fast query, it's not making them across the Atlantic ocean, the network latency is small.

Here is the fix. I think we need to roll this out to all pagers that set the user and do straight join but that's for later. I appreciate code review on this.

It would be nice to roll this out this week before the code freeze - great catch Amir!

Ladsgroup renamed this task from Requesting Special:NewFiles in commons with non-existentant actors can bring the whole database down to Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down .Dec 14 2021, 6:49 PM

Here is the fix. I think we need to roll this out to all pagers that set the user and do straight join but that's for later. I appreciate code review on this.

That seems fine for now, yes. C+2.

Thanks for the deploy, @Ladsgroup. Now tracked at T276237 and will eventually be part of the next security release, due out around the end of March 2022.

We should monitor https://logstash.wikimedia.org/goto/7d3662149d8277dd9a9574c833b41163 to see if it's fully stopped.

Log is clean. I move this to done but won't close it so it gets properly processed by security team.

Log is fully cleaned up. This is done.

Reedy renamed this task from Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down to CVE-2022-: Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down .Mar 28 2022, 1:52 PM
Reedy renamed this task from CVE-2022-: Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down to CVE-2022-28203: Requesting Special:NewFiles in commons with actor as a condition can bring the whole database down .Mar 30 2022, 6:03 PM

Change 775972 had a related patch set uploaded (by Reedy; author: Amir Sarabadani):

[mediawiki/core@REL1_35] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775972 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775977 had a related patch set uploaded (by Reedy; author: Amir Sarabadani):

[mediawiki/core@REL1_36] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775983 had a related patch set uploaded (by Reedy; author: Amir Sarabadani):

[mediawiki/core@REL1_37] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775977 merged by jenkins-bot:

[mediawiki/core@REL1_36] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775983 merged by jenkins-bot:

[mediawiki/core@REL1_37] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775991 had a related patch set uploaded (by Reedy; author: Amir Sarabadani):

[mediawiki/core@master] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775994 had a related patch set uploaded (by Reedy; author: Amir Sarabadani):

[mediawiki/core@REL1_38] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775994 merged by jenkins-bot:

[mediawiki/core@REL1_38] SECURITY: pagers: Don't make a straight join if the user is set

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

Change 775991 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: pagers: Don't make a straight join if the user is set

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 31 2022, 11:05 PM
Reedy changed the edit policy from "Custom Policy" to "All Users".