Page MenuHomePhabricator
Create Task
Maniphest T341529

CVE-2023-45362: diff-multi-sameuser ("X intermediate revisions by the same user not shown") ignores username suppression
Closed, ResolvedPublicSecurity
Actions

Description

diff-multi-sameuser ("X intermediate revisions by the same user not shown") ignores username suppression (information leak).

Reproduction:

  • Edit a page (diff 1)
  • Edit the page again (diff 2)
  • Edit the page again (diff 3)
  • Edit the page again (diff 4)
  • oversight/suppress diff 2's and 3's usernames from the revision history
  • open the revision history, select diff 1 and 4 for comparison
  • diff-multi-sameuser is displayed, leaking this information

Works on enwiki, tested at https://en.wikipedia.org/w/index.php?title=User%3AMusikAnimal%2Fsandbox&diff=1164068997&oldid=1114423881 . MusikAnimal has been looking for, and may have found, something else they'll then probably be reporting separately.

Details

Risk Rating
Medium
Author Affiliation
Wikimedia Communities
SubjectRepoBranchLines +/-
SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisionsmediawiki/coremaster+11 -1
SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisionsmediawiki/coreREL1_35+11 -1
SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisionsmediawiki/coreREL1_40+11 -1
SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisionsmediawiki/coreREL1_39+11 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

ToBeFree created this task.Jul 10 2023, 9:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2023, 9:34 PM
ToBeFree updated the task description. (Show Details)Jul 10 2023, 9:44 PM
ToBeFree added subscribers: MusikAnimal, Barkeep49.
MusikAnimal added a project: MediaWiki-Revision-deletion.Jul 10 2023, 9:58 PM
MusikAnimal added a project: MediaWiki-Page-diffs.
sbassett subscribed.Jul 18 2023, 6:14 PM
sbassett assigned this task to Mstyles.Jul 18 2023, 6:18 PM
sbassett added a comment.EditedJul 18 2023, 9:02 PM

Chatting with @Mstyles a bit, the issue seems to be from the counts being generated here within DifferenceEngine.php, which is calling countRevisionsBetween and getAuthorsBetween. $nEdits should likely be decremented for each revision where the user was suppressed. This might involve refactoring this a bit to instead use getRevisionIdsBetween and getRevisionById to check for whether the relevant user for the revision was suppressed or not.

sbassett changed the task status from Open to In Progress.Jul 18 2023, 9:02 PM
sbassett triaged this task as Medium priority.
sbassett moved this task from Incoming to In Progress on the Security-Team board.
sbassett added a project: Vuln-Infoleak.
sbassett added a project: SecTeam-Processed.Jul 18 2023, 9:20 PM
Mstyles added a comment.Aug 5 2023, 3:58 AM
From 2aefcccb125c3edab8b7f89d063a1e860540666e Mon Sep 17 00:00:00 2001
From: Maryum Styles <[email protected]>
Date: Fri, 4 Aug 2023 20:47:35 -0700
Subject: [PATCH] DifferentEngine: hide diff-multi-sameuser message for
 supressed revisions

if username has been suppressed then diff-multi-sameuser message will not be displayed

