protecting as a security issue just in case

ContentModelChange doesn't go through the normal edit page checks before creating the new revision, but rather uses WikiPage::doEditContent after implementing some checks. Since TitleBlacklistHooks::onUserCan and the subsequent code takes into account the action, and allows for disallowing page creation while allowing page edits, tweaking core's ContentModelChange::checkPermissions to also run checks for the createpage/createtalk action if the page doesn't already exist should allow TitleBlacklist to prevent the edit accordingly

Might be okay to post the patch publically on gerrit with an inconspicuous commit message, but something like replacing line 115 of the current code (https://gerrit.wikimedia.org/g/mediawiki/core/+/8ee4aa0df786d1216b02d49c3ae36bc468ef31a2/includes/content/ContentModelChange.php#115) with:

$creationErrors = [];
if ( !$current->exists() ) {
	$creationAction = $current->isTalkPage() ? 'createtalk' : 'createpage';
	$creationErrors = $pm->getPermissionErrors( $creationAction, $user, $current );
}
$errors = wfMergeErrorArrays(
	// Potentially include creation errors, if applicable
	$creationErrors,

I was about to log off when I saw this task, and wanted to document ^^^ and will try and see if that actually works to fix the issue soon

Danny's patch works for me with TitleBlacklist, I'll add a test for that and attach a patch file here shortly.

That's using createpage or createtalk, MediaWiki-extensions-ArticleCreationWorkflow looks to be using create[0], I'll also test if that is vulnerable.

[0] https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/ArticleCreationWorkflow/+/refs/heads/master/includes/Hooks.php#31

PS1, which is Danny's original fix in T270988#6715865:

PS2, including my change to also check for create action which some extensions use:

I tried to add a test but failed with keeping the patch fairly small, I think that can be added after the fix publicly in Gerrit.

In T270988#6715890, @Majavah wrote:

PS1, which is Danny's original fix in T270988#6715865:

T270988-PS1.patch1 KBDownload

PS2, including my change to also check for create action which some extensions use:

T270988-PS2.patch1 KBDownload

I tried to add a test but failed with keeping the patch fairly small, I think that can be added after the fix publicly in Gerrit.

Looking through the various Constraints in EditPage::internalAttemptSave

• ContentModelChangeConstraint checks if the user has editcontentmodel and can use it on the title with the new content model, as well as if they can edit the page with the new content model
• CreationPermissionConstraint checks if the user can create the page
• EditRightConstraint checks if the user can edit the page

So I don't think my original fix (or PS2) is ideal. Pretty sure it should only be checking create here, not createpage/createtalk

See also T271038: Use edit constraints within ContentModelChange

PermissionManager will check for createpage/createtalk user rights when the action parameter is set to create . I don't think it checks for hooks modifying createpage/createtalk but all hooks I've seen do check for create, some also for createpage/createtalk.

PS3, only checking for create:

PS3 - LGTM, CR+1, untested though. @sbassett @Reedy do you think that this can be done publically on gerrit with an inconspicuous commit message?

@sbassett - Hi, unfortunately 22 UTC is midnight local time which is too late for me.

In T270988#6719712, @sbassett wrote:

@DannyS712 - eh, let's be cautious and deploy this during the security window today. Would you or @Majavah be around at 22:00 UTC to help test? If not, I can probably manage the testing myself or just undeploy if anything bad happens.

I should be able to be around at 22:00 UTC tomorrow (didn't see it for today) but we'll need to figure out how I can test this, since I don't know if there are wikis where I have admin rights to be able to set up the blacklist appropriately, and there is a local user group that has editcontentmodel but does not have tboverride (eg testwiki's template editors have both rights, as do enwikis)

In T270988#6721467, @DannyS712 wrote:

I should be able to be around at 22:00 UTC tomorrow

Ok, let's try for that.

we'll need to figure out how I can test this, since I don't know if there are wikis where I have admin rights to be able to set up the blacklist appropriately

testwiki? We're both sysops there and I can definitely edit MediaWiki:Titleblacklist to try some examples pre and post patch.

Failed to deploy today due to train issues, particularly T271259. Will try again tomorrow, 2021-01-06.

Hi, could you please use a common date format (which doesn't make an international audience think of First of June for a short moment)? Thanks a lot. :)

Um... What happened to the patch for this? It doesn't seem to be deployed on WMF production, and the patch doesn't apply cleanly to master...

Conflict is from rMW932fbe24646d: Convert ContentModelChange to Authority

Patch as originally posted works fine on REL1_35.

For REL1_31, just needs moving back to includes/specials/SpecialChangeContentModel.php instead, and updates to use getUserPermissionErrors

commit 821b2250d57c2834b85ff31aeaa51df4edbf7024 (HEAD -> REL1_31)
Author: DannyS712 <dannys712.enwiki@gmail.com>
Date:   Fri Jan 1 12:40:41 2021 +0200

    SECURITY: ContentModelChange: Check that user can create pages
    
    Co-authored-by: Taavi Väänänen <hi@tassu.me>
    Change-Id: I2e3b79f36fa7c0a3ec4130de0ae9c68104cb3fdd

diff --git a/includes/specials/SpecialChangeContentModel.php b/includes/specials/SpecialChangeContentModel.php
index 87c899f4e0..8204dde46d 100644
--- a/includes/specials/SpecialChangeContentModel.php
+++ b/includes/specials/SpecialChangeContentModel.php
@@ -169,8 +169,16 @@ class SpecialChangeContentModel extends FormSpecialPage {
                $titleWithNewContentModel = clone $this->title;
                $titleWithNewContentModel->setContentModel( $data['model'] );
                $user = $this->getUser();
+
+               $creationErrors = [];
+               if ( !$current->exists() ) {
+                       $creationErrors = $this->title->getUserPermissionErrors( 'create', $user );
+               }
+
                // Check permissions and make sure the user has permission to:
                $errors = wfMergeErrorArrays(
+                       // Potentially include creation errors, if applicable
+                       $creationErrors,
                        // edit the contentmodel of the page
                        $this->title->getUserPermissionsErrors( 'editcontentmodel', $user ),
                        // edit the page under the old content model

New patch for master

commit 180e0403774d535133c3b32d51ffa0fb4122df7b (HEAD -> master)
Author: DannyS712 <dannys712.enwiki@gmail.com>
Date:   Fri Jan 1 12:40:41 2021 +0200

    SECURITY: ContentModelChange: Check that user can create pages
    
    Co-authored-by: Taavi Väänänen <hi@tassu.me>
    Change-Id: I2e3b79f36fa7c0a3ec4130de0ae9c68104cb3fdd

diff --git a/includes/content/ContentModelChange.php b/includes/content/ContentModelChange.php
index 16a2ee520a..258bdf4953 100644
--- a/includes/content/ContentModelChange.php
+++ b/includes/content/ContentModelChange.php
@@ -113,6 +113,9 @@ class ContentModelChange {
                $titleWithNewContentModel->setContentModel( $this->newModel );
 
                $status = PermissionStatus::newEmpty();
+               if ( !$current->exists() ) {
+                       $authorizer( 'create', $current, $status );
+               }
                $authorizer( 'editcontentmodel', $current, $status );
                $authorizer( 'edit', $current, $status );
                $authorizer( 'editcontentmodel', $titleWithNewContentModel, $status );

Which just leaves the question as to why it was undeployed from master... I'm guessing it's because of the conflict, maybe... But that really isnt' an acceptable reason.

It wasn't ever on the list on {T276237} either...

Re-deployed master patch to wmf.37. Tracking again at T276237.

Change 678040 had a related patch set uploaded (by Reedy; author: DannyS712):

[mediawiki/core@REL1_35] SECURITY: ContentModelChange: Check that user can create pages

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

Change 678040 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: ContentModelChange: Check that user can create pages

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

There is something wrong in the patch for 1.31. $current is not defined in line 174, it should be $this->title. And getUserPermissionErrors should be getUserPermissionsErrors in line 175.

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

[mediawiki/core@REL1_31] Bug: T270988

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