Make WordPress Core

Opened 5 years ago

Last modified 9 months ago

#50136 new defect (bug)

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

Reported by: nikschavan's profile Nikschavan Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Upload Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

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.

Attachments (2)

50136.patch (2.6 KB) - added by ayeshrajans 5 years ago.
50136-2.patch (2.6 KB) - added by ayeshrajans 5 years ago.
Tests: https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/332563179

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
5 years ago

  • Component changed from General to Upload

#2 @ayeshrajans
5 years ago

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

Last edited 5 years ago by ayeshrajans (previous) (diff)

#3 @Nikschavan
5 years ago

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.

#4 @ayeshrajans
5 years ago

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

@ayeshrajans
5 years ago

#5 @dd32
5 years ago

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

  • ms-functions.php

     
    18491849        $site_mimes = array();
    18501850        foreach ( $site_exts as $ext ) {
    18511851                foreach ( $mimes as $ext_pattern => $mime ) {
    1852                         if ( '' != $ext && false !== strpos( $ext_pattern, $ext ) ) {
     1852                        if ( '' != $ext && preg_match( '!^(' . $ext_pattern . ')$!i', $ext ) ) {
    18531853                                $site_mimes[ $ext_pattern ] = $mime;
    18541854                        }
    18551855                }

#6 follow-up: @ayeshrajans
5 years ago

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.

#7 in reply to: ↑ 6 @dd32
5 years ago

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

This ticket was mentioned in PR #293 on WordPress/wordpress-develop by Nikschavan.


5 years ago
#8

  • Keywords has-patch has-unit-tests added

#9 @Nikschavan
5 years ago

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

#10 @ayeshrajans
5 years ago

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.

Note: See TracTickets for help on using tickets.