28 Apr

Security Tip for Developers: Remove Testing Files from Third Party Libraries Included in Your Plugin

Every time you add additional code to a WordPress plugin (or any other software) you are adding more potentially insecure code. That includes third party libraries and other third party code. Earlier this week we looked at the details of a local file inclusion (LFI) vulnerability in the Booking Calendar plugin caused by its use of a file from another plugin (interestingly the vulnerability doesn’t look like it was every exploitable directly in the other plugin). In that case the file had not been updated in Booking Plugin after it was initially introduced in  to it, so while the original plugin’s code had been fixed 5 years ago, Booking Calendar remained vulnerable until weeks ago, which seems like a good reminder to keep third party code up to date.

While making sure that third party code in a plugin doesn’t contain any vulnerabilities would be rather difficult, something we ran across recently does seem to be an easy way to reduce the security risk that come from including them. While doing a security review of the plugin BackUpWordPress, which was selected to have that done by our customers, we found that the plugin had a minor vulnerability, a full path disclosure vulnerability. What is relevant here is that the file with the vulnerability was a testing file for a third party library included with the plugin, which didn’t appear to being used in the plugin. Testing files for third party libraries shouldn’t be something that normally is included in a plugin, since they won’t normally be used in that context and if someone had a need for them they can always add them themselves.

25 Apr

Security Tip for Developers: Avoid Using esc_sql() When Trying to Prevent SQL Injection Vulnerabilities

Two weeks ago there was discussion on our post detailing a vulnerability in the plugin Gallery – Video Gallery over the escaping method being used to fix a SQL injection vulnerability in the plugin. While the changes made look to have fixed the issue, they were less than ideal. Part of the issue was that instead of using a prepared statement to prevent the possibility of SQL injection the function esc_sql() was used. That is despite documentation for that that function making a pretty clear suggestion that it is not recommend in most situations:

In 99% of cases, you can use $wpdb->prepare() instead, and that is the recommended method. This function is only for use in those rare cases where you can’t easily use $wpdb->prepare().

The documentation then notes that:

Note: Be careful to use this function correctly. It will only escape values to be used in strings in the query.

That discussion reminded us of an attempt to fix a SQL injection vulnerability in another plugin, Simple Personal Message, where that warning was not heeded and it mattered. What made that situation troubling was that plugin had been removed from the Plugin Directory due to the vulnerability and was restored without actually being fixed, due to esc_sql() being used where it wasn’t appropriate (after we reported that the change didn’t fix the issue it was removed again).

The problem was that the value that permitted SQL injection is run through esc_sql() then placed in to query where it is not in quotes:

$id = esc_sql(esc_attr($_GET['message'])); 
 
$message = $wpdb->get_results("SELECT * FROM $table WHERE id = $id");

Using a prepared statement isn’t much more difficult than using esc_sql() for that code and it avoids the problem that comes with using esc_sql(). The prepared statement replacement for the code could be done like this:

$message = $wpdb->get_results( $wpdb->prepare( "SELECT * FROM $table WHERE id = %d", $_GET['message']) );

With that the SQL query and the user input value are separated, so the code understands the user input is only intended to contain a value and not additional SQL code, which would permit SQL injection to occur.

24 Apr

Security Tip for Developers: .htaccess Based Protection Won’t Work on All WordPress Websites

One of the ways we see plugin developers try to stop improper access to files generated by their WordPress plugin is to restrict direct access to the files over the Internet through the use of access restrictions placed in a .htaccess file (as the was the case with a vulnerability we disclosed last week). The problem with this is that this only works if the website is hosted on a web server that utilizes .htaccess files. While they are used by the most popular web server Apache, they are not used by the Nginx, which along with Apache is recommended for use with WordPress, or Microsoft’s IIS, which WordPress supports with its own release of WordPress.

It isn’t clear how widespread usage of different web servers is on websites running WordPress since the WordPress statistics page doesn’t include a breakdown of that. Looking wider, Netcraft found in April that 46% of active websites were using Apache, 20% using Nginx, and 9% were using IIS.

There isn’t a perfect solution for dealing with this. The IIS web server uses web.config files to provide similar functionality to .htaccess files, so you can add that alongside a .htaccess file. Nginx by comparison doesn’t provide a directory level configuration file, so you would need to suggest that the plugin’s user add code to the existing Nginx configuration file.

