0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patch
Legoktm (Legoktm)
Actions

Authored By

	Legoktm
	Dec 14 2021, 6:13 AM

Size

5 KB

Referenced Files

None

Subscribers

None

0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patch
View Options

	From 75af6b1e70873f79f10aa768c91b422dce71eeb9 Mon Sep 17 00:00:00 2001
	From: Kunal Mehta <legoktm@debian.org>
	Date: Wed, 8 Dec 2021 15:31:43 -0800
	Subject: [PATCH] SECURITY: Fix permissions checks in undo actions
	MIME-Version: 1.0
	Content-Type: text/plain; charset=UTF-8
	Content-Transfer-Encoding: 8bit

	Both traditional action=edit&undo= and the newer
	action=mcrundo/action=mcrrestore endpoints suffer from a flaw that
	allows for leaking entire private wikis by enumerating through revision
	IDs when at least one page was publicly accessible via $wgWhitelistRead.
	This is CVE-2021-44857.

	05f06286f4def removed the restriction that user-supplied undo IDs belong
	ot the same page, and was then copied into mcrundo. This check has been
	restored by using RevisionLookup::getRevisionByTitle(), which returns
	null if the revid is on a different page. This will break the workflow
	outlined in T58184, but that could be restored in the future with better
	access control checks.

	action=mcrundo/action=restore suffer from an additional flaw that allows
	for bypassing most editing restrictions. It makes no check on whether
	user has the 'edit' permission or can even edit that page (page
	protection, etc.). This is CVE-2021-44858.

	This has been fixed by requiring the 'edit' permission to even invoke
	the action (via Action::getRestriction()), as well as checking the
	user's permissions to edit the specific page before saving.

	The EditFilterMergedContent hook is also run against the revision before
	it's saved so SpamBlacklist, AbuseFilter, etc. have a chance to review
	the new page contents before saving.

	Kudos to Dylsss for the identification and report.

	Bug: T297322
	Co-authored-by: Taavi Väänänen <hi@taavi.wtf>
	Change-Id: I496093adfcf5a0e30774d452b650b751518370ce
	---
	includes/EditPage.php \| 6 ++--
	includes/actions/McrUndoAction.php \| 48 ++++++++++++++++++++++++++++--
	2 files changed, 50 insertions(+), 4 deletions(-)

	diff --git a/includes/EditPage.php b/includes/EditPage.php
	index fd76a63590..72ce66fcb5 100644
	--- a/includes/EditPage.php
	+++ b/includes/EditPage.php
	@@ -1288,8 +1288,10 @@ class EditPage implements IEditObject {
	$undo = $request->getInt( 'undo' );

	if ( $undo > 0 && $undoafter > 0 ) {
	- $undorev = $this->revisionStore->getRevisionById( $undo );
	- $oldrev = $this->revisionStore->getRevisionById( $undoafter );
	+ // The use of getRevisionByTitle() is intentional, as allowing access to
	+ // arbitrary revisions on arbitrary pages bypass partial visibility restrictions (T297322).
	+ $undorev = $this->revisionStore->getRevisionByTitle( $this->mTitle, $undo );
	+ $oldrev = $this->revisionStore->getRevisionByTitle( $this->mTitle, $undoafter );
	$undoMsg = null;

	# Sanity check, make sure it's the right page,
	diff --git a/includes/actions/McrUndoAction.php b/includes/actions/McrUndoAction.php
	index d500ad9d90..992543558d 100644
	--- a/includes/actions/McrUndoAction.php
	+++ b/includes/actions/McrUndoAction.php
	@@ -5,6 +5,7 @@
	* @ingroup Actions
	*/

	+use MediaWiki\MediaWikiServices;
	use MediaWiki\Revision\MutableRevisionRecord;
	use MediaWiki\Revision\RevisionLookup;
	use MediaWiki\Revision\RevisionRecord;
	@@ -72,6 +73,11 @@ class McrUndoAction extends FormAction {
	return '';
	}

	+ public function getRestriction() {
	+ // Require 'edit' permission to even see this action (T297322)
	+ return 'edit';
	+ }
	+
	public function show() {
	// Send a cookie so anons get talk message notifications
	// (copied from SubmitAction)
	@@ -142,8 +148,10 @@ class McrUndoAction extends FormAction {

	$this->initFromParameters();

	- $undoRev = $this->revisionLookup->getRevisionById( $this->undo );
	- $oldRev = $this->revisionLookup->getRevisionById( $this->undoafter );
	+ // We use getRevisionByTitle to verify the revisions belong to this page (T297322)
	+ $title = $this->getTitle();
	+ $undoRev = $this->revisionLookup->getRevisionByTitle( $title, $this->undo );
	+ $oldRev = $this->revisionLookup->getRevisionByTitle( $title, $this->undoafter );

	if ( $undoRev === null \|\| $oldRev === null \|\|
	$undoRev->isDeleted( RevisionRecord::DELETED_TEXT ) \|\|
	@@ -347,8 +355,44 @@ class McrUndoAction extends FormAction {
	return Status::newFatal( 'mcrundo-changed' );
	}

	+ $permissionManager = MediaWikiServices::getInstance()->getPermissionManager();
	+ $errors = $permissionManager->getPermissionErrors(
	+ 'edit', $this->context->getUser(), $this->getTitle()
	+ );
	+ if ( count( $errors ) ) {
	+ throw new PermissionsError( 'edit', $errors );
	+ }
	+
	$newRev = $this->getNewRevision();
	if ( !$newRev->hasSameContent( $curRev ) ) {
	+ $hookRunner = Hooks::runner();
	+ foreach ( $newRev->getSlotRoles() as $slotRole ) {
	+ $slot = $newRev->getSlot( $slotRole, RevisionRecord::RAW );
	+
	+ $status = new Status();
	+ $hookResult = $hookRunner->onEditFilterMergedContent(
	+ $this->getContext(),
	+ $slot->getContent(),
	+ $status,
	+ trim( $this->getRequest()->getVal( 'wpSummary' ) ),
	+ $this->getUser(),
	+ false
	+ );
	+
	+ if ( !$hookResult ) {
	+ if ( $status->isGood() ) {
	+ $status->error( 'hookaborted' );
	+ }
	+
	+ return $status;
	+ } elseif ( !$status->isOK() ) {
	+ if ( !$status->getErrors() ) {
	+ $status->error( 'hookaborted' );
	+ }
	+ return $status;
	+ }
	+ }
	+
	// Copy new slots into the PageUpdater, and remove any removed slots.
	// TODO: This interface is awful, there should be a way to just pass $newRev.
	// TODO: MCR: test this once we can store multiple slots
	--
	2.33.1

File Metadata

Mime Type: text/x-diff
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 9303425
Default Alt Text: 0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patch (5 KB)

0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patchLegoktm (Legoktm)Actions

0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patchView Options

File Metadata

Event Timeline

0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patch
Legoktm (Legoktm)
Actions

0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_37.patch
View Options