@Daimona Do you know if this is a mw.org specific problem by any chance?

In T277466#6937940, @Niharika wrote:

@Daimona Do you know if this is a mw.org specific problem by any chance?

Probably not. This seems like an old problem with the block logic. It wants to show a relevant log entry, and silently skips deleted ones without using a placeholder.

In T277466#6937966, @Daimona wrote:

In T277466#6937940, @Niharika wrote:

@Daimona Do you know if this is a mw.org specific problem by any chance?

Probably not. This seems like an old problem with the block logic. It wants to show a relevant log entry, and silently skips deleted ones without using a placeholder.

Thanks.

A quick pointer for engineers that will work on this task.

This bug doesn't seem block-specific. The log excerpt is generated by LogEventsList::showLogExtract(), which can be used for any log type. Internally, that method uses LogPager to get a list of entries. This is the same backend that is used when browsing Special:Log. In order to enforce visibility restrictions, LogPager adds SQL conditions doing something like

if ( !$this->getAuthority()->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) {
	$this->mConds[] = $this->mDb->bitAnd( 'log_deleted', LogPage::SUPPRESSED_ACTION ) . ' != ' . LogPage::SUPPRESSED_USER;
}

Which means that any deleted row will not even make it to the DB result. Due to this early filtering, once the query is ran there is no way to know if any rows were skipped due to lacking privileges.

For this task to be actionable, the query should be unfiltered, and the visibility of results should be checked afterwards. However, this would probably be specific to showLogExtract, in that the current SQL-level filtering makes sense for Special:Log. So I guess LogPager should have a flag (off by default) for skipping SQL-level filtering and applying visibility checks afterwards, e.g. replacing log entries with a placeholder. What makes this less trivial is that, while the example reported in this task uses a limit of 1 entry (and thus can either show the entry or a message saying that it was deleted), it is possible to specify arbitrary limits (default is 50), and then you might have some visible and deleted entries mixed together, for which a simple "all or nothing" wouldn't apply.

in that the current SQL-level filtering makes sense for Special:Log.

I think there's a bit of context into why that is in T19342 and, someone please correct me if I'm wrong, it seems like the intent was to prevent anyone from bruteforce looking up targets who were intended to be hidden. Given that this is happening on the target's page, I think it's safe to say that this is not the use case we're attempting to avoid.

So I guess LogPager should have a flag (off by default) for skipping SQL-level filtering and applying visibility checks afterwards, e.g. replacing log entries with a placeholder.

What should the placeholder say? Here's what it (sort of) could look like right now:

I'm not sure how clear it would be to state that the log entry does not exist, since the existence of even the placeholder would imply that there is still a block that is in effect.

What makes this less trivial is that, while the example reported in this task uses a limit of 1 entry (and thus can either show the entry or a message saying that it was deleted), it is possible to specify arbitrary limits (default is 50), and then you might have some visible and deleted entries mixed together, for which a simple "all or nothing" wouldn't apply.

I'm not very familiar with all the block use cases, but do you know of another instance where this sort of problem would occur? In places where we view multiple log lines, it seems like this is working as intended. It's only here, where we care that there's a latest and relevant block entry, that it's causing problems (because LogPager defines latest and relevant based on what the user is allowed to see, as opposed to what the system can see).

In T277466#6975125, @STran wrote:

What should the placeholder say?

I think we might start with something very simple, without checking the visibility of each field. So perhaps:

This user is currently blocked. You don't have enough privileges to view the latest block log entry.

It'd be better than the status quo, and should be easy to improve incrementally.

What makes this less trivial is that, while the example reported in this task uses a limit of 1 entry (and thus can either show the entry or a message saying that it was deleted), it is possible to specify arbitrary limits (default is 50), and then you might have some visible and deleted entries mixed together, for which a simple "all or nothing" wouldn't apply.

I'm not very familiar with all the block use cases, but do you know of another instance where this sort of problem would occur? In places where we view multiple log lines, it seems like this is working as intended. It's only here, where we care that there's a latest and relevant block entry, that it's causing problems (because LogPager defines latest and relevant based on what the user is allowed to see, as opposed to what the system can see).

