Page MenuHomePhabricator

AbuseFilterCheckMatch API reveals suppressed edits and usernames (CVE-2021-31547)
Closed, ResolvedPublic

Description

The fix for T207085 is incomplete.

(Part 1) Suppressed usernames can still be recovered (by any user, even logged out) from page histories via the page_recent_contributors variable. This can be done by examining any un-suppressed recent change to the page, or (possibly) by triggering a filter that lazy-loads that variable. Of course, this will only work until ten distinct editors touch the page, but for some pages (for instance user pages that have been accidentally edited logged out) that could take a very long time.
-->This is covered by T71367

(Part 2) Both api.php?action=abusefiltertestmatch and Special:Abusefilter/test allow anyone with the abusefilter-view-private or abusefilter-modify rights to recover, one bit at a time, the full details of any suppressed abuse log entry, and the partial details of any suppressed recent change. For example, suppose new_wikitext == "XYZZY".

Then we can try:

new_wikitext rlike "^[\x80-\xff]" (false, so first bit is 0)
new_wikitext rlike "^[\x40-\x7f]" (true, so first bits are 01)
new_wikitext rlike "^[\x60-\x7f]" (false, so first bits are 010)
...

This would be tedious to do by hand, but a trivial script could unhide any desired variable within minutes or hours. The same trick also works on recent changes, but only on the username and summary, not the wikitext.

Event Timeline

Part 1 is duplicate of T71367, and the solution is not straightforward.

As for Part 2, yes, that's true. I guess we should forbid /test etc. for suppressed entries. Although, TBH, I thought we already did it.

@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 "fuck", 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 "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.

(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.?

@suffusion_of_yellow Well, that variable is definitely open to info leaks, whatever the source. As you said, (3) is very hard to fix because when the later-suppressed stuff is already in the DB, there's little we can do. As for (1) and (2), what I meant to say (and what I sort of said in the other task), is that checking rev_deleted against the user rights means that filtering depends on the user performing the action, and using the visibility for a public audience means limiting the amount of available data anyway. I'll explain with an example. Suppose you have an LTA who creates usernames containing the name of another user + some swears; suppose this LTA always edits pages they've already edited. In this case, it may be logical to check page_recent_contributors contains "recognizable-piece". However, suppose that edits from this LTA are constantly being suppressed because the usernames are deemed too offensive. In this case, you cannot use such a check if page_recent_contributors is redacted. A situation like this may be very, very rare (and one could use other checks/countermeasures), but there might be others of which I didn't think. Of note, this creates a distinction between (1) and (2): for (1) we can definitely use the current user's right, whereas (2) is exactly about the example above.
That being said, we can surely check rev_deleted if we think the costs would outweigh the benefits. I just wanted to make it clear that the correctness of checking rev_deleted is not obvious.

Daimona added a subscriber: DannyS712.

The page_recent_contributors part already has its own task, so I've moved it there. Now I'm going to patch the second part.

Both api.php?action=abusefiltertestmatch and Special:Abusefilter/test allow anyone with the abusefilter-view-private or abusefilter-modify rights to recover, one bit at a time, the full details of any suppressed abuse log entry,

For Special:AbuseFilter/test: yes, that's "intended" and there's not much we can do. That page allows testing a filter against recent changes, not AbuseLog entries. If you want to forbid testing a suppressed AbuseLog entry, you should also suppress the corresponding revision (which is usually done, AFAIK). /test has no knowledge about the AbuseLog, so there's nothing we can do about that.

and the partial details of any suppressed recent change.

I also /test with a suppressed revision (and not AbuseLog entry). It is correctly refusing to load the edit if you don't have enough rights.

The API, however, does let unprivileged users test the edit in case of both suppressed AbuseLog and RC entries. That's because the module doesn't check the visibility, and this should really be fixed.

The same trick also works on recent changes, but only on the username and summary, not the wikitext.

I think that's covered by the comment above.

So, IIUC the only thing to be fixed here is the CheckMatch API not checking rc_deleted for RC entries, and afl_deleted (+ rc_deleted) AbuseLog entries. I'm going to write a patch for that.

@Daimona: +1 to the patch above (T223654#5502973) in that it seems sane, though I did not test locally. Let me know if you'd like me to do that.

Daimona renamed this task from AbuseFilter examine and test features reveal suppressed edits and usernames to AbuseFilterCheckMatch API reveals suppressed edits and usernames.Sep 21 2019, 10:37 AM

@Daimona: +1 to the patch above (T223654#5502973) in that it seems sane, though I did not test locally. Let me know if you'd like me to do that.

As you wish :) I'm going to give reproduction steps anyway:

Preliminary

  • Have two users: user A in the 'suppress' group and with the 'abusefilter-hide-log' permission; user B with only the abusefilter-view-private permission.
  • Create a catch-all abuse filter (1==1), with no consequences

1 - Deleted revision

  • Create a page, make two edits
  • Suppress the second edit while logged as user A
  • Log in as B and make a request to `api.php?action=abusefiltercheckmatch&format=json&filter=1&rcid=<RecentChanges ID of the second edit>
  • It should return an error saying that the revision is hidden

2 - Deleted AbuseLog entry

  • Pick one of the AbuseLog entries created while editing the page (possibly one not referring to the now-suppressed revision)
  • Log as user A and change its visibility from Special:AbuseLog, by clicking 'change visibility'. Ensure to tick the checkbox in the next page.
  • Log as user B and make a request to `api.php?action=abusefiltercheckmatch&format=json&filter=1&logid=<AbuseLog ID of the chosen entry>
  • It should return an error saying that the entry is hidden

For Special:AbuseFilter/test: yes, that's "intended" and there's not much we can do. That page allows testing a filter against recent changes, not AbuseLog entries. If you want to forbid testing a suppressed AbuseLog entry, you should also suppress the corresponding revision (which is usually done, AFAIK). /test has no knowledge about the AbuseLog, so there's nothing we can do about that.

Yeah, sorry, I should have been clearer (and got back to this sooner!). What I meant was that /test allows matching against the user_name and summary of revdelled (and, I think, suppressed) recent changes (not the revision text). For example:

  1. Go to /test on enwiki.
  2. Put User_talk:ST47 in the page field
  3. Set the pattern to user_name rlike "Artvro"
  4. Do not check "Show changes that do not match the filter"

This is the only match:

diff hist User talk:ST47‎ 07:01 +197‎ (Username or IP removed)‎ (edit summary removed) (hidden because revision has been deleted)

Now change the pattern to user_name rlike "Brtvro"

The same line does not appear. So we can conclude that "Artvro" is somewhere in the username.

@suffusion_of_yellow Ahhhh thanks. Point (4.) is vital here, then. If you do check that checkbox, deleted/suppressed edits have no symbol next to them, but indeed, it's still exploitable.

Given that it's unrelated to the API module, I'd like to send another patch for that (probably tomorrow unless someone beats me to it). We could deploy them at the same time, though.

Here is the whole thing:

Patch for ViewTestBatch:

Patch for the API module:

Both need to be approved & merged & deployed.

chasemp triaged this task as Medium priority.Dec 9 2019, 4:12 PM

Rebased and improved both patches:

API patch rebased and updated to avoid adding i18n.

Rebased view patch:

And API patch again, I forgot to remove i18n files in the end:

Fixed an error discovered while testing

Urbanecm added a subscriber: Urbanecm.

Deployed.

15:07 <Urbanecm> !log Deploy security patch for T223654
15:08 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Adding Security-Team for backports, etc.

Deployed.

Thanks.

Adding Security-Team for backports, etc.

I'm tracking this on the supplemental patch for now: T270466. I'll try to get to the backports (at least for master) and CVE this week.

jeena raised the priority of this task from Medium to Unbreak Now!.Feb 23 2021, 6:56 PM
jeena added a subscriber: jeena.

/extensions/AbuseFilter/02-T223654-api.patch failed to apply for 1.36.0-wmf.32 (conflict on line 28)
As a result the train is blocked, help! :)

/extensions/AbuseFilter/02-T223654-api.patch failed to apply for 1.36.0-wmf.32 (conflict on line 28)
As a result the train is blocked, help! :)

