Page MenuHomePhabricator

CVE-2021-41801: ReplaceText continues performing actions if the user no longer has the correct permission (such as by being blocked)
Closed, ResolvedPublicSecurity

Description

I recently came across massive ongoing page move vandalism on a Miraheze wiki caused by the use of ReplaceText. I blocked the user expecting for the edits to stop, but they did not. I locked the account, and that made no difference. Thankfully I could enforce a temporary measure by creating an abuse filter, however this should not be necessary to stop ongoing text replacement.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

And it looks like replace text restarted the failed moves an hour later, which is also pretty problematic, since I thought it was safe to disable the abuse filter that was preventing the page moves.

Urbanecm triaged this task as High priority.Apr 1 2021, 7:46 PM

Will be pushing a patch soon to phab

RhinosF1 changed Author Affiliation from N/A to Other (Please specify in description).Apr 1 2021, 9:23 PM

if ( $permissionManager->userCan( ' replacetext ', $currentuser ) ) {
	$this->error('replacetext: permission no longer valid')
	return false;
}

isn't this logic flipped? Shouldn't there be a ! in the if statement - if the user can use the right, the permissions are valid.

-1:
return false means "repeat the job", you should return true. Also does the replacetext right correct here? I'm not 100% sure, shouldn't be some like edit the title?

Other point, this extension is not deployed in wmf production, it seems weird going through the security process of wmf for this.

Other point, this extension is not deployed in wmf production, it seems weird going through the security process of wmf for this.

It's still a security bug and the code is hosted with Gerrit.

-1:
return false means "repeat the job", you should return true. Also does the replacetext right correct here? I'm not 100% sure, shouldn't be some like edit the title?

Understood on the return. Not sure on the second part.

Other point, this extension is not deployed in wmf production, it seems weird going through the security process of wmf for this.

Replace Text is bundled in the tarball and should have security issues disclosed through the normal MediaWiki security releases.

Ack.
Regarding the second part: You're checking against "replacetext" right. I might be misunderstanding how rights work in mediawiki but the right should stay around even when the user is blocked, right? Let's draw a comparison between this right and "autopatrolled" right, if a user is autopatrolled and is blocked, the right doesn't go away. Shouldn't the check also be "edit"?

Regarding the second part: You're checking against "replacetext" right. I might be misunderstanding how rights work in mediawiki but the right should stay around even when the user is blocked, right? Let's draw a comparison between this right and "autopatrolled" right, if a user is autopatrolled and is blocked, the right doesn't go away. Shouldn't the check also be "edit"?

Not necessarily. The userCan method of the PermissionsManager checks whether or not the user is blocked for basically every permissions check, provided that the rigor is above quick. I'm also a bit confused by what you mean regarding the return values, given that the other error checks inside the Job return false upon an error. Does that mean they will still be retried? It doesn't make a whole lot of sense to me that the job would be retried if some of those failed.

Also, it looks like userCan also requires the page title as an argument. Which means this patch may not work as written.

Replace Text is bundled in the tarball and should have security issues disclosed through the normal MediaWiki security releases.

Indeed, and this patch should be held (i.e. not pushed through gerrit or otherwise disclosed - I know @Legoktm already knows this) for the next security release, which is probably not the one (T270458) that is due out within the next week or so, but the release after that.

Other point, this extension is not deployed in wmf production, it seems weird going through the security process of wmf for this.

The Security-Team would encourage the repurposing of the current process for wikimedia production security patch deployment for any and all wikimedia-related codebases. Such a process likely isn't possible to adopt and retool for certain projects, but our team is fine with mediawiki, etc. developers using Phabricator to create protected tasks to track and manage any potential security issues, even for code which is not deployed to wikimedia production.

Also, it looks like userCan also requires the page title as an argument. Which means this patch may not work as written.

This problem likely stems from confusion with other common userCan functions within various revision. etc classes (e.g. this one). But yes, PermissionManager::userCan requires a title. That should be available via ReplaceTextJob's constructor as it's used elsewhere within the run function. No clue if there are potential performance issues with this, and I don't have a great way to test that out. Anyhow, here's an updated patch:

return false means "repeat the job", you should return true.

It appears that all other error-handling logic within the run function returns false when an error is found. So either much of that code is broken or this patch should follow the same return false paradigm.

Also does the replacetext right correct here? I'm not 100% sure, shouldn't be some like edit the title?

It appears to be the right that gets checked for the corresponding special page.

