Page MenuHomePhabricator

CVE-2021-30152: action=protect lets users with 'protect' permission protect to higher protection level
Closed, ResolvedPublicSecurity

Description

If a user with protect permission protects a page using APISandbox they can protect the page to a higher protection level than they can edit. If action=edit and action=protect are not restricted, the user can also unprotect the page

How to reproduce

  • Add the following to LocalSettings.php:
$wgGroupPermissions['protect']['protect'] = true;
  • Create an account and assign it to the protect group (for example, using maintenance scripts, php maintenance/createAndPromote.php --custom-groups protect TestUser TestPassword)
  • Login as the newly created user
  • Go to index.php?title=TestPage&action=protect
    • see that user can't protect to "allow only administrators"
    • Screenshot 2021-01-04 at 16.58.40.png (960×1 px, 142 KB)
  • Go to index.php?title=Special:ApiSandbox#action=protect&format=json&title=TestPage&protections=create%3Dsysop
    • you can also choose other protection levels and types from $wgRestrictionLevels and $wgRestrictionTypes respectively
    • click action=protect
    • hit "auto-fill the token"
    • hit "make request"
    • Screenshot 2021-01-04 at 17.05.20.png (616×686 px, 54 KB)
  • Go to index.php?title=TestPage&action=history
    • see that page is protected so that only administrators may create it
    • Screenshot 2021-01-04 at 17.06.16.png (1×982 px, 151 KB)
  • You can also remove the protection level again by going to Special:ApiSandbox#action=protect&format=json&title=TestPage&protections=create%3Dall
    • this doesn't work if you've previously restricted the edit and/or protect types
    • Screenshot 2021-01-04 at 17.13.56.png (616×712 px, 53 KB)

(Credit for finding this goes to my friend Magiczocker10)

Event Timeline

Tobi_406 renamed this task from APISandbox lets users with 'protect' permission bypass protection levels to APISandbox lets users with 'protect' permission protect to higher protection level.Dec 22 2020, 4:54 PM
Legoktm set Security to Software security bug.Dec 22 2020, 6:33 PM
Legoktm added projects: Security, Security-Team.
Legoktm changed the visibility from "Public (No Login Required)" to "Custom Policy".
Legoktm changed the subtype of this task from "Task" to "Security Issue".
Legoktm added a subscriber: Legoktm.

@Tobi_406 are you sure this is due to the api sandbox and not the api itself? The api should just be calling the relevant action specified and not override any of the behavior

The api should just be calling the relevant action specified and not override any of the behavior

