Validation of filenames (while unzipping) causes unexpected failures

[Ipstenu]

Related to #41457

I have a theme zip (proprietary themeforest, sorry) that has files like this:

     1972  09-20-2017 16:17   THEME/tag.php
        0  09-20-2017 16:17   THEME/template-parts/
        0  09-20-2017 16:17   THEME/template-parts/./
        0  09-20-2017 16:17   THEME/template-parts/../
        0  09-20-2017 16:17   THEME/template-parts/archive/
        0  09-20-2017 16:17   THEME/template-parts/archive/./
        0  09-20-2017 16:17   THEME/template-parts/archive/../

When I try to install the theme, it throws an error:

Unpacking the package…

Could not extract file from archive. THEME/./

It appears that this change is seeing those and failing, where as if you force the file to pclzip it works fine.

Should we not just silent discard the /./ and /../ files instead of failing? That would prevent PHPzip from doing stupid things, and allow uploads when people build clever zips.

Given the theme is a purchase from a customer, I'm reluctant to share it publicly.

[voldemortensen]

#42026 was marked as a duplicate.

[DavidAnderson]

To add some more info: In #42046, the problem is that the check is too broad. Disallowing anything with .. in causes ordinary files to cause the whole unzip to abort, not just preventing directory-traversal attempts (and with no means to use a filter to over-ride it). e.g. my-image..jpg (we've already seen this pattern among our real-world users twice). To prevent directory-traversal, it should be checking for ../ and ..\, not just for ... Regardless of whether it's tasteful or not, .. is a valid, in-use filename pattern and not an automatic indication of a security issue.

[Ipstenu]

In theory ./ and ../ and ..\ and .\ can all be 'skipped' and not processed (i.e don't expand it, don't try to save it, just log the error). While they may cause issues, we can use an alert similar to what we do when we have a headers alert when you activate a plugin that dumps a bunch of stuff.

"Unexpected zip output. If you experience issues with your [theme|plugin] please contact the developer directly, as their zip may have been improperly packaged."

As for the 'real' use of filename..jpg I'm on the fence here. It's legit, but if we can't sanely trap it, then we should also be discarding them with the same alert.

[DavidAnderson]

As for the 'real' use of filename..jpg I'm on the fence here. It's legit, but if we can't sanely trap it, then we should also be discarding them with the same alert.

I just ran some tests on a shared hosting server that's currently hosting 188 widely varying (in content and ownership) WP sites. 12 of them (i.e. 6.3%) contain this pattern somewhere in wp-content/uploads (searching for ..jpg, ..jpeg, ..gif, ..png, ..pdf). The number of sites with files matching this pattern is likely huge.

There's no danger posed by two consecutive periods that aren't followed by a slash. If WP core decides to handle it as potentially dangerous, I think it at least needs a filter so that plugins can trap it if they want to (so that they can at least get the pre-4.8.2 behaviour). Though, I'd strongly propose only trapping when it's followed by a slash, so that every plugin author who handles unzipping doesn't have to learn about it the hard way.

[johnbillion]

Confirmed that multiple consecutive periods in a file name is not an issue and shouldn't be caught when unzipping.

[DavidAnderson]

This patch allows developers to filter/over-ride the validate_file() results. It is not a complete fix for this issue (I will put that in a separate patch). N.B. The change of order in the function is to ensure the same result code is returned. e.g. A developer who knows that he wants to allow some things can filter the result (e.g. it's a plugin that scans third-party zips, or that (cough!) unzips existing site backups).

[DavidAnderson]

This version additionally (i.e. as well as adding the filter) corrects the dots check. I will say more in the ticket comments.

[DavidAnderson]

I've just added a patch which changes the dots check as follows:

• Removes the check for ./ . This is completely harmless (means "current directory"). You cannot perform directory traversal with it.

• Replaces the check for .. with a more sophisticated check for a ../ which occurs anywhere other than the end of the line.

This patch should fix both @Ipstenu 's reported issue (a single ../ at the end of the path is not harmful; at the worst (if it is the only thing present in the path) it would indicate the unzip folder's parent directory, which necessarily already exists) and mine (the unnecessary forbidding of any .. sequence anywhere), whilst still maintaining the intended protection of prevent directory traversal via ../.

