@aaron, would you mind looking over these patches, particular the change to core? Thanks.

@dpatrick - minor comment on patches. I think it would be better to pass 'visibility' => Revision::FOR_PUBLIC instead of 'FOR_PUBLIC' as a string where we derive the constant. Otherwise I think things look good.

In T71367#1737201, @csteipp wrote:

@dpatrick - minor comment on patches. I think it would be better to pass 'visibility' => Revision::FOR_PUBLIC instead of 'FOR_PUBLIC' as a string where we derive the constant. Otherwise I think things look good.

Updated both patches.

@dpatrick, @aaron merged https://gerrit.wikimedia.org/r/#/c/248989/ a few days ago to AbuseFilter. Can you merge your patch on top of that?

@csteipp, yep, here you go.

I'm not sure this should block the security release. The core patch doesn't actually change any functionality or fix a vulnerability and AbuseFilter isn't bundled so it would just be announced and fixed anyway.

Late to the party, but -1. Using Revision::FOR_THIS_USER while filtering means that depending on the user's rights the variable would take a different value. I don't really have another solution, just making it clear that we avoid permissions checks for a reason.

From the task description:

1. Alice edits logged out, contacts oversight. Oversight does its thing. Later, Bob sees the struck out entry, goes to /examine and finds the IP. No abuse filters are ever tripped on the page.

This is "easy" to fix. We just need to check the appropriate visibility when retrieving vars from RC row. We may need to add a parameter to the AFComputedVariable object. If that's the case, the only thing to look out for is making AFComputedVariable check if that parameter exists: some abuse_filter_log rows hold a serialized AFComputedVariable object (T213006), and they don't have this parameter.

Side note: getLastPageAuthors doesn't differentiate RC rows. Instead, it will always display the last 10 authors at the time you're examining the RC (and not at the time of the edit). That also affects other variables, and is tracked at T102944. Some of the variables are easy to fix, others aren't. But I think it won't matter for the scope of this task.

1. Alice edits logged out, conatcts oversight. Oversight does its thing. Later, Bob sees the struck out entry, tries to replace the page with "fuck", then goes to his abuse log entry and finds the IP.

This is, I think, the problem originally reported in this task. As I've already said, both here and in T223654#5204202, checking the visibility when filtering means that the content of _contributor(s) variables will be different depending on the current user. The easy solution here would be to check rev_deleted, but again, is that correct?

An alternative: include deleted contributors but redact them in the stored var dump. Or even redact the whole variable content. However, this could be exploited (e.g., if the matched filter reads page_recent_contributors contains 'hiddenstuff', you check the value of page_recent_contributors and there's no 'hiddenstuff', then you know what's there). Moreover, it could be confusing even if there's no intention to exploit the leak (e.g., you can't easily figure out why the filter above matched, especially if that's buried into lots of other conditions).

But we can also check rev_deleted when filtering, and hope that the few edge cases where it could make a difference will really be edge.

1. Alice edits logged out, contacts oversight. Meanwhile, Vance, who knows nothing of these goings-on, tries to replace the page with "fuck". Too late, oversight does its thing. Later, Bob notices the struck-out entry, goes to the abuse log for the page, finds Vance's entry, and recovers the IP from the log.

I think this is very hard to fix. If the variable was computed and stored in the DB, there's no way to know when to delete it. Sure, we could add a handler for the ArticleRevisionVisibilitySet hook, query the AbuseLog for the given page, see if we find a match, and update the visibility accordingly. But that's a lot of stuff. If (1) and (2) are fixed, I find it acceptable (at least in the medium term) to just have oversights suppress any AbuseLog entry for the same page that happened in the meanwhile. For the long-term, we could really implement the hook handler, but I don't think it's urgent.

So I'm probably going to patch 1 and 2.

As part of my preliminary investigation, there are two more things to determine for the revision query.

1 - The query used by page_recent_contributors is weird. It will query the DB for the last 100 revisions, then extract the last 10 distinct authors. That's probably due to former performance issues, e.g. T116557. Hence, it's not already guaranteed that the variable will contain the last 10 authors. My question is whether deleted entries should be counted or not. That's because we must remove them altogether (adding a placeholder like "[redacted]" could bring to false positives). So we can either count them, and the variable will contain less than 10 authors, or just ignore them.

2 - There are two ways to filter deleted revisions out: with pure SQL, or in the foreach after the query. I don't know if the SQL version would be slower, but if we go for that, we need a way to set the bitmask. So, we'd benefit from T233222. If we want to do this in PHP, we can instead create a Revision object, and call userCan on it. In both cases, we'll need a User object to be passed in. However, given that page_first_contributor (see below) is already using FOR_PUBLIC, we may just use rev_deleted != 0 without the need for a User object (which we can leave as @todo).