rEABF5d4025d8c90214ff4446d5166232496d739000f9 is the conflicting commit, and the conflict itself is easy to fix. I can't do anything beyond pointing to that, though.

@jeena - per @Daimona's comment above, this is due to a change in one conditional FWICT. The fastest way to fix this is probably to do a git apply -3 and just manually select the new conditional, which should be the one here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/665374/3/includes/Api/CheckMatch.php

(until we can get an updated patch ready)

So turning the conditional change in the patch for includes/Api/CheckMatch.php into a noop doesn't corrupt it and lets the patch apply. A little hacky, but this should be fine for /srv/patches unless/until something else changes.

New patch:

Eh, I was being a bit lazy. This one should work and avoids the hackiness above:

sbassett lowered the priority of this task from Unbreak Now! to Low.Mar 9 2021, 9:44 PM
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".

Change 670304 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/AbuseFilter@master] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

Change 670305 had a related patch set uploaded (by SBassett; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

Change 670304 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

Change 670305 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

Change 678650 had a related patch set uploaded (by Reedy; author: SBassett):

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

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

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

Change 678650 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

Change 678651 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

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

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

Change 678654 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Skip deleted RCs in /test if we're only showing matches

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

Change 678657 had a related patch set uploaded (by Reedy; author: SBassett):

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

Change 678657 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Avoid info leaks in ApiAbuseFilterCheckMatch

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

sbassett renamed this task from AbuseFilterCheckMatch API reveals suppressed edits and usernames to AbuseFilterCheckMatch API reveals suppressed edits and usernames (CVE-2021-31547).Apr 23 2021, 6:49 PM