This message comes from this bit in Article.php, added in rMWdc703748d469: Provide link to view diff of deleted revision in missing-rev message:

				// T251066: Try loading the revision from the archive table.
				// Show link to view it if it exists and the user has permission to view it.
				$pa = new PageArchive( $title, $this->getContext()->getConfig() );
				$revRecord = $pa->getArchivedRevisionRecord( $oldid );
				if ( $revRecord && $revRecord->audienceCan(
					RevisionRecord::DELETED_TEXT,
					RevisionRecord::FOR_THIS_USER,
					$this->getContext()->getUser()
				) ) {
					$text = wfMessage(
						'missing-revision-permission', $oldid,
						$revRecord->getTimestamp(),
						$title->getPrefixedDBkey()
					)->plain();
				} else {
					$text = wfMessage( 'missing-revision', $oldid )->plain();
				}

The audienceCan() here looks correct to me, so I looked deeper. The $revRecord is an instance of RevisionArchiveRecord. And it looks like RevisionArchiveRecord (representing a revision of a deleted page) has no special checking for permissions, it just inherits from RevisionRecord (representing generally any kind of a revision). That class has all the checks for revision deletion in userCanBitfield(), but I don't see anything in either class that would actually check whether the user is allowed to view revisions of deleted pages. It seems that RevisionArchiveRecord should check that, but it doesn't.

This seems bad so I'm making this task private. I haven't found a way to access anything I shouldn't (e.g. I can't actually view the deleted revision by following the link in the example you gave), but it might be possible if someone continues looking into it.

Also CC'ing @Ammarpad who has been active in this area.

I saw this task before it was made private and I looked at the code since then.

The code in Article still looks correct to me, the error seems to stems from PageArchive::getArchivedRevisionRecord() or further down the stack.

Locally, I noticed that the revision record returned by PageArchive::getArchivedRevisionRecord() may not have proper visibility flag, that's the visibility check above is evaluating to truthy for everyone. (though not for the actual revisions as far as I can see).

Here's a simple way to reproduce locally:

1. Have a page with rev, say x
2. Now delete the page completely.
3. Construct instance of PageArchive for the page.
4. Use PageArchive::getArchivedRevisionRecord() to get revision x.
5. Now use any RevisionRecord method or property to check visibility of x.

The result will say the revision is visible while it shouldn't.

The erroneous result will then be passed to RevisonRecord::audienceCan() then to RevisionRevord::userCan(). From there it will terminates at
ReviosionRecord::userCanBitfield() which will shortcircuits and return true because its $bitfied arg is now 0 which is the incorrect visibility bitfield of the archived revision in question. I am not sure how that came to be, but apparently it's not caused by the above linked patch.

I'm looking at this again, since a duplicate was filed: T345037.

Summary of the previous comments: The message is displayed incorrectly, because RevisionArchiveRecord::userCan() only checks the revision-deletion permissions, but not the page deletion permissions, causing us to think that the user could view the non-revision-deleted revision of the deleted page.

Is this bug a security bug? Technically, we're revealing that the given revision ID belonged to the given page title, and we're revealing its timestamp, both of which are not supposed to be public information. This seems like a very, very low severity issue to me.

@sbassett Could you help us decide whether this task could be made public?

Proposed patch: (I'll submit to Gerrit instead if this task is made public)

Are there any more serious issues caused by the same pattern? I've reviewed (hopefully) all code that deals with RevisionArchiveRecord, using these searches:

• https://codesearch.wmcloud.org/search/?q=ArchivedRevisionLookup&files=\.php%24&excludeFiles=&repos=&i=fosho
• https://codesearch.wmcloud.org/search/?q=newRevisionFromArchiveRow&files=\.php%24&excludeFiles=&repos=

It looks like everything checks that the user has the necessary page deletion permissions ('undelete', 'deletedhistory', 'deletedtext') before revealing anything from the RevisionArchiveRecord – with the exception of CheckUser, which reveals some information about deleted revisions to users with checkuser permissions, which is a known problem reported at T268147 (and mostly theoretical, since checkusers are trusted with more private information anyway).

Should RevisionArchiveRecord::userCan() also check the page deletion permissions?

We should still consider whether RevisionArchiveRecord::userCan() should check that the user can view deleted pages, in addition to the currently existing checks that the user can view revision-deleted revisions.

Arguments for:

• As evidenced by this bug, forgetting it is an easy mistake to make
• RevisionArchiveRecord extends RevisionRecord, so your code could receive a RevisionArchiveRecord without being aware of it and leak stuff (however, there are currently separate methods for getting a RevisionRecord and a RevisionArchiveRecord)

Arguments against:

• It would break CheckUser until something is done about T268147
• These methods are only called when showing revision-deletable parts of a revision, but not when showing e.g. its timestamp or the related title, so additional permission checks are necessary anyway

I'm not really convinced either way. Since it would require at least some refactoring in CheckUser, I think we should leave this unchanged for now.

In T264765#9137884, @matmarex wrote:

Is this bug a security bug? Technically, we're revealing that the given revision ID belonged to the given page title, and we're revealing its timestamp, both of which are not supposed to be public information. This seems like a very, very low severity issue to me.

@sbassett Could you help us decide whether this task could be made public?

It was basically disclosed in T345037 and it seems low-risk enough IMO that this could go through gerrit. I think it would be advisable to get this deployed sooner than later though, so if it doesn't make the train cut this week (it likely won't) we should probably pick it to 1.41.0-wmf.25 and deploy it by Thursday.

