WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#42016 closed defect (bug) (fixed)

Validation of filenames (while unzipping) causes unexpected failures

Reported by: Ipstenu Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8.2
Component: Filesystem API Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (8)

filter-validate-file-results.diff (1.2 KB) - added by DavidAnderson 2 months ago.
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).
correct-dots-check.diff (1.2 KB) - added by DavidAnderson 2 months ago.
This version additionally (i.e. as well as adding the filter) corrects the dots check. I will say more in the ticket comments.
correct-dots-check-only.diff (681 bytes) - added by DavidAnderson 2 months ago.
Finally... this patch contains only the correction to the validation, without adding the filter, in case that is preferred.
correct-dots-check.2.diff (1.2 KB) - added by DavidAnderson 2 months ago.
v2 of the correct-dots-check.diff patch, correcting an error in the previous version
42016-sample-zip.zip (2.0 KB) - added by philipjohn 2 months ago.
sample zip for testing for this issue
42016.tests.diff (1.6 KB) - added by birgire 2 months ago.
correct-dots-check-v3.diff (1.2 KB) - added by DavidAnderson 2 months ago.
Fix the count() check, use mb_substr, and make comparison of allowed files strict
42016.diff (5.7 KB) - added by johnbillion 8 weeks ago.

Download all attachments as: .zip

Change History (27)

#1 @Ipstenu
3 months ago

  • Summary changed from Validation of filenames (pre unzipping) causes unexpected failures to Validation of filenames (while unzipping) causes unexpected failures

#2 @johnbillion
3 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.8.3

#3 @voldemortensen
3 months ago

#42026 was marked as a duplicate.

#4 @DavidAnderson
3 months ago

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.

#5 @Ipstenu
2 months ago

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.

#6 @DavidAnderson
2 months ago

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.

#7 @johnbillion
2 months ago

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

@DavidAnderson
2 months ago

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
2 months ago

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

#8 @DavidAnderson
2 months ago

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
2 months ago

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

@DavidAnderson
2 months ago

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

#9 @DavidAnderson
2 months ago

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

@philipjohn
2 months ago

sample zip for testing for this issue

#10 @philipjohn
2 months ago

  • Keywords has-patch added; needs-patch removed
  • Resolution set to invalid
  • Status changed from new to closed

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!

#11 @philipjohn
2 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

@birgire
2 months ago

#12 @birgire
2 months ago

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

Last edited 2 months ago by birgire (previous) (diff)

@DavidAnderson
2 months ago

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

#13 @DavidAnderson
8 weeks ago

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

#14 @johnbillion
8 weeks ago

  • Milestone changed from 4.8.3 to 4.9
  • Owner set to johnbillion
  • Status changed from reopened to reviewing

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.

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


8 weeks ago

#16 @johnbillion
8 weeks ago

In 42007:

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

See #42016, #36170, #41017

@johnbillion
8 weeks ago

#17 @johnbillion
8 weeks ago

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

#18 @johnbillion
8 weeks ago

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

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

#19 @birgire
8 weeks ago

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.

Note: See TracTickets for help on using tickets.