Plugins without headers don't show in the plugins page, keeping some exploits hidden

Just a quick run over for detecting plugins which are active which are not valid and/or are not stored in the WordPress plugin directory.

The extra processes are only run when accessing the plugins page, so no overhead is added for most pageloads.

However, Thanks to WordPress's filters, It'll allways be possible for exploits to hide themselves in cases like this. So Unless SQL's are hard-coded into the plugins page skipping the get_option/update_option routines, an exploit can filter it all too..

Sounds like a nice start. I insist on including this check also at the Dashboard, since the plugins page is rarely visited and with this kind of threat, the sooner it is removed the better.

I think it's a good idea to deactivate invalid plugins, but I'm not sure that this will provide much protection from this kind of attack. Once an attacker has managed to include his "plugin" in the list of active plugins, all he has to do is provide a legitimate header format, then filter the output buffer to hide all mention of it from the admin.

Since he can apparently write to your filesystem and modify your database, a rogue plugin is really just the beginning of your worries.

I think it's a good idea to deactivate invalid plugins, but I'm not sure that this will provide much protection from this kind of attack.

It provides Zero protection for exploits written/modified after its implemented, It provides little protection for exploits written before implementation.

If a exploit can execute php, write files, or access the database, then nothing WordPress does will be safe.

I'm going to set the Milestone to 2.6, any patches made there can be back-ported to 2.5 if need be, But i honestly do not see any ways that the issue in this ticket can be solved 100% by WordPress, The only way is to actually implement some security on the server to proect against it.

I only say that any plugin that !Wordpress is willing to execute in wp-settings.php should be shown at the plugins page, filters aside. This is at least an inconsistency in the user interface.

Replying to DD32:

It provides Zero protection for exploits written/modified after its implemented, It provides little protection for exploits written before implementation.

I'm sorry to disagree. There are other kinds of attack you don't seem to be considering. This hack in particular seems to have been executed using the following steps:

1) Through TinyMCE, uploaded PHP code as a .jpg temporary file. 2) Through SQL injection (entry point still unknown), added the uploaded code as a plugin. 3) Finished instalation of the plugin by executing the blog's home page.

So, DD32's proposal would REALLY be effective against this kind of attack. Let me change the milestone back to 2.5.2. If you still disagree, change it back to 2.6 and I will not touch it again.

I'm sorry to disagree.

The only reason i pointed it out was because of 2 things:

1. The exploit itself can filter the plugins list on access to a page which causes invalid plugins to be deactivated. exploit:

   If page is going to kill me then
     add_filter('active_plugins', 'Remove myself from the active list for that page!');
   end if
   

2. The exploit itself can reactivate itself in event of deactivation

   register_shutdown_function:
   if I am not longer in the active plugins list then
     $current = get_option('active_plugins');
     $current[] = __FILE__;
     update_option('active_plugins', $current);
   endif
   

Or

add_filter('update_active_plugins'):
If list does not include me then
  Add myself to the updated list
end if

Some people are not going to like me posting that as they may feel its pointing out how to hide a exploit in wordpress, but anyone with any knowledge of WP/filters could figure it out, They definately could (They being the exploiters).

So it protects against the current generation, but it will not protect against any of next generation which specifically target WordPress

The only reason i set it to 2.6 is as new functionality (Which this is, its not just a simple bug fix) goes into the trunk(2.6) branch first for testing, and then if its decided it needs to be in the 2.5 branch which is bugfixes only, then it gets backported.

Theres nothing stopping exploits from appending their code to existing plugins which are active, appending it to files, or simply inserting the file in a place where WordPres? sautomatically includes them.

