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.

Will be pushing a patch soon to phab

In T279090#6966696, @RhinosF1 wrote:

T279090 (PS1).patch1 KBDownload

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.

In T279090#6970544, @Ladsgroup wrote:

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.

In T279090#6970543, @Ladsgroup wrote:

-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.

In T279090#6970544, @Ladsgroup wrote:

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"?

In T279090#6970957, @Ladsgroup wrote:

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.

In T279090#6970670, @Legoktm wrote:

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.

In T279090#6970544, @Ladsgroup wrote:

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.

In T279090#6971068, @TheVoidwalker wrote:

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:

In T279090#6970543, @Ladsgroup wrote:

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.

In T279090#6977982, @sbassett wrote:

In T279090#6970543, @Ladsgroup wrote:

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.

In T279090#6978214, @Ladsgroup wrote:

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):

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.

In T279090#7380601, @Reedy wrote:

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.

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

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.

In T279090#7391692, @Reedy wrote:

Did anyone test this? :)

Per my previous comment:

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

$ 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 T279090#7025771, @sbassett wrote:

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 ) {

The patches LGTM

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

Looks like it's broken on MediaWiki 1.35

Missing use MediaWiki\MediaWikiServices;

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

In T279090#7398361, @Ciencia_Al_Poder wrote:

Looks like it's broken on MediaWiki 1.35

Missing use MediaWiki\MediaWikiServices;

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

Should be fixed with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ReplaceText/+/725911/ and https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ReplaceText/+/725408.