return false means "repeat the job", you should return true.

It appears that all other error-handling logic within the run function returns false when an error is found. So either much of that code is broken or this patch should follow the same return false paradigm.

Please check the JobRunner class (search for "status"). That's not correct, there are different types of errors. There is an error when the database connection goes away, when there is a network partition and that's when the job should be retried. But also there is logical errors like missing permissions that returning false would just trigger reties (30 times) and puts big burden on the system. I understand this logic is terrible but it's what we have atm, if other jobs doing it, they are doing it wrong as well and should be fixed.

Please check the JobRunner class (search for "status"). That's not correct, there are different types of errors. There is an error when the database connection goes away, when there is a network partition and that's when the job should be retried. But also there is logical errors like missing permissions that returning false would just trigger reties (30 times) and puts big burden on the system. I understand this logic is terrible but it's what we have atm, if other jobs doing it, they are doing it wrong as well and should be fixed.

New patch with s/false/true/ for the return value (plus file move and syntax error fixes):


Lints fine, but didn't really test locally, so would appreciate some CR. In re-reading the original post, I'm not entirely sure this patch would be enough to halt jobs already within the queue? Looking at the special page, it looks like jobs are built within createJobsForTextReplacements (where this new PM check would happen) and then get pushed onto the queue within doSpecialReplaceText. So if the user was blocked after said push, but there were longer-running jobs (and/or a decent number of them), those would still execute, no?

RhinosF1 moved this task from Miraheze-Linked to Extensions & Core on the User-RhinosF1 board.

Need to decide if this patch is appropriate and can go out in the security release (due this week, as it is a bundled extension), or it is bumped to the next one.

Need to decide if this patch is appropriate and can go out in the security release (due this week, as it is a bundled extension), or it is bumped to the next one.

Pretty sure my patch from above never got CR'd and merged. Might need a rebase, but then we could sneak it in with the upcoming release? Assuming it really does work :) ReplaceText is definitely a bit of an oddball in that it's bundled but not Wikimedia-deployed.

Reedy renamed this task from ReplaceText continues performing actions if the user is blocked to CVE-2021-PENDING: ReplaceText continues performing actions if the user is blocked.Wed, Sep 29, 4:05 PM

CVE requested along with MW core ones due to extension being tarballed.

Reedy renamed this task from CVE-2021-PENDING: ReplaceText continues performing actions if the user is blocked to CVE-2021-41801: ReplaceText continues performing actions if the user is blocked.Wed, Sep 29, 8:45 PM

Just to point out this patch wouldn't pass PHPCS (due to spacing)... And wouldn't apply (to master) without using -3...

And there's no "error" function on Job.. It's a member variable to be assigned to.

Did anyone test this? :)

And $current_user is defined, then $currentuser is used...

And for another one...

There's a disconnect between "ReplaceText continues performing actions if the user is blocked" and adding a permissions check.

If the user is blocked, do they actually lose that permission?

Patch should apply (and be valid function calls) for 1.35, 1.36, 1.37 and master.

Will need reworking for 1.31...

After I test whether the blocked user still can 'replacetext' or not, that is.

Did anyone test this? :)

Per my previous comment:

Pretty sure my patch from above never got CR'd and merged.

Reedy renamed this task from CVE-2021-41801: ReplaceText continues performing actions if the user is blocked to CVE-2021-41801: ReplaceText continues performing actions if the user no longer has the correct permission (such as by being blocked).Thu, Sep 30, 3:22 PM
$ php maintenance/eval.php 
> $permissionManager = MediaWiki\MediaWikiServices::getInstance()->getPermissionManager();

> $current_user = User::newFromName( 'BlockdReplaceTextUser' );

