Skip to content

Commit

Permalink
SECURITY: Fix permissions checks in undo actions
Browse files Browse the repository at this point in the history
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.

05f0628 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 <hi@taavi.wtf>
Change-Id: I496093adfcf5a0e30774d452b650b751518370ce
  • Loading branch information
2 people authored and reedy committed Dec 15, 2021
1 parent 60543fc commit 1b10092
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 4 deletions.
6 changes: 4 additions & 2 deletions includes/EditPage.php
Expand Up @@ -1271,8 +1271,10 @@ protected function getContentObject( $def_content = null ) {
$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,
Expand Down
47 changes: 45 additions & 2 deletions includes/actions/McrUndoAction.php
Expand Up @@ -42,6 +42,11 @@ public function getDescription() {
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)
Expand Down Expand Up @@ -114,8 +119,10 @@ protected function checkCanExecute( User $user ) {

$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 ) ||
Expand Down Expand Up @@ -321,8 +328,44 @@ public function onSubmit( $data ) {
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.
Expand Down

0 comments on commit 1b10092

Please sign in to comment.