WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 6 days ago

#48316 reopened defect (bug)

Changeset 46482 breaks upload when using ".." in upload_path.

Reported by: xpoon Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 5.2.4
Component: Filesystem API Keywords:
Focuses: Cc:
PR Number:

Description (last modified by SergeyBiryukov)

Hi,

We just found out that changeset [46482] in the latest WordPress 5.2.4 broke a huge number of our customer's sites (tens or thousands).

We uses a separate subdomain as upload directory. This is done by changing the option "upload_path" to "../../media.example.com/www/" (and "upload_url_path" to "http://media.example.com").

This change means that new directories (for example "./2019/10/") can't be created, which breaks the entire upload functionality.

If this changeset fixed some critical vulnerability which can't be fixed another way or if we are the only ones utilizing this feature, so be it. Otherwise this change might have to be reverted and reimplemented some other way.

Attachments (2)

48316.diff (1.2 KB) - added by DreadLox 3 weeks ago.
Patch to better check that directory traversal still is allowed base directories
absolute-uploads.diff (541 bytes) - added by xpoon 13 days ago.
Enable usage of absolute paths in the UPLOADS constant.

Download all attachments as: .zip

Change History (25)

#1 @SergeyBiryukov
5 weeks ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.2.5

#2 @xpoon
5 weeks ago

It should be "tens OF thousands", not "tens OR thousands" in the description. :)

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


4 weeks ago

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


4 weeks ago

#5 @Clorith
4 weeks ago

Hi there, and welcome to WordPress trac!

You're right that the changeset mentioned made the path traversals stricter, I noticed you're referring to a relative path for your uploads folder though.

Would you be able to use an absolute path here instead, as that should avoid the stricter path rules?

#6 @xpoon
4 weeks ago

Hi Clorith,

Using absolute paths in the database is a thing we would like to avoid, as it makes it much harder to move the site internally as well as for our customers to move the site to other providers.

If we are the only one affected by this I have no problem with you just closing this ticket. I'm sure we can find some workaround. But if other is affected too (I guess you will notice in the beginning of november when new catalogues are to be created) it would be nice too see this change be implemented in a more sensible way.

I have seen no depreciation warning and no mentions in any changelog about this. Does this change patch any known vulnerability, or what was the reason this was implemented in the first place?

#7 @whyisjake
4 weeks ago

@xpoon, yeah I think we want to avoid making this change at the moment. Like @Clorith mentions, a workaround would be using the full path, or filtering the option on a platform level.

#8 @whyisjake
4 weeks ago

  • Milestone 5.2.5 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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


3 weeks ago

#10 @DreadLox
3 weeks ago

It broke dozen of websites for me. The UPLOAD constant cannot hold an absolute path as it is used to construct URL too.

I tried to filter, but it break wp-cli:

<?php
/** Sets up WordPress vars and included files. */
require_once( ABSPATH . 'wp-settings.php' );

add_filter( 'upload_dir', function ( $upload_dir ) {
    $prev_basedir          = $upload_dir['basedir'];
    $upload_dir['basedir'] = realpath( $upload_dir['basedir'] );
    $upload_dir['path']    = str_replace( $prev_basedir, $upload_dir['basedir'], $upload_dir['path'] );

    return $upload_dir;
} );

With that filter wordpress works but not wp-cli:

PHP Fatal error:  Uncaught Error: Call to undefined function add_filter() in phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php(1169) : eval()'d code:77

We need a proper secure workaround please.

#11 @DreadLox
3 weeks ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

This workaround works but I think it just make the security fix useless:

<?php
/** Absolute path to the WordPress directory. */
if ( ! defined( 'ABSPATH' ) ) {
    define( 'ABSPATH', dirname( __FILE__ ) . '/' );
}
/** WPINC is not set yet **/
/** require_once( ABSPATH . WPINC . '/plugin.php' ); **/
require_once( ABSPATH . 'wp-includes' . '/plugin.php' );
add_filter( 'upload_dir', function ( $upload_dir ) {
    $prev_basedir          = $upload_dir['basedir'];
    $upload_dir['basedir'] = realpath( $upload_dir['basedir'] );
    $upload_dir['path']    = str_replace( $prev_basedir, $upload_dir['basedir'], $upload_dir['path'] );
    return $upload_dir;
} );
/** Sets up WordPress vars and included files. */
require_once( ABSPATH . 'wp-settings.php' );

I am reopening because I think the security fix should be improved to check if the file path points to a file being in a base dir, that could be the directory of index.php by default but can be configured or filtered.

#12 @SergeyBiryukov
3 weeks ago

  • Milestone set to Awaiting Review

#13 @DreadLox
3 weeks ago

I will submit a patch here soon.