OK, you are right. After checking what the 6871.diff patch does I don't think it would be of any help for any kind of attack. It seems to have some error too, since it didn't remove any plugins from the active_plugins option I faked to test with, nor showed any messages at the plugins admin page. Instead, please consider these changes (sorry, I don't have an SVN client for the moment, I performed a simple diff):

wp-settings.php

355,358c355,356
<               foreach ($current_plugins as $plugin) {
<                       if ('' != $plugin && file_exists(ABSPATH . PLUGINDIR . '/' . $plugin))
<                               include_once(ABSPATH . PLUGINDIR . '/' . $plugin);
<               }
---
>               foreach ($current_plugins as $plugin)
>                       wp_validate_load_plugin($plugin);

wp-includes/functions.php

1751a1752,1766
> /**
>  * wp_validate_load_plugin() - Loads a plugin only if it exists below the plugins directory
>  *
>  * @param string $plugin e.g. akismet/akismet.php
>  * @return bool
>  */
> function wp_validate_load_plugin($plugin) {
>       $ppath = str_replace('\\','/',ABSPATH . PLUGINDIR) . '/';
>       if ('' != $plugin && file_exists($ppath . $plugin) &&
>               str_replace('\\','/',substr(realpath($ppath . $plugin),0,strlen($ppath))) == $ppath) {
>               include_once($ppath . $plugin);
>               return true;
>       } else return false;
> }
>

I think this would be effective protection for future attacks of this kind, since the attacker doesn't have full writing permission on the file system until the plugin is installed and executed; they can only write temporary files, and PHP code can only be executed after SQL injection by marking their temporary file as the plugin. This change eliminates the possibility of executing plugins outside the plugin directory. Let me hear your thoughts.

Guille

I think a combination would be good.

My patch was simply designed to deactivate invalid plugins, not to protect from them being loaded as such, Allthough it would be useful in cleaning up afterwards for some instances.

There are problems using realpath on windows platforms too, and unfortunately in the current instance using plugin_basename() would be useless(As it only works with a correct input) (By problems with realpath, I mean if its not a valid path, it'll return false, And AFAIK, it can cause extra IO in some cases, I'm not 100% sure on that, but it seems not needed in this case anyway)

Something like this could be used instead:

foreach ($current_plugins as $plugin)
    if ('' != $plugin && strpos($plugin, '..') === false && file_exists(ABSPATH . PLUGINDIR . '/' . $plugin))
        include_once(ABSPATH . PLUGINDIR . '/' . $plugin);

that would prevent loading of any that had a obviously bad path, Then the plugin would be blown from the active plugins list upon loading the plugin admin (Assuming it hadnt attempted to filter itself out, But it wouldnt be a problem anymore, as the exploit code shouldn't be loaded with the plugins).

It'll still include any malicious code which is inside the plugin directory however, Its not possible to perform all the checks for a proper plugin on every page load in those cases, its just too much loss of performance.

It seems to have some error too, since it didn't remove any plugins from the active_plugins option I faked to test with, nor showed any messages at the plugins admin page.

Not sure why.. I tested by activating a plugin and then removing its metadata. Just tried like this:

$current = get_option('active_plugins');
var_dump($current);
$current[] = '../../../../../../../../../../../../../../../../../../../../../../tmp/tmp4Z0MYa/sess_56b48e283b26c4dd342c25be2e4d22e7';
update_option('active_plugins', $current);
$current = get_option('active_plugins');
var_dump($current);

$invalid = validate_active_plugins();
var_dump($invalid);

$current = get_option('active_plugins');
var_dump($current);

?> 
array
  0 => string 'add-from-server/add-from-server.php' (length=35)
array
  0 => string 'add-from-server/add-from-server.php' (length=35)
  1 => string '../../../../../../../../../../../../../../../../../../../../../../tmp/tmp4Z0MYa/sess_56b48e283b26c4dd342c25be2e4d22e7' (length=117)
array
  '../../../../../../../../../../../../../../../../../../../../../../tmp/tmp4Z0MYa/sess_56b48e283b26c4dd342c25be2e4d22e7' => 
    object(WP_Error)[206]
      public 'errors' => 
        array
          'plugin_invalid' => 
            array
              0 => string 'Invalid plugin.' (length=15)
      public 'error_data' => 
        array
          empty
array
  0 => string 'add-from-server/add-from-server.php' (length=35)

so it appears to work for me.

Hi, DD32. Your method using strpos is better indeed, although it would rule out any strange plugin name like 'my...'. Perhaps going a little deeper in the same direction?:

strpos($plugin,'/../') === false && substr($plugin,0,3) != '../'

About the patch you wrote before, I think I tested it incorrectly. I manually changed the serialized array from active_plugins using phpMyAdmin and I inadvertently left two elements with index [0], so my fake plugin never existed in the first place. Sorry about that. It does work as expected. :)

I created a new patch including all the changes for you to consider and added the has-patch keyword.

Includes changes discusssed up to 05/01/08 12:52:09 with DD32

1. substr($plugin,0,3) != '../' is really not needed, Simply because it should be caught by the other strpos IMO, ./../ is just as valid, and as such, would be used by any more exploits.
2. strpos($plugin,'/../') === false That gets rid of the chance of someone having multiple dots in the actual filename, But really, Who does that? Granted, strpos($plugin,'../') might be a better option, As it catches both cases 2 & 1
3. What about on Windows platforms? C:\www\app\..\ is valid, it resolves to C:\www\

Replying to DD32:

1. substr($plugin,0,3) != '../' is really not needed, Simply because it should be caught by the other strpos IMO, ./../ is just as valid, and as such, would be used by any more exploits. 1. strpos($plugin,'/../') === false That gets rid of the chance of someone having multiple dots in the actual filename, But really, Who does that? Granted, strpos($plugin,'../') might be a better option, As it catches both cases 2 & 1 1. What about on Windows platforms? C:\www\app\..\ is valid, it resolves to C:\www\

Mmmm... how about this?:

strpos(str_replace('\\','/','/'.$plugin),'/../') === false

That should take care of all the cases: ..\something --> CATCHED ..\\something --> CATCHED ..//something --> CATCHED ..//something --> CATCHED something/../something --> CATCHED something//..//something --> CATCHED something... --> PASSES something.../something --> PASSES ..something --> PASSES

As of comment #13