Symlinked directories should not be deleted recursively

[andy]

When deleting a symlinked plugin, the current behavior is to recursively delete everything in the plugin's real directory and then fail to unlink the symlink because rmdir won't work on a symlink. This is probably not what the site admin intended when they installed the plugin via a symlink. The desired behavior is to unlink only the symlink, leaving the external directory intact so that other symlinks remain intact.

My patch fixes this in WP_Filesystem rather than in the plugin deletion logic because it seems generally applicable to the use cases for symlinks.

What makes this hard is that trailing slashes are significant when dealing with symlinked directories. The trailing slash causes the link to be followed:

is_link('/link/') => false
is_link('/link') => true

The patch fixes deletion of symlinked plugins: it unlinks the symlink and leaves the real files intact. It should be carefully checked against other uses of delete because they might not include the trailing slash. In such cases, adding a trailing slash to the new is_dir() check might help. Could be a minefield, could be fine.

Related to #29408 but not a duplicate.

[ocean90]

I suppose that we need this for the other implementation too?

cc: @dd32

[ocean90]

#34067 was marked as a duplicate.

[Dreamsorcerer]

This is actually a fairly serious security flaw as well, e.g. if a plugin author puts a symlink in their plugin, and get it uploaded to the plugin repository.

If the plugin includes a symlink pointing to '../../..', then WP will recursively delete itself. I've tested this with a symlink to '../../themes' and WP successfully deleted all the themes while trying to upgrade the plugin. If the server is really poorly configured, then a symlink to '/' or similar might even be able to wipe out the whole server.

What's worse, is even if the plugin author managed to do something like this accidentally, and later realised their mistake, there would be no way for them to fix it. Providing any update at all to the plugin repository would trigger the deletion.

Attached is a patch which fixes the bug while also closing this security hole.

[Dreamsorcerer]

Fix symlink security flaw

[Otto42]

Replying to Dreamsorcerer:

This is actually a fairly serious security flaw as well, e.g. if a plugin author puts a symlink in their plugin, and get it uploaded to the plugin repository.

The plugin/theme repository generates its own ZIP files, it doesn't take them as given from the original uploader. Even if they were able to get a symlink into the SVNs, the symlink would not be put in the resulting ZIP file that is generated by WordPress.org to be sent to end-users, because we do not include the necessary parameters to the "zip" program to allow it to include such symlinks in the resulting ZIP files.

[pbiron]

I just bumped into this problem.

What is needed to move this forward?

[pbiron]

refreshes fix-symlinks.diff

[pbiron]

36710.2.diff refreshes fix-symlinks.diff.

Note, however, that it does not include the changes to /wp-admin/includes/class-wp-upgrader.php because I'm not seeing the described behavior of is_writable() returning false for symlinks (PHP 7.1.24 on Windows 10).

[Otto42]

Replying to pbiron:

I'm not seeing the described behavior of is_writable() returning false for symlinks (PHP 7.1.24 on Windows 10).

I know Windows has a symlink like mechanism, but I'm unfamiliar with it, so maybe this needs to be tested on a linux box to be sure. PHP probably doesn't have the exact same behaviors here.

[pbiron]

adds Windows-specific behavior for deleting a symlink that points to a directory.

[pbiron]

On Windows, trying to unlink() a symlink that points to a directory fails with "Permission denied"...you have to use rmdir().

36710.3.diff adds that Windows-specific conditional behavior.

See ​https://bugs.php.net/bug.php?id=52176.

[pbiron]

Replying to Otto42:

Replying to pbiron:

I'm not seeing the described behavior of is_writable() returning false for symlinks (PHP 7.1.24 on Windows 10).

I know Windows has a symlink like mechanism, but I'm unfamiliar with it, so maybe this needs to be tested on a linux box to be sure. PHP probably doesn't have the exact same behaviors here.

Yeah. If someone has access to a linux box they can test the refreshed patch on and it turns out it is needed in that case, I'll refresh again and put that change back in.