Do you mean the api samdbox in that sentence?
But no, I'm not sure, I wasn't able to get it working to check with the API directly (I don't use it often, sorry).
So I used ApiSandbox and to make the task as detailed as possible I specified I used ApiSandbox.
But yes, I do think it has to do with the API, just don't have any evidence for that.

The api should just be calling the relevant action specified and not override any of the behavior

Do you mean the api samdbox in that sentence?
But no, I'm not sure, I wasn't able to get it working to check with the API directly (I don't use it often, sorry).
So I used ApiSandbox and to make the task as detailed as possible I specified I used ApiSandbox.
But yes, I do think it has to do with the API, just don't have any evidence for that.

Yes, sorry, I meant "the api sandbox should just be calling the relevant action specified and not override any of the behavior"

I will note that allowing users to protect higher than their rights allow isn't necessarily an issue. Being able to remove that is more of an issue.

And there's the discrepency between endpoints

Reedy updated the task description. (Show Details)

The user facing form does

		$levels = $this->permManager->getNamespaceRestrictionLevels(
			$this->mTitle->getNamespace(), $this->mContext->getUser()
		);

and

			$val = $request->getVal( "mwProtect-level-$action" );
			if ( isset( $val ) && in_array( $val, $levels ) ) {
				$this->mRestrictions[$action] = $val;
			}

The API doesn't do anything with getNamespaceRestrictionLevels. No validation

We can't fix the API allowed parameters, to be documented in the autogenerated page, as it can vary between pages (or, well, Namespaces)

			'protections' => [
				ApiBase::PARAM_ISMULTI => true,
				ApiBase::PARAM_REQUIRED => true,
			],

Ok, so looking further, the problem is the way the API does the validation;

			if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
				$this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
			}

It just uses effectively $wgRestrictionLevels which is not modified for the user

> var_dump( $wgRestrictionLevels );
/var/www/wiki/mediawiki/core/maintenance/eval.php(82) : eval()'d code:1:
array(3) {
  [0] =>
  string(0) ""
  [1] =>
  string(13) "autoconfirmed"
  [2] =>
  string(5) "sysop"
}

The error message is

	"apierror-protect-invalidlevel": "Invalid protection level \"$1\".",

So for a patch that should be good for production.. As yet untested...

diff --git a/includes/api/ApiProtect.php b/includes/api/ApiProtect.php
index 16f7a55a56..50f2521caf 100644
--- a/includes/api/ApiProtect.php
+++ b/includes/api/ApiProtect.php
@@ -67,6 +67,10 @@ class ApiProtect extends ApiBase {
                }
 
                $restrictionTypes = $titleObj->getRestrictionTypes();
+               $levels = $this->getPermissionManager()->getNamespaceRestrictionLevels(
+                       $titleObj->getNamespace(),
+                       $user
+               );
 
                $protections = [];
                $expiryarray = [];
@@ -85,7 +89,7 @@ class ApiProtect extends ApiBase {
                        if ( !in_array( $p[0], $restrictionTypes ) && $p[0] != 'create' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidaction', wfEscapeWikiText( $p[0] ) ] );
                        }
-                       if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
+                       if ( !in_array( $p[1], $levels ) && $p[1] != 'all' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
                        }

It will give the "Invalid protection level" error, which technically isn't correct, but is better than just allowing it.

Also, adding i18n messages into WMF production in security patches is painful. So we won't do that

We can do a followup/add on patch for this to add another message to be used in this case. It depends whether we really care about differentiating between "invalid" restrictions, and restrictions that are "invalid" (or rather, not allowed) for that user

Which means a patch more like

diff --git a/includes/api/ApiProtect.php b/includes/api/ApiProtect.php
index 16f7a55a56..7abedede93 100644
--- a/includes/api/ApiProtect.php
+++ b/includes/api/ApiProtect.php
@@ -67,6 +67,10 @@ class ApiProtect extends ApiBase {
                }
 
                $restrictionTypes = $titleObj->getRestrictionTypes();
+               $levels = $this->getPermissionManager()->getNamespaceRestrictionLevels(
+                       $titleObj->getNamespace(),
+                       $user
+               );
 
                $protections = [];
                $expiryarray = [];
@@ -88,6 +92,10 @@ class ApiProtect extends ApiBase {
                        if ( !in_array( $p[1], $this->getConfig()->get( 'RestrictionLevels' ) ) && $p[1] != 'all' ) {
                                $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
                        }
+                       if ( !in_array( $p[1], $levels ) && $p[1] != 'all' ) {
+                               // TODO: Add new message for "user isn't allowed to add this protection"
+                               $this->dieWithError( [ 'apierror-protect-invalidlevel', wfEscapeWikiText( $p[1] ) ] );
+                       }
 
                        if ( wfIsInfinity( $expiry[$i] ) ) {
                                $expiryarray[$p[0]] = 'infinity';

I don't imagine this is actually necessary, so we can probably just re-purpose this message for the master/deployment branch patches

is basically the first first posted above; no i18n changes

Reedy renamed this task from APISandbox lets users with 'protect' permission protect to higher protection level to action=protect lets users with 'protect' permission protect to higher protection level.Jan 4 2021, 6:11 PM

Wouldn't returning a permissions error be the better message? That message should already exist as well and the user is in fact missing the permission to protect to that level.

If you can find a better pre-existing message, sure.

Like I say, I'm not adding (or altering an existing) message for the patch deployed to Wikimedia production. We can do so for a patch that eventually lands in master and release branches.

There's some similar, but not quite right messages, for example:

	"protect-locked-access": "Your account does not have permission to change page protection levels.\nHere are the current settings for the page <strong>$1</strong>:",

It kinda looks like the form for protect just silently discards it where the user doesn't have the right. so some error from the API is better than the apparent silent failure

			$val = $request->getVal( "mwProtect-level-$action" );
			if ( isset( $val ) && in_array( $val, $levels ) ) {
				$this->mRestrictions[$action] = $val;
			}

Should probably get this deployed...

What's the status here? Is this fixed in master?

What's the status here? Is this fixed in master?

The security patch is deployed per T270713#6794141 (also tracked at T276237) and being held for the next security release, which I'd imagine will be out in the next couple of weeks. So no, not on master yet. The tracking task for embargoed security bugs is here: T270459.

Master patch applies cleanly on 1.35 and 1.31. Wheee.

Reedy renamed this task from action=protect lets users with 'protect' permission protect to higher protection level to [CVE-2021-30152] action=protect lets users with 'protect' permission protect to higher protection level.Apr 6 2021, 7:09 PM
Reedy renamed this task from [CVE-2021-30152] action=protect lets users with 'protect' permission protect to higher protection level to CVE-2021-30152: action=protect lets users with 'protect' permission protect to higher protection level.

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

[mediawiki/core@REL1_31] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

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

[mediawiki/core@REL1_35] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

Change 678032 merged by jenkins-bot:

[mediawiki/core@REL1_31] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

Change 678038 merged by jenkins-bot:

[mediawiki/core@REL1_35] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

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

[mediawiki/core@master] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

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

Change 678073 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

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

[mediawiki/core@REL1_36] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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

Change 677962 merged by jenkins-bot:

[mediawiki/core@REL1_36] SECURITY: Allow user to only apply protection they have right to do so via action=protect

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