The message is displayed incorrectly, because RevisionArchiveRecord::userCan() only checks the revision-deletion permissions, but not the page deletion permissions, causing us to think that the user could view the non-revision-deleted revision of the deleted page.

In the proposed patch, it looks like the existing conditional is checking RevisionRecord::DELETED_TEXT, which should check for the deletedtext right via userCanBitfield, if RevisionArchiveRecord is inheriting from RevisionRecord, no? Is the new isAllowedAny call within the patch intending to also check for deletedhistory? Otherwise it seems like we're checking for the deletedtext right twice?

userCanBitfield() only checks the permissions if the given field is marked as deleted, which it usually isn't in this case (instead the whole page is deleted). I copied my isAllowedAny() check from other places that deal with viewing content of revisions of deleted pages, notably ApiComparePages.php (which is about diffs, just like the buggy feature here).

But some places seem to only check 'deletedhistory' for the same thing (not all the result shown).

I think what would be more future-proof approach here is to override userCanBitfield() in RevisionArchiveRecord and consolidate the logic that'd take in the fact the entire revisions require extra privilege to be accessed there. Subsequently these checks such as the one in ApiComparePages.php may no longer be necessary.

Something like this (untested)

public static function userCanBitfield( $bitfield, $field, Authority $performer, PageIdentity $page = null ) {
    $permissions = [ 'deletedhistory', 'undelete' ]; // any required for all deleted contents

    if ( $bitfield & $field ) { 
       if ( $bitfield & self::DELETED_RESTRICTED ) { // for suppressed, deleted contents
          $permissions[] = 'suppressrevision';
	  $permissions[] = 'viewsuppressed';
       } elseif ( $bitfield & self::DELETED_TEXT ) { // for rev-deleted, deleted contents
	  $permissions[] = 'deletedtext';
       }
    }
    ...
}

In T264765#9145063, @Ammarpad wrote:

But some places seem to only check 'deletedhistory' for the same thing (not all the result shown).

deletedhistory is different from deletedtext – the first allows you to see the metadata about revisions of deleted pages, the second allows the content. (ref)

undelete is a bit of a mess – I think back in the day, this one right used to cover both viewing and undeleting revisions, and the viewing has been split off to the other two rights – however some parts of the code still allow undelete to check for viewing, either as a back-compat measure or because the code hasn't been properly updated.

The important part for me is that the checks for displaying the link should be the same as the checks done by the page that the link points to, which is what I implemented.

I think what would be more future-proof approach here is to override userCanBitfield() in RevisionArchiveRecord and consolidate the logic that'd take in the fact the entire revisions require extra privilege to be accessed there. Subsequently these checks such as the one in ApiComparePages.php may no longer be necessary.

I mentioned that as well earlier (see "Should RevisionArchiveRecord::userCan() also check the page deletion permissions?" in T264765#9137884), it may be a good change but it seems less obvious to me. I tried writing a patch for this now, if anyone would like to give it a try:

In T264765#9144657, @matmarex wrote:

userCanBitfield() only checks the permissions if the given field is marked as deleted, which it usually isn't in this case (instead the whole page is deleted). I copied my isAllowedAny() check from other places that deal with viewing content of revisions of deleted pages, notably ApiComparePages.php (which is about diffs, just like the buggy feature here).

Ok, sounds good, even if this all is a bit confusing. I'd consider this low risk to go through gerrit. Again, it'd be nice if we could get it deployed to production sooner than later, which I can help out with tomorrow or even Friday (even though, yes, I know we shouldn't generally do Friday deploys).

Change 955372 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Article: Check permissions before showing link to view deleted revision

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

Should RevisionArchiveRecord::userCan() also check the page deletion permissions?

IMO yes. But otherwise, the phpdoc should at least make it very clear what is and isn't checked.

Change 955372 merged by jenkins-bot:

[mediawiki/core@master] Article: Check permissions before showing link to view deleted revision

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

Change 955054 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.41.0-wmf.24] Article: Check permissions before showing link to view deleted revision

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

Change 955055 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.41.0-wmf.25] Article: Check permissions before showing link to view deleted revision

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

Change 955054 merged by jenkins-bot:

[mediawiki/core@wmf/1.41.0-wmf.24] Article: Check permissions before showing link to view deleted revision

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

Change 955055 merged by jenkins-bot:

[mediawiki/core@wmf/1.41.0-wmf.25] Article: Check permissions before showing link to view deleted revision

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

Ok, I see this is getting deployed now via the backport window. Thanks.

Deployed now (I guess @Stashbot couldn't access the task to notify about that, I didn't think to add it before).

@sbassett Could you make the task public now?

I filed T345777: Consider making RevisionArchiveRecord::userCan() also check the page deletion permissions to discuss @Ammarpad's proposal.

Change 961218 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_40] Article: Check permissions before showing link to view deleted revision

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

Change 961219 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):

[mediawiki/core@REL1_39] Article: Check permissions before showing link to view deleted revision

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

Change 961219 merged by jenkins-bot:

[mediawiki/core@REL1_39] Article: Check permissions before showing link to view deleted revision

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

Change 961218 merged by jenkins-bot:

[mediawiki/core@REL1_40] Article: Check permissions before showing link to view deleted revision

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

FWIW, it's really helpful that if fixes like this are made in public, they are also backported appropriately at the time...

This got CVE-2023-45364 assigned: https://www.cve.org/CVERecord?id=CVE-2023-45364