Another option that can be use in conjunction with doing that is to give the directory a randomly generated unique name. So for example, instead of calling the directory “backups” on one website it would be “backups-fux6sAseben8” and on another it “backups-2haHatrubRac”. That way even if the protections didn’t work someone would have a hard time finding the directory. You could also provide unique names at the file level as well.

20 Apr

Security Tip for Developers: You Don’t Need to Restrict Direct Access to .php Files Twice

One of the items we check for during our security reviews of plugins selected by our customers is to see if the plugin’s .php files can be accessed directly when they are not intended to. While being able to access them directly when that isn’t necessary usually doesn’t have any security impact, it is easy to prevent that from happening and it could in some cases prevent serious issues, like a remote code execution (RCE) vulnerability we found being exploited in a security plugin last year.

Something we ran across recently in our monitoring of security changes made to plugins, which allows us to include data on vulnerabilities that you won’t find in other sources of WordPress plugin vulnerability data, indicates that developers implementing such protection don’t always understand what it is that they are implementing.

The changelog entry for the new version of a plugin was “Security plugin fix.”, but in looking at the changes made we found the only change was that the following code was added:

if ( ! defined( 'ABSPATH' ) ) { 
    exit; 
}

That is code that will prevent direct access to the file, as we will explain in a second.

That wasn’t a security fix since the code was added directly below this piece of code:

if ( ! defined( 'WPINC' ) )  die;

While that looks a bit different than the first one, it does exactly the same thing.

Then yesterday in the same plugin another change was made that added the following lines to the beginning of the code in a number of files:

if ( ! defined( 'WPINC' ) )  die; 
if ( ! defined( 'ABSPATH' ) ){ exit;  }

Seeing as both lines do the same thing, we were confused as to what might be going on. We wondered if we were missing something or if we could find an explanation as to what might be going on.

In our search for answers we couldn’t find anyone suggesting doing both of those, but we did run into a StackExchange question from someone wondering which to use. Seeing as there still seems to confusion on this let’s take a closer look at what they both do.

First let’s deal with the easy part of this, “die” and “exit” are the same. PHP just allows you terminate a script with either one.

The first part checks if a constant is defined, which for either “WPINC” or “ABSPATH” will not be true if you make a request directly to a .php file from a plugin. So which one is checked doesn’t make any difference for what is being done. When they are not defined “die” or “exit” executes and no more code in the file will run.

To full explain things though, “WPINC” or “ABSPATH” are constants that are defined when WordPress is loading. Normally, when requesting a front-end page  “ABSPATH” will be defined by the file /wp-load.php and “WPINC” will defined later when the file /wp-settings.php is loaded. Since neither one of those files will have been loaded when first accessing a plugin’s file directly, it doesn’t matter which one you check for, so you just need to add one of those line before the rest of the code in the file to prevent direct access.

16 May

Security Tip for Developers: Make Sure To Check a User’s Capabilities When Processing an Admin AJAX Request

One common cause of security issues with WordPress plugins that we continue to see happening is a failure to properly check on whether a user should be able to use admin AJAX functions they are sending requests to. Since the wp_ajax_ hook makes the AJAX function accessible to any logged in user, without checking their capabilities even a Subscriber level users can access functions meant only for Administrators.

In most cases you will also want to make sure you are protecting against cross-site request forgery (CSRF) in those ajax requests as well.

13 May

Security Tip for Developers: The is_admin() Function Doesn’t Tell You If Someone is an Administrator

One reoccurring cause of security issues in WordPress plugins is the misuse of the function is_admin(). Based on its name you might reasonably assume that it checks if someone is Administrator level user in WordPress and that seems to have tripped up lots of plugin developers. In reality it just “checks if the Dashboard or the administration panel is attempting to be displayed”. It will also “return true when trying to make an ajax request (both front-end and back-end requests)”.

How to Actually Check if Someone is an Administrator

If you need to check is someone is an Administrator you have several options.

One option is to use the function is_super_admin(), which will:

Determine if user is a network (super) admin. Will also check if user is admin if network mode is disabled.

You can also use the function current_user_can(), which can used to check the role of the user:

current_user_can('administrator')

or you can check if user has a capability, usually a check for the manage_options capability is used:

current_user_can('manage_options')

Checking a capability has the advantage that it will still work even if someone is using a non-standard roles in their WordPress installation.