Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

CVE-2023-45371: Merging items is not rate limited and only partially protected by AbuseFilter
Closed, ResolvedPublic5 Estimated Story PointsSecurity

Description

As a Wikidata administrator or site operator, I want merging items to be subject to the same rate limits as other edits, and to be checked against AbuseFilter like other edits, so that the amount of damage that vandals can do is limited.

Problem:
ItemMergeInteractor bypasses MediaWikiEditEntity and directly uses the underlying WikiPageEntityStore, probably because it has to edit two entities at once. This means that ItemMergeInteractor is responsible for repeating all the checks done in MediaWikiEntity; it does permission checks, but doesn’t ping the rate limiter nor run the EditFilterMergedContent hook.

Rate limits are, as a result, not checked at all as far as I can tell. (I tested Special:MergeItems, but I assume the wbmergeitems API is equally affected.)

AbuseFilter is still checked at some point, but too late: when testing with a simple AbuseFilter that just blocks all edits unconditionally, there is an error message “EditFilterHook stopped redirect creation” on Special:MergeItems (which apparently comes from the EntityRedirectCreationInteractor) and the last edit (“redirected to target”) doesn’t take place, but the first two edits (“merged item into target”, which clears source the item without redirecting it, and “merged item from source”, which updates the target item with the data from the source item) still happen and aren’t blocked.

To test this, it can be useful to configure a very low rate limit:

LocalSettings.php
$wgRateLimits['edit']['anon'] = [ 1, 60 ]; // 1 per minute

Acceptance criteria:

  • Special:MergeItems and wbmergeitems don’t make any edits when the user has hit their rate limit
  • Special:MergeItems and wbmergeitems don’t make any edits when there is an AbuseFilter that blocks all edits

Event Timeline

(The only other place I can see that bypasses MediaWikiEditEntity is PropertyDataTypeChanger – that one doesn’t even check any permissions, but that’s fine because it’s only accessible via a maintenance script. So I think ItemMergeInteractor is the only one we need to worry about.)

I think EntityRedirectCreationInteractor also doesn’t check rate limits, and it probably should – it’s accessible separately from ItemMergeInteractor, after all (via the wbcreateredirect API or Special:RedirectEntity).

Lucas_Werkmeister_WMDE set the point value for this task to 5.

Pulling into the sprint, estimated at 5 for now. (I’ll pick it up soon if nobody beats me to it.)

Alright, here’s a patch:

(Guessing at the Vuln- tag, please change if I guessed wrong.)

It looks good to me (still have to have a closer look at the tests), but I'm confused that these files do the saving by themselves. Don't we have the MediaWikiEditEntity class for exactly that? Its class phpdoc even says:

Handler for editing activity, providing a unified interface for saving modified entities while performing permission checks and handling edit conflicts.

That being said, the current patch is probably the right way to go for a security change.

The changed tests look good to me as well, and they do pass locally on my system.

=> Gives the change their +1

It looks good to me (still have to have a closer look at the tests), but I'm confused that these files do the saving by themselves. Don't we have the MediaWikiEditEntity class for exactly that? Its class phpdoc even says:

Handler for editing activity, providing a unified interface for saving modified entities while performing permission checks and handling edit conflicts.

MediaWikiEditEntity can only save an EntityDocument (currently), not an EntityRedirect, so at least EntityRedirectCreationInteractor can’t use it. I’m not sure if there’s really a reason why ItemMergeInteractor can’t use it to create the two non-redirect revisions; sure, it has to make two edits, but it’s not clear to me that you couldn’t just call MediaWikiEditEntity twice? (Or perhaps you’d have to get two separate instances from the MediaWikiEditEntityFactory, not sure.) But we can look into that after the security change, IMHO.

Alright, here’s a patch:

Deployed.

(I doubt there’s a non-disruptive way to check this in production; I would just rely on the local testing here.)

(I doubt there’s a non-disruptive way to check this in production; I would just rely on the local testing here.)

At least for the abuse filter part, don't we have any test-focused rules on testwikidata in place?
Though I guess, if 1) someone is scrutinizing your edits closely, and 2) it actually fails to block the edit and it does get saved, then they might be able to work out what we're trying to fix.

True, I was only thinking about the rate limit. AbuseFilter should be testable.

Seems to be working – I changed filter #5 to block edits by myself, then tried to merge Q151357 into Q132000, and got this:

image.png (414×1 px, 42 KB)

Seems to be working – I changed filter #5 to block edits by myself, then tried to merge Q151357 into Q132000, and got this:

image.png (414×1 px, 42 KB)

Though this is just identical to the status quo, in that the edit filter only seems to have blocked the redirect creation. But my contributions don’t show any earlier edits either… I think this is just because there was nothing to edit on the source or target item (the source item was already empty, so there was nothing to clear nor anything to add to the target item). Let me try again with a non-blank source item.

Okay, that’s better:

image.png (414×1 px, 40 KB)

This is an error message that you wouldn’t have gotten before. (And still no edits in my contributions, yay.)

sbassett triaged this task as Medium priority.Sep 5 2023, 4:45 PM
sbassett moved this task from Incoming to Watching on the Security-Team board.
sbassett added a project: SecTeam-Processed.
sbassett changed Risk Rating from N/A to Medium.

Change 961264 had a related patch set uploaded (by Mstyles; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] SECURITY: Add rate limits and edit filters to item merging

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

Change 961264 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SECURITY: Add rate limits and edit filters to item merging

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

Mstyles renamed this task from Merging items is not rate limited and only partially protected by AbuseFilter to CVE-2023-45371: Merging items is not rate limited and only partially protected by AbuseFilter.Oct 10 2023, 4:01 PM
Mstyles closed this task as Resolved.
Mstyles changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 10 2023, 5:07 PM
Mstyles changed the edit policy from "Custom Policy" to "All Users".

Change 1002965 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_40] SECURITY: Add rate limits and edit filters to item merging

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

Change 1003466 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: Add rate limits and edit filters to item merging

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

Change 1002965 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_40] SECURITY: Add rate limits and edit filters to item merging

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

Change 1003466 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_39] SECURITY: Add rate limits and edit filters to item merging

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