CheckUsers have unlogged access to IP addresses via the AbuseFilter API
Open, NormalPublic

Description

Any account with the abusefilter-private right can perform a query such as

https://en.wikipedia.org/w/api.php?action=query&list=abuselog&aflprop=ip|user

and see the IP address of registered users. Unlike similar requests made through the web interface, this action is not logged at the private details access log, as is required by the CheckUser policy.

It should be noted that at least two admin accounts (and many non-admin accounts) have been recently compromised. If the same attacker were to gain control of a checkuser account, they could obtain IP addresses in batches of 5000 without anyone noticing for a long time. So, perhaps the ability to check abuselog IP addresses via the API could simply be disabled, as a quick fix.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSat, Nov 24, 6:34 PM
Huji triaged this task as High priority.Sat, Nov 24, 6:42 PM
Huji claimed this task.
Huji added a subscriber: Daimona.

Definitely a very high priority. Since an active threat is not identified and no CU account has been compromised, I am not yet marking it as UBN.

I think the short-term solution of disabling the feature is not that much easier to code and implement than the long-term solution of adding logging functionality to the API calls.

I am adding @Damiona who is trustworthy. I am claiming this task, as it is likely that I will be able to set aside time to make a patch tomorrow (Sunday).

Daimona, you should also start working on this in parallel. The sooner we make the patch the better. I think the log entry should clarify "via API".

A CU account was recently compromised, actually, and we cannot take this risk (again). This is a really serious info leak (potentially) and should be addressed ASAP. It's not worth to disable the feature, as long as we're fast in pushing a fix. However, unfortunately I won't be able to work on this before tomorrow (yep, Sunday, UTC+1 here). I'll try to get this done tomorrow morning.

Huji raised the priority of this task from High to Unbreak Now!.Sat, Nov 24, 7:23 PM

Thanks for pointing that out. Elevating to UBN.

Tomorrow is fine. If we were to out-right disable this, it would still probably take until tomorrow anyway.

I thought we had this feature disabled (?)

Huji added a comment.Sat, Nov 24, 7:32 PM

Which feature? The ability to see private details (through the web interface) was enabled after a loooooooong process. See T152934 and related tasks.

But if you mean the ability to do the same "via API" was disabled, then I cannot find that in the code.

A very quick fix would be to restore the status quo, and temporarily remove the abusefilter-private right from all CheckUsers.

@Bawolff yes, but just because previously the access through web interface wasn't logged. Then logging was added, and the feature enabled on WMF prod. API access logging slipped through apparently.

I gave a quick look through the code (that's all I can do right now), and apparently we need to change this line by calling SpecialAbuseLog::addLogEntry. The signature of the function should probably be updated to accept an extra parameter to differentiate API access; i18n should be added for the message itself; and we should also handle the fact that multiple rows can be accessed at the same time via API. And maybe we should ask for some sort of confirm from the user... I cannot really envision a clean process at the moment.

Huji added a comment.Sat, Nov 24, 7:52 PM

I think this will do the job for now. Later, we should make sure the actual reason is a required field in the API call.

MaxSem added a subscriber: MaxSem.Sat, Nov 24, 10:36 PM

-1 on the patch, GET requests should not result in writes like creating log entries. I'd rather we removed viewing the IPs as an emergency patch and then decide what to do without rush.

Huji added a comment.Sun, Nov 25, 12:31 AM

In terms of a long-term solution: how about we never show the IP in the GET request at all, but allow for a brand new POST-only query which would take the log ID and a reason, and return its private details?

That would line-up well with the short-term solution of removing the IP from the GET request.

Huji added a comment.Sun, Nov 25, 1:18 AM

Uploading a new patch that would just remove the IP field from the results for now.

We can add the new functionality (for a POST request to get the private details of a log, having provided a reason) later.

Ah, I didn't realize this is a GET request. Then, the first thing is to remove it from the API, and the patch above is almost fine. I also agree that we may add a new module to retrieve it via POST as stated above.

This would require adding a new file, and it's probably easier to review it on gerrit without rushing. I can start working on it as soon as this patch will be merged and backported.

As for the patch above, this line has to be removed as well; then, AFAICS it'll be ready for deployment.

Huji added a comment.Sun, Nov 25, 9:07 PM

Fair comment, Daimona; I removed that line as well. New patch attached.

Which feature? The ability to see private details (through the web interface) was enabled after a loooooooong process. See T152934 and related tasks.

Ah, I missed that that ticket was resolved, so I got confused. Never mind me. :)