Also, in my previous comment I said I was going to patch 2. I didn't actually mean to do that, due to the open question about 2. But thinking of that again, given that this is a long-standing security issue, I'm going with checking rev_deleted, and add a @todo for the edge cases. Also, given that page_first_contributor is already excluding deleted stuff, I don't think it'll be a problem.

Of note, page_first_contributor is already using FOR_PUBLIC. My fault, bad rename at T71367#5502820.

Last but not least, page_first_contributor has the same problem as page_recent_contributors for names already saved in the DB.

Rebased:

Everything from T71367#5503946 still applies.

Rebased

17:02 <Urbanecm> !log Deploy security patch (T71367)
17:02 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Deployed and tested. @sbassett Can you do backports, etc, please? Thanks!

Results of testing:

• (1.) is no longer exploitable; if an edit is suppressed, it's no longer possible for non-oversighters to examine it;
• (2.) is now fixed
• (3.) is unchanged as expected

NOTE: You can close this task once the backports etc. are out, but please don't make it public until the subtask is also fixed (which might as well never happen).

In T71367#6811589, @Daimona wrote:

Results of testing:

• (1.) is no longer exploitable; if an edit is suppressed, it's no longer possible for non-oversighters to examine it;
• (2.) is now fixed
• (3.) is unchanged as expected

NOTE: You can close this task once the backports etc. are out, but please don't make it public until the subtask is also fixed (which might as well never happen).

Also, for completeness, page_first_contributor is not affected because it was already using FOR_PUBLIC as audience.

In T71367#6811579, @Urbanecm wrote:

17:02 <Urbanecm> !log Deploy security patch (T71367)

Thanks.

Deployed and tested. @sbassett Can you do backports, etc, please? Thanks!

I'll try to get to the backports (at least for master) and CVE this week.

In T71367#6811589, @Daimona wrote:

NOTE: You can close this task once the backports etc. are out, but please don't make it public until the subtask is also fixed (which might as well never happen).

Sure, I'll tag with PermanentlyPrivate for the time being. If the subtask is ever completed, we can remove that label and/or make this task public. I assume the backports are fine to go through gerrit and we can at least announce the general issue within the supplemental security release announcement?

In T71367#6813446, @sbassett wrote:

I assume the backports are fine to go through gerrit and we can at least announce the general issue within the supplemental security release announcement?

Sure, as long as there's no reference to the subtask.

This patch no longer applies cleanly without -3way:

# --no-3way
brennen@deploy1002:/srv/mediawiki-staging/php-1.36.0-wmf.34/extensions/AbuseFilter$ git apply --no-3way --check /srv/patches/1.36.0-wmf.34/extensions/AbuseFilter/04-T71367.patch
error: patch failed: includes/Variables/LazyVariableComputer.php:305
error: includes/Variables/LazyVariableComputer.php: patch does not apply

# --3way
brennen@deploy1002:/srv/mediawiki-staging/php-1.36.0-wmf.34/extensions/AbuseFilter$ git apply --check --3way /srv/patches/1.36.0-wmf.34/extensions/AbuseFilter/04-T71367.patch
error: patch failed: includes/Variables/LazyVariableComputer.php:305
Falling back to three-way merge...
Applied patch to 'includes/Variables/LazyVariableComputer.php' cleanly.

See recent discussion on T269153 - I'm going to proceed with this unless I'm told otherwise, but the patch should be fixed.

In T71367#6897634, @brennen wrote:

See recent discussion on T269153 - I'm going to proceed with this unless I'm told otherwise, but the patch should be fixed.

For clarity, I presume you mean you're going to proceed with this patch applied using --3way right? :)

For clarity, I presume you mean you're going to proceed with this patch applied using --3way right? :)

Affirmative. Also discussed with @sbassett.

(We've traditionally gone ahead silently with patches that require --3way, but a recent process change to use scap apply-patches during train means it now fails for those and we fall back to the manual process. I think we're still sorting out how/whether this matters and what the process / tooling should be.)

This will be backported to master once this change set is merged: https://gerrit.wikimedia.org/r/670308

And here's an updated patch which should apply (without a 3-way fallback) to wmf.34 - I'll upload it to /srv/patches on deployment as well:

Holding off on making this public just yet per T71367#6813446.

Leaving this task open for now since the patch did not cleanly apply to REL1_31 and REL1_35, if it even will at all.

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

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Avoid deleted usernames leak in page_recent_contributors

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

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

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Avoid deleted usernames leak in page_recent_contributors

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

Change 678652 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Avoid deleted usernames leak in page_recent_contributors

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

Change 678653 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Avoid deleted usernames leak in page_recent_contributors

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