WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 4 weeks ago

#26626 new defect (bug)

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

Reported by: DavidAnderson Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.7
Component: Upgrade/Install Keywords: has-patch dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (2)

upgrader.patch (492 bytes) - added by DavidAnderson 4 months ago.
1-line patch
upgrader.2.patch (506 bytes) - added by DavidAnderson 7 weeks ago.
Updated patch

Download all attachments as: .zip

Change History (13)

DavidAnderson4 months ago

1-line patch

comment:1 SergeyBiryukov4 months ago

  • Description modified (diff)

comment:2 follow-up: DavidAnderson2 months ago

  • Keywords has-patch added

comment:3 in reply to: ↑ 2 adamsilverstein2 months ago

  • Keywords dev-feedback added

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?

comment:4 DavidAnderson2 months ago

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

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

comment:5 follow-up: dd328 weeks ago

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.

comment:6 DavidAnderson7 weeks ago

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

DavidAnderson7 weeks ago

Updated patch

comment:7 in reply to: ↑ 5 ; follow-up: adamsilverstein7 weeks ago

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?

comment:8 in reply to: ↑ 7 ; follow-up: DavidAnderson7 weeks ago

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.

comment:9 ocean907 weeks ago

  • Summary changed from WP_Upgrader::unpack_package() can overflow path name length limits (patch attached) to WP_Upgrader::unpack_package() can overflow path name length limits

comment:10 in reply to: ↑ 8 adamsilverstein7 weeks ago

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.

comment:11 jeremyfelt4 weeks ago

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