#14 @DreadLox
3 weeks ago

Also I am wondering why this has been done, why not using the WP built in function in the same file ?

<?php

if ( false !== strpos( $target, '../' ) || false !== strpos( $target, '..' . DIRECTORY_SEPARATOR ) )

Instead of:

<?php

if( 1 === validate_file( $target ) )
Last edited 3 weeks ago by DreadLox (previous) (diff)

@DreadLox
3 weeks ago

Patch to better check that directory traversal still is allowed base directories

@xpoon
13 days ago

Enable usage of absolute paths in the UPLOADS constant.

#15 @xpoon
13 days ago

The ways you currently can use a upload path outside the current document root is by:

  • defining an absolute path in "upload_path",
  • using a filter as described above.

To use absolute paths in the database is not a great solution, neither is the second one as descibed by @DreadLox.

I suggest we'll make it possible to define absolute paths in the UPLOADS constant, that way we will have a solid solution for those who want to use an upload path outside ABSPATH.

#16 @DreadLox
13 days ago

It seem good to me if it doesn't break building the attachments URL.

#17 @peterwilsoncc
11 days ago

#48506 was marked as a duplicate.

#18 @mpcube
11 days ago

IMHO the right point to address the issue is in _wp_upload_dir() where two trusted constants (ABSPATH and UPLOADS) are concatenated. At this point the suspicous '../' at the beginning of the right part could be eliminated by stripping off the last bit of the left part.

#19 @xpoon
10 days ago

I might misunderstand you, but redefining ABSPATH some level(s) up will not be a good solution because it will break a lot of other things. If you mean we should resolve paths in _wp_upload_dir() that would be counterproductive to the initial change in wp_mkdir_p. In that case that initial commit might as well be reverted.

I still think that allowing defining absolute paths in UPLOADS would be the best solution if we don't want to remove the change [46482].

#20 @mpcube
10 days ago

What? No. Redefining ABSPATH? Would that even work? It's a constant!

What i meant:

Upload directory is constructed in function _wp_upload_dir(), where ABSPATH is concatenated with UPLOADS. For easier explanation let's look at an example:

My Website is in

/var/www/customername/www.example.com

but as i have WordPress installed in a Subdirectory, ABSPATH will be

/var/www/customername/www.example.com/wp/

I'd like to have uploads in

/var/www/customername/www.example.com/uploads/

so I set UPLOADS to

../uploads/

When _wp_upload_dir() is asked for the directory to put the uploads in, it simply concatenates ABSPATH with UPLOADS:

$dir = ABSPATH . UPLOADS;

This results is the somehow not-so-nice upload_dir()

/var/www/customername/www.example.com/wp/../uploads/

This worked unil now, but now no directorys can be created any more beacuse the path has a '../' in it. Obviously at the point, where directories are made, the $path is not trusted for some reason, we don't want to go one level up on '../'.

So we need to get rid of the '../' at a point where we know, that there's no harm in interpreting the string '../' as 'go up one level' - which is in _wp_upload_dir() where we know that this comes from constants in our code and not from some user input. At this point we can trust the values (both are constants) and strip of 'wp/../' (in my example).

I think it would be better to modify _wp_upload_dir() in a way that upload_dir() becomes

/var/www/customername/www.example.com/uploads/

just by instead of

$dir = ABSPATH . UPLOADS;


writing

        $uploads = UPLOADS;
        $base_parts = explode ( '/', trim(ABSPATH, '/' ) );
        while ( substr ( $uploads , 0, 3 ) == '../' ) {
                $uploads = substr($uploads, 3);
                array_pop($base_parts);
        }
        $dir = '/' . implode( '/', $base_parts );
        $dir .= '/' . ltrim($uploads, '/');

All that would be easier if i knew the reasons for the initial change 46482 - but I can't find any information about that.

#21 @xpoon
10 days ago

That sounds reasonable. If we would resolve just "trusted" paths, the question is where to put the code. This might apply to more places than just the one you mention, for example when the upload path is defined as a database option.

One way might be to create a wp_realpath() that you can use to resolve paths that you know are not containing user input.

Another way might be to have a parameter (allow_path_traversals) in wp_mkdir_p.

I guess the first alternative would be the best as you get more control of what path is being resolved.

I also agree that this would be easier to discuss if the reasons for [46482] where known.

#22 @mpcube
10 days ago

The reasons for [46482] would be also intresting because of the fact, that it just prevents the creation of directories. I'm still allowed to place files in these paths that can't be created any more. I can not think of a situation where creating a direcory is a security problem but uploading a file to the same location is not.

#23 @SergeyBiryukov
6 days ago

  • Version changed from trunk to 5.2.4
Note: See TracTickets for help on using tickets.