Page MenuHomePhabricator

Merging lexemes is only partially rate limited and protected by AbuseFilter
Closed, ResolvedPublicSecurity

Description

Summary: Merging two lexemes (via API or special page) makes two edits; only the second edit is rate limited and run though edit filters.


If the title of this task sounds familiar, that’s because I based it on T345064: CVE-2023-45371: Merging items is not rate limited and only partially protected by AbuseFilter. In that task, we saw that Wikibase PHP code has two interfaces to save edits: the higher-level EditEntity, and the lower-level EntityStore. EditEntity, as implemented by MediaWikiEditEntity, handles permission checks, rate limits, edit filter hooks (e.g. AbuseFilter), and more; you really want to be using it if you can. However, it’s not quite suited for saving redirects. To solve the problem of missing rate limits and edit filters for merges and redirects in T345064, we added code to explicitly handle those things to ItemMergeInteractor and EntityRedirectCreationInteractor. (Side note: We’ll have to add more custom code to them in T356149 to handle temporary users correctly.)

Enter WikibaseLexeme. WikibaseLexeme also has the ability to merge entities, implemented in MergeLexemesInteractor (and exposed via a special page and an API). To perform the redirect, MergeLexemesInteractor uses an EntityRedirectCreationInteractor (implemented by MediaWikiLexemeRedirector), so it gets rate limit and edit filter handling from there. However, before redirecting the “source” lexeme to the “target” lexeme, it also edits the “target” lexeme to add data from the “source” lexeme; and this edit is done using the EntityStore interface (wrapped in a MediaWikiLexemeRepository), bypassing all the checks at the EditEntity level, and doesn’t implement those checks anywhere else either. Therefore, this first edit is not rate limited nor checked against AbuseFilter. (It is still permission-checked; that said, on Wikidata all users have the right to merge lexemes as far as I’m aware, so the permission check adds no little protection in production [edit: actually, it should at least protect against blocked users].)

Steps to reproduce:

  • create two lexemes
    • (they need to be compatible for merging, so they should not both have a lemma in the same language)
  • create and enable an AbuseFilter that would block the first edit, but not the second edit, such as:
    • conditions !(added_lines contains "#REDIRECT")
    • prevent the user from performing the action in question, with message abusefilter-disallowed
  • try to merge one lexeme into the other on Special:MergeLexemes

Expected result: an error from the AbuseFilter; actual result: both edits go through.

Event Timeline

Lucas_Werkmeister_WMDE added a subscriber: Michael.

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.

I dropped the ball on this in the other task, but I think it’s worth looking into this now. Instead of copy+pasting the rate limit + edit filter code in yet another place, and then later also having to add TempUserCreator stuff there – can ItemMergeInteractor and MergeLexemesInteractor just use EditEntity for the non-redirecting edits?

Lucas_Werkmeister_WMDE renamed this task from Merging items is only partially rate limited and protected by AbuseFilter to Merging lexemes is only partially rate limited and protected by AbuseFilter.Feb 6 2024, 1:36 PM

I did a quick check in ItemMergeInteractor and didn’t see any obvious problems with using EditEntity there. It seems to work.

I would also like to fix this task publicly. It directly touches code that we also need to edit for public tasks (T356149, IP Masking), so if we do it as a security patch, it’s going to be quite painful (either we also develop the other changes as security patches, or we have to fix merge conflicts each train). IMHO this issue is harmless enough that it should be okay to fix it in public. What do others think?

There’s a slight difference between ItemMergeInteractor and MediaWikiEditEntity, which is that ItemMergeInteractor checks permissions for EntityPermissionChecker::ACTION_MERGE (which WikiPageEntityStorePermissionChecker translates into read, edit and item-merge rights for MediaWiki core), while MediaWikiEditEntity hard-codes EntityPermissionChecker::ACTION_EDIT (which translates into just read and edit). Apparently EditEntity used to have an addRequiredPermission() method, but it was removed in 2017 (though the method name is still referenced in phpdoc).

But I don’t think that’s a blocker. It just means we can’t remove the permission check in ItemMergeInteractor in favor of the one in EditEntity; we can just keep it, and check “merge” permissions there (and then check “edit” permission again in EditEntity, which is duplicated work but shouldn’t be a problem). Or we make the permission in EditEntity configurable again after all.

(MergeLexemesInteractor also checks EntityPermissionChecker::ACTION_MERGE, but because WikibaseLexeme doesn’t define the lexeme-merge permission, I don’t think it actually behaves any diferent from EntityPermissionChecker::ACTION_EDIT. The permission checker assembles the permissions to check based on the entity type and action, and only checks the permissions that are defined somewhere else, like item-merge is in Wikibase’s extension-repo.json.)

I would also like to fix this task publicly. It directly touches code that we also need to edit for public tasks (T356149, IP Masking), so if we do it as a security patch, it’s going to be quite painful (either we also develop the other changes as security patches, or we have to fix merge conflicts each train). IMHO this issue is harmless enough that it should be okay to fix it in public. What do others think?

Refined suggestion: I can put the patches up for review here first; once we agree they’re ready, put them on Gerrit, quickly merge them, and also backport them into production. The commit message should avoid making it too obvious that a security issue is being fixed (we can frame it as an improvement to “reduce duplicate code” and “prepare for IP Masking”, I think). Backport to supported release branches. And then keep this task private for a little longer. I think that should reduce the risk of the issue being abused.

Just to put it in writing here: I’ve asked a few people (at WMDE and WMF) about the public fixing, and heard neutral to mildly positive responses so far; unless I hear an objection from someone (here or elsewhere), I’ll go ahead with it. (My day tomorrow is pretty full, so the publishing + deployment might have to wait until next week. But I can hopefully start posting patches here tomorrow or Friday.)

I would also like to fix this task publicly. It directly touches code that we also need to edit for public tasks (T356149, IP Masking), so if we do it as a security patch, it’s going to be quite painful (either we also develop the other changes as security patches, or we have to fix merge conflicts each train). IMHO this issue is harmless enough that it should be okay to fix it in public. What do others think?

As a representative of the Security-Team, I'm fine with classifying this as low-risk to go through gerrit, especially if it gets merged soon and lands in Wikimedia production next week.

sbassett triaged this task as Medium priority.Feb 7 2024, 7:53 PM
sbassett changed Risk Rating from N/A to Low.
sbassett added a project: Vuln-MissingAuthz.

As a representative of the Security-Team, I'm fine with classifying this as low-risk to go through gerrit, especially if it gets merged soon and lands in Wikimedia production next week.

Thanks!

Here’s a proposed patch for Wikibase:

And for WikibaseLexeme:

Here’s a proposed patch for Wikibase:

And for WikibaseLexeme:

Yeah, I think these can just go through gerrit and hopefully get a few more eyes on them for CR.

Thanks for these @Lucas_Werkmeister_WMDE . The changes look good to me - I can't see anything wrong with them - but I have to admit I don't really have enough of a context to say if this resolves the issue.

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

[mediawiki/extensions/Wikibase@master] Use EditEntity for ItemMergeInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@master] Use EditEntity for MergeLexemesInteractor

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

Change 1002998 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use EditEntity for ItemMergeInteractor

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

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

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.17] Use EditEntity for ItemMergeInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.17] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.18] Use EditEntity for ItemMergeInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.18] Use EditEntity for MergeLexemesInteractor

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

Change 1002999 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Use EditEntity for MergeLexemesInteractor

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

Change 1002950 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.17] Use EditEntity for ItemMergeInteractor

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

Change 1002951 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.17] Use EditEntity for MergeLexemesInteractor

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

Change 1002952 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.42.0-wmf.18] Use EditEntity for ItemMergeInteractor

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

Change 1002953 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@wmf/1.42.0-wmf.18] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/Wikibase@REL1_41] Use EditEntity for ItemMergeInteractor

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

Change 1002959 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

Reason:

will cherry pick again once I8bfb95f45e (REL1_41, change 1002961) is merged

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

Change 1002959 restored by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

Change 1002960 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_41] Use EditEntity for ItemMergeInteractor

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

Change 1002959 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@REL1_41] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/Wikibase@REL1_40] Use EditEntity for ItemMergeInteractor

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

Fuck me, I only noticed while backporting the changes for this task that we never actually backported the fix for T345064 to REL1_40 and REL1_39…

(It’s in REL1_41, that branch was cut after the fix was merged on master.)

Change 1002962 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_40] Use EditEntity for ItemMergeInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@REL1_40] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/Wikibase@REL1_39] Use EditEntity for ItemMergeInteractor

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

Change 1003477 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_40] Use EditEntity for MergeLexemesInteractor

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

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

[mediawiki/extensions/WikibaseLexeme@REL1_39] Use EditEntity for MergeLexemesInteractor

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

Change 1003478 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@REL1_39] Use EditEntity for ItemMergeInteractor

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

Change 1003480 merged by Lucas Werkmeister (WMDE):

[mediawiki/extensions/WikibaseLexeme@REL1_39] Use EditEntity for MergeLexemesInteractor

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

Alright, I think this is now fully deployed; I guess we can make the task public with the next supplemental release (T353904)?

I guess we can make the task public with the next supplemental release (T353904)?

Sure, sounds good. Thanks for all of the clean-up work here.

I think we can make this task public now? As far as I understand, the release happened and T353904 only remains open because the CVEs haven’t been assigned yet.

Also, suggested adjusted row for the table in that task (I’m not allowed to edit it myself):

| T357101 | [WikibaseLexeme](https://www.mediawiki.org/wiki/Extension:WikibaseLexeme) | [CVE-2024-xxxxx](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-xxxxx) | [Yes](https://gerrit.wikimedia.org/r/1003480) | [Yes](https://gerrit.wikimedia.org/r/1003477) | [Yes](https://gerrit.wikimedia.org/r/1002959) | [Yes](https://gerrit.wikimedia.org/r/1002999)
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".

I think we can make this task public now? As far as I understand, the release happened and T353904 only remains open because the CVEs haven’t been assigned yet.

Done. Unfortunately, Mitre has a sizable backlog of CVEs FWIU that is affecting everyone :/

Also, suggested adjusted row for the table in that task (I’m not allowed to edit it myself):

| T357101 | [WikibaseLexeme](https://www.mediawiki.org/wiki/Extension:WikibaseLexeme) | [CVE-2024-xxxxx](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2024-xxxxx) | [Yes](https://gerrit.wikimedia.org/r/1003480) | [Yes](https://gerrit.wikimedia.org/r/1003477) | [Yes](https://gerrit.wikimedia.org/r/1002959) | [Yes](https://gerrit.wikimedia.org/r/1002999)

Thanks, done.

sbassett claimed this task.
sbassett reassigned this task from sbassett to Lucas_Werkmeister_WMDE.