The EditFilterMergedContent hook in ContentModelChange class required returning false to interrupt edit action, but handling function of AbuseFilter don't have a return value.
So when use Special:ChangeContentModel in a non-existent title, it will create the page, while the AbuseFilter's log assumed the creation was blocked.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Reedy | T292226 Release MediaWiki 1.35.5/1.36.3/1.37.1 | |||
| Resolved | Reedy | T292227 Tracking bug for MediaWiki 1.35.5/1.36.3/1.37.1 | |||
| Resolved | Security | matmarex | T271037 CVE-2021-44856: Title blocked in AbuseFilter can be created via Special:ChangeContentModel due to the mishandling of EditFilterMergedContent hook return value |
Note that the handling of EditFilterMergedContent hook's return value in ContentModelChange class is different compare to which in EditFilterMergedContentHookConstraint class, should it to be modified or put up a warning to avoid such a mistake?
Returning false from the hook seems a good temporary solution, and I think it can be pushed publicly on gerrit, pretending it's some standardization / cleanup. But yes, core being inconsistent is the real problem.
Yeah, core is really inconsistent. You can see the logic used for saving edits at EditFilterMergedContentHookConstraint. Note that switching to returning false might result in different behavior for saving edits, not sure since I'm not familiar enough with what AbuseFilter currently does in that hook
OK, fair. I didn't check the core code, but then perhaps we can copy part of EditFilterMergedContentHookConstraint into ContentModelChange? Specifically the part which checks the Status.
If return false, you should set $status->value after $status->merge( $filterResultApi ); manually, such as $status->value = MediaWiki\EditPage\IEditObject::AS_HOOK_ERROR_EXPECTED;.
I tested in my install of mw1.35, it's fine with edit and changecontentmodel both. But I wonder if that is a correct way to handle this hook?
I hope the code in ContentModelChange can be improved, because almost all hooks (originally) provide by EditPage.php support set Status object without return false.
I wonder if the intention with ignoring the status in ContentModelChange is to allow users to go (content1, model1) -> (content1, model2) -> (content2, model2) when there's no content that would be valid under both model1 and model2 so you couldn't otherwise get from one to the other.
On the net it still seems like a bad idea though. The API allows changing the content and the model at the same time, and it's a sufficiently infrequent problem that that should be a valid solution.
Sorry for my poor english, what change should be done to solve this bug in your opinion?
Extensions outside of our version control could also trigger this issue, right?
If so, it seems to me that it should be fixed in the ContentModelChange class, by changing the logic to match the EditFilterMergedContentHookConstraint class. And then that's the patch that should be backported.
The security team would be happy to deploy this patch as soon as it can be reviewed by another person.
Patch from T271037#7178772 was deployed during today's security window:
22:19 mstyles@deploy1002: Synchronized php-1.38.0-wmf.9/includes/content/ContentModelChange.php: Deploy security patch for T271037 (duration: 00m 56s)
Tracking as part of the next security release: T292226. We're not seeing any errors in logstash, but if other folks subscribed on this task could perform some additional verification that the patch is behaving properly within production, that'd be great.
Change 747574 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):
[mediawiki/core@REL1_35] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747581 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):
[mediawiki/core@REL1_36] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747588 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):
[mediawiki/core@REL1_37] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747599 had a related patch set uploaded (by Reedy; author: Bartosz Dziewoński):
[mediawiki/core@master] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747574 merged by jenkins-bot:
[mediawiki/core@REL1_35] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747588 merged by jenkins-bot:
[mediawiki/core@REL1_37] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747581 merged by jenkins-bot:
[mediawiki/core@REL1_36] SECURITY: Fix use of EditFilterMergedContent hook when changing content model
Change 747599 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Fix use of EditFilterMergedContent hook when changing content model