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

[xpoon]

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.

[xpoon]

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

[Clorith]

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?

[xpoon]

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?

[whyisjake]

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

[DreadLox]

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.

[DreadLox]

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.

[DreadLox]

I will submit a patch here soon.

[DreadLox]

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

[DreadLox]

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

[xpoon]

Enable usage of absolute paths in the UPLOADS constant.

[xpoon]

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.

[DreadLox]

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

[peterwilsoncc]

#48506 was marked as a duplicate.

[mpcube]

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.

[xpoon]

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].

[mpcube]

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.

[xpoon]

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.

[mpcube]

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.

[mpcube]

_wp_merge_trusted_paths()

[DreadLox]

@mpcube why not using ​realpath to resolve relative path ?

[peterwilsoncc]

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.

[DreadLox]

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

[mpcube]

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

[DreadLox]

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

[peterwilsoncc]

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

[mpcube]

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]

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

[xpoon]

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.

[DreadLox]

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

[costdev]

@peterwilsoncc just following up on this. Did you speak with other members of the Security Team about this?

[peterwilsoncc]

I'm going to close this off as I don't think there is anything that WordPress can safely do to account for advanced configurations in which UPLOADS intentionally includes path traversal.

For sites were the uploads folder's real path is outside the content directory, using a symlink remains an effective method for handling the situation. I know the method is quite ​effective from personal experience.

From a security perspective, choosing to allow path traversal is very risky. To do so for a set up that can be solved with a symlink isn't worth the risk.

[gerardreches]

How all of this started and why is it so difficult to fix may be far from my understanding, but I'm in the following situation where I want the following structure:

public_html/
--> content/
|----> plugins/
|----> themes/
--> core/
|----> wp-includes/
|----> wp-admin/
|----> etc
--> uploads/
--> wp-config.php 
--> index.php

This is based in ​WordPress-Skeleton and ​this guide from deliciousbrains. The former repository has over 1800 stars and 600 forks, quite a big user base. My only change here is that I don't want the uploads directory inside the wp-content. It makes more sense for me to have it separate, as one is meant to be for the public while the other one isn't.

I am not using Git and I am not using symlinks, nor intend to. Not all hosting providers allow you to make use of those.

While moving the core files and the wp-content directory is easy, it was impossible to achieve this structure by using the UPLOADS constant in the wp-config.php file. The only way to do it was by using the upload_path option with either the absolute path or the relative path ../uploads.

Using the database option for this matter isn't a good approach, because it doesn't allow for easy migrations (after migrating the database from another site, the option will be replaced and the uploads path will be broken). Swapping the database could also break it. It's simply not obvious as a constant in the wp-config.php file is.

Now, I do not understand what is going on with the UPLOADS constant and why it cannot be an absolute path nor why it doesn't behave in the same way as upload_path. Maybe it is because the constant is taking care of both the path and the URL while the option only takes care of the path while there is another option taking care of the URL.

My point is, why don't we have a new pair of constants for the upload path just like we have WP_CONTENT_DIR and WP_CONTENT_URL for the wp-content directory that mimic what the database options do?

Constants could be named WP_UPLOAD_DIR and WP_UPLOAD_URL and they should not do other thing than exactly what the upload_path and upload_url_path options do. Checking if the constant is defined, and in that case, using it wherever get_option( 'upload_path' ) and get_option( 'upload_url_path' ) are used. Their goal would be no more and no less than overriding the database option when defined.

It makes so much sense in my head that I don't understand why this haven't been done yet, there has to be a catch. I'm no security expert, but if we can use an absolute path in wp-config.php for WP_CONTENT_DIR why couldn't we have it for a new WP_UPLOAD_DIR constant? It would just take precedence over the database value, and less than UPLOADS. I can't imagine any backward compatibility issues either.

PS: There are barely 9 usages of get_option( 'upload_path' ) across 4 files and 5 usages of get_option( 'upload_url_path' ) across 3 files. If you all agree with adding those new constants I can make the necessary changes myself.