Bug: T341529
---
 includes/diff/DifferenceEngine.php | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 9349186191..4cd48bf361 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1734,12 +1734,22 @@ class DifferenceEngine extends ContextSource {

 		// Don't show the notice if too many rows must be scanned
 		// @todo show some special message for that case
-		$nEdits = $this->revisionStore->countRevisionsBetween(
+		$nEdits = 0;
+		$revisionIdList = $this->revisionStore->getRevisionIdsBetween(
 			$this->mNewPage->getArticleID(),
 			$oldRevRecord,
 			$newRevRecord,
 			1000
 		);
+		// only count revisions that are visible
+		if ( count( $revisionIdList ) > 0 ) {
+			foreach ( $revisionIdList as $revisionId ) {
+				$revision = $this->revisionStore->getRevisionById( $revisionId );
+				if ( $revision->getVisibility() == 0 ) {
+					$nEdits++;
+				}
+			}
+		}
 		if ( $nEdits > 0 && $nEdits <= 1000 ) {
 			// Use an invalid username to get the wiki's default gender (as fallback)
 			$newRevUserForGender = '[HIDDEN]';
--
2.40.0

I left out the suppression check since that is per user and not per revision. I tested, and any revisions that have been suppressed will not show the message, and revisions that are not suppressed will show the message. I think even if a user has suppression right, the suppressed revisions still should not show up for the diff-multi-sameuser message.

sbassett added a comment.Aug 8 2023, 10:30 PM

CR-1 for the above patch:

  1. Subject should use [SECURITY] or SECURITY: as opposed to just [PATCH]
  2. Subject should state "DifferenceEngine" as opposed to "DifferentEngine"
  3. I'm not quite certain the subject and commit message describe what should be happening for the fix. We do want to display the diff-multi-sameuser message (and the other two similar messages), we just want to reduce the edit count if a user for a given revision is suppress-deleted, for any performer or viewer who does not have appropriate rights.
  4. The patch is very close, but after testing locally, the following conditional should be changed from:
if ( $revision->getVisibility() == 0 ) {
    $nEdits++;
}

to:

if ( $revision->getUser( RevisionRecord::FOR_THIS_USER, $this->getAuthority() ) ) {
    $nEdits++;
}

That way we deal with the specific issue of the whether or not the user was suppress-deleted for the revision and if the performer has the appropriate suppress rights to view such information (via testing either the truthy UserIdentity or falsey null returned by RevisionRecord->getUser).

One last note is that the right that gets checked for this is deletedhistory not viewsuppressed as I'd originally assumed. At least according to the relevant portions of RevisionRecord->getUser and RevisionRecord::userCanBitfield.

Mstyles added a comment.Aug 10 2023, 8:25 PM

@sbassett thank you for the feedbacks and links! I've updated the patch as seen below

From 547015b1dd66e7309def288943b4694b91f2a4eb Mon Sep 17 00:00:00 2001
From: Maryum Styles <[email protected]>
Date: Fri, 4 Aug 2023 20:47:35 -0700
Subject: [SECURITY] DifferenceEngine: hide diff-multi-sameuser message for
 supressed revisions

reduce the edit count if a user for a given revision is suppress-deleted

Bug: T341529
---
 includes/diff/DifferenceEngine.php | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 9349186191..3ffdb780d2 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1734,12 +1734,22 @@ class DifferenceEngine extends ContextSource {

 		// Don't show the notice if too many rows must be scanned
 		// @todo show some special message for that case
-		$nEdits = $this->revisionStore->countRevisionsBetween(
+		$nEdits = 0;
+		$revisionIdList = $this->revisionStore->getRevisionIdsBetween(
 			$this->mNewPage->getArticleID(),
 			$oldRevRecord,
 			$newRevRecord,
 			1000
 		);
+		// only count revisions that are visible
+		if ( count( $revisionIdList ) > 0 ) {
+			foreach ( $revisionIdList as $revisionId ) {
+				$revision = $this->revisionStore->getRevisionById( $revisionId );
+				if ( $revision->getUser( RevisionRecord::FOR_THIS_USER, $this->getAuthority() ) ) {
+					$nEdits++;
+				}
+			}
+		}
 		if ( $nEdits > 0 && $nEdits <= 1000 ) {
 			// Use an invalid username to get the wiki's default gender (as fallback)
 			$newRevUserForGender = '[HIDDEN]';
--
2.40.0
sbassett added a comment.Aug 10 2023, 8:34 PM

Ok, I think we should likely use SECURITY: instead of [SECURITY] since the latter doesn't play nicely in git log, etc. Sorry about that, I thought either would've been fine.

Anyhow, after that, CR+2 for security deployment. We can maybe polish this up and add a test during release.

Mstyles added a comment.Aug 10 2023, 10:30 PM

@sbassett I didn't see too many examples of tests that had suppressed revisions but I would be more than happy to work on a test for this. I'll deploy this on Monday with the subject updated as below.

From 547015b1dd66e7309def288943b4694b91f2a4eb Mon Sep 17 00:00:00 2001
From: Maryum Styles <[email protected]>
Date: Fri, 4 Aug 2023 20:47:35 -0700
Subject: SECURITY: DifferenceEngine: hide diff-multi-sameuser message for
 supressed revisions

reduce the edit count if a user for a given revision is suppress-deleted

Bug: T341529
---
 includes/diff/DifferenceEngine.php | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 9349186191..3ffdb780d2 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1734,12 +1734,22 @@ class DifferenceEngine extends ContextSource {

 		// Don't show the notice if too many rows must be scanned
 		// @todo show some special message for that case
-		$nEdits = $this->revisionStore->countRevisionsBetween(
+		$nEdits = 0;
+		$revisionIdList = $this->revisionStore->getRevisionIdsBetween(
 			$this->mNewPage->getArticleID(),
 			$oldRevRecord,
 			$newRevRecord,
 			1000
 		);
+		// only count revisions that are visible
+		if ( count( $revisionIdList ) > 0 ) {
+			foreach ( $revisionIdList as $revisionId ) {
+				$revision = $this->revisionStore->getRevisionById( $revisionId );
+				if ( $revision->getUser( RevisionRecord::FOR_THIS_USER, $this->getAuthority() ) ) {
+					$nEdits++;
+				}
+			}
+		}
 		if ( $nEdits > 0 && $nEdits <= 1000 ) {
 			// Use an invalid username to get the wiki's default gender (as fallback)
 			$newRevUserForGender = '[HIDDEN]';
--
2.40.0
Mstyles moved this task from In Progress to Security Patch To Deploy on the Security-Team board.Aug 10 2023, 10:32 PM
sbassett added a comment.Aug 11 2023, 2:52 PM
In T341529#9085594, @Mstyles wrote:

@sbassett I didn't see too many examples of tests that had suppressed revisions but I would be more than happy to work on a test for this. I'll deploy this on Monday with the subject updated as below.

Yes, tests definitely aren't necessary to get a security patch deployed. But we can look into adding them when this eventually gets backported during the next security release (T340864).

Mstyles moved this task from Security Patch To Deploy to Waiting on the Security-Team board.Aug 14 2023, 9:38 PM
In T341529#9085594, @Mstyles wrote:

@sbassett I didn't see too many examples of tests that had suppressed revisions but I would be more than happy to work on a test for this. I'll deploy this on Monday with the subject updated as below.

From 547015b1dd66e7309def288943b4694b91f2a4eb Mon Sep 17 00:00:00 2001
From: Maryum Styles <[email protected]>
Date: Fri, 4 Aug 2023 20:47:35 -0700
Subject: SECURITY: DifferenceEngine: hide diff-multi-sameuser message for
 supressed revisions

reduce the edit count if a user for a given revision is suppress-deleted

Bug: T341529
---
 includes/diff/DifferenceEngine.php | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 9349186191..3ffdb780d2 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1734,12 +1734,22 @@ class DifferenceEngine extends ContextSource {

 		// Don't show the notice if too many rows must be scanned
 		// @todo show some special message for that case
-		$nEdits = $this->revisionStore->countRevisionsBetween(
+		$nEdits = 0;
+		$revisionIdList = $this->revisionStore->getRevisionIdsBetween(
 			$this->mNewPage->getArticleID(),
 			$oldRevRecord,
 			$newRevRecord,
 			1000
 		);
+		// only count revisions that are visible
+		if ( count( $revisionIdList ) > 0 ) {
+			foreach ( $revisionIdList as $revisionId ) {
+				$revision = $this->revisionStore->getRevisionById( $revisionId );
+				if ( $revision->getUser( RevisionRecord::FOR_THIS_USER, $this->getAuthority() ) ) {
+					$nEdits++;
+				}
+			}
+		}
 		if ( $nEdits > 0 && $nEdits <= 1000 ) {
 			// Use an invalid username to get the wiki's default gender (as fallback)
 			$newRevUserForGender = '[HIDDEN]';
--
2.40.0

deployed

Mstyles added a parent task: Restricted Task.Aug 14 2023, 9:39 PM
Mstyles moved this task from Waiting to Watching on the Security-Team board.Aug 14 2023, 9:41 PM
Reedy added projects: MW-1.35-release, MW-1.39-release, MW-1.40-release.Sep 27 2023, 10:46 AM
Reedy added a subscriber: gerritbot.Sep 27 2023, 12:09 PM
Reedy closed this task as Resolved.Sep 27 2023, 12:27 PM
Reedy subscribed.

Patch applies cleanly to REL1_35/REL1_39/REL1_40/master.

Closing for ease of tracking!

gerritbot added a comment.Sep 28 2023, 11:29 PM

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

[mediawiki/core@REL1_35] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a project: Patch-For-Review.Sep 28 2023, 11:29 PM

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

[mediawiki/core@REL1_39] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 28 2023, 11:31 PM

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

[mediawiki/core@REL1_40] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 28 2023, 11:32 PM

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

[mediawiki/core@master] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 28 2023, 11:50 PM

Change 961941 merged by jenkins-bot:

[mediawiki/core@REL1_39] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 29 2023, 12:10 AM

Change 961948 merged by jenkins-bot:

[mediawiki/core@REL1_40] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 29 2023, 12:20 AM

Change 961935 merged by Jforrester:

[mediawiki/core@REL1_35] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

gerritbot added a comment.Sep 29 2023, 12:40 AM

Change 961955 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: DifferenceEngine: hide diff-multi-sameuser message for supressed revisions

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

Reedy renamed this task from diff-multi-sameuser ("X intermediate revisions by the same user not shown") ignores username suppression to CVE-2023-45362: diff-multi-sameuser ("X intermediate revisions by the same user not shown") ignores username suppression.Oct 9 2023, 1:31 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.Oct 12 2023, 7:32 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett changed Risk Rating from N/A to Medium.
Maintenance_bot removed a project: Patch-For-Review.Oct 12 2023, 8:11 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits