WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#9164 closed defect (bug) (fixed)

#6871 Regression for Plugin Dir

Reported by: hakre Owned by: ryan
Milestone: 2.7.2 Priority: high
Severity: normal Version: 2.7
Component: Security Keywords: 2nd-opinion dev-feedback
Focuses: Cc:

Description

#6871 fixes a Bug that enabled a code-injection feature for wordpress. Even the specific describben injection (in (#6871)) is fixed, it does not fix an injection into an existing plugin dir.

There is not much strict information provided in the official docs, on how a plugin must be designed. To fix this issue it is not only important to have better checks in the code but to provide a strict definition in parallel as a kind of contract for all plugin developers. This is why I write some more words about the Issue.

To give an example:

By current defintion, a plugin must have a PHP-Comment containing Information about the Plugin (called Plugin Headers). But the Function validate_plugin() in /wordpress/wp-admin/includes/plugin.php does not check this (Keep in mind that this matches even #6871 and renders it kinda unfixed).

Additionally it is written, that you must/have to create a php file. WordPress Coding Styleguide does not explicitly name it (does it?), but I assume that for WordPress all PHP Files must have the file-suffix of .php. This is something that could be checked against as well.

get_plugin_data() might come in handy to verify plugin metadata. at least this looks like the undocumented but recommended API Function to read Metadata out of a Wordpress Plugin. The Plugin Listing Page in the Admin is using it.

In the Admin itself (/wordpress/wp-admin/plugins.php) the function get_plugins() (/wordpress/wp-admin/includes/plugin.php) is used to retrieve the list of plugins. But this Function is not used in the validation function as well so that the listing becomes inconsistent with the already coded protection against invalid plugins. I strongly encourage to create more consistency here to gain an overall better stability. That function for example checks for the '.php' file-ending. So I see no problems to add it in the other check as well.

Anyway, this sole change won't help to create a better consistency here. But it would be a start.

The main focus in my opinion should be put on the fact, that some plugins that are active aren't in the listing of the actvive plugins. So the value from the database can be validated against the value gathered in the listing OR the other way round. For the listing, filesystem operations are necessary (traversing directories), so the later would at least enable an Admin to verify the current setup with lesser loss of performance.

It might be a good Idea to create check routine that is used on the Admin-Plugin-Page (only) and those who want to make their site more secure can use it in their Frontend as well.

So finally I would suggest to provide a better Check in the Admin-Plugin-Page (add function validate_active_plugins_listing() that uses the same code as the listing for the admin panel, maybe add another method for the listing of plugins only first), add a .php file Ending Check in the standard Plugin Check (function validate_plugin() in /wordpress/wp-admin/includes/plugin.php).

Change History (8)

comment:1 DD325 years ago

  • Keywords 2nd-opinion dev-feedback added
  • Version set to 2.7

First up, How did #6871 cause a regression? Or was it simply another vector that was ignored?

Secondly, On the plugins page, The plugins metadata is checked (ie. If the plugin contains no metadata, then its deactivated), Checking the contents of the files on every load would be too slow - No, Seriously, Getting plugin meta data is a really expensive task.. no-one in their right mind would read it every pageload

So.. Your suggestion to combat all of this is:

  • Check that the plugin filename ends in .php
  • make a function that users can call in their theme to read the metadata and validate the plugins..

(sorry if i've missed something)

comment:2 hakre5 years ago

Well, #687 fixed that exact attack, but if you use the same attack while placing the payload inside the plugins path somewhere, you are still free to go. That is what I called the Rergression. Kinda another vector ingnored.

The Admin Plugin Page only checks the Plugins it finds in the filesystem. But that are not all Plugins that are active. Active Plugins are those referenced as the option "active_plugins" in the database.

Because the Admin Page does not check those values, it fails to get attention about plugins that are activated insecurely through direct option value access.

This technique is used to inject malicious code. Since the Plugin Check can not decide wether or not the code loaded by a plugin is malicous or not, the Admin Page should at least list all _activated_ plugins, not only those which it auto-discoveres on filesystem and also set as actived.

Do you understand what I mean?

comment:3 hakre5 years ago

I create another Ticket with a related Patch to this Issue as well: #9175 It is a patch that enables the Admin to view serialized Values in options.php. This helps to figure out which plugins are activated in the database for example: active_plugins Screenshot

comment:4 ryan5 years ago

(In [10594]) Deactivate plugins that don't have valid plugin headers. see #9164

comment:5 hakre5 years ago

Hi ryan, thanks for the fast fix. I'll test it right now.

comment:6 hakre5 years ago

Fix does what it is intended for: Cleans out current payloads but I was able to circumvent the protection in a relativly easy way. Do not get me wrong, this should go in, it adds more consistency and cleans out current payloads.

Anyway I must strongly speak for things like my suggestion/patch in #9175. It's like with Anit-Rootkit Tools: The Backend there directly accesses the Database and gets untainted values. That is in contrast to situations where a malicious script can already filter stuff (like with get_plugins or the plugin admin page). That ensured, an Admin at least can manually figure out that something went gaga. This helps in support as well.

comment:7 Denis-de-Bernardy5 years ago

imo we can close this one as fixed, given that we also added a validation when including the plugin files in wp-settings.php.

comment:8 hakre5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Denis, payloading still works, please review #9175.

Note: See TracTickets for help on using tickets.