WP_Upgrader::unpack_package() can overflow path name length limits

[DavidAnderson]

I had no idea that in 2013, there were still operating systems with pathname length limits of only 256 characters. But, apparently there are - I've had 3 reports of it in the last week (which came after I included an SDK in one of my plugins that had a lot of sub-directories).

Two of the victims got hit when unpacking the plugin through a normal update. To prevent this in future, I restructured my plugin.

The other victim was running Bitnami WAMP, with PHP 5.4.22 (Windows NT AFLAPTOP 6.1 build 7601 (Windows 7 Business Edition Service Pack 1) i586), and he was accessing WP_Upgrader::unpack_package() via a restore operation in UpdraftPlus Backup/Restore (​http://wordpress.org/plugins/updraftplus), which uses this method to unpack zip files.

See ​http://wordpress.org/support/topic/restore-fails-could-not-create-directory for more information on that. Basically, it boiled down to "WP_Upgrader::unpack_package() uses the basename of the zip file to create a directory in WP_CONTENT_DIR/upgrade/". So, if the zip file name is very long, then a meaningful amount of the potential 256-character limit can be lost.

The guy running Bitnami lost 80 characters to reach his upgrade folder: C:\Program Files\BitNami WAMP Stack\apps\wordpresslucid\htdocs\wp-content/upgrade. That leaves 176 characters.

The plugin he was unpacking needed 98 characters for its longest pathname: /plugins/<slug>/opencloud/symed/Symfony/Component/EventDispatcher/EventDispatcherInterface.php. That leaves 78 characters to spare. Should be plenty... except for WP_Upgrader::unpack_package() wanting to use basename($zipfile) to create a directory for unpacking into. The zipfile did indeed have a filename of over 78 characters long. Boom!

To prevent other scenarios in which someone might get hit by this, we can just change this line:

$working_dir = $upgrade_folder . basename($package, '.zip');

to:

$working_dir = $upgrade_folder . substr(md5(basename($package, '.zip')), 0, 8);

The use of md5() to prevent collisions might be over-cautious (especially given that WP_Upgrader::unpack_package() already tries to empty out the upgrades directory), but it's also harmless.

[DavidAnderson]

1-line patch

[adamsilverstein]

David, thanks for identifying the issue and your clear bug description and your sensible patch. A unit test would be nice here, demonstrating the issue and validating the fix.

I'm curious why you chose to use a substring of the md5 instead of the full (32 character) string?

[DavidAnderson]

Hi Adam,

I chose a substring of md5() out of an abundance of caution - it has only alphanumeric output. I assume it's rather unlikely that $package could contain weird characters that are going to cause problems on primitive filesystems, but, rather than research the possibilities of that, I thought it more efficient to just use md5() to side-step them. (The reason I posted the original bug was because of the unanticipated surprise that 256-character pathname limits still exist in the wild - so, it seemed most efficient to prevent any future surprises in this area).

Here's how to see the original issue in action. Run this from within a default WP install (or similar):

$ echo wp-includes/version.php | zip -@ /tmp/testing1234567890abcdefg.zip
  adding: wp-includes/version.php (deflated 59%)
$ php -r 'require("wp-load.php"); require("wp-admin/includes/file.php"); require("wp-admin/includes/class-wp-filesystem-base.php"); require("wp-admin/includes/class-wp-filesystem-direct.php"); $wp_filesystem = new WP_Filesystem_Direct(true); require("wp-admin/includes/misc.php"); require("wp-admin/includes/class-wp-upgrader.php"); $u = new WP_Upgrader(new WP_Upgrader_Skin); $u->unpack_package("/tmp/testing1234567890abcdefg.zip");'
$ ls -l wp-content/upgrade/

Results:

total 4
drwxrwxr-x. 3 david david 4096 Feb 25 00:22 testing1234567890abcdefg

i.e. as expected, the directory created is based on basename($zipfile), and thus uses as many characters up as the zipfile name has.

David

[dd32]

One case that this change would break: Some legacy/older plugins have their zip structure without a directory inside (so the files are just /plugin.php, instead of /plugin/plugin.php), making this change may break plugins in that case which expect their plugin folder to be named the same as the plugin itself.

The above applies to themes as well as plugin.

In general, my feeling is that this isn't something we should do... WordPress being installed in a deep path, and a plugin having a really deep path, and the zip having a super-long name seems like an extreme edge case..

Perhaps we can simply truncate the working directory name to something sensible - 32 characters for example, something that would prevent issues with a zip file with an insanely long name (which shouldn't be one of the before-mentioned legacy plugins) and at the same time deal with this specific case.

Also worth noting, that by using basename() of the zip archive we've avoided clashes with other instances of WordPress in general (not by design), as the filename is unique in the temporary files directory (which might be /tmp/ or wp-content/) so upgrade directories ended up with wordpress1.tmp/ wordpress2.tmp/ etc.

Instead of using random data and hoping it'll be random, the case where we clean up existing directories (by deleting the new $working_dir) should perhaps simply append a number to the directory name.
This causes problems for the before-mentioned legacy plugins though. It would also mean that failed updates would fail to clean up the upgrade folder the next time an update is run, potentially ending up with a upgrade folder full of useless copies of WordPress and/or plugin/themes.

[DavidAnderson]

Thanks for the feedback. I'm attaching a new patch that cuts out md5() and just uses substr( ..., 0, 32).

This isn't an extreme edge case - I've seen pathname overflows happen in 3 different ways (the most recently was a few days ago):

1) For packages downloaded over HTTP, basename($package), $package ends up as being a temporary folder followed by basename() on the URL that the package was fetched from. I had a plugin downloading from a URL like this:

​http://example.com/?udm_action=download&slug=whatever&muid=1&token=2ffd3c5308ca075df83f8b53717efbac

basename() on that is ?udm_action=download&slug=whatever&muid=1&token=2ffd3c5308ca075df83f8b53717efbac - 80 characters long. I got a number of support requests on that... I then fixed it by the kludge of adding an extra URL parameter on the end of the URL the updates server was offering up that contained a / so that basename() cut from there.

2) The case given in the bug report, multiple times. An 80-character zip filename was enough to cause it. But, it'd be easy to get cases with a shorter one. Some Windows users love long directory names - I've seen stuff like "Copy of the website for the meeting next Friday". Add on C:\Users\Smith Family\Our Company\Website Development\Clients\AcmeCo - and 256 is getting pretty easy to hit. That's not theory - I've had plenty of support requests from ones like that.

