WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 months 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: normal Version: 5.2.4
Component: Filesystem API Keywords:
Focuses: Cc:

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 (5)

48316.diff (1.2 KB) - added by DreadLox 7 months ago.
Patch to better check that directory traversal still is allowed base directories
absolute-uploads.diff (541 bytes) - added by xpoon 7 months ago.
Enable usage of absolute paths in the UPLOADS constant.
_wp_merge_trusted_paths.diff (2.7 KB) - added by mpcube 6 months ago.
_wp_merge_trusted_paths()
48316-minimal-realpath-fix.diff (788 bytes) - added by mpcube 6 months ago.
Minimal fix using realpath(): Just for UPLOADS constant, not for option 'siteurl', as siteurl can be absolute. Just for directory, nor for URL, as '../' in URL ist ugly, but nor a showstopper
48316-fix.diff (528 bytes) - added by mpcube 6 months ago.
Fixing Just UPLOADS (not upload_path), just normal installs (not MU), just directories (not URLs), just if upload-path already exists.

Download all attachments as: .zip

Change History (36)

#1 @SergeyBiryukov
7 months ago

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

#2 @xpoon
7 months 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.


7 months ago

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


7 months ago

#5 @Clorith
7 months 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
7 months 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
7 months 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
7 months 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.


7 months ago

#10 @DreadLox
7 months 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
7 months 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
7 months ago

  • Milestone set to Awaiting Review

#13 @DreadLox
7 months ago

I will submit a patch here soon.

#14 @DreadLox
7 months 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 7 months ago by DreadLox (previous) (diff)

@DreadLox
7 months ago

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

@xpoon
7 months ago

Enable usage of absolute paths in the UPLOADS constant.

#15 @xpoon
7 months 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
7 months ago

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

#17 @peterwilsoncc
7 months ago

#48506 was marked as a duplicate.

#18 @mpcube
7 months 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
7 months 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
7 months 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
7 months 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
7 months 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
7 months ago

  • Version changed from trunk to 5.2.4

@mpcube
6 months ago

_wp_merge_trusted_paths()

#24 @DreadLox
6 months ago

@mpcube why not using realpath to resolve relative path ?

#25 @peterwilsoncc
6 months ago

realpath() doesn’t work for cases in which WordPress needs to create the directory before using it. The function only works for existing directories.

I like the idea of trusted and untrusted paths if trusted paths are limited to paths defined in wp-config.php constants. Determining if the value is trustworthy would need to happen during bootstrapping to allow for constants defined via options if they’re not set.

Before going down this path, I’d like to check in for some historical context with members of the WordPress security team.

#26 @DreadLox
6 months ago

I think we should use realpath() to build the upload dir base path and then url which have to exists. It would be simplier and faster and would resolve any back ref (../). Then we forbid any ../ in final paths (to directories or files)

Version 0, edited 6 months ago by DreadLox (next)

@mpcube
6 months ago

Minimal fix using realpath(): Just for UPLOADS constant, not for option 'siteurl', as siteurl can be absolute. Just for directory, nor for URL, as '../' in URL ist ugly, but nor a showstopper

#27 @DreadLox
6 months ago

@mpcube good one. I think this minimal fix can be merged and backported ASAP:

  • it immediately fixes this issue
  • it relies entirely on GLOBALS
  • it doesn't hurt the "security" hot fix that caused this issue
  • it doesn't modify URIs

Then we can take time to discuss the trusted paths approach.

#28 @peterwilsoncc
6 months ago

  • Severity changed from major to normal

It's worth restating, if uploads directory is a sub-directory of WP_CONTENT_DIR or ABSPATH the ideal solution is to remove any path traversal from constants.

If the UPLOADS directory truly needs to be outside of ABSPATH, then symlinks and redirects will be helpful too. It's an approach I've used in the path successfully.

While valid, defining the uploads path in this manner is not a common configuration of WordPress, it's for advanced users. I've reduced the severity of the bug due to the advanced usage and availability of multiple work-arounds.

@mpcube all this said, thank you very much for your efforts and helping a middle-ground be found to allow for advanced usage while considering path traversal should generally be avoided. I very much appreciate. it.

---

Before this approach can be considered, I'm still seeking further feedback on whether it is safe from members of the security team.

Presuming it's safe, the patch will need a little more work:

  • realpath( /* UPLOAD directory */ ) will return false if the uploads directory doesn't exist. This is a fairly common situation on fresh installs as the call mkdir_p( 'path/to/uploads/yyyy/mm' ) will create the directory.
  • The patch would need to check for false === realpath() before doing anything with the return value, otherwise WP could end up modifying/adding directories to the file systems root directory. This would be a downer ;)
  • Multisite defines UPLOADS in the function ms_upload_constants() so any code needs to ensure that value is trusted too
  • Unit tests would be very helpful too, need to figure out what they look like

#29 @mpcube
6 months ago

OK, is there a chance we can have a small bugfix for:

  • just UPLOADS-Constant
  • just on non-MU-installs
  • for just the directory (not modifing the URL)
  • just if UPLOADS already exists (means: just for the monthy folders)

really soon? (Otherwise I need to figure out a way to create all those 2019/12-direcories, because that's the only workaround I have right now. For november I made a script that did that on all the installs on one server, but I needed to log in to every server via ssh.)

Other topics, like a solution for MU, a solution for upload_path-setting, getting rid of "../" in the URLs and unifying usage of UPLOADS and upload_path could be addressed in a later patch.

I didn't expect having...

  1. WordPress (ABSPATH)
  2. customer's wp-content directory (WP_CONTENT_DIR) and
  3. the files in uploads (UPLOADS)

...in three sepreate direcories next to eachother to be so advanced, as having wordpress not writeable for my users, wp-content not writeable for the webserver and uplods not executableis a second line of defense i dont't want to miss - and managing them seperatly is convenient.

@mpcube
6 months ago

Fixing Just UPLOADS (not upload_path), just normal installs (not MU), just directories (not URLs), just if upload-path already exists.

#30 @xpoon
6 months ago

To be honest, I don't think we should commit a rushed solution just so me and you, @mpcube, can get our sites working. I have deployed temporary workarounds for about 100k sites, so I'm fine for a while.

If more people are affected by this though, I think we should consider rolling back the inital change and make a better implementation with some kind of "trusted paths" in where path traversals are allowed, like suggested before. But as I don't know whether this change was made to fix some critical vulnerability or just because it's best practice to do it this way, I can't know if rolling back is an actual option or not.

#31 @DreadLox
6 months ago

@mpcube the workaround I posted here works by editing wp-config.php

Note: See TracTickets for help on using tickets.