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

[guillep2k]

There's a new exploit that leaves a bogus plugin in the active_plugins option which doesn't show in the plugins page. The plugin (in my case) was at:

../../../../../../../../../../../../../../../../../../../../../../tmp/tmp4Z0MYa/sess_56b48e283b26c4dd342c25be2e4d22e7

You can see more info at:

​http://wordpress.org/support/topic/169246?replies=8#post-746480
(my reply as guillep2k)

WordPress should show SOME information about invalid/incomplete plugins in the plugins page in order to quickly detect this situation AND quickly disable them. More information in the Dashboard would be great too.

[DD32]

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..

[guillep2k]

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.

[filosofo]

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.

[DD32]

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.

[guillep2k]

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.

[guillep2k]

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.

[DD32]

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.

[guillep2k]

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

[DD32]

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.

[guillep2k]

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. :)

[guillep2k]

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

[guillep2k]

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

[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\

[guillep2k]

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.
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\

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

[guillep2k]

As of comment #13

[ryan]

Milestone 2.5.2 deleted

[guillep2k]

I wonder if this bug is still current as of version 2.6. The validate_plugin() function now calls validate_file(), which should cover the most sensitive aspect of the issue.

[guillep2k]

I take back what I said; the most important part is the change needed in wp-settings.php, which 2.6 does not include. I'll try to write a new patch for 2.6 and patch it here. I hope it won't get moved forward to Milestone 51009554.6567.

[santosj]

Keep in mind that validation should be in the plugins page. If you add this to the wp-settings.php then you could increase the load and decrease the performance. The reason why it is done on the plugins administration page is because it will only affect that page.

You really don't need to do this every time. If the user will visit the plugins page every once in a while, then that should be good enough.

[guillep2k]

In some aspects you are right, santosj; most validations should be made in the plugins page, but this one is so important that should be performed every time. The exploit I'm trying to prevent is a real world exploit that happened to at least three blogs of my knowledge and was performed through a bug in TinyMCE. The injected plugin was not validated and serious damage could have happened if the plugin was a little more aggresive. Such damage would have occurred way before the user had any chance to visit the plugins page which, by the way, noone visits regularly once the plugins they need are in place and working.

Anyway, what I'm trying to add to the wp-settings.php file (the 'performance sensitive' change) is just some strpos() and str_replace() function calls:

if( __existing_validation__ && \
    strpos(str_replace('\\','/','/'.$plugin),'/../') && \
    __existing_validation__ ) {
    ...
}

I doubt anyone would notice such a small addition.

I'm adding the "6871 version 4 for 2.6.diff" file after this comment. I tested it with version 2.6 right out of the SVN tags/2.6 branch (8342).

[guillep2k]

Tested with 2.6.

[guillep2k]

Note: it's strpos(...) === false, not just strpos(...). The patch is correct, I only messed up with the Trac comment. :)

[jacobsantos]

If that is the case and a plugin was injected, then that is a separate issue. You are only working around the original issue at the cost of performance. Well, you do have a point, if another exploit was made, then this would be a good thing, since you are again right, not many will visit the plugins.php page after it is set up.

How did the plugin get injected through TinyMCE? Was that bug fixed in 3.0.x? How can it be prevented in the future?

[guillep2k]

How did the plugin get injected through TinyMCE? Was that bug fixed in 3.0.x? How can it be prevented in the future?

I'm sorry I couldn't find out. I could only do some forensics on the issue, and I found the injection script in the TinyMCE temporary folder. You can see more info at: ​http://wordpress.org/support/topic/169246?replies=8#post-746480 (my reply as guillep2k)

I agree with you that the TinyMCE bug is a separate issue, but it is beyond my possibilities to track it down ATM.

[santosj]

Hmm. It seems that in order to solve this problem. We should be checking that the path is within the WP_PLUGIN_DIR. A simple regex that strips all "../" from the path and checks that the file exists within that directory should be efficient and solve the issue.

All plugins should be relative to WP_PLUGIN_DIR, so this should work and file_exists() should work fine. It won't validate the file has plugin metadata, which can still be done in the plugins administration.

Do you agree?

[guillep2k]

Well, more or less that's what my modification does, only it avoids using RegEx which are considerably slower.

[santosj]

Ah, I didn't see the new patch. It looks good, except it appears you are testing for ../ and failing if it exists. I'm thinking strpos is faster than str_replace (which is what I meant when I said regex). Actually, I'm sure it is.

I think your patch is great and I hope it gets in. Sorry about the misunderstanding.

[santosj]

Might want to change:

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

to just

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

[santosj]

Or since you use str_replace already:

if ('' != $plugin && file_exists(WP_PLUGIN_DIR . '/' . str_replace(array('../', '..\'), '', $plugin)))

[guillep2k]

Mmm... that would rule out folders "my_strange_folder../subfolder", which are valid in linux/unix. Notice the '/'. before $plugin in the comparision, which merges the '/../' with the '../' cases together. I gave thorough thinking to these changes to make them comprehensive and as fast as possible.

[santosj]

Okay. Cool.

If you get on IRC, you can let the core developers know that the patch is worth committing or at least looking over.

[azaozz]

Looks good, would stop certain type of exploits. One question: why use srt_replace when only testing for false? Wouldn't another strpos do it a bit faster like in 6871.5.

[guillep2k]

I think the method of using two strpos() could be fooled by using mixed slashes ('/..\'). I think "6871 version 4 for 2.6.diff" is safer.

[santosj]

str_replace is fast enough function for string replacing, so you're probably doing premature optimization with exploitable cases. You would need an extra four strpos() to make up for all known possible ways to handle it and by then you're not going to be much faster than str_replace

[azaozz]

(In [8495]) Include only valid plugins. Props guillep2k, fixes #6871

[xknown]

There is a small XSS issue when showing plugin's name -- it's better to avoid any vector attack.

[DD32]

There is a small XSS issue when showing plugin's name

While it displays the plugins filename rather than "Plugin Name" I guess you're saying that it could include html in the active_plugins option?

[westi]

Replying to DD32:

There is a small XSS issue when showing plugin's name

While it displays the plugins filename rather than "Plugin Name" I guess you're saying that it could include html in the active_plugins option?

Indeed if someone has inserted something into active_plugins it could be anything so we should def protect against this.

Good catch xknown once again :-)

[westi]

(In [8499]) Avoid possible XSS when displaying the list of invalid plugins fixes #6871 for trunk props xknown.

[westi]

Ok. [8495] and [8499] fix it for trunk but I think we should back port this to 2.6.1 as well.

[westi]

(In [8500]) Include only valid plugins. Props guillep2k and xknown, fixes #6871 for 2.6.1