@Dreamsorcerer A couple of other questions about the is_writable() change: in trunk right now there are 2 calls to $wp_filesystem->is_writable() and your patch only changed the 2nd one. I'm not sure if the 1st one wasn't present when you created your patch or not. Can you verify? [this is another reason I left that change out my refresh...wasn't sure if both places needed the change or not].

If the change is really needed, wouldn't it be better to encapsulate it in WP_Filesystem_Direct::is_writable() rather than in WP_Upgrader::clear_destination()?

[Dreamsorcerer]

I don't have everything setup right now, in order to test this properly.

However, when I wrote this patch, I simply followed the traceback and fixed the errors where needed, so if there was a 2nd is_writable() call, it didn't cause any errors (for me). If you can check if the call existed on whichever WP version was around 21 months ago (according to the diff upload date), then that would answer your question.

I did a quick test on my server, to see how is_writable() is working, and it looks like it now returns true on a symlink (maybe it was an issue with PHP 5...):

# touch a
# ln -s a b
# ls -l
total 65340
-rw-r--r-- 1 root root        0 Jul  7 16:12 a
lrwxrwxrwx 1 root root        1 Jul  7 16:12 b -> a
# php --version
PHP 7.3.4-2 (cli) (built: Apr 13 2019 19:05:48) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.4, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.3.4-2, Copyright (c) 1999-2018, by Zend Technologies
# php -a
php > print(is_writable('a'));
1
php > print(is_writable('b'));
1

[pbiron]

patch refreshed. Any chance can move this forward for 5.5?

[pbiron]

Related: #29408

Also, the ​WordPress Auto-updates feature, which will hopefully be merged into core in 5.5.

[bookdude13]

Fix strtoupper typo

[bookdude13]

Patch 36710.4 had a typo, strotupper() instead of strtoupper(). Fixed in 36710.5.

Testing locally on Windows this works. When a symlinked plugin is updated the symlink is removed and the updated package is stored in plugins/. Note that this will break the symlink for the site maintainer. If they don't want the link broken they can update their own location, or copy the files over afterwards.

[pbiron]

Replying to bookdude13:

Patch 36710.4 had a typo, strotupper() instead of strtoupper(). Fixed in 36710.5.

Thanx for catching the typo in my update...I'm a dyslexic typer :-)

Testing locally on Windows this works. When a symlinked plugin is updated the symlink is removed and the updated package is stored in plugins/. Note that this will break the symlink for the site maintainer. If they don't want the link broken they can update their own location, or copy the files over afterwards.

Correct. I'll be proposing another fix for the auto-update part (on a separate ticket).

[bookdude13]

This still needs someone to test on Linux. Still works fine on Windows using admin powershell and New-Item -ItemType SymbolicLink

[pbiron]

refreshing patch

[pbiron]

36710.6.diff​ refreshes the patch so that it cleanly applies.

As with the previous patch, this still needs testing on linux!

[SergeyBiryukov]

Related: #15134

[Boniu91]

@pbiron Could you add exact testing steps, so we'll have clear instructions how to test the patch properly?

[hellofromTonya]

5.9 Beta 1 is happening in less than 4 hours. As this ticket needs testing instructions and testing and is not a bug introduced in the 5.9 cycle, moving it to 6.0.

[hellofromTonya]

Given how long ago active contributions happened in this ticket and that it needs testing instructions for testers to properly test it to reproduce the issue and validate if the patch resolves it, moving this ticket back to Future Release.

Once step-by-step testing instructions are provided, this one is ready to move back into a major release cycle.

[Dreamsorcerer]

It's been several years since I looked at this, but test instructions should be fairly easy, something along the lines of:

Download a plugin outside of the WP directory.
Symlink that plugin into the plugins directory (ln -s /path/to/plugin /path/to/wp-content/plugins/).
Observe that the plugin is installed and working correctly on the website.
Delete (and probably try upgrading on a second test) the plugin through the website admin.
Observe that no errors occurred and the plugin is no longer on the website.
Observe that the original plugin (outside WP directory) is still present with no files deleted (symlinking again should result in the plugin being installed and working again).

[etisse]

Hi. Successfully tested on Linux (debian 11+php8.0 in VM).

[etisse]

Edit. I didn't want to close this ticket (sorry I don't know how it works yet).
But the patch work for me.