Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F34883075
0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_36.patch
Legoktm (Legoktm)
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Authored By
Legoktm
Dec 14 2021, 6:13 AM
2021-12-14 06:13:32 (UTC+0)
Size
5 KB
Referenced Files
None
Subscribers
None
0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_36.patch
View Options
From d68bc863b00502b71b8f3bff040e668cd894e2dc 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 | 47 ++++++++++++++++++++++++++++--
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/includes/EditPage.php b/includes/EditPage.php
index 23c715fb45..d216d9d064 100644
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -1304,8 +1304,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 f34d6e4119..2699f209ca 100644
--- a/includes/actions/McrUndoAction.php
+++ b/includes/actions/McrUndoAction.php
@@ -42,6 +42,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)
@@ -114,8 +119,10 @@ class McrUndoAction extends FormAction {
$revisionLookup = MediaWikiServices::getInstance()->getRevisionLookup();
- $undoRev = $revisionLookup->getRevisionById( $this->undo );
- $oldRev = $revisionLookup->getRevisionById( $this->undoafter );
+ // We use getRevisionByTitle to verify the revisions belong to this page (T297322)
+ $title = $this->getTitle();
+ $undoRev = $revisionLookup->getRevisionByTitle( $title, $this->undo );
+ $oldRev = $revisionLookup->getRevisionByTitle( $title, $this->undoafter );
if ( $undoRev === null || $oldRev === null ||
$undoRev->isDeleted( RevisionRecord::DELETED_TEXT ) ||
@@ -321,8 +328,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;
+ }
+ }
+
$revisionStore = MediaWikiServices::getInstance()->getRevisionStore();
// Copy new slots into the PageUpdater, and remove any removed slots.
--
2.33.1
File Metadata
Details
Attached
Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
9303426
Default Alt Text
0001-SECURITY-Fix-permissions-checks-in-undo-actions-REL1_36.patch (5 KB)
Attached To
Mode
T297322: CVE-2021-44857, CVE-2021-44858: Unauthorized users can undo edits on any protected page and view contents of private wikis using mcrundo
Attached
Detach File
Event Timeline
Log In to Comment