3) I added a copy of the Rackspace SDK to a plugin (a plugin which needed to access Rackspace Cloud Files) - ​https://github.com/rackspace/php-opencloud. I used composer (as the README recommends), and the maximum element in there came to something like 85 characters. This is without wp-content/plugins/myslug... with the plugin in question, that came to 115 characters. I got 3 support requests from users who'd been hit by a 256-character limit within the next 3 days. This case won't be fixed by this ticket - for that one, I had to move lots of the SDK around - I'm just using this as an example of how the limits get hit without doing anything extreme.

I reckon if I've managed to get hit by 256-character pathname limits in 3 independent ways, then it must be happening to others. I've lost plenty of man-hours on it (most recently earlier this week).

[DavidAnderson]

Updated patch

[adamsilverstein]

Replying to dd32:

One case that this change would break: Some legacy/older plugins have their zip structure without a directory inside (so the files are just /plugin.php, instead of /plugin/plugin.php), making this change may break plugins in that case which expect their plugin folder to be named the same as the plugin itself.

The above applies to themes as well as plugin.

I don't understand how this breaks these legacy plugins: doesn't the patch only change the folder used during upgrades?

[DavidAnderson]

Replying to adamsilverstein:

The above applies to themes as well as plugin.

I don't understand how this breaks these legacy plugins: doesn't the patch only change the folder used during upgrades?

In the legacy case, the plugin installer takes the folder that it used for the unpacking and puts that inside the plugins folder.

i.e. If your zip is myplugin.zip, and it contains a single file (with no subdirectory) called thisismyplugin.php, then (assuming for simplicity that your content+plugins folder structure is default) you end up with wp-content/plugins/myplugin/thisismyplugin.php

Whereas, my use of md5() was causing the result to be wp-content/plugins/md5(myplugin)/thisismyplugin.php = wp-content/plugins/c6f30677/thisismyplugin.php

The new version of the patch restores the previous behaviour, in case the plugin was relying on its installation path.

[adamsilverstein]

Replying to DavidAnderson:

Replying to adamsilverstein:

The above applies to themes as well as plugin.

I don't understand how this breaks these legacy plugins: doesn't the patch only change the folder used during upgrades?

In the legacy case, the plugin installer takes the folder that it used for the unpacking and puts that inside the plugins folder.

i.e. If your zip is myplugin.zip, and it contains a single file (with no subdirectory) called thisismyplugin.php, then (assuming for simplicity that your content+plugins folder structure is default) you end up with wp-content/plugins/myplugin/thisismyplugin.php

Whereas, my use of md5() was causing the result to be wp-content/plugins/md5(myplugin)/thisismyplugin.php = wp-content/plugins/c6f30677/thisismyplugin.php

The new version of the patch restores the previous behaviour, in case the plugin was relying on its installation path.

ok, got it - thanks for explaining that bit.

[dd32]

#29672 was marked as a duplicate.

[cklosows]

Maybe somewhat related? #38231

[pbiron]

refreshing patch

[pbiron]

26626.diff​ refreshes the patch.

No comment (at this time) on whether the patch is sufficient to solve the problem :-)