Page MenuHomePhabricator

CVE-2021-30155: Titleblacklist didn't prevent creation of pages by Special:ChangeContentModel when a rule was met
Closed, ResolvedPublicSecurity

Description

If a page does not exist, and someone just created it by [[Special:ChangeContentModel]], even if it matched [[MediaWiki:Titleblacklist]], it will still be created.

Eg. Add a line .* in [[MediaWiki:Titleblacklist]], then let other user (who is not a sysop) open [[Special:ChangeContentModel]], input any title, then change the model to whatever you like, then the page is created.

Steps to test: (all on testwiki)

  1. Revoke tboverride from template editors (https://gerrit.wikimedia.org/r/654449/)
  2. Add User:DannyS712\/Bug.* to title blacklist
  3. Grant template editor rights to an alternate test account
  4. Switch to using that alternate test account
  5. Try to create User:DannyS712/Bug 1 manually, to confirm its properly blacklisted
  6. Try to create it via Special:ChangeContentModel, to confirm the bug exists
  7. <deploy fix>
  8. Try to create User:DannyS712/Bug 2 via Special:ChangeContentModel, should fail

includes/content/ContentModelChange.php added in T107174: Add an API action to just change content model/rMW0789d1568d76: Add a ContentModelChange helper, and an api module that uses it

Event Timeline

Majavah set Security to Software security bug.Jan 1 2021, 9:38 AM
Majavah added projects: Security, Security-Team.
Majavah changed the visibility from "Public (No Login Required)" to "Custom Policy".
Majavah changed the subtype of this task from "Task" to "Security Issue".
Majavah added a subscriber: Majavah.

protecting as a security issue just in case

DannyS712 added a subscriber: DannyS712.

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.

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.

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

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?

Reedy renamed this task from Titleblacklist failed when creating pages by Special:ChangeContentModel to Titleblacklist didn't prevent creation of pages by Special:ChangeContentModel when a rule was met.Jan 4 2021, 4:17 PM
sbassett triaged this task as Medium priority.Jan 4 2021, 4:22 PM
sbassett moved this task from Incoming to In Progress on the Security-Team board.

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

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

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

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.

sbassett lowered the priority of this task from Medium to Low.

Deployed to .22 and .25 with @DannyS712's testing expertise. Let's hold this task/patch for the next security release (T270458).

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

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

Reedy renamed this task from Titleblacklist didn't prevent creation of pages by Special:ChangeContentModel when a rule was met to CVE-2021-30155: Titleblacklist didn't prevent creation of pages by Special:ChangeContentModel when a rule was met.Apr 6 2021, 7:11 PM

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

Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 8 2021, 9:07 PM

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