Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#9406 closed defect (bug) (fixed)

harden the security of the active_plugins array

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 2.7.2 Priority: high
Severity: major Version:
Component: Security Keywords: has-patch tested
Focuses: Cc:


In the past few weeks I've been assisting a couple of users whose sites got hacked. Granted, they're at fault, and they had not upgraded their sites.

It occurred to me, however, that a simple tweak to the WP source code would have gone a great length to minimize the impact of their site getting hacked. Specifically, every hacked site I've bumped into ultimately took advantage of the active_plugins array, as follows:

include_once(WP_PLUGIN_DIR . '/' . $plugin);

The $plugin is checked against for dots, and the like, but that definitely doesn't seem to be enough.

Why are we not validating that the plugin files ends with .php? It would prevent hackers from including txt, bak, jpg and whatnot types of files...

This check should be done immediately before the plugin is included.

Along the same lines, the uploads folder should be validated to make sure it doesn't look fishy before it gets used.

Fishy looking files and values should not just be rejected -- the site's admin should additionally get a hourly email until he has fixed the issues.

Attachments (1)

9406-patch-validate-plugin-in-plugin-loader.patch (1.4 KB) - added by hakre 9 years ago.

Download all attachments as: .zip

Change History (6)

#1 @hakre
9 years ago

Denis, thanks for taking the time to share your thoughts. I see no problems with limiting the fileextension to .php for plugins. this should be checked directly before inclusion (at least).

I have created a patch that reflects those changes. Additionally, the decision what a blacklisted plugin value is, is better documented and structured.

Finally the temporary variable $current_plugins is unset (as this was already done with the $plugins variable).

Please feel free to take a look to my other patchset, that I created for a better plugin security as well. It enables the admin to display the database values human readable that are used by get_option - especially those which are serialized in the database: #9175

#2 @hakre
9 years ago

  • Keywords has-patch added

#3 @Denis-de-Bernardy
9 years ago

  • Keywords tested added

one tiny tweak:

case strlen($plugin) < 4:

seems unneeded based on my testing of your patch -- the substr call catches that too.

#4 @ryan
9 years ago

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

(In [10892]) Require active plugin files to end in .php. Props hakre. fixes #9406

#5 @hakre
9 years ago

thanks. just reviewed, looks good to me.

Note: See TracTickets for help on using tickets.