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.

I thought we had this feature disabled (?)

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.

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.

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

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.

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.

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

In T210329#4771724, @Huji wrote:

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

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

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

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

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

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

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.

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

Change 477501 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add a new API module to retrieve private details from AbuseLog

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