WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 9 days ago

#40175 accepted defect (bug)

Upload Validation / MIME Handling

Reported by: blobfolio Owned by: joemcgill
Milestone: 4.9 Priority: normal
Severity: critical Version: 4.7.3
Component: Media Keywords: has-unit-tests has-patch needs-testing
Focuses: administration Cc:

Description

A security fix implemented in WordPress 4.7.1 relies on a PHP extension (fileinfo) with inconsistent reporting behavior. As a result, many users (even after #39550) trying to upload various types of files (office documents, multimedia, fonts, etc.) have received validation errors.

In a nutshell, this is because the media types returned by fileinfo vary from server to server and file to file. If PHP returns a media type beginning application/*, that media type must be whitelisted or the result will fail.

Because most incorrect/historical answers from fileinfo begin application/*, this is resulting in a large number of false-positives.

There are three main ways to address this, with a combination approach being preferred:

1) The conditional in wp_check_filetype_and_ext could be restricted so that rather than searching application/* broadly, it looks only at the narrow file types at the heart of the original security issue. This option requires review from the Security Team.

2) The WordPress Core could be extended to provide "MIME alias" awareness. This would allow WordPress to properly match a given extension/MIME pairing even in cases where the MIME type is historically valid, but not the singular type in the whitelist. See #39963 for related information.

3) WP could be extended to maintain its own mime.types file, which can be passed to fileinfo, providing more consistent responses. This option requires the MIME alias handling to avoid breaking sites or plugins which hook into upload_mimes.

Duplicate/related tickets are being collapsed into this thread. Please continue all related discussion here.

Attachments (2)

002713876-0.XLS (9.0 KB) - added by lovelucy 6 months ago.
cannot upload xls with strange MIME: application/CDFV2-unknown
40175.diff (11.3 KB) - added by blobfolio 2 months ago.
Implements an explicit "greylist" for file types requiring additional validation, rather than scrutinizing every application/* type.

Download all attachments as: .zip

Change History (24)

#1 @blobfolio
6 months ago

#40078 was marked as a duplicate.

#2 @blobfolio
6 months ago

#40170 was marked as a duplicate.

#3 @blobfolio
6 months ago

#40101 was marked as a duplicate.

#4 @blobfolio
6 months ago

  • Severity changed from normal to critical

#5 follow-up: @joemcgill
6 months ago

  • Owner set to joemcgill
  • Status changed from new to accepted

Thanks for consolidating all of this here @blobfolio.

It's probably helpful to define some base assumptions here. Before [39831], WordPress essentially trusted that all uploaded files were exactly what they claimed to be, based on the file extension. The only exception is that image files were verified in an attempt to rename image files that had accidentally been saved with the wrong extension (see #11946). From what I can tell, this was mainly a UX improvement when working with images, to avoid editor errors and was not strict about allowing uploads based on actual mime types.

#6 in reply to: ↑ 5 @blobfolio
6 months ago

Replying to joemcgill:

It's probably helpful to define some base assumptions here. Before [39831], WordPress essentially trusted that all uploaded files were exactly what they claimed to be, based on the file extension.

I agree.

Prior to 4.7.1, WordPress itself was the only source of information about a file's media type, so all file handling had a predictable framework to deal with. But now that an outside source of information has been added (two actually), we've crossed the Rubicon.

If any amount of outside information is to be used by WordPress, there needs to be a system in place to reconcile that information with WordPress' own whitelisting system. Otherwise any contradiction will result in failure.

That isn't addressed by (1), however limiting the amount of outside information being used for validation will result in fewer innocent files being mistakenly caught in the net.

(2) tackles the problem directly by acting as a sort of Babel Fish, and will help futureproof the platform. It also affords opportunities for later enhancements, such as more aggressive upload validation or (3) below.

(3) is more of an enhancement than an immediate fix to the problem. It would be helpful for data consistency, but would create conflicts with existing sites and plugins that have extended their whitelists (unless we have (2) already in place).

From what I can tell, this was mainly a UX improvement when working with images, to avoid editor errors and was not strict about allowing uploads based on actual mime types.

Definitely. (Correctly) renaming images before handing them off to the thumbnail components is necessary to avoid errors. Insofar as I can tell, this piece has worked as expected since 4.7.1.

The only image-related upload validation issues were due to unusual types like WEBP or SVG that individuals may have added to their whitelists. #39550 fixed that, except in cases where the server returns an (incorrect) application/* type for the file.

@lovelucy
6 months ago

cannot upload xls with strange MIME: application/CDFV2-unknown

#7 follow-up: @lovelucy
6 months ago

@blobfolio Related to #39550, I found another xls that cannot be uploaded to WordPress, which is tested with result MIME: application/CDFV2-unknown

File attached

#8 in reply to: ↑ 7 @blobfolio
6 months ago

Replying to lovelucy:

@blobfolio Related to #39550, I found another xls that cannot be uploaded to WordPress, which is tested with result MIME: application/CDFV2-unknown

Awesome, thanks @lovelucy! It seems CDFV2 is another hardcoded non-MIME MIME living inside the fileinfo.so extension, only unlike application/ms-office, it gets variable bits tacked onto the end like "unknown" and "corrupt". I'll add a workaround of some sort to the #39963 patch.

#9 follow-up: @hivewebstudios
6 months ago

For what it is worth, our plugin https://wordpress.org/plugins/font-organizer/ is suffering heavily from this bug, and a lot of our users are reporting issues with uploading fonts to their WP.
Right now our official instruction is to actually download the plugin https://wordpress.org/plugins/disable-real-mime-check/ to sort that out, but obviously it is not an actual solution.
Is there some easy way around for us to implement a solution within the plugin that will allow our users to upload fonts without getting that error?

We first figured we would just wait for 4.7.3 to solve that issue but since its ongoing, we need some other elegant solution that won't affect users later on when thte bug is solved and also will minimize compatibility issues.

Also I truly hope I am not breaking any rules by posting this, I never had to deal with core issues before and we are currently testing our next major plugin release.

#10 in reply to: ↑ 9 @blobfolio
6 months ago

Replying to hivewebstudios:

For what it is worth, our plugin https://wordpress.org/plugins/font-organizer/ is suffering heavily from this bug, and a lot of our users are reporting issues with uploading fonts to their WP.

Yeah, unfortunately font files are particularly likely to be hit with this issue. Prior to IANA's designation of a font/ type, many fell under the application/ banner, and a lot of different versions of PHP will still see them that way.

In your case, at least, there is a possible workaround. The upload validation code causing the trouble includes a filter; your plugin could hook into this and alter the response to "A-OK" for font files. Please see https://core.trac.wordpress.org/ticket/39550#comment:110, which worked for another author.

Also I truly hope I am not breaking any rules by posting this, I never had to deal with core issues before and we are currently testing our next major plugin release.

No worries! It is helpful when users share their own experiences.

#11 @blobfolio
6 months ago

The proposed fix (via #39963) has been repackaged as a plugin for broader testing! (The ticket also includes the changes as a patch if you'd rather apply it that way.)

For anybody affected by the issues in this ticket, please give it a try: https://core.trac.wordpress.org/attachment/ticket/39963/blob-mimes.zip

If you install it, please be sure to follow #39963 so you are kept abreast of plugin updates, if any. And of course, report any happy or sad news from your use there.

For reference, it is pretty much the opposite of the earlier quick-fix https://wordpress.org/plugins/disable-real-mime-check/. It does not disable the security features introduced in 4.7.1 — in fact it greatly expands them — but instead is much smarter about reconciling the variation in type detection that exists from server to server.

@lovelucy, this also addresses the detection inconsistencies we've seen within a single file "type". Please let me know if it fixes your Excel document upload issues.

Thanks everyone!

#12 @blobfolio
6 months ago

  • Focuses administration added

The plugin implementing the proposed fix (via #39963) can now be found in the main WordPress Plugins repository: https://wordpress.org/plugins/blob-mimes/

If you had already installed version 0.1.2 directly from the ticket, you should automatically see the usual orange upgrade notice within the next 24 hours, which will transition you to the WP-hosted version.

If you are currently using an older version or running it from a server that can't reach Github (the previous host), please uninstall the plugin and reinstall it from the official WordPress Plugins repo to catch up.

The new release includes a handy upload debug tool for users with manage_options privileges (e.g. admins) under Tools > Debug File Validation. If at any point in the future the Media Library is rejecting something it shouldn't, please use that tool to gather some useful feedback information and post it to #39963 (or email me if you aren't comfortable posting it publicly).

Thanks!

Last edited 6 months ago by blobfolio (previous) (diff)

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


6 months ago

#14 in reply to: ↑ 13 ; follow-up: @wjlonien
2 months ago

Replying to slackbot:

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.

Works like a charm - now I can finally upload .ogg files to WP4.8. Thanks @joemcgill and @blobfolio !

#15 in reply to: ↑ 14 @blobfolio
2 months ago

Replying to wjlonien:

Works like a charm - now I can finally upload .ogg files to WP4.8. Thanks @joemcgill and @blobfolio !

Woo! Yeah, ogg is a tough format, particularly the ogx variant. They can really confuse the heck out of servers (which is odd, you'd think with Linux's domination in that area ogg would get first-class treatment!). Haha.

If you ever stumble across a random ogg file that doesn't work, or any other kind of file, please let me know! Lord of the Files comes with a handy admin tool for that: Tools > Debug File Validation. The information spit out by that page can be emailed to me or posted to https://github.com/Blobfolio/blob-mimes, whichever is easier for you. Thanks!

@blobfolio
2 months ago

Implements an explicit "greylist" for file types requiring additional validation, rather than scrutinizing every application/* type.

#16 @blobfolio
2 months ago

  • Keywords has-unit-tests has-patch needs-testing added

This is a long thread, so here's a quick summary:

In response to a security issue, WordPress 4.7.1 added some content-based evaluations for uploaded media. That implementation affects any file that PHP says is application/*. Because PHP is not very good or consistent at this type of detection, a lot of legitimate files are also being blocked.

This patch instead switches to a "greylist" system, containing explicit extensions and MIMEs (any extension can have multiple types) deserving further scrutiny. When a file is uploaded, the content-derived type is compared against the greylist, and if it matches, only then will it dive deeper and maybe block the upload.

This approach both limits collateral damage and helps WP find more of the sorts of files it was looking for in the first place.

What we need now are some eyeballs!

Please and thank you!

PS: Just be sure to temporarily disable Lord of the Files if your test site has that installed. (And don't forget to reactivate it afterwards; it contains a number of other file-related security improvements unrelated to this ticket. Haha.)

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core-media by blobfolio. View the logs.


9 days ago

#22 @joemcgill
9 days ago

  • Milestone changed from Awaiting Review to 4.9
Note: See TracTickets for help on using tickets.