CVE-2021-30159: Non-admin deleted enwiki page in fast double move

**

CVE-2021-30159: Non-admin deleted enwiki page in fast double move

Closed, ResolvedPublicSecurity

**

• Edit Task

• Edit Related Tasks…

• Edit Related Objects…

• Mute Notifications

• Protect as security issue

• Award Token

• Flag For Later

Something odd happened when a non-admin moved https://en.wikipedia.org/wiki/Draft:Reign_of_X to https://en.wikipedia.org/wiki/Reign_of_X at 10:48 (UTC), 19 January 2021.
Two moves were registered within a second and all revisions of the moved page at the time were deleted. I have restored them. As far as I can tell from page histories, logs, and deleted page histories:

Draft:Reign_of_X had 38 edits at the time and had never been a redirect.
At 10:48 Mr Scribbles666, an autoconfirmed user with 11 edits, moved Draft:Reign_of_X to Reign_of_X which didn’t exist at the time.
https://en.wikipedia.org/wiki/Special:Log/Mr_Scribbles666 says, in an admin account with time preference showing seconds and newest logs shown first:

2021-01-19T10:48:13 Mr Scribbles666 (talk|contribs|block) moved page Draft:Reign of X to Reign of X over redirect (thank)
2021-01-19T10:48:13 Mr Scribbles666 (talk|contribs|block) deleted redirect Reign of X by overwriting (G6: Deleted to make way for move) (view/restore) (thank)
2021-01-19T10:48:12 Mr Scribbles666 (talk|contribs|block) moved page Draft:Reign of X to Reign of X (revert) (thank)

“G6: Deleted to make way for move” is an automatic enwiki move summary made by https://en.wikipedia.org/wiki/MediaWiki:Delete_and_move_reason
These are the logs you would expect if the second move had been to a redirect to the source with no other edits in the page history, meaning non-admins can overwrite the redirect per https://en.wikipedia.org/wiki/Wikipedia:Moving_a_page#Moving_over_a_redirect

At the time of the second move, Draft:Reign_of_X was a redirect left behind by the first move with no other page history. But MediaWiki claims Mr Scribbles666 deleted a redirect at the target Reign_of_X. At the time there should have been 38 revisions and never any redirect. As far as I can tell, MediaWiki deleted those 38 revisions with no other entry in the logs.

The first move, the alleged redirect deletion and the second move are consecutive within 31 total log entries that minute:
https://en.wikipedia.org/w/index.php?title=Special:Log&offset=202101191049&limit=31&type=&user=

The two moves are also recorded in the page history of the source where they have two consecutive oldid and are shown 1 second apart. There were 186 oldid that minute between these:
https://en.wikipedia.org/w/index.php?oldid=1001364985
https://en.wikipedia.org/w/index.php?oldid=1001365170

Mr Scribbles666 was understandibly confused. Reign_of_X was now a redirect to itself with no other page history. He moved it back to https://en.wikipedia.org/wiki/Draft:Reign_of_X. This was an allowed non-admin move over a redirect. He and 110.33.26.115 (probably himself) tried some edits there which didn’t help. 110.33.26.115 then asked for help at
https://en.wikipedia.org/w/index.php?title=Wikipedia:Help_desk&oldid=1001379883#Please_get_back_the_original_version_of_Reign_of_X

That’s where I came in. After a thorough investigation I restored all 39 deleted revisions at Reign_of_X. This was the 38 original revisions and 1 revision about the first move which was deleted in the second move.

I don’t know whether Mr Scribbles666 did something unusual like double-clicking on "Move page". I didn’t want to draw attention in case it could be abused so I didn’t ask or indicate something strange was going on, and I’m filing this as a security bug.

• Task Graph
• Mentions

Event Timeline

Comment Actions

MovePage::isValidMoveTarget() uses FOR UPDATE, but it’s only called if Title::getArticleID() returns non-zero with no special flags. Then MovePage::moveToInternal() will delete the page if getArticleID(READ_LATEST) is non-zero. So it would seem that if the page is missing in the replica DB, isValidMove() will return true, and then moveToInternal() will unconditionally delete the page if it can be found in the master.

So it would seem that there’s quite a wide exploitable window for unprivileged deletion of movable pages.

Comment Actions

I was able to reproduce this with a lagged slave by inserting a sleep at the start of ChronologyProtector::shutdown(). Using two separate client IPs probably also would have worked. With a single client IP, the second request needs to execute after the first move completes, but before the first request commits the ChronologyProtector position.

Comment Actions

This patch appears to be sufficient and should be deployed as a security update.

diff --git a/includes/MovePage.php b/includes/MovePage.php index cdc994cab6…ffadf55586 100644 — a/includes/MovePage.php +++ b/includes/MovePage.php @@ -231,7 +231,9 @@ class MovePage {

    if ( $this->oldTitle->equals( $this->newTitle ) ) {
        $status->fatal( 'selfmove' );

- } elseif ( $this->newTitle->getArticleID() && !$this->isValidMoveTarget() ) {

•   } elseif ( $this->newTitle->getArticleID( Title::READ\_LATEST /\* T272386 \*/ )
  

•       && !$this->isValidMoveTarget()
  

•   ) {
        // The move is allowed only if (1) the target doesn't exist, or (2) the target is a
        // redirect to the source, and has no history (so we can undo bad moves right after
        // they're done). If the target is a single revision redirect to a different page,
  

It seems fragile though, since READ_LATEST flags often get lost during refactoring, so I’m thinking about other redundant ways to ensure this bug doesn’t recur.

Comment Actions

This patch appears to be sufficient and should be deployed as a security update.

Thanks for the patch! @Reedy or I can plan to deploy this on Monday, February 1st during the next security window.

It seems fragile though, since READ_LATEST flags often get lost during refactoring, so I’m thinking about other redundant ways to ensure this bug doesn’t recur.

Sounds good. If these result in additional security patches, we can deploy those as well. Or if they’re less sensitive and more code-hardening, I’d imagine they could just go through gerrit.

Reedy closed this task as Resolved.Apr 4 2021, 10:52 PM

Comment Actions

git am -3 applies the master patch on REL1_35. Trivial rebase needed for REL1_31 (it’s an if not an elseif, and code in a slightly different places)

Reedy renamed this task from Non-admin deleted enwiki page in fast double move to CVE-2021-30159: Non-admin deleted enwiki page in fast double move.Apr 6 2021, 7:14 PM

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL