Page MenuHomePhabricator

page_recent_contributors leaks revdeleted user names (CVE-2021-31545)
Closed, ResolvedPublic

Description

Spotted during code review for T69793 - https://gerrit.wikimedia.org/r/#/c/145014/ .


@Daimona: I can't see T71367, but are you sure it's the same? To be clear, there are really three ways page_recent_contributors (and, I just realized, page_first_contributor) can reveal a hidden username:

  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.
  2. Alice edits logged out, conatcts oversight. Oversight does its thing. Later, Bob sees the struck out entry, tries to replace the page with "[expletive redacted]", then goes to his abuse log entry and finds the IP.
  3. Alice edits logged out, contacts oversight. Meanwhile, Vance, who knows nothing of these goings-on, tries to replace the page with "[expletive redacted]". 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.

(3), indeed, sounds like a PITA to fix, since the username is already baked into the log, but it's not going to be exploitable in most cases. Plus, oversighters can suppress Vance's entry as well, if they remember to look for it. However, couldn't (1) and (2) be fixed by checking rev_deleted in getLastPageAuthors(), etc.?

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dpatrick changed the task status from Stalled to Open.Oct 13 2015, 9:52 PM
dpatrick added a subscriber: aaron.

@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.

@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.

Daimona renamed this task from article_recent_contributors leaks revdeleted user names to page_recent_contributors and page_first_contributor leak revdeleted user names.Sep 18 2019, 12:29 PM
Daimona removed a project: Security-Extensions.
Daimona updated the task description. (Show Details)
Daimona removed a subscriber: wikibugs-l-list.

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.

Daimona renamed this task from page_recent_contributors and page_first_contributor leak revdeleted user names to page_recent_contributors leaks revdeleted user names.Sep 18 2019, 5:12 PM
Daimona added subscribers: sbassett, Reedy, Bawolff.

Meh, I changed my mind again. At this point, I think FOR_PUBLIC is better than FOR_THIS_USER + permissions check. With the former, at least you know what to expect. The second can vary unpredictably.

I sneaked in the todos for the DB part by linking the phab task without further explanations. If you think they're too obvious, I can remove them.

Again, CC @sbassett @Reedy, @Huji and @matej_suchanek for review/deployment.

Note: Ideally, this task should not be made public until the subtask is also resolved.

Daimona added a subscriber: Tks4Fish.

Rebased:

Everything from T71367#5503946 still applies.

Urbanecm added a subscriber: Urbanecm.
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).

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.

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.

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?

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.

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.)

sbassett lowered the priority of this task from High to Low.Mar 9 2021, 9:50 PM
sbassett removed a project: Patch-For-Review.

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.

sbassett updated the task description. (Show Details)
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
Restricted Application changed the visibility from "Public (No Login Required)" to "Custom Policy". · View Herald TranscriptMar 12 2021, 3:31 PM
Restricted Application changed the edit policy from "All Users" to "Custom Policy". · View Herald Transcript
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

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

sbassett renamed this task from page_recent_contributors leaks revdeleted user names to page_recent_contributors leaks revdeleted user names (CVE-2021-31545).Apr 23 2021, 3:48 PM