CVE-2020-26121: ⚓ T262628 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)

**

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

**

• Edit Task

• Edit Related Tasks…

• Edit Related Objects…

• Mute Notifications

• Protect as security issue

• Award Token

• Flag For Later

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.

• Mentions

Event Timeline

Majavah set Security to Software security bug.Sep 11 2020, 6:59 AM

Majavah changed the visibility from "Public (No Login Required)" to "Custom Policy".

Majavah changed the subtype of this task from “Task” to "Security Issue".

Comment Actions

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.

Comment Actions

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.

0001-Block-page-titles-which-are-marked-as-fully-protected.patch5 KBDownload

Comment Actions

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.

Comment Actions

@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.

Comment Actions

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?

Comment Actions

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?

Lena_WMDE set the point value for this task to 1.

Comment Actions

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.

Comment Actions

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.

Comment Actions

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

Comment Actions

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. :-)

Comment Actions

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.

Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL