Make WordPress Core

Opened 13 years ago

Last modified 9 months ago

#15134 accepted defect (bug)

WordPress should not try to remove themes or plugins recursively if the directory is a symlink

Reported by: vladimir_kolesnikov's profile vladimir_kolesnikov Owned by: pbiron's profile pbiron
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch needs-testing dev-feedback has-testing-info
Focuses: Cc:

Description

Consider the situation: there is a server with multiple WordPress blogs hosted in it. Some plugins are common for all/many blogs and to save several (hundreds in our case) megs of the disk space, shared plugins are stored somehwere else (say, /var/www/wp-plugins) and there are symbolic links to /var/www/wp-plugins/<plugins> from /home/<user>/wp-content/plugins/<plugins>.

The onwer of the blog (user1) may not know these details and wants to update one of the plugins (plugin1) using automatic update feature. WordPress will then try to remove /home/user1/wp-content/plugins/plugin1/ recursively although /home/user1/wp-content/plugins/plugin1 is a symlink to /var/www/wp-plugins/plugin1.

The obvious solution is to add a check to the filesystem classes that checks if the file is a symlink and if so, remove symlink with unlink() instead of trying to follow it and remove everything it sees.

The advantage of this approach is that if the user symlinks a plugin to other user's data, those data will not be removed by WordPress (this can be very good for those hosts where all users are served by the same Apache user etc).

Attachments (4)

filesystem.patch (1.7 KB) - added by vladimir_kolesnikov 13 years ago.
fix-symlinks.diff (2.6 KB) - added by Dreamsorcerer 6 years ago.
Fix symlink security flaw
15134.2.diff (4.0 KB) - added by pbiron 3 years ago.
refreshing patch
15134.3.diff (3.4 KB) - added by costdev 2 years ago.
Refreshes 15134.2.diff with PHPCS fixes and a slight cleanup of a multi-line condition.

Download all attachments as: .zip

Change History (69)

#1 follow-ups: @Denis-de-Bernardy
13 years ago

Seems to me that this is a use-case where site admins shouldn't be allowed to upgrade plugins, and where all installs should be maintained from the shell using svn, git, or scripts.

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

#2 follow-up: @scribu
13 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Another thing to note is that activation hooks won't work with sym-linked directories.

You can have a common directory for all plugins - this can be achieved using the WP_PLUGIN_DIR and WP_PLUGIN_URL constants.

Or, as Denis suggested, disable upgrades for common plugins.

#3 in reply to: ↑ 1 @vladimir_kolesnikov
13 years ago

Replying to Denis-de-Bernardy:

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

No. If the symlink is removed, only one site gets updated (the user wants to have a newer version of the plugin on their own risk). But if the whole tree is removed (what WordPress does now), ALL sites are affected and this is an unexpected result and can be security risk.

Using WP_PLUGIN_DIR/URL is not an option if the sites can have their own plugins (ie, when only a subset of plugins is shared between all installations).

#4 in reply to: ↑ 2 @vladimir_kolesnikov
13 years ago

Replying to scribu:

Another thing to note is that activation hooks won't work with sym-linked directories.

Have to disagree:

<?php
/*
Plugin Name: Test Plugin
Description: Test Plugin
Version: 1.0
*/

function test_activate()
{
        wp_die("Activation hook");
}

function test_deactivate()
{
        wp_die("Deactivation hook");
}

add_action('activate_test.php', 'test_activate');
add_action('deactivate_test.php', 'test_deactivate');

Works fine.

# pwd
/home/user1/htdocs/wp-content/plugins
# ls -la test.php
lrwxrwxrwx 1 user1 user1 37 2010-10-16 15:59 test.php -> /var/www/em-stuff/wp-plugins/test.php

#5 follow-up: @scribu
13 years ago

Yes, it works because you're hardcoding the plugin basename. If you to use register_activation_hook() (which you should) it won't work.

#6 in reply to: ↑ 5 @vladimir_kolesnikov
13 years ago

Replying to scribu:

