Files types not included in Upload file types are allowed to be uploaded because of loose file extension check

[Nikschavan]

A loose file-extension check in WordPress allows an extended number of file-types to be uploaded despite not be mentioned in Upload file types setting in a multisite.

This happens because the condition to check the file extensions passes even if part of the extension passes. (​Code Link)

Steps To Reproduce:
On a WordPress Multisite -

1. Navigate to the Network settings, Add file type tx to the setting Upload file types
2. On any sub-sites, try to upload a .txt file and it should be uploaded.
3. Any file extension has to match in just part with the extensions allowed in the network setting to be allowed to be uploaded.

For example - If you add xls file type files xlsm, xlsx ,xlsb etc. are allowed to be uploaded.

[ayeshrajans]

Hi @Nikschavan - isn't this a case of overly strict file type check? Looking at the line you have linked:

strpos( $ext_pattern, $ext ) !== false <- This should block tx and tx* file extensions if tx in in the allow-list. Not the other way around that txt is allowed when tx is in the allow-list.

However, I think we probably should improve the strpos call to allow case sensitivity and check with proper word boundaries to make the comparison more strict.

!preg_match('/\b' . preg_quote($ext, '/') . '\b/i') would be a better check.

[Nikschavan]

Hi @ayeshrajans

This should block tx and tx* file extensions if tx in in the allow-list. Not the other way around that txt is allowed when tx is in the allow-list.

I am not sure about this. Can you try this out once if that is the case? I could reproduce the steps that I have mentioned where the txt file is allowed when the tx is allowed.

Here is a test snippet - ​https://3v4l.org/jUQY3 This demonstrates that over 10 file extensions are allowed when tx xls files are only approved. It can be seen that pptx, potx are allowed as they match tx in the above-mentioned condition.

!preg_match('/\b' . preg_quote($ext, '/') . '\b/i') would be a better check.

One thing to note is - There are also file extension groups, eg onetoc|onetoc2|onetmp|onepkg from which anyone extension can be matched.

[ayeshrajans]

@Nikschavan - you are absolutely right, my bad, the extension list was allowed with a simple text match, and I believe that's a big oopsie. For example, leaving extensions like h might allow html extension.

!preg_match('/\b' . preg_quote($ext, '/') . '\b/i') would be a better check.

One thing to note is - There are also file extension groups, eg onetoc|onetoc2|onetmp|onepkg from which anyone extension can be matched.

The \b should take care of it. For example the regex above will not match ach|html|tht when h is allowed as an extension. But it will allow mime type key txt|h|flw for h key.

I will work on a patch now.

[dd32]

This can probably be simplified down to a singular regular expression as used in ​wp_check_filetype():

ms-functions.php

@@ -1849 +1849 @@
         $site_mimes = array();
         foreach ( $site_exts as $ext ) {
                 foreach ( $mimes as $ext_pattern => $mime ) {
-                        if ( '' != $ext && false !== strpos( $ext_pattern, $ext ) ) {
+                        if ( '' != $ext && preg_match( '!^(' . $ext_pattern . ')$!i', $ext ) ) {
                                 $site_mimes[ $ext_pattern ] = $mime;
                         }
                 }

[ayeshrajans]

Thank you @dd32 - I also think the regex will make things cleaner. I don't know if we can get the file name passed to the check_upload_mimes function, because it looks like this function is supposed to return an allow-list of file mimes from the configuration, and functionality down stream will check the extension.

check_upload_mimes function takes a list of mime types as a parameter, but fetches the allowed file extensions from the database. This makes it not possible to unit test it, so I refactored it to filter_upload_mimes function that takes array of mimes _and_ the allowed extensions as a string.

I will upload another patch to fix the missing case-insensitive flag in the first patch. Thank you.

[ayeshrajans]

Tests: ​https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/332563179

[dd32]

Replying to ayeshrajans:

I don't know if we can get the file name passed to the check_upload_mimes function, because it looks like this function is supposed to return an allow-list of file mimes from the configuration, and functionality down stream will check the extension.

Correct, there's no need for the filename extension here, only the allowed list from the option.

This makes it not possible to unit test it, so I refactored it to filter_upload_mimes function that takes array of mimes _and_ the allowed extensions as a string.

It's still possible to unit test it, you simply need to either set the option prior to calling the function (The unit tests should take care of resetting that) or use a pre_option filter.

My problem with 50136.patch​ was primarily that it's a new function just for unit testing purposes, and using \b to match within a string which itself is intended to be used as a regular expression

Trac ticket: https://core.trac.wordpress.org/ticket/50136

[Nikschavan]

Added a new PR - ​https://github.com/WordPress/wordpress-develop/pull/293/ which improves upon work of @ayeshrajans and implement feedback from @dd32

Here is the updated snippet from ticket:50136#comment:3 which shows the new change does work in limiting the file extensions to only which are selected - ​https://3v4l.org/FaWn9

[ayeshrajans]

It didn't occur to me that these array keys were meant to be regular expressions in the first place, hence my initial patch with the elaborate word-boundary patterns. Some expressions like mpeg|mpg|mpe could be simplified to mpe?g?, but this of course loses the readability.

Changes from @Nikschavan's PR #293 looks good to me. We probably need some sort of notice to say these are regular expressions delimited by this character, to prevent the new entries from using unquoted regex delimiters or inefficient expressions that might backtrack excessively.