> var_dump( $permissionManager->userCan( 'replacetext', $current_user, Title::newFromID( 371 ) ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(84) : eval()'d code:1:
bool(true)

Blocked the user (sitewide).

$ php maintenance/eval.php 
> $permissionManager = MediaWiki\MediaWikiServices::getInstance()->getPermissionManager();

> $current_user = User::newFromName( 'BlockdReplaceTextUser' );

> var_dump( $permissionManager->userCan( 'replacetext', $current_user, Title::newFromID( 371 ) ) );
/var/www/wiki/mediawiki/core/maintenance/eval.php(84) : eval()'d code:1:
bool(false)

So that part is fine

In re-reading the original post, I'm not entirely sure this patch would be enough to halt jobs already within the queue? Looking at the special page, it looks like jobs are built within createJobsForTextReplacements (where this new PM check would happen) and then get pushed onto the queue within doSpecialReplaceText. So if the user was blocked after said push, but there were longer-running jobs (and/or a decent number of them), those would still execute, no?

Kind of. It would only stop users who were blocked between the job being submitted, and the job being run.

One job is submitted to the job queue per page to be edited, so I don't think we need to worry about that. If a job has already been popped, and is running (past this check), but the user is then blocked before the edit, meh.

		$jobs = [];
		// These are OOUI checkboxes - we don't determine whether they
		// were checked by their value (which will be null), but rather
		// by whether they were submitted at all.
		foreach ( $request->getValues() as $key => $value ) {
			if ( $key !== 'replace' && $key !== 'use_regex' ) {
				if ( strpos( $key, 'move-' ) !== false ) {
					$title = Title::newFromID( (int)substr( $key, 5 ) );
					$replacement_params['move_page'] = true;
				} else {
					$title = Title::newFromID( (int)$key );
				}
				if ( $title !== null ) {
					$jobs[] = new Job( $title, $replacement_params );
				}
			}
		}

It would mostly fix the expected action of after a user being blocked that their replace job didn't continue. There might be some (depending on job queue length, number of job queue runners etc). I don't think there's much value checking it much later in the Job execution code. You're always going to get these edge cases.

In most cases, it should only mean a few edits to revert, not potentially hundreds etc.

Modified patch for REL1_31

commit cbf1b619ed12396901406d7027747dafac3fc00b (HEAD -> REL1_31)
Author: RhinosF1 <rhinosf1@gmail.com>
Date:   Wed Apr 21 14:16:18 2021 -0500

    SECURITY: Check permissions before job execution
    
    CVE-2021-41801
    
    Bug: T279090
    Change-Id: Ibc299edf626ca9aa1cd9d83b888820f5aca9af7c

diff --git a/src/ReplaceTextJob.php b/src/ReplaceTextJob.php
index 281fdb0..9f2678c 100644
--- a/src/ReplaceTextJob.php
+++ b/src/ReplaceTextJob.php
@@ -41,6 +41,14 @@ class ReplaceTextJob extends Job {
         * @return bool success
         */
        function run() {
+               // T279090
+               $current_user = User::newFromId( $this->params['user_id'] );
+               if ( !$this->title->userCan( 'replacetext', $current_user ) ) {
+                       $this->error = 'replacetext: permission no longer valid';
+                       // T279090#6978214
+                       return true;
+               }
+
                if ( isset( $this->params['session'] ) ) {
                        $callback = RequestContext::importScopedSession( $this->params['session'] );
                        $this->addTeardownCallback( function () use ( &$callback ) {

Change 725050 had a related patch set uploaded (by Reedy; author: RhinosF1):

[mediawiki/extensions/ReplaceText@REL1_36] SECURITY: Check permissions before job execution

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

Change 725051 had a related patch set uploaded (by Reedy; author: RhinosF1):

[mediawiki/extensions/ReplaceText@REL1_35] SECURITY: Check permissions before job execution

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

Change 725052 had a related patch set uploaded (by Reedy; author: RhinosF1):

[mediawiki/extensions/ReplaceText@REL1_31] SECURITY: Check permissions before job execution

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

Change 725053 had a related patch set uploaded (by Reedy; author: RhinosF1):

[mediawiki/extensions/ReplaceText@master] SECURITY: Check permissions before job execution

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

Change 725054 had a related patch set uploaded (by Reedy; author: RhinosF1):

[mediawiki/extensions/ReplaceText@REL1_37] SECURITY: Check permissions before job execution

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

Change 725052 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@REL1_31] SECURITY: Check permissions before job execution

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

Change 725054 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@REL1_37] SECURITY: Check permissions before job execution

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

Change 725051 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@REL1_35] SECURITY: Check permissions before job execution

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

Change 725050 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@REL1_36] SECURITY: Check permissions before job execution

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

Change 725053 merged by jenkins-bot:

[mediawiki/extensions/ReplaceText@master] SECURITY: Check permissions before job execution

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".
Reedy changed the edit policy from "Custom Policy" to "All Users".

Looks like it's broken on MediaWiki 1.35

Missing use MediaWiki\MediaWikiServices;

See https://www.mediawiki.org/wiki/Topic:Whs218tqkuhd8mln