WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 57 minutes ago

Last modified 6 minutes ago

#39550 closed defect (bug) (fixed)

Some Non-image files fail to upload after 4.7.1

Reported by: greatislander Owned by: joemcgill
Milestone: 4.7.3 Priority: normal
Severity: critical Version: 4.7.1
Component: Upload Keywords: fixed-major
Focuses: Cc:

Description (last modified by joemcgill)

UPDATE: This issue affects more than just Word documents as initially reported. This ticket can be used to track related issues with all non-image files failing to load after 4.7.1 with an error message of Sorry, this file type is not permitted for security reasons.

===

Since [39831], a valid Word document (with the .docx extension) which has the application/octet-stream mime type can no longer be uploaded as the comparison in this block will fail.

A Word document can end up with this mime type when downloaded from a web server (this happens on GitHub, and I'm sure many other places) or when exported from an application other than Microsoft Word (e.g. Apple Pages, macOS 10.12).

There are certainly security considerations around modifying this behaviour, but as it stands, this change appears to be a regression from earlier versions, as many valid files can no longer be uploaded.

Attachments (12)

pkcs12-test-keystore.tar.gz (3.3 KB) - added by shfarr 6 weeks ago.
The archive contains a key store (X.509 test certificate with private key) and a file with the password to the certificate so you can test the validity of the key store. You will not need the password to test the upload process though.
patch-finfo.diff (374.5 KB) - added by blobfolio 6 weeks ago.
Patch to improve MIME handling/name corrections for uploads.
patch-finfo.2.diff (374.5 KB) - added by blobfolio 6 weeks ago.
patch-finfo.3.diff (374.4 KB) - added by blobfolio 6 weeks ago.
Replace static:: with self::
patch-finfo.4.diff (375.8 KB) - added by blobfolio 6 weeks ago.
integrate advanced type checks with allowed_mimes(), provide new extension when renaming is suggested, minor code standards fixes
39550.3.diff (4.4 KB) - added by joemcgill 13 days ago.
39550.4.diff (5.6 KB) - added by joemcgill 4 days ago.
39550-uploads.zip (35.1 KB) - added by joemcgill 4 days ago.
Test files
39550.5.diff (6.3 KB) - added by iandunn 3 days ago.
Adds unit test for comment:95
39550-uploads.2.zip (35.3 KB) - added by iandunn 3 days ago.
Adds test file for 39550.5.diff
39550.6.diff (5.8 KB) - added by joemcgill 3 days ago.
39550-multisite-tests.diff (2.8 KB) - added by joemcgill 42 hours ago.
Fixes tests for multisite installs

Download all attachments as: .zip

Change History (140)

#1 @SergeyBiryukov
7 weeks ago

  • Milestone changed from Awaiting Review to 4.7.2

This ticket was mentioned in Slack in #core by stiofansisland. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #forums by clorith. View the logs.


7 weeks ago

#5 @sterndata
7 weeks ago

I can verify that I'm unable to upload .mp3 files unless I add

define( 'ALLOW_UNFILTERED_UPLOADS', true );

to wp-config.php, but that doesn't seem to be a good, long term solution.

#6 @stiofansisland
7 weeks ago

This seems to also break .csv uploads on some servers, i guess it's also affected by PHP version or maybe a certain PHP extension.

#7 @Kenshino
7 weeks ago

  • Focuses administration added

#8 follow-ups: @greatislander
7 weeks ago

@stiofansisland Can you download this gist and test one of the CSV files you are trying to upload? From your command line, enter:

php check-mimetype.php /path/to/upload.csv

I can confirm that in my testing of two .docx files, one of them returns application/vnd.openxmlformats-officedocument.wordprocessingml.document and uploads to WordPress as expected, while the other returns application/octet-stream and fails to upload.

#9 follow-up: @joemcgill
7 weeks ago

  • Description modified (diff)
  • Focuses administration removed
  • Keywords needs-patch added
  • Owner set to joemcgill
  • Status changed from new to assigned
  • Summary changed from Non-image files with the application/octet-stream mime type cannot be uploaded to Some Non-image files fail to upload after 4.7.1

@greatislander Thanks for the report. You're correct that [39831] introduced more strict filetype checking in 4.7.1, which is resulting in previously valid uploads to fail. As @sterndata noted, setting
define( 'ALLOW_UNFILTERED_UPLOADS', true ); is a short term workaround, but one that should only be taken if you trust the users of your site not to upload insecure files.

In the mean time, it would help use test potential fixes for this issue by uploading or linking to example files that were previously working, but no longer are.

UPDATE: Please see https://wordpress.org/plugins/disable-real-mime-check/ as a better alternative to allowing unfiltered uploads.

Last edited 7 weeks ago by joemcgill (previous) (diff)

#10 in reply to: ↑ 9 @greatislander
7 weeks ago

Replying to joemcgill:

In the mean time, it would help use test potential fixes for this issue by uploading or linking to example files that were previously working, but no longer are.

Here's the file that first revealed this issue to me: styles_language_clean.docx

I'm not sure if it has the wrong mimetype because I downloaded it from GitHub, but it does—testing against my Gist shows that it's application/octet-stream.

#11 in reply to: ↑ 8 @stiofansisland
7 weeks ago

Replying to greatislander:

@stiofansisland Can you download this gist and test one of the CSV files you are trying to upload? From your command line, enter:

php check-mimetype.php /path/to/upload.csv

I can confirm that in my testing of two .docx files, one of them returns application/vnd.openxmlformats-officedocument.wordprocessingml.document and uploads to WordPress as expected, while the other returns application/octet-stream and fails to upload.

On a server where the csv fails i get: "text/plain" (PHP 7.0.8)
on a server where it works i get: "Fatal error: Call to undefined function finfo_open()", probably not helpful but maybe related?(PHP 5.6.22)

Sample csv: http://ayecode.io/test.csv

Stiofan

#12 @stevengliebe
7 weeks ago

I have been allowing the upload of proprietary .wie files in my Widget Importer & Exporter plugin.

<?php
function wie_add_mime_types( $mime_types ) {

        $mime_types['wie'] = 'application/json';

        return $mime_types;

}

add_filter( 'upload_mimes', 'wie_add_mime_types' );

This no longer works as of 4.7.1 without ALLOW_UNFILTERED_UPLOADS set true. In Media it will return "Sorry, this file type is not permitted for security reasons". Downgrading to 4.7 causes it to work again.

Same result on PHP 7.0.3 as with 5.6.20.

Last edited 7 weeks ago by stevengliebe (previous) (diff)

#13 follow-up: @Ipstenu
7 weeks ago

Same as @stiofansisland - On my server where it works, it throws the info() error. That said, it's not related to why it's working or not.

That just means I have to enable it. Of course when I do I get this:

Warning: finfo_file(): Empty filename or path in /check-mimetype.php on line 4

Tested on PHP 5.6 and 7.0 and everything uploads fine here on DreamHost boxes. I can get more info if needed.

This ticket was mentioned in Slack in #forums by ipstenu. View the logs.


7 weeks ago

#15 @SpencerFinnell
7 weeks ago

@stiofansisland's test.csv shows a text/plain mimetype for me and I am unable to upload on 4.8-alpha-39850

#16 @brunal
7 weeks ago

I also found ALLOW_UNFILTERED_UPLOADS set to true allows Administrator users to upload files which fail to upload since 4.7.1. But this trick doesn't work with user with role Author.

As a Author, I can upload .mp3 files, but not .wav files.

#17 in reply to: ↑ 13 @greatislander
7 weeks ago

Replying to Ipstenu:

Tested on PHP 5.6 and 7.0 and everything uploads fine here on DreamHost boxes. I can get more info if needed.

Does this word file work for you on Dreamhost?

#18 @Ipstenu
7 weeks ago

@greatislander - No, docx does not work.

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


7 weeks ago

#20 follow-up: @Ipstenu
7 weeks ago

I managed to create a box where uploads fail _but_ the script correctly spits out file types.

For people impacted, I recommend using https://wordpress.org/plugins/disable-real-mime-check/ for now.

#21 in reply to: ↑ 20 @stevengliebe
7 weeks ago

Replying to Ipstenu:

For people impacted, I recommend using https://wordpress.org/plugins/disable-real-mime-check/ for now.

Thank you for sharing that.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


7 weeks ago

#23 @Ipstenu
7 weeks ago

#39572 was marked as a duplicate.

#24 @emtatech
7 weeks ago

From what we've discovered with this bug, uploads seems to add a "x-" in the filetype which then is rejected as it does not match the original file type.

Example:

Uploaded a "audio/wav"
After upload, PHP reports type is set to "audio/x-wav"
Miss match, thus fails.

#25 follow-up: @Njones35
7 weeks ago

Not sure if this helps, but I have this issue (can't upload .csv files) on my Linux hosted sites after I upgraded to 4.7.1 - regardless of php version (I tried everything from 5.3 up to 7.1). I'e tried deactivating all plugins, reverting to TwentySixteen - I even installed a plugin which is supposed to allow extra file types and specifically whitelisted csv files - but the issue persists.

But - my localhost sites running on a windows server are unaffected and I can upload just like i used to.

I'm a non-coder so not sure if the Linux vs Windows thing is significant - I'm not even sure how to test it properly,.. but hopefully its a clue to help someone cleverer than me figure it out.

#26 in reply to: ↑ 25 ; follow-up: @pross
7 weeks ago

Replying to Njones35:

Not sure if this helps, but I have this issue (can't upload .csv files) on my Linux hosted sites after I upgraded to 4.7.1 - regardless of php version (I tried everything from 5.3 up to 7.1). I'e tried deactivating all plugins, reverting to TwentySixteen - I even installed a plugin which is supposed to allow extra file types and specifically whitelisted csv files - but the issue persists.

But - my localhost sites running on a windows server are unaffected and I can upload just like i used to.

I'm a non-coder so not sure if the Linux vs Windows thing is significant - I'm not even sure how to test it properly,.. but hopefully its a clue to help someone cleverer than me figure it out.

Did you try the plugin previously mentioned above? https://wordpress.org/plugins/disable-real-mime-check/

#27 in reply to: ↑ 8 @mensmaximus
7 weeks ago

Replying to greatislander:

I can confirm that in my testing of two .docx files, one of them returns application/vnd.openxmlformats-officedocument.wordprocessingml.document and uploads to WordPress as expected, while the other returns application/octet-stream and fails to upload.

I cant see any malfunction in WP 4.7.1. The file you provided has the mime type 'application/octet-stream'. Function get_allowed_mime_types() returns 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' as the expected mime type for Word documents that carry .docx as extension. Both mime types don't match therefore the upload is denied.

I opend the file in Word and saved it as a docx with different name. The new file could be uploaded without problem and shows the mime type 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'.

This behavior is expected in WP 4.7.1. You can check it e.g. by creating a CSV from an Excel file and rename it to .txt. Trying to upload that file will fail because the real mime type 'text/csv' won't match the expected mime type 'text/plain' as indicated by the extension.

Imho the real issue is having files not telling the truth about their real nature. Users need to be educated to not mess around with extensions and developers need to be educated to use the correct mime types while creating files programmaticaly.

Those servers throwing the finfo_file error message probably run on PHP prior to 5.3.

What maybe a problem is having extension allowing multiple mime types. In that case get_allowed_mime_types() would need to take care of it and return an array of mime types.

Last edited 7 weeks ago by mensmaximus (previous) (diff)

#28 in reply to: ↑ 26 ; follow-up: @AlexeyS_AS
7 weeks ago

The same problem with *.rar

Replying to pross:

Replying to Njones35:
Did you try the plugin previously mentioned above? https://wordpress.org/plugins/disable-real-mime-check/

Thank you happened to download

#29 in reply to: ↑ 28 @mensmaximus
7 weeks ago

Replying to AlexeyS_AS:

The same problem with *.rar

Can you check what mime type the *.rar file returns? wp_get_mime_types() returns 'application/rar'. According to wikipedia rar files use either 'application/x-rar-compressed' or 'application/octet-stream'. Imho 'application/octet-stream' is misused to much and should be used for real binaries (executable files) only (e.g. bin, exe or dll).

#30 follow-up: @shfarr
7 weeks ago

Hi Everybody,

Our plugin facilitates PKI based login and I can also confirm that in 4.7.1 people are unable to upload key store files. Up to this point we were enabling it by using the upload filters:

function identity_plus_enable_extra_extensions($mime_types =array() ) {
		$mime_types['p12']  = 'application/x-pkcs12';
		return $mime_types;
}

add_filter('upload_mimes', 'identity_plus_enable_extra_extensions');

I can confirm that the filter is executed in the upload media section and the array contains all the standard files and the added ones, still the files are rejected.

While you are at it, would it be possible to allow by default uploading this type of files?

As another observation, ".svg" files are also not allowed by default. This is a type that is being more and more often used over the web. Perhaps it should be added to the list.

Take care everybody,
Stefan

#31 in reply to: ↑ 30 ; follow-up: @mensmaximus
7 weeks ago

Replying to shfarr:

function identity_plus_enable_extra_extensions($mime_types =array() ) {
		$mime_types['p12']  = 'application/x-pkcs12';
		return $mime_types;
}

add_filter('upload_mimes', 'identity_plus_enable_extra_extensions');

Can you confirm the .p12 files have 'applicatio/x-pkcs12' as mime type and not 'application/keychain_access'?

#32 in reply to: ↑ 31 ; follow-ups: @shfarr
7 weeks ago

It is listed as "application/x-pkcs12" in this list (https://www.sitepoint.com/web-foundations/mime-types-complete-list/), or "application/pkcs-12" alternatively. We've so far used "x-pkcs12" and it worked as such.

I can send you a valid .p12 file for testing if that would help.

Thanks,
Stefan
~

Replying to mensmaximus:

Replying to shfarr:

function identity_plus_enable_extra_extensions($mime_types =array() ) {
		$mime_types['p12']  = 'application/x-pkcs12';
		return $mime_types;
}

add_filter('upload_mimes', 'identity_plus_enable_extra_extensions');

Can you confirm the .p12 files have 'applicatio/x-pkcs12' as mime type and not 'application/keychain_access'?

#33 in reply to: ↑ 32 ; follow-up: @mensmaximus
6 weeks ago

Replying to shfarr:

I can send you a valid .p12 file for testing if that would help.

This would be fantastic

#34 @mensmaximus
6 weeks ago

Comparing the mime types as defined by Apache with the mime types returned from wp_get_mime_types() I see some differences which will cause problems because WordPress is not checking the correct mime type.

The following list contains all non standard mime types in WordPress as well as those defined in WordPress but not included in the Apache mime types (http://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types)

https://gist.github.com/ocean90/5d0d0d52a6f3d31be4f821d8f0a6cf4d

Edit: Moved list to gist.github.com

Last edited 6 weeks ago by mensmaximus (previous) (diff)

#35 @mensmaximus
6 weeks ago

Same comparison between wp_get_mime_types() and a linux (Centos 7) mime.type file:

https://gist.github.com/ocean90/73b537e34353a8874bc9658f480d62d4

Edit: Moved list to gist.github.com

Last edited 6 weeks ago by ocean90 (previous) (diff)

#36 in reply to: ↑ 32 @mensmaximus
6 weeks ago

Replying to shfarr:

It is listed as "application/x-pkcs12" in this list (https://www.sitepoint.com/web-foundations/mime-types-complete-list/), or "application/pkcs-12" alternatively. We've so far used "x-pkcs12" and it worked as such.

This turns out to get tricky. IANA (http://www.iana.org/assignments/media-types/media-types.xhtml) says 'application/pkcs-12' while Apache says 'application/x-pkcs12'.

If this is true for a couple of mime types it will be fun to get the list of supported mime types implemented in WordPress. Imho the list returned from wp_get_mime_types needs to be reviewed anyway because allowing flashfiles to be uploaded but not SVG files seems wired to me.

Last edited 6 weeks ago by mensmaximus (previous) (diff)

#37 @mensmaximus
6 weeks ago

While playing around with the mime types I just noticed the function wp_get_ext_types() which defines extensions separately. This sounds to me redundant and may lead to further issues if those definitions get out of sync.

Within wp_check_filetype_and_ext() there is ounce more an array ($mime_to_ext) defining mime-extension pairs.

Maybe those functions and definitions should be combined like returning an array with all informations together like

default_mimes = array(
	'extension_1' => array(
		'group' => 'images'
		),
		'mimes' => array(
			'mime_type_1',
			'mime_type_2'
		),
		'roles_allowed' => array(
			'administrator',
			'publisher'
		)
	),
	'extension_2' => array(
		'group' => 'audio'
		),
		'mimes' => array(
			'mime_type_1',
			'mime_type_2'
		),
		'roles_allowed' => array(
			'administrator',
			'publisher'
		)
	)
);

For backward compatibility functions like wp_get_ext_types() could than simply fetch the array and rebuild the formerly used array to return it.

@shfarr
6 weeks ago

The archive contains a key store (X.509 test certificate with private key) and a file with the password to the certificate so you can test the validity of the key store. You will not need the password to test the upload process though.

#38 in reply to: ↑ 33 ; follow-up: @shfarr
6 weeks ago

Please see the test key store file in the attachment section. Cheers.

Replying to mensmaximus:

Replying to shfarr:

I can send you a valid .p12 file for testing if that would help.

This would be fantastic

#39 in reply to: ↑ 38 ; follow-up: @mensmaximus
6 weeks ago

Replying to shfarr:

Please see the test key store file in the attachment section. Cheers.

The p12 file you provided returns 'application/octet-stream' instead of 'application/x-pkcs12'.
Tested within WordPress and directly at the linux shell with finfo_file() from PHP 7.0.14.

#40 @ShpxLbh
6 weeks ago

Even Plugin disabling mime types test doesn't allow Gravity Forms to upload STL/ZIP/RAR files... what can we do to enable upload via gravity forms? all my sites are based on uploading files.
damn!

#41 follow-up: @mensmaximus
6 weeks ago

The deeper I dig into this the more I feel checking the real mime type for security reasons wont work well. As soon as a file or http stream does not provide a mime type it will be set to application/octet-stream (server side fallback).

#42 in reply to: ↑ 39 @shfarr
6 weeks ago

I am really sorry about that. I set the mime-type but never checked, as it was never necessary. Apparently as the file goes through the download process, the type changes back to octet-stream by the time it ends up in a file. Whether this is a browser issue or something in between remains to be seen. I will need to check.

On your other line of thought, the "server side fallback" method might be a good option. It is a practice for web applications to deliberately set the content type to octet-stream to facilitate download over opening the file by the browser, which probably creates a lot of mismatched files out there.

Replying to mensmaximus:

Replying to shfarr:

Please see the test key store file in the attachment section. Cheers.

The p12 file you provided returns 'application/octet-stream' instead of 'application/x-pkcs12'.
Tested within WordPress and directly at the linux shell with finfo_file() from PHP 7.0.14.

#43 in reply to: ↑ 41 @greatislander
6 weeks ago

Replying to mensmaximus:

The deeper I dig into this the more I feel checking the real mime type for security reasons wont work well. As soon as a file or http stream does not provide a mime type it will be set to application/octet-stream (server side fallback).

This conclusion was the basis of my initial report. Even exporting a Word document from Apple Pages sets the wrong mime type, causing this check to fail. It would be ideal to have a more secure upload validation method but the way mime types are set (often incorrectly) on various systems and in various contexts makes it hard to get reliable results.

#44 follow-up: @Ipstenu
6 weeks ago

FWIW, it's not setting the 'wrong' mime-type, it's setting a perfectly valid and acceptable mime-type.

There can be multiple acceptable mime types, as it turns out (that was when I started bashing my head into my desk, for the record).

The array idea @mensmaximus pitched is a good idea I think, but we have to make sure it'll support the existing way of adding new mimes to the okay list.

I mentioned in slack yesterday but here for posterity:

FWIW, I was testing with various things and trying to figure out why _some_ files worked and others didn't. Using file -I on my mac, I determined that if the filetype didn't match what we had in wp_get_mime_types it failed. In the case of the test.csv in the ticket, it comes back as text/plain - which is actually a valid mime type for CSV files (along with 9 others, depending on the OS/browser/app). It's the same reason the docx file in the ticket fails. It comes back as application/octet-stream; charset=binary and we're looking for application/vnd.openxmlformats-officedocument.wordprocessingml.document

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


6 weeks ago

#46 in reply to: ↑ 44 @mensmaximus
6 weeks ago

Replying to Ipstenu:

The array idea @mensmaximus pitched is a good idea I think, but we have to make sure it'll support the existing way of adding new mimes to the okay list.

After a second look I am not sure we will gain much from storing multiple mime types for an extension. If every file will fall back to application/octet-stream in case it reports no proper mime type we would have to allow application/octet-stream for any and all allowed extensions.

#47 follow-up: @kosso
6 weeks ago

4.7.1 immediately broke .m4a audio file uploads.

wp-includes/functions.php incorrectly assigns it as audio/mpeg

According to rfc4337 it should be set to audio/mp4

Changing it to this fixed m4a uploads for me.

  • Note: Many places have different ideas about what the correct MIME type for 'Apple MPEG-4 audio' should be. Observed are :


audio/mp4
audio/m4a
audio/x-m4a
audio/mp4a-latm (Low-overhead MPEG-4 Audio Transport Multiplex)


Last edited 6 weeks ago by kosso (previous) (diff)

#48 follow-up: @karolis92
6 weeks ago

Hello,
I'm also having problems after 4.7.1 update with uploading non image files.
It seems to break at the part where validation tries to get $real_mime using finfo_file

As i see $real_mime is undefined when it's compared to $type

Googled that finfo_file is only on PHP >= 5.3.0, and I'm running on 5.2.17.
Maybe that's the peroblem? I think that function_exists( 'finfo_file' ) should then return false, but it returns true. But I'm not a PHP developer so thats not the staff I really understand.

Last edited 6 weeks ago by karolis92 (previous) (diff)

#49 @blobfolio
6 weeks ago

I think the safest way forward, assuming finfo_file support is still considered a net gain, is to make a new MIME mapping function that accepts an extension and a MIME type, and compares that against a comprehensive database of historically-acceptable possibilities, etc.

<?php
function wp_check_ext_mime( $ext, $mime ) {
        $extensions = apply_filters( 'all_mime_types', array(
                ...
                'woff' => array(
                        'application/font-woff',
                        'font/woff',
                        'font/x-woff'
                )
                ...
        ));

        //lowercase arguments for easier comparison
        $ext = strtolower( $ext );
        $mime = strtolower( $mime );

        // Unknown extension
        if ( !isset( $extensions[$ext] ) ){
                return false;
        }

        return in_array( $mime, $extensions[$ext] );
}

Some historical variation for MIME types is predictable, allowing us to cut down on the number of explicit entries. For example, we could omit any whatever/x-whatever variants from the $extensions list and str_replace( "/x-", "/", $ext) prior to searching.

Anyhoo, wp_check_filetype_and_ext() should then whitelist $real_mimes evaluating to FALSE, "", and application/octet-stream. (It isn't the file's fault that PHP can't read it.)

If an actual MIME is given, the result can then be checked against wp_check_ext_mime().

This approach, though tedious, will at least preserve backward compatibility with existing functions and filters, unlike attempts to directly modify wp_get_mime_types(), etc., to be more flexible.

#50 follow-up: @blobfolio
6 weeks ago

On the MIME database front, this seems like a good place to start: http://www.iana.org/assignments/media-types/media-types.xhtml

#51 in reply to: ↑ 50 @kosso
6 weeks ago

Replying to blobfolio:

On the MIME database front, this seems like a good place to start: http://www.iana.org/assignments/media-types/media-types.xhtml

Interestingly, that doesn't even list MPEG-4 Audio .m4a audio/mp4

https://tools.ietf.org/html/rfc4337

#52 @blobfolio
6 weeks ago

I put together a MIME/extension helper library for PHP: https://github.com/Blobfolio/blob-mimes

It pulls together MIME/extension sources from IANA, Apache, Nginx, and freedesktop.org. If running inside a WP install, it also merges the entries returned by wp_get_mime_types(). This provides about 14x the data WP currently handles.

Maybe something like this should be integrated to provide more robust file type validation handling? I don't think the behaviors in the main WordPress MIME/type functions should change (at least not without a flag or filter to preserve backward compatibility), but the validation performed in wp_check_filetype_and_ext() could be improved to better work with the range of content users might upload.

The \blobmimes\file() helper provides "magic" finfo analysis and suggested alternative file names when there is a mismatch. Seems like non-image files should be renamed as well to give their "true" type a chance to pass the upload_mimes test.

Example file info: https://github.com/Blobfolio/blob-mimes/blob/master/docs/FILE.md#example
Example MIME types by extension: https://github.com/Blobfolio/blob-mimes/blob/master/docs/EXTENSION.md#example

The \blobmimes\extension()->has_mime() method also provides soft matching of "x-" type variants, which should help catch some additional outliers.

Any thoughts or suggestions welcome!

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

#53 follow-up: @Joe76000
6 weeks ago

Hello Folks,

After few days of searching, I found this very welcome topic.
My issue is with the latest WP 4.7.1 when trying to upload *.OGG files I have created.
I have also tried to upload .OGG files which were previously well uploaded but now they fail too.

In both case I'm getting the following error message:
"Désolé, ce type de fichier n’est pas autorisé pour des raisons de sécurité."
i.e. "Sorry, this file type is not permitted for security reasons.".

For example, a link to one of my previous .OGG file which cannot be uploaded anymore:

I have no upload issue with .jpg, .png, .gif and .mp3 files.

My site server configuration:

Thank you.

#54 in reply to: ↑ 53 ; follow-up: @blobfolio
6 weeks ago

Replying to Joe76000:

My issue is with the latest WP 4.7.1 when trying to upload *.OGG files I have created.
I have also tried to upload .OGG files which were previously well uploaded but now they fail too.

Please try installing this plugin as a workaround: https://wordpress.org/plugins/disable-real-mime-check/

#55 @dd32
6 weeks ago

#39596 was marked as a duplicate.

#56 in reply to: ↑ 54 @Joe76000
6 weeks ago

Replying to blobfolio:

Replying to Joe76000:

My issue is with the latest WP 4.7.1 when trying to upload *.OGG files I have created.
I have also tried to upload .OGG files which were previously well uploaded but now they fail too.

Please try installing this plugin as a workaround: https://wordpress.org/plugins/disable-real-mime-check/


Thank you very much. It's working like a charm!

#57 @programmin
6 weeks ago

+5 sites affected and it looks like updated wp4.7.2 alpha had problems uploading ogg as well.

@blobfolio
6 weeks ago

Patch to improve MIME handling/name corrections for uploads.

#58 @blobfolio
6 weeks ago

  • Keywords has-patch added; needs-patch removed

I rewrote the blob-mimes library to work within the WordPress core and altered wp_check_filetype_and_ext() to:

-- only pass expected image types to the image handling (i.e. fix SVG breakages)
-- run FINFO (if possible) if, after the image code, $type is still false
-- FINFO checks do not trigger false if the server cannot figure out what kind of file it is, or if the MIME returned is the generic "application/octet-stream"
-- the MIME database is greatly expanded (more than 10-fold) via blob-mimes, though only hooked into this one area at the moment. More work could be done to improve/expand MIME-related checks throughout the code base. But one step at a time...

#59 @andyimatest
6 weeks ago

I've had the same issue in 4.7.1 with .vtt files. Arbitrarily, they resolve as "text/plain" or "text/x-c" using finfo_file(), no matter where they came from, so I have to detect if the upload fails, then change the acceptable mimetype and try again.

Also, error messages using async_upload are not displayed in the Media page, I have to use Firebug to get the error message.

This ticket was mentioned in Slack in #core by rachelbaker. View the logs.


6 weeks ago

#61 follow-up: @rmccue
6 weeks ago

Just want to note: application/octet-stream isn't a real file type, it's just a default ("octet" = byte, the type just means "byte stream", aka unknown binary data).

@blobfolio Thanks for the patch. Can't comment on the functionality (not really my area), but in terms of structure: WordPress requires PHP 5.2+, so cannot use namespaces. To use your library, it would need to be 5.2-compatible.

#62 in reply to: ↑ 61 ; follow-up: @blobfolio
6 weeks ago

  • Keywords dev-feedback added

Replying to rmccue:

@blobfolio Thanks for the patch. Can't comment on the functionality (not really my area), but in terms of structure: WordPress requires PHP 5.2+, so cannot use namespaces. To use your library, it would need to be 5.2-compatible.

Thank you Ryan! I removed the namespace, and also added a condition to check whether the constant FILEINFO_MIME_TYPE is defined, as that also did not make an appearance until PHP 5.3.

#63 in reply to: ↑ 62 @shazahm1@…
6 weeks ago

I could be wrong... but late static binding (correct term???) was introduced in PHP5.3, correct? So, the method calls using the static keyword for scope resolution such as static::method_name() would not be compatible with PHP5.2.

Replying to blobfolio:

Replying to rmccue:

@blobfolio Thanks for the patch. Can't comment on the functionality (not really my area), but in terms of structure: WordPress requires PHP 5.2+, so cannot use namespaces. To use your library, it would need to be 5.2-compatible.

Thank you Ryan! I removed the namespace, and also added a condition to check whether the constant FILEINFO_MIME_TYPE is defined, as that also did not make an appearance until PHP 5.3.

@blobfolio
6 weeks ago

Replace static:: with self::

#64 @blobfolio
6 weeks ago

Thanks @shazahm1@…! I believe you're right. I replaced all instances of static::xxx with self::xxx.

#65 follow-ups: @glb955
6 weeks ago

I am having a problem uploading .mp3 files. Getting the "Sorry, this file type is not permitted for security reasons". I am not a coder so I need some direction on how & where to apply the patch mentioned above.

#66 in reply to: ↑ 65 @blobfolio
6 weeks ago

Replying to glb955:

I am having a problem uploading .mp3 files. Getting the "Sorry, this file type is not permitted for security reasons". I am not a coder so I need some direction on how & where to apply the patch mentioned above.

Applying a patch is a bit technical. An easier workaround for the time being is to install the following plugin: https://wordpress.org/plugins/disable-real-mime-check/

#67 in reply to: ↑ 65 @greatislander
6 weeks ago

Replying to glb955:

I am having a problem uploading .mp3 files. Getting the "Sorry, this file type is not permitted for security reasons". I am not a coder so I need some direction on how & where to apply the patch mentioned above.

If you aren't a coder your best bet is to install this plugin rather than attempting to patch WordPress core: https://wordpress.org/plugins/disable-real-mime-check/

Hope that works for you.

#68 @btolman
6 weeks ago

  • Severity changed from normal to critical
  • Version changed from trunk to 4.7.1

Hi!

We're seeing this problem with 4.7.1 Multi-Site running on a windows server on Azure (aka - the worst case scenario) when uploading PDF's. This is a municipal site, so everything is a pdf.

I just tried the plug-in, activated it and still cannot upload PDF's.

Before I apply diff's have they been tested on multi-site? Will they solve my PDF upload problem?

Note - sorry for the status change - but this is critical to us. We will have to revert if we can't get a resolution. Also, why does a mere "user" like myself even have access to the those particulars?

Last edited 6 weeks ago by btolman (previous) (diff)

#69 @blobfolio
6 weeks ago

Hi @btolman,

I haven't tested the third iteration of the patch on Multi-Site or Windows yet. I would be grateful if you'd give it a shot and report back. That would be a big help!

There shouldn't be any conflicts with Multi-Site. The only WordPress functions referenced by the new class (which is limited in scope to trying to find a file extension/MIME pairing) are sanitize_mime_type(), wp_check_invalid_utf8(), and wp_get_mime_types().

I did anticipate a few Windows-specific quirks with finfo_file() that I hope will result in a much more conservative effect on upload validation (erring on the side of "trust the filename"). The MIME/ext database this draws from has about 14x the amount of data the built-in WP version contains, so it should be much better at mapping OS/PHP-specific variations.

This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.


6 weeks ago

@blobfolio
6 weeks ago

integrate advanced type checks with allowed_mimes(), provide new extension when renaming is suggested, minor code standards fixes

#71 @blobfolio
6 weeks ago

Worked with @btolman to test the patch under Windows and Multi-Site; FINFO/MIME-handling worked as expected! Woo!

The latest version (4) of the patch integrates additional validation post-FINFO, re-checking the file information against the list of allowed MIMEs. This probably won't result in a change of heart very often, but is preferable to allowing everything through (or denying everything as 4.7.1 accidentally does).

If anyone else has had a chance to test, please post feedback. Let's get this bug fixed! Thanks!

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


5 weeks ago

#73 in reply to: ↑ 48 ; follow-up: @karolis92
5 weeks ago

Replying to karolis92:

Hello,
I'm also having problems after 4.7.1 update with uploading non image files.
It seems to break at the part where validation tries to get $real_mime using finfo_file

As i see $real_mime is undefined when it's compared to $type

Googled that finfo_file is only on PHP >= 5.3.0, and I'm running on 5.2.17.
Maybe that's the peroblem? I think that function_exists( 'finfo_file' ) should then return false, but it returns true. But I'm not a PHP developer so thats not the staff I really understand.

Seems to work after I asked to move my website to newer server (5.4)

#74 in reply to: ↑ 73 ; follow-up: @blobfolio
5 weeks ago

Replying to karolis92:

Replying to karolis92:

Googled that finfo_file is only on PHP >= 5.3.0, and I'm running on 5.2.17.

Part of finfo was introduced in PHP 5.2, but the part this WordPress function relied on was indeed not introduced until PHP 5.3. The included patch fixes that, among other issues.

@joemcgill, is there anything I can do to expedite getting this patch included? This issue is affecting quite a number of installations and it would be good to see it fixed.

#75 follow-up: @inderpreet99
5 weeks ago

This happens in WP 4.6.2 as well running with PHP 5.3 and PHP 7.0.

Will the upcoming patch also be added to WP 4.6.x?

#76 in reply to: ↑ 75 @blobfolio
5 weeks ago

Replying to inderpreet99:

This happens in WP 4.6.2 as well running with PHP 5.3 and PHP 7.0.

Will the upcoming patch also be added to WP 4.6.x?

At the moment the patch is just built against the main src, but it doesn't contain any 4.7-specific dependencies. It should be a straight copy-and-paste job to port it to the 4.6 branch.

#77 in reply to: ↑ 74 @joemcgill
5 weeks ago

Replying to blobfolio:

@joemcgill, is there anything I can do to expedite getting this patch included? This issue is affecting quite a number of installations and it would be good to see it fixed.

Hi @blobfolio. Thank you for all the effort you've put into this issue, it's much appreciated. I want to give you, and others following this issue, a few updates.

First off, it's a top priority on my end to have this issue addressed in the next scheduled minor release, which is currently planned for a few weeks from now (but is always subject to change). As there are some security concerns related to this issue, I want to be careful about handling the solution, so there may be progress being made that isn't discussed publicly at this time.

That said, the work you and others have done to discuss, patch, and test this issue is quite helpful so please feel free to continue. I definitely don't want you to think that lack of a quick response does not reflect a lack of interest or appreciation.

I am grateful for everyone's patience while related issues are being worked out.

#78 @blobfolio
5 weeks ago

Thanks @joemcgill! I figured as much. :) I've been using variations of the patched technique for years at a theme/plugin level to tighten upload handling and avoid passing nasty objects through certain overly-privileged server libraries and browser plugins. While I didn't include them in the patch (tried to stick within the scope of the original work), there are some other ways to try and determine true MIME types that might be worth including as fallbacks for the < 5.3 crowd. As I'm sure you've discovered none of the methods are perfect, but they may be better than nothing.

If there's anything I can do off-channel to help, please let me know (I assume you have access to my contact info, but if not the linked web site in the profile does).

#79 in reply to: ↑ 47 @hearvox
5 weeks ago

We (Echoes.org) were also no longer able to upload .m4a files in 4.7.1. Disable Real MIME Check plugin fixed it. I'll monitor this ticket: We'd be happy to do any testing.

Thanks for everyone's work on this.

#80 @chrisl27
5 weeks ago

People that were specifically whitelisting unusual mime types/extensions via the upload_mimes files could restore the original functionality just for those whitelisted items using this wp_check_filetype_and_ext filter, if you don't want to edit core: (Note, not extensively tested)

 function wp_check_filetype_and_ext($wp_check_filetype_and_ext, $file, $filename, $mimes) {
    // If the content was already okay
    if ($wp_check_filetype_and_ext['ext'] && $wp_check_filetype_and_ext['type']) {
      return $wp_check_filetype_and_ext;
    }

    // If wp_check_filetype can establish the mime type from the upload_mimes filter, merge it into the result
    return array_merge($wp_check_filetype_and_ext, wp_check_filetype($filename, apply_filters('upload_mimes', array())));
  }

#81 follow-up: @wpmadd
5 weeks ago

Hi,

I have recently come across an issue for uploading web fonts and favicon's to Media Library ... this has now become a problem on all of my (previous) sites (most have a theme called AVADA where you have the option to upload you fav web fonts) and for some reason I cannot even upload web fonts (via WPML) on a fresh install of WP 4.5.3, where I now get the error: I now get the error: Sorry, this file type is not permitted for security reasons., let alone 4.7.1 ... this error did not appear when I built the previous sites ...

Would there (or has there) been a background update on older versions of WP, prior to 4.7.1? ...

Even using Disable Real MIME Check (By Sergey Biryukov) has not helped ... although that did fix the favicon upload issue but has not done the same for web fonts ...

thanks.

Last edited 5 weeks ago by wpmadd (previous) (diff)

#82 in reply to: ↑ 81 ; follow-up: @blobfolio
5 weeks ago

Replying to wpmadd:

I have recently come across an issue for uploading web fonts and favicon's to Media Library

The Disable Real Mimes plugin should reverse the breaking behavior in the latest dot-dot releases. In cases where it does not, it is likely another plugin or theme is adding to the trouble. Do you by any chance have any other file/upload-related plugins, like for example something that allows you to override the allowable file types?

#83 in reply to: ↑ 82 @wpmadd
5 weeks ago

Replying to blobfolio:

Do you by any chance have any other file/upload-related plugins, like for example something that allows you to override the allowable file types?

No, I have no file/upload-related plugins installed ...

I only have:
A backup plugin (Duplicator) ... the theme itself and the plugins that come with the theme ...

But as I stated, this now seems to be an issue even with a fresh vanilla install of WP 4.5.3, as well as 4.7.1 ... these installs have had nothing added to them ...

thanks.

Last edited 5 weeks ago by wpmadd (previous) (diff)

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


5 weeks ago

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


4 weeks ago

#86 @contrid
4 weeks ago

We noticed this problem with CSV files.

With wp_check_filetype(), WordPress returns text/csv as the type. The wp_handle_upload() function uses wp_check_filetype_and_ext() function which uses finfo_file() PHP function to see what the real mime type of the file is. For CSV it seems to return text/plain. Could be related to some versions of PHP?

When the real mime coming from finfo_file() PHP and the mime coming from wp_check_filetype() don't match up, it returns false. The code which creates the "Sorry, this file type is not permitted for security reasons." error message is in wp-includes/functions.php.

There are several ways to fix this such as using the wp_check_filetype_and_ext filter or possibly setting test_type to false in the wp_handle_upload() overrides. This is a security risk so you need to do manual checking on the file and it's mime then. At least you can then check if the type is one of many CSV mimes often used eg. application/csv, text/csv, text/plain, text/comma-separated values.

Correct me if I'm wrong, I'm just trying to help.

This specifically became a problem in WordPress 4.7.1 and worked fine before that, detecting the real mime and mime from supported/allowed mime types list correctly before.

Update:

It looks like the finfo_file() function is a new thing in WordPress 4.7.1 checking the real mime type of the file and it wasn't there before. If you don't have finfo_file() function available on your PHP, you won't experience problems but if you do and the mime of WordPress and PHP don't match you get the security error.

The Disable Real Mime Check plugin does the trick: https://wordpress.org/plugins/disable-real-mime-check/ . It hooks to the wp_check_filetype_and_ext and replaces whatever the real mime check did back with wp_check_filetype() results, making sure the mimes match so the file can complete.

Last edited 4 weeks ago by contrid (previous) (diff)

#87 @Clorith
3 weeks ago

#39777 was marked as a duplicate.

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


3 weeks ago

#89 @mdgl
3 weeks ago

Why on earth are we pretending that finfo_file() returns the "real" MIME type of a file?

Doesn't this function just examine the first few bytes of a file, compare it against various patterns defined in the "magic" file and take a guess at what the MIME type might be?

Most file systems don't store the MIME type of a file separately, so this kind of guess is the best that you can do. But it's often going to be wrong! As others have pointed out, multiple values are also valid. A file containing HTML can be perfectly correctly described by the MIME type text/plain as well as text/html or even application/octet-stream. It all depends on what you want to do with the data!

We can declare the MIME type of a file when it is being transferred by HTTP, because the protocol provides a way to indicate this and specify how we want the data to be provided and/or interpreted through the Accept and Content-Type headers. But once the file is stored on the system, this information is lost and it's up to other applications to determine how they want to process the data. For example, a Web browser might want to treat the file as text/html whereas a backup program might want to consider it as application/octet-stream.

The only real way to determine whether a particular file contains data that conforms to a certain MIME type is to parse it *fully* and see whether it complies with the specification. Even then, multiple values are possible. You need to know first what MIME type you want to test the file against! For example, many files produced by the current Microsoft Office suite can be considered valid ZIP archives as well as Word, PowerPoint and so on. It is not in general possible to determine a single "real" MIME type of a file just by examining the data.

I can understand the security concerns here and why WordPress would want to "batten down the hatches" with respect to uploaded files, but I do wonder if we are barking up the wrong tree with the current solution approach.

#90 @scotterickson
3 weeks ago

I am also experiencing this problem, specifically with mp3 files. Interestingly, the original mp3 file created by iTunes (extracting audio from a video file) will fail to upload, but either extracting the audio from the video OR just "converting" from the original (failing) mp3 to a new mp3 using VLC Media player results in an mp3 file that successfully uploads. If it is helpful, I could upload both the failing and succeeding mp3 files on this ticket - let me know.

#91 follow-up: @programmin
3 weeks ago

@scotterickson Did the mp3 uploading problem happen for that file on older versions of Wordpress? If so that is likely bug #32318 where a seemingly working mp3 file has some header info that WP does not like, leaving the upload uploaded but never added to Wordpress media library.

I have seen that on several occasions and I agree it's annoying, but that may not be related to this 4.7.1 update, that bug has been happening for at least since WP 4.3.

Last edited 3 weeks ago by SergeyBiryukov (previous) (diff)

#92 in reply to: ↑ 91 @scotterickson
3 weeks ago

This issue began happening in January 2017 and had never occurred before that. I can't guarantee which version of WP was running when it started to occur. However, the remedy suggested on this ticket (using disable mime check plugin) did resolve the problem. I know that is no guarantee that this is the same bug (the remedy may just have happened to resolve the issue), but am tracking this ticket and will see when 4.7.3 comes out (assuming that is when this is resolved) whether that resolves the problem without the disable mime check plugin (as I would prefer not to use that workaround any longer than required).

Replying to programmin:

@scotterickson Did the mp3 uploading problem happen for that file on older versions of Wordpress? If so that is likely bug #32318 where a seemingly working mp3 file has some header info that WP does not like, leaving the upload uploaded but never added to Wordpress media library.

I have seen that on several occasions and I agree it's annoying, but that may not be related to this 4.7.1 update, that bug has been happening for at least since WP 4.3.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 weeks ago

#94 @leemon
3 weeks ago

I can confirm this issue when uploading some MP4 videos.
Installing the Disable Real MIME Check plugin "fixes" it.

#95 @goldsounds
2 weeks ago

This regression broke uploads for my plugin https://wordpress.org/plugins/gltf-media-type/

This plugin identifies .gltf files as type 'model/gltf+json', but the finfo_file function identifies them as 'text/plain' (because under the hood they're JSON).

This results in wp_check_filetype_and_ext returning false for ext and type, and the security error.

This is basically a 100% functionality breakage as far as my plugin is concerned!

Last edited 2 weeks ago by goldsounds (previous) (diff)

This ticket was mentioned in Slack in #core by jnylen. View the logs.


2 weeks ago

@joemcgill
13 days ago

#97 follow-up: @joemcgill
13 days ago

39550.3.diff is a modified approach to mime/type checking with a more narrow implementation. This approach is designed to add hardening over the pre-4.7.1 approach while not being overly strict.

As @blobfolio and others have noted, there are various methods for determining mimes for every filetype, and none are without their flaws. For now, strict filetype checking in all cases is probably overkill for our needs. Sites requiring that level of security can implement more strict checking using methods described by others in this thread.

Testing and feedback of this patch is appreciated.

#98 in reply to: ↑ 97 ; follow-up: @blobfolio
13 days ago

Replying to joemcgill:

39550.3.diff is a modified approach to mime/type checking with a more narrow implementation. This approach is designed to add hardening over the pre-4.7.1 approach while not being overly strict.

Thanks @joemcgill.

The patch does not address image-related MIME troubles 4.7.1 introduced. By calling out MIMEs matching "image/XXX" specifically, any valid image type that is not something the thumbnailer is meant to manipulate will fail (SVG, WebP, etc.).

It would be safer to move the $mime_to_ext assignment higher up the chain and change the condition to if ( $type && 0 === strpos( $type, 'image/' ) && isset ( $mime_to_ext[ $type ] ) ) {

Also the constant FILEINFO_MIME_TYPE was not introduced until PHP 5.3, but some other fileinfo_* functionality was present earlier. Changing the condition to if ( $type && ! $real_mime && extension_loaded( 'fileinfo' ) && defined( 'FILEINFO_MIME_TYPE' ) ) { should be good enough.

#99 @SergeyBiryukov
13 days ago

#39877 was marked as a duplicate.

#100 in reply to: ↑ 98 ; follow-up: @joemcgill
13 days ago

Replying to blobfolio:

Thanks for the quick response. I don't believe $mime_to_exts was ever being used to affect whether an image was allowed to be uploaded, but instead was used to rename image files to use the correct extension if it were determined to be different than what is returned by wp_check_filetype() but found in $mime_to_exts. Previously, if WP couldn't rename the file, it would pass back whatever wp_check_filetype() reported.

This means that now any mime types you want to add via the upload_mimes filter would need to pass our checks in wp_check_filetype_and_ext() or you will also need to filter the output of wp_check_filetype_and_ext() as well. A bit annoying, but perhaps an acceptable tradeoff? What we don't want to do is make $mime_to_exts a blanket whitelist of mime-to-extension possibilities.

Thanks for the catch on the FILEINFO_MIME_TYPE introduction. I'll make that update.

#101 in reply to: ↑ 100 ; follow-up: @blobfolio
13 days ago

Replying to joemcgill:

Replying to blobfolio:

What we don't want to do is make $mime_to_ext a blanket whitelist of mime-to-extension possibilities.

Definitely. I'm not suggesting making $mime_to_ext a blanket whitelist, but the point of that particular block of code is to try and generate web-able images where it can. That doesn't apply to any file types outside $mime_to_ext. Because the outer check ("image/*") is looser than the inner (the $mime_to_ext list), anything in between will be irrevocably failed.

If an upload doesn't match the $mime_to_ext list, it is outside the purview of that particular challenge and should be passed down the chain to finfo and eventually upload_mimes.

It's one thing for WordPress to not explicitly image-ize unusual graphics formats, but another to say, "Sorry, you can't use these at all no matter what." Particularly with SVG, which is so prominent (even the core is using some now, haha).

While it is possible to require both a hook into upload_mimes and another into wp_check_filetype_and_ext, that seems both tedious and a bit dangerous (it would be pushing people toward messing with a nuanced and sensitive function they wouldn't otherwise have to).

Last edited 13 days ago by blobfolio (previous) (diff)

#102 @pampfelimetten
11 days ago

Same problem here. with some mp3 files it's working just fine, with others it doesn't.

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


10 days ago

#104 in reply to: ↑ 101 @joemcgill
10 days ago

Replying to blobfolio:

If an upload doesn't match the $mime_to_ext list, it is outside the purview of that particular challenge and should be passed down the chain to finfo and eventually upload_mimes.

I tend to agree that we should revalidate anything that misses the wp_get_image_mimes() check, rather than blocking them at that point. I've thought about combining all validation for all mimes into a single place, rather than different paths for images vs other files, but my early testing showed that doing so would increase memory/time for image uploads. Still, might be worth attempting, but in the mean time using the finfo_file() check as a fallback for any file that isn't validated at that point is probably the best option.

#105 @SergeyBiryukov
4 days ago

#39951 was marked as a duplicate.

#106 follow-up: @kontur
4 days ago

Even when passing in my augmented mimes again as $overwrites (which I shouldn't have to) in https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/functions.php#L2326 finfo returns the type for my uploaded woff as ""application/octet-stream", which contradicts the mime received from the file extension which wp_check_filetype() correctly extracted, alas $ext and $type get set to false, which triggers the error message then.

#107 in reply to: ↑ 106 ; follow-up: @blobfolio
4 days ago

Replying to kontur:

Even when passing in my augmented mimes again as $overwrites [...] finfo returns the type for my uploaded woff as ""application/octet-stream", which contradicts the mime received from the file extension which wp_check_filetype() correctly extracted, alas $ext and $type get set to false, which triggers the error message then.

Yeah, in general, application/octet-stream, text/plain, and anything falsey like NULL, FALSE, or "" should basically be treated as a lack of finfo support rather than a hard-stop error.

At any rate, the role of finfo in the upload process is going to have to be diminished quite a bit from its 4.7.1 implementation. It can be used to reliably detect certain limited types of deception, but because the WordPress core only natively supports a single ext:mime mapping, it can't be used for broader validation.

The WOFF spec does include magic number support, but since the format has been associated with a half-dozen different MIME types over the years, even if PHP does produce a match, it might not be the one match WordPress is looking for.

There is, however, potential for plugins to provide a more robust finfo()-based validation process. Depending on how this ticket plays out, I'll put something together for everyone. ;)

#108 in reply to: ↑ 107 ; follow-up: @kontur
4 days ago

Yes, I understand. At any rate, this bug pretty much renders a plugin of mine useless, since it is centred around uploading and displaying webfonts, all of which, I think, require that the accepted mime type gets augmented, and then successfully passes this check - which currently it doesn't, or seemingly only sporadic on some installations.

Replying to blobfolio:

Replying to kontur:

Even when passing in my augmented mimes again as $overwrites [...] finfo returns the type for my uploaded woff as ""application/octet-stream", which contradicts the mime received from the file extension which wp_check_filetype() correctly extracted, alas $ext and $type get set to false, which triggers the error message then.

Yeah, in general, application/octet-stream, text/plain, and anything falsey like NULL, FALSE, or "" should basically be treated as a lack of finfo support rather than a hard-stop error.

At any rate, the role of finfo in the upload process is going to have to be diminished quite a bit from its 4.7.1 implementation. It can be used to reliably detect certain limited types of deception, but because the WordPress core only natively supports a single ext:mime mapping, it can't be used for broader validation.

The WOFF spec does include magic number support, but since the format has been associated with a half-dozen different MIME types over the years, even if PHP does produce a match, it might not be the one match WordPress is looking for.

There is, however, potential for plugins to provide a more robust finfo()-based validation process. Depending on how this ticket plays out, I'll put something together for everyone. ;)

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


4 days ago

#110 in reply to: ↑ 108 ; follow-up: @blobfolio
4 days ago

Replying to kontur:

Yes, I understand. At any rate, this bug pretty much renders a plugin of mine useless, since it is centred around uploading and displaying webfonts, all of which, I think, require that the accepted mime type gets augmented, and then successfully passes this check - which currently it doesn't, or seemingly only sporadic on some installations.

You could add a filter to your plugin to specifically bypass the issue only for font files, either re-implementing a looser finfo check (but again for font files I don't expect that will work consistently), or just rebuilding the ext/type data by passing the original filename back through wp_check_filetype. Take a look at this function for inspiration: https://github.com/Blobfolio/blob-common/blob/master/wp/functions-behavior.php#L31 Obviously all the class stuff is a bit out of scope for your needs, but if you remove 75% of it you should have what you need.

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


4 days ago

@joemcgill
4 days ago

@joemcgill
4 days ago

Test files

#112 follow-up: @joemcgill
4 days ago

  • Keywords needs-testing added; dev-feedback removed

39550.4.diff Updates the approach from 39550.3.diff to revalidate any image files that fail to pass the image validation step in wp_check_filetype_and_ext(). I've updated and added the tests to account for a few cases where the upload_mimes filter is in use. To run the tests, you'll need to add files in 39550-uploads.zip to tests/phpunit/data/uploads.

#113 in reply to: ↑ 110 ; follow-up: @kontur
3 days ago

Thanks a lot! I will refactor this into my plugin to work around the issue until a fix is in place.

I couldn't find "wp_check_filetype_and_ext" in this reference: https://codex.wordpress.org/Plugin_API/Filter_Reference but it seems to work anyhow.

Replying to blobfolio:

Replying to kontur:

Yes, I understand. At any rate, this bug pretty much renders a plugin of mine useless, since it is centred around uploading and displaying webfonts, all of which, I think, require that the accepted mime type gets augmented, and then successfully passes this check - which currently it doesn't, or seemingly only sporadic on some installations.

You could add a filter to your plugin to specifically bypass the issue only for font files, either re-implementing a looser finfo check (but again for font files I don't expect that will work consistently), or just rebuilding the ext/type data by passing the original filename back through wp_check_filetype. Take a look at this function for inspiration: https://github.com/Blobfolio/blob-common/blob/master/wp/functions-behavior.php#L31 Obviously all the class stuff is a bit out of scope for your needs, but if you remove 75% of it you should have what you need.

#114 in reply to: ↑ 113 @blobfolio
3 days ago

Replying to kontur:

I couldn't find "wp_check_filetype_and_ext" in this reference: https://codex.wordpress.org/Plugin_API/Filter_Reference but it seems to work anyhow.

Yeah, I found that one in wp-includes/functions.php. Haha.

<?php
/**
         * Filters the "real" file type of the given file.
         *
         * @since 3.0.0
         *
         * @param array  $wp_check_filetype_and_ext File data array containing 'ext', 'type', and
         *                                          'proper_filename' keys.
         * @param string $file                      Full path to the file.
         * @param string $filename                  The name of the file (may differ from $file due to
         *                                          $file being in a tmp directory).
         * @param array  $mimes                     Key is the file extension with value as the mime type.
         */

This filter triggers after the main upload file checking, so with that in mind just be sure you are only overriding the yay/nay for font files. (It is easy to accidentally allow everything or deny everything... oops.)

#115 in reply to: ↑ 112 ; follow-up: @blobfolio
3 days ago

Replying to joemcgill:

39550.4.diff Updates the approach from 39550.3.diff to revalidate any image files that fail to pass the image validation step in wp_check_filetype_and_ext().

The $real_mime reset you added to the image bit seems to do the trick!

I wonder if there's an opportunity to offer to rename a file based on what finfo reports? Should be non-blocking (i.e. just fallback to leaving the file named as was), but if WordPress has an entry for both the file extension-derived type and the $real_mime and they're different, it could do an extension swap (like what happens for images now).

For that check in particular, we'd want to ignore application/octet-stream or any type beginning text/ (or else things would be incorrectly renamed all the time), but given two specific types, it could save some error log space down the road. Wouldn't work on all servers since type detection isn't consistent, but would help sometimes.

@iandunn
3 days ago

Adds unit test for comment:95

@iandunn
3 days ago

Adds test file for 39550.5.diff

#116 @iandunn
3 days ago

39550.5.diff and 39550-uploads.2.zip​ add a unit test for application/ mimes with sub-types, like the case that @goldsounds mentioned in comment:95.

The test currently fails, because $real_mime is application/xml, and only application/ttml+xml was allowed via upload_mimes.

This ticket was mentioned in Slack in #wptv by iandunn. View the logs.


3 days ago

This ticket was mentioned in Slack in #core by jnylen. View the logs.


3 days ago

#119 @joemcgill
3 days ago

Thanks @iandunn, I had missed @goldsounds' comment.

The main issue with your new exmaple is that the extra mime type being added via the filter does not match the mime type that is being picked up by finfo. As you pointed out, if application/xml was already whitelisted by get_allowed_mime_types() then the test would not fail. Since we can't arbitrarily trust files with 'application' mime types for security reasons, I think this would be up to a developer to work around by making sure the mime type they were adding through the upload_mimes filter matched with the verification step in wp_check_filetype_and_ext() or by filtering the output of wp_check_filetype_and_ext() itself.

In the future, we can look into adding more robust mime-type checking (see #39963) which could help us whitelist multiple subtypes for a given file extension. In the mean time, 39550.4.diff restores functionality lost in 4.7.1 for most cases.

#120 in reply to: ↑ 115 ; follow-up: @joemcgill
3 days ago

I wonder if there's an opportunity to offer to rename a file based on what finfo reports? Should be non-blocking (i.e. just fallback to leaving the file named as was), but if WordPress has an entry for both the file extension-derived type and the $real_mime and they're different, it could do an extension swap (like what happens for images now).

Thanks @blobfolio. I think this enhancement is worth considering, but I think that's out of scope for this current issue.

#121 in reply to: ↑ 120 @blobfolio
3 days ago

Replying to joemcgill:

I wonder if there's an opportunity to offer to rename a file based on what finfo reports? Should be non-blocking (i.e. just fallback to leaving the file named as was), but if WordPress has an entry for both the file extension-derived type and the $real_mime and they're different, it could do an extension swap (like what happens for images now).

Thanks @blobfolio. I think this enhancement is worth considering, but I think that's out of scope for this current issue.

Agreed, especially as we do not want to add further delay to this patch. Once we figure out #39963 we'll have an opportunity for more robust renaming anyhow.

@joemcgill
3 days ago

#122 @joemcgill
3 days ago

39550.6.diff is improves a few inline comments in 39550.4.diff but is otherwise identical.

#123 @joemcgill
2 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40124:

Media: Reduce failing uploads following 4.7.1.

[39831] introduced more strict MIME type checking for uploads, which
resulted in unintetionally blocking several filetypes that were
previously valid. This change uses a more targeted approach to MIME
validation to restore previous behavior for most types.

Props blobfolio, iandunn, ipstenu, markoheijnen, xknown, joemcgill.
Fixes #39550, #39552.

#124 @joemcgill
2 days ago

  • Keywords fixed-major added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to fix in previous branches.

@joemcgill
42 hours ago

Fixes tests for multisite installs

#125 @joemcgill
42 hours ago

A few of the multisite tests were failing after [40124] because multisite filters upload_mimes with the check_upload_mimes() function to reduce the set of allowed mime types. 39550-multisite-tests.diff only fixes this by skipping a the tests for adding additional mime types and only tests file types assumed to be allowed.

#126 @joemcgill
28 hours ago

In 40125:

Media: Fix unit tests for MIME checks on multisite.

A few of the multisite tests were failing after [40124] because
multisite filters upload_mimes with the check_upload_mimes()
function to reduce the set of allowed MIME types. This fixes those
errors by skipping the tests for adding additional MIME types and
only tests file types assumed to be allowed.

See #39550.

#127 @joemcgill
57 minutes ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40134:

Media: Reduce failing uploads following 4.7.1.

[39831] introduced more strict MIME type checking for uploads, which
resulted in unintetionally blocking several filetypes that were
previously valid. This change uses a more targeted approach to MIME
validation to restore previous behavior for most types.

Props blobfolio, iandunn, ipstenu, markoheijnen, xknown, joemcgill.
Merges [40124] and [40125] to the 4.7 branch.
Fixes #39550, #39552.

#128 @pavelevap
6 minutes ago

Ticket is closed, but this will be fixed also for 4.6.x, right?

Note: See TracTickets for help on using tickets.