I think block log entries are a bit "special", in that knowing that a user is blocked (especially if it's a long/infinite block) is much more important than knowing e.g. that a certain page revision was deleted. I haven't audited the usages of LogEventsList::showLogExtract, though.

tl;dr - let's just do the flag proposal and if the edge case where we need accurately counted logs let's solve it then.

Hm...okay! So I did a very shallow audit by grepping for showLogExtract and then looking at the comments to make a quick judgement on whether or not it was going to be a problem. I'm looking for evidence that we should prioritize thinking about the more complicated fix - that there will be a case where we will need to show the not-first result regardless of the user's permissions.

$ ag showLogExtract -l
includes/page/ImagePage.php
includes/page/Article.php
includes/specials/SpecialRevisionDelete.php
includes/specials/SpecialContributions.php
includes/specials/SpecialEditTags.php
includes/specials/SpecialUpload.php
includes/specials/SpecialUndelete.php
includes/specials/SpecialPageLanguage.php
includes/specials/SpecialChangeContentModel.php
includes/specials/SpecialUserrights.php
includes/specials/SpecialMovepage.php
includes/specials/SpecialBlock.php
includes/specials/SpecialMergeHistory.php
includes/specials/SpecialDeletedContributions.php
includes/ProtectionForm.php
includes/FileDeleteForm.php
includes/actions/HistoryAction.php
includes/EditPage.php
includes/Hook/ProtectionForm__showLogExtractHook.php
includes/logging/LogEventsList.php
includes/HookContainer/HookRunner.php
HISTORY

grepping for blocked-notice-logextract as well as looking for other 'lim' => 1 usages shows other candidates for the flag fix (alongside includes/specials/SpecialContributions.php):

includes/page/Article.php
includes/EditPage.php
includes/specials/SpecialMovepage.php
includes/specials/SpecialDeletedContributions.php // although technically fine I think because only admins can see this page

I confirmed with Article.php and presumably the others will be similar.

The possibly problematic page is includes/specials/SpecialBlock.php, which can contain conflicting information after the proposed flag change. It's possible for a user to see that another user is blocked but see no evidence of it in the logs. Given that this all came about because we wanted to hide brute-force searching for target information, I think it's reasonable to maintain this discrepancy since if you have the target's username (to access their Special:Contributions page), then we aren't leaking any additional information.

Some due diligence:

Out of all of these, these are lists of revision histories as opposed to block information and presumably do not need to be changed:

includes/page/ImagePage.php
includes/specials/SpecialRevisionDelete.php
includes/specials/SpecialEditTags.php
includes/specials/SpecialUpload.php
includes/specials/SpecialUndelete.php
includes/specials/SpecialMergeHistory.php
includes/ProtectionForm.php
includes/FileDeleteForm.php
includes/actions/HistoryAction.php

and I don't know what these are but the comments also make them look like lists and in the case of lists, I think we want to maintain the invisibility of log lines with detailed omitted so I'm going to hope they're not problems because they also don't deal with block data:

includes/specials/SpecialPageLanguage.php
includes/specials/SpecialUserrights.php
includes/specials/SpecialChangeContentModel.php

Given all this, I think it's best to assume for now that we only have these use cases:

• Users should not be aware of blocks they do not have permission to see when accessing block log lists
• Users should be aware of the latest block when a page calls for the single latest block, regardless if they have permission to see the logline details

And therefore we only need to do the flag solution which can be done by:

• adding a parameter to LogEventsList::showLogExtract() which will pass it along to LogPager::enforceActionRestrictions() and LogPager::enforcePerformerRestrictions(), allowing it to bypass the getAuthority() checks
• Update pages as necessary to use this flag

I suppose technically, this reduces some of the security that was implemented earlier. Given that it's all internal, do we need to think about any other preventative measures? Say if the flag is set allowHidden or whatever, we enforce a limit of 1?

Change 678841 had a related patch set uploaded (by STran; author: STran):

[mediawiki/core@master] Allow for single logline retrieval via flag

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

In T277466#6998264, @Niharika wrote:

As being discussed on the patch above, there are security concerns to doing this. Capturing what @DannyS712 wrote on the patch: [...]

While I do agree that there are security concerns with the current approach, I think that is not enough to decline this task. In particular, there are several alternatives that would improve the status quo without security concerns. For instance, resolving this task can be achieved without showing the log entry at all. I believe the patch above could be easily adapted to do that; it would look something like:

LogEventsList::showLogExtract( [ ..., 'showAtMostLatest' => true ] );

// [When building SQL conds]
if ( $this->showAtMostLatest ) {
    // Unset visibility conditions from the SQL where clause
}

// [ in formatRow() or similar place; pre: $row is the current row]
if ( $this->showAtMostLatest ) {
    $canBeViewed = canViewSingleRow( $row ); // This method would use the same logic of enforceActionRestrictions
    if ( !$canBeViewed ) {
        return '';
    }
    return $this->mLogEventsList->logLine( $row );
}

With this code in place, if the block log is deleted we would simply not show anything on Special:Contributions (ensure it doesn't build an empty box, though), which is much better than showing a non-relevant entry. At the same time, there should be no info leaks; in fact, we'd even show less information than what we currently do (because non-relevant entries aren't shown).

I've added myself as reviewer on gerrit, so feel free to ping me there for any clarification. I can also push my own attempt as a POC.

Change 678841 abandoned by STran:

[mediawiki/core@master] Allow for single logline retrieval via flag

Reason:

security

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

Well that update timing went well. 🙃 A few things:

• Showing nothing means that if a user exists and is blocked, but the log information is suppressed, then the viewer will not know that the user is blocked. I'm not sure that's a better status quo although I agree that the status quo of "wrong information" isn't great either.*
• Something I came across while doing this and why the patch I pushed up doesn't add the message like we discussed earlier is because at the time the message is being generated at showLogExtract, I wasn't sure if we could tell if the user had permission to view the log line. As it turns out we do since we pass along a log_deleted column which can be checked against during the proposed canBeViewed.

Anyway it's possible, but I think @sbassett has said that we just can't do this from a legal perspective (from the patch):

I concur with Danny's -2 above. I don't mind the idea of this functionality being added to core, but it cannot be enabled by default within Wikimedia production, if for nothing else various legal reasons. Abstracting showSingleLatest to a config layer would likely be fine.

If we wanted to continue this, it sounds like it'd be as a config and for other mediawiki users.

Change 679352 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] [POC] Don't show non-relevant block entries on Special:Contributions

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

Above is my stab at it, implementing the solution I outlined above.

In T277466#6998462, @STran wrote:

• Showing nothing means that if a user exists and is blocked, but the log information is suppressed, then the viewer will not know that the user is blocked. I'm not sure that's a better status quo although I agree that the status quo of "wrong information" isn't great either.*

Well, if we put it this way, showing a non-relevant entry would be an info leak. Either we show the entry or don't show it. Showing a non-relevant entry (which in turn implies that there's a hidden relevant entry) doesn't feel right in any case.

• Something I came across while doing this and why the patch I pushed up doesn't add the message like we discussed earlier is because at the time the message is being generated at showLogExtract, I wasn't sure if we could tell if the user had permission to view the log line. As it turns out we do since we pass along a log_deleted column which can be checked against during the proposed canBeViewed.

Yup, that's how I've just implemented it (i.e. log_deleted). It's the same check used in enforceActionRestrictions, except at the PHP-level (as opp. to SQL-level).

Anyway it's possible, but I think @sbassett has said that we just can't do this from a legal perspective (from the patch):

I concur with Danny's -2 above. I don't mind the idea of this functionality being added to core, but it cannot be enabled by default within Wikimedia production, if for nothing else various legal reasons. Abstracting showSingleLatest to a config layer would likely be fine.

I think my implementation above should be free of similar concerns, since no deleted information is being shown at all.

In T277466#6998605, @Niharika wrote:

Pinging @sbassett and @DannyS712 for their thoughts on the above proposal.

In T277466#6998591, @Daimona wrote:

Anyway it's possible, but I think @sbassett has said that we just can't do this from a legal perspective (from the patch):

I concur with Danny's -2 above. I don't mind the idea of this functionality being added to core, but it cannot be enabled by default within Wikimedia production, if for nothing else various legal reasons. Abstracting showSingleLatest to a config layer would likely be fine.

I think my implementation above should be free of similar concerns, since no deleted information is being shown at all.

So the primary security issues are that we don't want to display suppressed information to users who aren't supposed to see it, no matter what (which I believe the original patch did) and we also don't want to introduce novel ways for malicious actors to enumerate such data, which @Daimona's approach appears to mitigate, though I haven't reviewed it in-depth. The data enumeration issue is lower-risk for sure, given the non-triviality of the attack (brute-forcing an enormous set of possible strings) but is still not something we should ever consciously introduce in mw core, etc.

it is then possible to tell that the users that are hidden actually exist, and thus leaking the hidden information.

It is not possible to hide the existence of hidden or suppressed users completely as you may check whether the user exist using Special:CreateAccount (see T298312#7600168). The things we do is to make the difference of suppressed accounts and non-existent ones as little as possible.

In T277466#7015794, @STran wrote:

Going to pass this one back to @Daimona since they've got the patch up but I'll be around for review if you need it!

Thank you, I've removed the "POC" tag from the patch, so it's formally ready for review now.

One thing I'm wondering about is what's more important for the end user to see? Assuming that we've mitigated the data enumeration risk, if a user is viewing the Special:Contributions page as intended, is it more important for them to know that the user is blocked? Or is it more important that any block information the user sees is correct?

I think if a block was hidden, then nobody should know that the user is blocked. At least in the current state of things. Personally, I believe that it should always be possible to know if a user is blocked, but since some features explicitly prevent this (e.g. hideuser), then nobody should know as I said. And more generally, the block information should always be correct IMHO. Currently, it is possible to know that a user is blocked just by chance, if a previous block entry exists. But this doesn't work if no previous, visible entry exists. As such, I think that the current situation is a direct consequence of a bug, and should be remediated.

For a user's block status, it's visible via Special:CentralAuth even if the block log is hidden. (This may also apply to users that is locally suppressed but neither globally suppressed or hidden.)

I am going to go ahead and close this ticket + abandon the patch due to security reasons mentioned all over this conversation from 2021.

If the concern is T277466#6998264, the current system is already flawed per T277466#7024242 and T298312#7600168.

Change #679352 abandoned by Hashar:

[mediawiki/core@master] Don't show non-relevant block entries on Special:Contributions

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

Change #679352 restored by Thcipriani:

[mediawiki/core@master] Don't show non-relevant block entries on Special:Contributions

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