Page MenuHomePhabricator

AbuseFilter doesn't see Wikibase's impersonation of admins when removing pages from a WD item
Closed, ResolvedPublicSecurity

Description

Kitayama, a svwiki admin, deleted a page at svwiki, and WMF job executors removed the page from the WD item on the admin's behalf, see https://www.wikidata.org/w/index.php?diff=1237019602. However, abusefilter processed the edit as it was anonymous, see https://www.wikidata.org/wiki/Special:AbuseLog/14099540. Through that doesn't leak anything sensitive per se (the IP doesn't say anything besides it runs from WMF's network), but it might indicate AbuseFilter sometimes ignores logged-in status for logged-in users.

I believe abusefilter log entry should've been logged under the user's own username rather than the IP.

Event Timeline

Adding @Lydia_Pintscher, the PM for Wikidata. Lydia, please add other members of your team as you deem appropriate.

Thank you!
I seem to remember something similar but can't find the ticket. Maybe others know.

Doesn’t ring a bell here. I assume this doesn’t always happen?

but it might indicate AbuseFilter sometimes ignores logged-in status for logged-in users.

Unlikely, it could at most happen with other jobs.

First random thought I'm throwing in: the message wikibase-entity-summary-clientsitelink-remove is almost impossible to find. It's never used directly in the code, and it's not grep-friendly (no comment mentioning it). I had to grep for 'clientsitelink' to get here.

That said, AbuseFilter does nothing more than extracting a User object from the ContextSource passed to the EditFilterMergedContent hook (this is obviously only true for edits), see here. This could in fact be improved, if we change the hook handler to use the $user param directly.

I haven't verified whether this would fix the issue, though. Another quick fix might be to set the context user to be the same as the User object in WikiBase. This leads to the question of whether having the Context hold a different User object makes sense or not, but I guess it does.

Also, note that I cannot push a fix right now, but please do add me as reviewer.

Doesn’t ring a bell here. I assume this doesn’t always happen?

Does that mean you were unable to reproduce it? I'm not sure if this happens all the time, tbh.

I think the problem happens because the edits happen through a job and not an actual context of edit (which makes sense) the jobs are: UpdateRepoOnDelete and UpdateRepoOnDelete. We can make the job set the user in context the same as the "user" in job params, it should be a good band-aid for now. What do you think?

I pushed a public fix for this, see https://gerrit.wikimedia.org/r/c/mediawiki/extensions/AbuseFilter/+/621254

Firstly because I forgot that this task was private, secondly because I think the commit message isn't giving anything away. Also, I'm unsure whether this task should stay sec-protected.

I +2'd the patch, we can backport it today

Thanks!

Firstly because I forgot that this task was private, secondly because I think the commit message isn't giving anything away. Also, I'm unsure whether this task should stay sec-protected.

I filled it as a protected task, because i wasn't sure whether this includes only "impersonated edits", or also some other edits, with the possibility of disclosure of a real IP. If you think that cannot happen, feel free to publish it :-).

Urbanecm changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 20 2020, 11:41 AM
Urbanecm changed the edit policy from "Custom Policy" to "All Users".
13:38 <+logmsgbot> !log urbanecm@deploy1001 Synchronized php-1.36.0-wmf.5/extensions/AbuseFilter/includes/AbuseFilterHooks.php: 00da39b6913ac2eab600bbb61258472b60d2cbcb: Use $user param when filtering edits (T258717) (duration: 01m 05s)

Should be fixed at production now :).

Mentioned in SAL (#wikimedia-operations) [2020-08-20T11:44:17Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.4/extensions/AbuseFilter/includes/AbuseFilterHooks.php: rEABFd762e7b5526d: Use $user param when filtering edits (T258717) (duration: 01m 05s)

Urbanecm assigned this task to Daimona.

All should be done, fix is merged to master, backported to active wmf branches, and I confirmed it works at testwikidatawiki.