Opened 4 years ago
Last modified 8 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 | 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 -
- Navigate to the Network settings, Add file type tx to the setting Upload file types
- On any sub-sites, try to upload a .txt file and it should be uploaded.
- 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)
Change History (12)
#3
@
4 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
@
4 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.
#5
@
4 years ago
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(); 1850 1850 foreach ( $site_exts as $ext ) { 1851 1851 foreach ( $mimes as $ext_pattern => $mime ) { 1852 if ( '' != $ext && false !== strpos( $ext_pattern, $ext ) ) {1852 if ( '' != $ext && preg_match( '!^(' . $ext_pattern . ')$!i', $ext ) ) { 1853 1853 $site_mimes[ $ext_pattern ] = $mime; 1854 1854 } 1855 1855 }
#6
follow-up:
↓ 7
@
4 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
@
4 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.
4 years ago
#8
- Keywords has-patch has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/50136
#9
@
4 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
@
4 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.
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 blocktx
andtx*
file extensions iftx
in in the allow-list. Not the other way around thattxt
is allowed whentx
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.