@Huji Thanks, I tested the patch and it's fine. So, what do they say...? +2! @Bawolff could you please deploy it? Then, Huji, you can upload the patch to gerrit and self-merge it, on the following branches: master, 1.32, 1.31, 1.30 and 1.27 (watch out for possible merge conflicts with the old ones).

Huji moved this task from Backlog to Logging on the AbuseFilter board.Mon, Nov 26, 3:21 PM
Huji moved this task from Backlog to Doing on the Privacy board.

This patch doesn't seem to apply cleanly

patching file includes/api/ApiQueryAbuseLog.php
Hunk #1 FAILED at 56.
Hunk #2 FAILED at 66.
Hunk #3 FAILED at 95.
Hunk #4 FAILED at 190.
Hunk #5 FAILED at 283.
5 out of 5 hunks FAILED -- saving rejects to file includes/api/ApiQueryAbuseLog.php.rej

Can it be rebased (and preferably be in the format provided by git format-patch --stdout HEAD^)

Huji added a comment.Wed, Nov 28, 1:19 AM

@Bawolff here you are!

I'm writing the patch for the new API module and it's almost ready, however I won't send it to gerrit until this one is merged. @Bawolff any chance you could deploy it?

@Daimona, @Huji - I checked that T210329.patch applies locally to the abusefilter master branch. Looks good. Not sure if @Bawolff or @Reedy are around right now, but we do have the security deployment window today at 22:00 UTC, so just a shade under two hours away (https://wikitech.wikimedia.org/wiki/Deployments#Monday,_December_03). I've got deployment rights and have done config deployments before, so I could probably do this, but:

  1. I don't have CU anywhere, so the best I'd be able to test is locally. The patch doesn't look volatile, but if anything looked amiss in the logs, I'd have to revert immediately.
  2. I've never done a full security patch and deploy before, by myself.

@sbassett Thanks for looking! I'm afraid I can't help (won't be around at 22UTC and I don't have CU rights), however the patch is pretty simple and I don't think it'll need much testing.

@Bawolff and I are deploying this now.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Mon, Dec 3, 11:04 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptMon, Dec 3, 11:04 PM

Change 477425 had a related patch set uploaded (by SBassett; owner: Huji):
[mediawiki/extensions/AbuseFilter@master] SECURITY: Remove private information from the API results

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

Change 477425 merged by SBassett:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Remove private information from the API results

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

Change 477430 had a related patch set uploaded (by SBassett; owner: Huji):
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Remove private information from the API results

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

Change 477431 had a related patch set uploaded (by SBassett; owner: Huji):
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Remove private information from the API results

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

Change 477430 merged by Brian Wolff:
[mediawiki/extensions/AbuseFilter@REL1_32] SECURITY: Remove private information from the API results

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

Change 477431 merged by Brian Wolff:
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Remove private information from the API results

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

Bawolff added a comment.EditedTue, Dec 4, 12:04 AM

CheckUsers have unlogged access to IP addresses via the AbuseFilter API

Note we do have logging for this for 90 days. In the last 90 days there was only one use of this api (on enwiki on 24 Nov - i checked with the user in question and they verified it really was them)

Change 477439 had a related patch set uploaded (by SBassett; owner: Huji):
[mediawiki/extensions/AbuseFilter@REL1_27] SECURITY: Remove private information from the API results

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

Change 477439 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_27] SECURITY: Remove private information from the API results

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

Daimona lowered the priority of this task from Unbreak Now! to Normal.Tue, Dec 4, 9:35 AM

Nice, thanks to you guys for deploying. I'll send the patch for the new module in a minute.

Change 477501 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add a new API module to retrieve private details from AbuseLog

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

Huji added a comment.Tue, Dec 4, 12:53 PM

Thank you all for the great work!

@Bawolff can this task be closed now?

The quick solution has been merged and deployed. The patch for the long-term solution (new API) is still under review. Which is to say, the problem itself has been solved, although it still needs some work for a proper solution. This isn't a security problem anymore, although maybe we could keep the task open to discuss the new patch.