Page MenuHomePhabricator

FileImporter imports the file even when the target page is protected on Commons and the importer should not be able to create it (CVE-2020-26121)
Closed, ResolvedPublic1 Estimated Story PointsSecurity

Description

As reported by @CptViraj here, it appears that Extension:FileImporter imports the file even when the target page is protected against creation on Commons and the importer should not be able to create it. I tested this on File:ExampleForImportProtection.jpg, imported from testwiki with my test account, and I successfully imported it, even though the target page (File:ExampleForImportProtection.jpg) was under full-protection on Commons, and my test account is not an admin there.
Steps to reproduce

  1. Upload a file on a Wikimedia wiki other than Commons that supports local uploads (e.g. testwiki; using Special:Upload)
  2. Protect the title of the said file on Commons so that it can't be created by you (or your test account, in case your main account is an admin)
  3. Export the said file from the local wiki to Commons.

Event Timeline

Majavah set Security to Software security bug.Sep 11 2020, 6:59 AM
Majavah added projects: Security, Security-Team.
Majavah changed the visibility from "Public (No Login Required)" to "Custom Policy".
Majavah changed the subtype of this task from "Task" to "Security Issue".

Confirmed, marking as a security bug just in case. I tested this and it can't be used to overwrite existing files but it can be used to create files that have been protected from being created.

thiemowmde moved this task from Backlog to Tickets in sprint on the Move-Files-To-Commons board.

For comparison, here's how a similar feature has been implemented in the past: https://phabricator.wikimedia.org/rSVN70135 , when a protected title is chosen, the user is prompted to pick a different title.

Here's my proposed solution which checks whether the title is protected on the target wiki using Title::isProtected. I attempt to catch the error before the import process is started to allow the user to still change the title and recover, however, if this were to fail there are still checks in the final validation process which will cause an unrecoverable error.

I didn't include a check for Title::isSemiProtected since we should already be checking whether a user is logged in before they even get to the importer.

Here is a more minimal fix that achieves the same:

Note there was already code to check if the user is allowed to upload to the target title. But for some reason there is no upload restriction recorded in the database, even if the protection UI says otherwise. You can even see this when re-visiting the protection UI. It will appear like there is no upload protection. This behavior feels like a bug. Unfortunately it's an old one, as far as I can tell. It looks like the upload protection is intentionally not recorded for pages that don't exist – somehow assuming a page needs to be created first before a file can be uploaded. You can see the code for this in WikiPage::doUpdateRestrictions(), in line #2447.

FileImporter is written to do the same as in Special:Upload. And while there are checks for both create and upload in Special:Upload, they are in a strange order that makes it look like the upload permissions have a higher priority, and the create permissions are optional – the opposite of what is actually recorded.

The patch let's FileImporter check both. Technically the patch could be even simpler: just change upload to create, and never check the upload permissions again. However, I feel it's worth to always check both, just to be extra safe. It might be expensive, but isn't done often.

I've deployed T262628.patch to production, and verified with https://commons.wikimedia.org/wiki/Special:ImportFile?clientUrl=https%3A%2F%2Ftest.wikipedia.org%2Fwiki%2FFile%3AFa.jpg&importSource=FileExporter

The patch is also applied to the wmf.9 branch presumably going out this week, which gives us a little time to finish the work.

Now we can upload to gerrit and follow regular code review practices.

@awight - thanks for deploying.

I have this issue tracked for our supplemental security release email here: T256342. I also started backports within gerrit for the currently-supported release branches. REL1_35 applied cleanly, REL1_34 is failing a test in quibble-composer-mysql-php72-noselenium-docker and REL1_31 won't apply because runPermissionTitleChecks() doesn't exist there. I also noticed https://gerrit.wikimedia.org/r/627845 references this bug, but that seems supplemental to the original security patch on this task.

https://gerrit.wikimedia.org/r/627845 is not a security fix. The additional change was made because of the discussion here, but is not required to fix this issue.

I don't get the failure on REL1_34. It is entirely unrelated to what the patch does. Maybe it's fine to force-merge the patch and ignore the test failure?

https://gerrit.wikimedia.org/r/627845 is not a security fix. The additional change was made because of the discussion here, but is not required to fix this issue.

I don't get the failure on REL1_34. It is entirely unrelated to what the patch does. Maybe it's fine to force-merge the patch and ignore the test failure?

Still seems to be something that's annoyingly "wrong" on that branch. Could be easily fixed I guess but should probably go into separate patch, right?

I don't get the failure on REL1_34. It is entirely unrelated to what the patch does. Maybe it's fine to force-merge the patch and ignore the test failure?

Yes, we can do that if the failure is completely unrelated. That does tend to happen more often on cruftier release branches.

Still seems to be something that's annoyingly "wrong" on that branch. Could be easily fixed I guess but should probably go into separate patch, right?

Yes, filed here: T263690.

sbassett assigned this task to thiemowmde.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett moved this task from Watching to Our Part Is Done on the Security-Team board.

A CVE has been requested.

I guess CVE means "Common Vulnerabilities and Exposures". What does it mean if one has been requested? Is there anything the WMDE-TechWish can do?

Note that closing a ticket makes it disappear from all boards.

I guess CVE means "Common Vulnerabilities and Exposures". What does it mean if one has been requested? Is there anything the WMDE-TechWish can do?

This is correct. I've requested a CVE for this issue as that is the Security-Team's standard operating procedure for vulnerabilities reported in any MediaWiki core and deployed/bundled extensions and skins. Vulnerabilities for core/bundled code go out with the quarterly security release (the most recent one sent yesterday) and for all other extensions, skins, etc. the supplemental security release announcement (currently tracked at T256342, and will likely be sent out today or next Monday).

Note that closing a ticket makes it disappear from all boards.

Ok, I did not see anything else actionable remaining for this task given that 1) a security patch was written and deployed 2) backports to relevant release branches were made and 3) a CVE was requested. If the task needs to be tracked for any reason, please feel free to re-open.

sbassett renamed this task from FileImporter imports the file even when the target page is protected on Commons and the importer should not be able to create it to FileImporter imports the file even when the target page is protected on Commons and the importer should not be able to create it (CVE-2020-26121).Sep 28 2020, 4:55 PM

Thanks a lot for the detailed response. I'm still curious what "requesting a CVE" means, but understand it's not something my team is asked to do. :-)

Thanks a lot for the detailed response. I'm still curious what "requesting a CVE" means, but understand it's not something my team is asked to do. :-)

CVEs typically need to be proactively requested from Mitre. The 6th slide within this deck goes into some detail.