[DavidAnderson]

Finally... this patch contains only the correction to the validation, without adding the filter, in case that is preferred.

[DavidAnderson]

v2 of the correct-dots-check.diff patch, correcting an error in the previous version

[DavidAnderson]

@johnbillion Over to you! (Possible patches attached).

[philipjohn]

sample zip for testing for this issue

[philipjohn]

Using the attached 42016-sample-zip.zip I was able to reproduce the issue, and testing with the patch correct-dots-check.2.diff fixes it - all the files are uploaded as expected. Code looks good too!

[birgire]

The validate_file() does currently not have a test, so I think we need one here.

42016.tests.diff​ contains tests for:

status file-path allowed-files
0 null
0 ''
0 ' '
0 '.'
0 '..'
0 './'
0 'foo.ext' ( 'foo.ext' )
0 'foo..ext'
1 '../'
2 'c:'
3 'foo.ext' ( 'bar.ext' )

It would be helpful to expand this list for e.g.

'../../'
'../../.'
'../foo.ext'
'../../foo.ext'
'..\'

etc so we better understand the modified validate_file().

@DavidAnderson thanks for the patches.

I wonder if strpos() and substr_count() could be used instead of preg_match and count($matches) in

if ( preg_match( '#\.\./#', $file, $matches ) && ( count( $matches ) > 1 || '../' != substr( $file, -3, 3 ) ) ) 

I'm not sure the about the count( $matches ) > 1 check as

$file = '../../../';
preg_match( '#\.\./#', $file, $matches );
echo count( $matches );

outputs 1.

I also wonder about using mb_substr() instead of substr() and adding a true in the in_array() check? Also === instead of ==?

Ps: it might be helpful to look at

​https://github.com/fuzzdb-project/fuzzdb/blob/master/attack/path-traversal/traversals-8-deep-exotic-encoding.txt

​https://github.com/danielmiessler/SecLists/blob/master/Fuzzing/FUZZDB_WindowsAattacks.txt

[DavidAnderson]

Fix the count() check, use mb_substr, and make comparison of allowed files strict

[DavidAnderson]

@johnbillion Should this be marked for consideration for WP 4.9?
Is there even going to be a WP 4.8.3? (There's nothing else in https://core.trac.wordpress.org/milestone/4.8.3).

[johnbillion]

It looks like there won't be a 4.8.3. Bumping to 4.9 for visibility. Will need backporting too. I'm reviewing this now.

[johnbillion]

In 42007:

Docs: Improve the docs for validate_file() and validate_file_to_edit().

See #42016, #36170, #41017

[johnbillion]

In 42010:

Filesystem API: Don't immediately return an error for invalid file names contained within a Zip while it's being extracted.

This allows the extraction of the rest of the valid files within the archive to continue.

See #42016

[johnbillion]

In 42011:

Filesystem API: Add more specificity to the rules for valid files in validate_file().

This now treats files containing ./ as valid, and also treats files containing a trailing ../ as valid due to widespread use of this pattern in theme and plugin zip files.

Adds tests.

Props Ipstenu, borgesbruno, DavidAnderson, philipjohn, birgire
Fixes #42016, #36170

[birgire]

ps: I think we can simplify things by using

if( substr_count( $file, '../' ) > 1 ) {

instead of

if ( preg_match_all( '#\.\./#', $file, $matches, PREG_SET_ORDER ) && ( count( $matches ) > 1 ) ) {

for counting the number of ../ substrings in $file.

I just tested and this will pass the tests.

@johnbillion - Not sure if I should reopen or create a new ticket.