From 3a9a067d3e021ab9e1267eb1cba1c5d5e791bef1 Mon Sep 17 00:00:00 2001 From: Kunal Mehta 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-44858. 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-44857. 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 Change-Id: I496093adfcf5a0e30774d452b650b751518370ce --- includes/EditPage.php | 6 ++-- includes/actions/McrUndoAction.php | 47 ++++++++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/includes/EditPage.php b/includes/EditPage.php index ca7deb474a..f2730f5b89 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -1297,8 +1297,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; # Make sure it's the right page, diff --git a/includes/actions/McrUndoAction.php b/includes/actions/McrUndoAction.php index f33436aafd..fb241e65b9 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 ) || @@ -349,8 +357,43 @@ 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. -- 2.33.1