WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#20195 closed enhancement (wontfix)

Plugins uninstall.php

Reported by: wpsmith Owned by:
Milestone: Priority: normal
Severity: major Version:
Component: Plugins Keywords: dev-feedback
Focuses: Cc:

Description

Should there be some more security around the use of uninstall.php, possibly giving the user the ability to chose whether to delete the options stored by the plugin or not? Even though WP_UNINSTALL_PLUGIN is one safeguard, is it the best?

As it stands now, sadly, any plugin can add/update/delete options with this function. Or it would be great to be able to add the ability for WordPress to notify the user of what options the plugin is about to delete.

I am not sure the best course of action on this, but feel that there may be some room for improvement here.

Change History (17)

comment:1 scribu2 years ago

  • Type changed from defect (bug) to enhancement

Once it's activated, a plugin can delete anything, at any time, so the security concern is moot. Don't install plugins from untrusted sources.

That said, it might be useful for plugins to have the ability to show users a customisable message before an uninstall.

comment:2 lightningspirit2 years ago

  • Cc lightningspirit@… added
  • Keywords dev-feedback added
  • Severity changed from normal to major

Indeed, like scribu said, you should not be concerned about the security of uninstall.php for trusted plugins, internally...

...because for a less common scenario I think we have to be concerned about the attacks from external scripts (crawlers, etc...).

Question: What happens if an external script includes wp-load.php from a WordPress instance, defines WP_UNINSTALL_PLUGIN and then includes an uninstall.php from a known plugin, for example, WP Ecommerce plugin?

comment:3 nacin2 years ago

Well, that script is executing PHP, so they could do anything they wanted.

comment:4 follow-up: lightningspirit2 years ago

As they have access to a fully loaded WordPress environment they can fully execute uninstall.php. That can be a security break. I think we should check for referer or use the nonces API too...

comment:5 in reply to: ↑ 4 ; follow-up: nacin2 years ago

Replying to lightningspirit:

As they have access to a fully loaded WordPress environment they can fully execute uninstall.php. That can be a security break. I think we should check for referer or use the nonces API too...

No, this is not a security breach. The security breach is having access to a fully loaded environment in the first place. Once they are there, they can also fully execute raw database queries and delete files all they want. Forget uninstall.php, how about $wpdb->query("DROP TABLE $wpdb->posts")?

comment:6 in reply to: ↑ 5 wpsmith2 years ago

Using the constant WP_UNINSTALL_PLUGIN is a good idea. It definitely prevents one from accessing the file directly. However, any plugin can set WP_UNINSTALL_PLUGIN and uninstall plugins at their leisure without using WP_Filesystem, which granted may not be secure, but it is also not the best or recommended method. I was just thinking that a little hardening wouldn't hurt.

I would also like to second scribu's suggestion to enable to customize the uninstall with a custom message.

Replying to nacin:

Replying to lightningspirit:

As they have access to a fully loaded WordPress environment they can fully execute uninstall.php. That can be a security break. I think we should check for referer or use the nonces API too...

No, this is not a security breach. The security breach is having access to a fully loaded environment in the first place. Once they are there, they can also fully execute raw database queries and delete files all they want. Forget uninstall.php, how about $wpdb->query("DROP TABLE $wpdb->posts")?

comment:7 lightningspirit2 years ago

Okay, I made some tests in my local environment.
@nacin:

To include an http://websites/wp-load.php one have to be allow_url_include set in php.ini.

Even if they can include the file, this on line 21:

/** Define ABSPATH as this file's directory */
define( 'ABSPATH', dirname(__FILE__) . '/' );

defines the absolute path of the including external script, and then nothing but errors are thrown. I think we don't need to be worried about this pitfall! :)

comment:8 nacin2 years ago

I would also like to second scribu's suggestion to enable to customize the uninstall with a custom message.

I think it would be good for the uninstall confirmation page to be a choice between "Delete Files and Data" vs "Delete Files". The plugin's uninstall routine would only be fired if the "and Data" was chosen. This is by no means a security improvement; it's a UX improvement.

I feel there is an existing ticket for this, though.

comment:9 lightningspirit2 years ago

@wpsmith They could check for the uninstall nonce.
@nacin +1 on confirmation page! Most of the modern applications do that, I think its "time for a changing" in uninstall process.

comment:10 nacin2 years ago

They could check for the uninstall nonce.

If someone had access to the loaded WordPress environment, it would be trivial to re-create such a nonce.

comment:11 follow-up: lightningspirit2 years ago

If someone had access to the loaded WordPress environment, it would be trivial to re-create such a nonce.

What about using HTTP referer?

comment:12 in reply to: ↑ 11 nacin2 years ago

Replying to lightningspirit:

What about using HTTP referer?

Even easier to spoof. Don't even need access to the environment to do that.

No matter what you try to do, once you can execute code, you can do anything. There just isn't a security consideration here, at all.

comment:13 follow-up: lightningspirit2 years ago

Indeed, there is no way to secure once one have access to the environment. Perhaps we could do a "security by obscurity" to wp-load.php, giving user the option to move wp-load.php one level above, as we currently allow for wp-config.php.

@nacin Do you think my suggestion is feasible?

comment:14 lightningspirit2 years ago

I will create a new ticket for that if necessary.

comment:15 in reply to: ↑ 13 nacin2 years ago

Replying to lightningspirit:

Indeed, there is no way to secure once one have access to the environment. Perhaps we could do a "security by obscurity" to wp-load.php, giving user the option to move wp-load.php one level above, as we currently allow for wp-config.php.

Not really, because they could always include wp-blog-header, or index, or other files. Remote "inclusion" of files is not a vulnerability. I don't think allow_url_include does what you think it does.

Moving wp-config.php up one level is not there for security. It is to use WordPress in its own folder, as an SVN external, with wp-config.php sitting beside it.

comment:16 scribu2 years ago

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

The whole premise of this ticket is flawed. Once they're installed activated, plugins can do whatever the hell they want (including uninstall other plugins). Deal with it.

@lightningspirit If you know the term "security by obscurity", you should also know that it's ineffective.

Last edited 2 years ago by scribu (previous) (diff)

comment:17 scribu2 years ago

As for the option to delete a plugin without uninstalling it, the closest thing I found was #8783 which is kind of different.

Therefore: #20578

Note: See TracTickets for help on using tickets.