Yes, it works because you're hardcoding the plugin basename. If you to use register_activation_hook() (which you should) it won't work.

With the patch from #13550 it will work anyway.

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

Last edited 8 years ago by ocean90 (previous) (diff)

#7 @scribu
13 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

#8 follow-up: @scribu
13 years ago

  • Owner set to dd32
  • Status changed from reopened to assigned

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

That makes sense, I guess.

#9 follow-up: @vladimir_kolesnikov
13 years ago

Sorry, wrong files were attached :-(

#10 @scribu
13 years ago

  • Keywords has-patch added

#11 in reply to: ↑ 9 @scribu
13 years ago

Replying to vladimir_kolesnikov:

Sorry, wrong files were attached :-(

It's ok, I removed them.

#12 @dd32
13 years ago

What about when the upgrade is being completed through FTP? I realise the scenario's of that are less common than someone using SSH/Direct, however I can see that question coming up later down the line

#13 @dd32
13 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

Symlinks are not something which we are currently willing to handle in WordPress.

In the event that you symlink common plugins into installs, you should also disable upgrades for those specific plugins.

#14 @Dreamsorcerer
8 years ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

Apparently, WordPress now supports symlinked plugins, but this still appears to be an issue.
https://make.wordpress.org/core/2014/04/14/symlinked-plugins-in-wordpress-3-9/

What currently happens, is when updating, it says 'Warning: Could not remove the old plugin.' and fails to update. At this point, it has already deleted the contents of the old plugin, so you are left with an empty directory, and still no new plugin. Either upgrades should work through the symlink and upgrade it, or as previously suggested, it should delete the symlink without touching the contents and then install the new version.

#15 @ocean90
8 years ago

  • Milestone set to Awaiting Review

Related: #29408, #36710

#16 in reply to: ↑ 8 @matcho
7 years ago

Allow me to support this argument and initiative.

AFAIK no removal mechanism in the Unix world ever deletes the target of a symlink unless a specific option is given. While this behaviour might be discussed in the context of Wordpress, it is certainly not expected and might create unexpected large damage to the data.

Note that I'm in a use case where a homemade plugin needs a symlink to point to some application / lib; I agree this is probably not the best way to achieve what I'm looking for but again, doing this should not lead to WP deleting my data.

Thanks
Mathias

Replying to scribu:

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

That makes sense, I guess.

#17 @Dreamsorcerer
6 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
6 years ago

Fix symlink security flaw

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


4 years ago

#19 @pbiron
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner changed from dd32 to pbiron
  • Status changed from reopened to accepted

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


3 years ago

#21 @pbiron
3 years ago

  • Milestone changed from 5.6 to Future Release

With 5.6 beta 1 in a few hours, punting this.

@pbiron
3 years ago

refreshing patch

#22 @pbiron
3 years ago

15134.2.diff refreshes the patch so that it applies cleanly.

The refresh also incorporates the Windows-specific changes to WP_Filesystem_Direct::delete() from 36710.diff.

Now, the only difference between 15134.2.diff and 36710.diff is 15134.2.diff has the additional check in WP_Upgrader::clear_destination().

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


3 years ago

#24 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.9

This ticket was mentioned in Slack in #core-test by pbiron. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


3 years ago

#27 in reply to: ↑ description ; follow-ups: @marybaum
3 years ago

Replying to vladimir_kolesnikov:

May we edit the title of this very worthwhile ticket to say the following:

WordPress should not try to remove themes or plugins recursively if the directory is a symlink

In English we do not use apostrophes to indicate plural nouns.

Thanks!

Consider the situation: there is a server with multiple WordPress blogs hosted in it. Some plugins are common for all/many blogs and to save several (hundreds in our case) megs of the disk space, shared plugins are stored somehwere else (say, /var/www/wp-plugins) and there are symbolic links to /var/www/wp-plugins/<plugins> from /home/<user>/wp-content/plugins/<plugins>.

The onwer of the blog (user1) may not know these details and wants to update one of the plugins (plugin1) using automatic update feature. WordPress will then try to remove /home/user1/wp-content/plugins/plugin1/ recursively although /home/user1/wp-content/plugins/plugin1 is a symlink to /var/www/wp-plugins/plugin1.

The obvious solution is to add a check to the filesystem classes that checks if the file is a symlink and if so, remove symlink with unlink() instead of trying to follow it and remove everything it sees.

The advantage of this approach is that if the user symlinks a plugin to other user's data, those data will not be removed by WordPress (this can be very good for those hosts where all users are served by the same Apache user etc).

#28 in reply to: ↑ 27 ; follow-up: @pbiron
3 years ago

  • Summary changed from WordPress should not try to remove theme's or plugin's directory recursively if the directory is a symlink to WordPress should not try to remove themes or plugins recursively if the directory is a symlink

@marybaum done

#29 in reply to: ↑ 28 @marybaum
3 years ago

Replying to pbiron:

@marybaum done

Excellent!

#30 in reply to: ↑ 27 @Dreamsorcerer
3 years ago

Replying to marybaum:

Replying to vladimir_kolesnikov:

May we edit the title of this very worthwhile ticket to say the following:

WordPress should not try to remove themes or plugins recursively if the directory is a symlink

In English we do not use apostrophes to indicate plural nouns.

That seems very picky, but the original title was "theme's or plugin's directory". i.e. the directory owned by a theme or plugin. It was perfect English...

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#32 @audrasjb
2 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.9 to 6.0

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 years ago

#34 in reply to: ↑ 1 @jqz
2 years ago

This has become more of an issue now automatic plugin/theme updates are an option, and now that on occasion a plugin author may choose to force an update to patch a security vulnerability even if automatic updates are disabled - see https://github.com/woocommerce/woocommerce/issues/32111 - this can lead to a broken site that could go undetected for days.

Replying to Denis-de-Bernardy:

Seems to me that this is a use-case where site admins shouldn't be allowed to upgrade plugins, and where all installs should be maintained from the shell using svn, git, or scripts.

No, it is possible have some common plugins symlinked, whilst others are updated in the usual WordPress way.

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

I disagree. plugins/some-plugin/ can symlink to ~/shared-plugins/some-plugin/ which in turn symlinks to ~/shared-plugins/some-plugin-1.0/. Then to upgrade it is only necessary to relink ~/shared-plugins/some-plugin/ to ~/shared-plugins/some-plugin-1.1/. There is no downtime at all and therefore no need for maintenance mode.

There is a very small risk that any HTTP requests in progress at the time of the relinking may end up parsing newer versions of any PHP files it hadn't already parsed. However, that and other problems could happen anyway with the normal WordPress update procedure if a separate process was in the middle of serving an HTTP request at the time the update was triggered.

#35 @jqz
2 years ago

  • Severity changed from major to critical

Bump. 11 years, no fix. Disappointing.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-auto-updates by mike. View the logs.


2 years ago

@costdev
2 years ago

Refreshes 15134.2.diff with PHPCS fixes and a slight cleanup of a multi-line condition.

#38 @kirasong
2 years ago

  • Keywords dev-feedback added

This ticket was looked into today during a 6.0 bug scrub.

The patch still applies as expected.

Adding dev-feedback to try to get a review to help this move forward.

Also, I reached out to some of the folks that have been working on auto-updates, to see if a review from that team might be possible, as it overlaps with the maintainers of this component.

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 years ago

#40 @pbiron
2 years ago

  • Milestone changed from 6.0 to 6.1

As 6.0 is in Beta 2, moving this to 6.1 to give us more time to test the patch, especially for non-direct filesystems.

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


21 months ago

#42 @jqz
21 months ago

  • Severity changed from critical to blocker

Setting the symlink target to read-only doesn't help. The stupid WordPress update recurses the directories and deletes the files individually.

#43 @jqz
21 months ago

This is causing a lot of issues and downtime. Please fix ASAP.

#44 @peterwilsoncc
21 months ago

  • Severity changed from blocker to normal

@jqz This is been worked on presently and is on the 6.1 milestone. Periodically increasing the severity of the ticket and providing commentary on the tickets progress does not help, please refrain from doing so.

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


20 months ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


19 months ago

This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.


19 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


18 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


18 months ago

#51 @kirasong
18 months ago

Thanks so much for the updated PR / patch @audrasjb!

If anyone has the chance to test this to see if it resolves the issue for you, it'd be highly appreciated, as we get closer to 6.1 RC.

@audrasjb Do you have a recommended set of testing instructions? I considered providing a recommendation, but it seems like this ticket covers a variety of potential concerns, and I want to be sure it's accurate.

#52 @audrasjb
17 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone to give it more time for testing.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


17 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


15 months ago

#57 @bgoewert
14 months ago

Test Report

Patch tested: pr:3372

Steps to Test

Feel free to provide alternate instructions. This is just what I was able to come up with.

  1. Download an older version of a plugin. Example: Gutenberg 14.8.4
  2. Unzip to a directory other than "wp-content/plugins/". Example:
      unzip ~/Downloads/gutenberg.14.8.4.zip -d ~/wp-test/plugins
    
  3. Ensure whatever directory you place the plugin into is writable by the web server. Example:
      sudo find ~/wp-test/plugins -type d -exec chmod 777 {} \; && '
      sudo find ~/wp-test/plugins -type f -exec chmod 666 {} \;
    
  4. Navigate to plugin directory. Example:
      cd src/wp-content/plugins
    
  5. Create symlink. Example:
      ln -s ~/wp-test/plugins/gutenberg/ gutenberg
    
  6. Attempt to upgrade the plugin from the admin dashboard.

Expected Results

When reproducing the bug/defect:

  • ❌ folders/files are deleted during upgrade.

When testing the patch:

  • ✅ No folders/files are deleted during upgrade.

Environment

  • OS: Pop!_OS
  • Web Server: Docker-Desktop & wordpress-develop
  • PHP: 7.4.33
  • WordPress: 6.2-alpha-54642-src
  • Browser: Chrome 109
  • Theme: Twenty Twenty-Three
  • Active Plugins:
    • Gutenberg 14.8.4

Actual Results

When reproducing the bug/defect:

  • ❌ All files/folders in gutenberg/ were deleted. Plugin dashboard displays "Update failed: Could not remove the old plugin." because the files were deleted and there is nothing to remove.

When testing the patch:

  • ✅ No folders/files were deleted during upgrade.

Additional Notes

  • I only tested with the direct FS method and not ftp or ssh2.
  • Also, it looks like pr:3372 currently only addresses the direct FS method.
Last edited 14 months ago by bgoewert (previous) (diff)

#58 @bgoewert
14 months ago

  • Keywords has-testing-info added

This ticket was mentioned in Slack in #core by costdev. View the logs.


13 months ago

#60 @pbiron
13 months ago

  • Milestone changed from 6.2 to 6.3

there are still some unresolved issues with this and given how late we are in the 6.2 cycle, punting this to 6.3.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


9 months ago

#62 @chaion07
9 months ago

Thanks @vladimir_kolesnikov for reporting this. We discussed this ticket during a recent bug-scrub session. Here's the summary of the feedback received:

  1. Colin volunteered to contact Paul to ask if he thinks this will get attention in the next couple of weeks and to discuss what's needed and summarize it later.
  2. The ticket was punted to 6.3 just before 6.2 Beta 4. So it might have some time to get resolved.
  3. As this touches the Filesystem API component, we would realy be hesitant to leave it in the milestone any longer than that.

Props to @costdev for the contribution.

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.


9 months ago

#65 @costdev
9 months ago

  • Milestone changed from 6.3 to Future Release

This ticket was mentioned during the Upgrade/Install component meeting.

We still need to establish what's left to do on this one, and as it's getting late in the 6.3 cycle and the patch touches the Upgrade/Install and Filesystem components, I'm moving this to Future Release. It can be moved to a release milestone when we're confident it's ready for review.

Note: See TracTickets for help on using tickets.