WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#36710 new defect (bug)

Symlinked directories should not be deleted recursively

Reported by: andy Owned by:
Milestone: Future Release Priority: normal
Severity: major Version:
Component: Filesystem API Keywords: has-patch dev-feedback
Focuses: administration Cc:
PR Number:

Description

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.

Attachments (4)

36710.diff (818 bytes) - added by andy 3 years ago.
fix-symlinks.diff (2.6 KB) - added by Dreamsorcerer 2 years ago.
Fix symlink security flaw
36710.2.diff (2.2 KB) - added by pbiron 4 months ago.
refreshes fix-symlinks.diff
36710.3.diff (2.5 KB) - added by pbiron 4 months ago.
adds Windows-specific behavior for deleting a symlink that points to a directory.

Download all attachments as: .zip

Change History (15)

@andy
3 years ago

#1 @ocean90
3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.6
  • Version trunk deleted

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

cc: @dd32

#2 @ocean90
3 years ago

#34067 was marked as a duplicate.

#3 @ocean90
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from 4.6 to Future Release

#4 follow-up: @Dreamsorcerer
2 years ago

  • Severity changed from normal to major

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
2 years ago

Fix symlink security flaw

#5 in reply to: ↑ 4 @Otto42
2 years ago

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.

#6 @pbiron
4 months ago

I just bumped into this problem.

What is needed to move this forward?

@pbiron
4 months ago

refreshes fix-symlinks.diff

#7 follow-up: @pbiron
4 months ago

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

#8 in reply to: ↑ 7 ; follow-up: @Otto42
4 months ago

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
4 months ago

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

#9 @pbiron
4 months ago

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.

#10 in reply to: ↑ 8 @pbiron
4 months ago

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

#11 @Dreamsorcerer
3 months ago

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
Note: See TracTickets for help on using tickets.