How to fix the intentionally vulnerable plugin

If you have not reviewed the intentionally vulnerable plugin, then I suggest you do so before reading on.

The intentionally vulnerable plugin demonstrates a range of security vulnerabilities that are commonly seen in WordPress plugins. In this post I aim to go through each vulnerability giving a little background on why it is dangerous and how it should be fixed.

SQL injection

This is a rather famous vulnerability that can be exploited by an attacker to modify database queries. This can allow the attacker to read sensitive information from the database, modify data stored in the database, and possibly even execute commands on database server, among other things. The basic idea is that if untrusted user input is included directly in a query then maliciously formatted input can be used to alter the query’s syntax.

The first SQL injection vulnerability is caused by the incorrect usage of the wpdb::prepare() method on line 42:

$wpdb->query( $wpdb->prepare( "INSERT INTO login_audit (login, pass, ip, time) VALUES ('$login', '$pass', '$ip', '$time')" ) );

When using the prepare() method placeholders, such as %s and %d, should be used in the query string which is the first argument of the method — the method is not magic, it cannot parse the query to determine what was user input! The variables are then passed in as separate arguments so that they can be escaped properly before being included in the query. This rather common error has been made more obvious since the release of WordPress 3.5 which required more than one argument to be passed to the method.

The correct usage, and the fix, is:

$wpdb->query( $wpdb->prepare( "INSERT INTO login_audit (login, pass, ip, time) VALUES (%s, %s, %s, %s)", $login, $pass, $ip, $time ) );

The other two SQL injection vulnerabilities, on lines 102 and 127, are caused by incorrectly escaping user input that is not used in a quoted context of an SQL query. The esc_sql() function can only be safely used to escape input that will be used within quotes. However, the two queries are using input as a numeric argument that is not enclosed within quotes:

$log = $wpdb->get_row( "SELECT * FROM login_audit WHERE ID = " . esc_sql( $id ), ARRAY_A );
// ... and ...
$wpdb->query( "DELETE FROM login_audit WHERE ID = " . esc_sql( $_POST['id'] ) );

To fix this you should use the prepare() method with a %d placeholder:

$log = $wpdb->get_row( $wpdb->prepare( "SELECT * FROM login_audit WHERE ID = %d", $id ), ARRAY_A );
// ... and ...
$wpdb->query( $wpdb->prepare( "DELETE FROM login_audit WHERE ID = %d", $_POST['id'] ) );

It is also possible to sanitize numeric input by casting to int or using the absint() method.

Cross-site scripting

Cross-site scripting (XSS) is one of the most prevalent security vulnerabilities in web applications. XSS is also commonly found in WordPress plugins, so it features heavily in the intentionally vulnerable plugin. XSS is often classed as either “persistent” or “reflected”. An XSS vulnerability is persistent if the unsafe user input is stored in the database, and then output to other users in future requests. It is known as reflected if the unsafe user input is output immediately in response to a request containing the dangerous input. To exploit a reflected XSS vulnerability the attacker would have to convince a victim to click on their specially crafted link that triggers a malicious script. However, a persistent XSS vulnerability will allow the attacker to input their malicious script themselves and then wait for unsuspecting users to visit or click on benign looking links.

There are two instances of persistent XSS in the intentionally vulnerable plugin, in dvp_view_all_logs() and dvp_view_log(). Both of these functions output data straight from the database without escaping it. All of this data originated from the user in dvp_log_failed_login(). For example, an attacker could purposefully fail a login attempt using a password of <script>alert(1)</script>. The fix for this would be to use an appropriate escaping function, such as esc_html() or esc_attr() when outputting data from the database. Data can also be sanitized when being stored in the database using KSES, or strip_tags() when no HTML is expected, in addition to escaping on output.

There are also multiple examples of reflected XSS. On lines 104 and 115, the $id variable, which was originally $_GET['id'], is output without any escaping. Line 104 should be fixed with esc_html(), and line 115 should be fixed with esc_attr(). Another problem is the use of $_SERVER['PHP_SELF'] on line 116 to get the current URL. This is an extremely common problem that allows XSS since an attacker can control its value and include malicious HTML that will break out of the attribute and execute a script. This should be fixed by using a WordPress function, such as menu_page_url(), to get the correct URL.

Cross-site request forgery

Another extremely common vulnerability in web applications is the failure to prevent cross-site request forgeries (CSRF). This type of attack exploits a request handler that checks authorisation, but not intention. A malicious website is able to send a request to another site that the user is authenticated to. The vulnerable target site will accept this request since it is sent with valid authentication cookies. However, the user did not intend for the action to be performed. WordPress plugins can defend against this type of attack by using nonces. These are action dependent random strings that cannot be predicted by an attacker, but can be verified by WordPress. So, if a request does not include a valid nonce then it is rejected.

The intentionally vulnerable plugin looks as if it makes use of nonces to defend against CSRF, but the author has made a couple of mistakes that leave it exploitable. Firstly, the use of nonce generation and verification functions without passing the $action parameter is insecure. Lines 114 and 123 should be edited to add the use of an action. Note that this mistake would be caught when developing with WP_DEBUG enabled as this use of check_admin_referer() raises a notice.

The second mistake is more subtle. On line 137 the following check is made:

if (isset($_REQUEST['nonce']) && ! wp_verify_nonce($_REQUEST['nonce'], 'dvp_settings'))
    // ... failed nonce check

The problem with this check is that it requires the nonce request parameter to be set to ever evaluate to true. This means that an attacker can simply omit this parameter to bypass the nonce verification. This type of logic error means that it is highly recommended that you use check_admin_referer() instead of wp_verify_nonce() directly.

Missing capabilities checks

The lack of capabilities checks in the log deletion handler allows for a privilege escalation attack. Any logged in user is able to delete rows from the login_audit table since there are no privilege checks. Note that unauthenticated users cannot exploit this because the handler is not hooked into a _nopriv admin-post.php action. However, this is still a serious security failure. So, always remember to use the appropriate current_user_can() capabilities checks on your admin pages and privileged request handlers.

Failure to exit after a redirect

When a script redirects the user to another page execution will actually continue on the server side until it exits. This means that it is almost always desirable to make a call to exit or die() when making a redirect. The failure to do this on line 140 means that the capabilities check and CSRF defence (even if it didn’t have a logic error) are rendered useless as a forged request or request made by a low privilege user will actually cause the settings to be updated even though the conditional triggered a redirect. (Note that using check_admin_referer() would have also nullified the problem for a forged request since it calls die() on failure.) The other locations where redirects are used are not vulnerable in the same way because they are called at the end of script execution, but it would be good practice to add exits there as well.

This vulnerability is exacerbated by the fact that the plugin has a ridiculous update loop that would allow a successful attacker to update any option in the database. Instead a plugin should know which options it is in charge of and only modify those whitelisted options.

Open redirect

On a redirect related note, there is an open redirect vulnerability in the deletion handler on line 130. In this plugin the intention is to return the user to the list of failed login attempts, so wp_safe_redirect() should have been used instead of plain old wp_redirect(). However, in this situation it would be even better to redirect to a hardcoded URL using menu_page_url() or admin_url() and then remove the ability to specify the redirect through the request parameters.

IP forgery

The dvp_get_ip() function can be tricked into logging an incorrect IP address since the X-Forwarded-For header is user controllable. This is bad news for a variety of reasons. For security logging purposes it degrades the utility of the audit trail as an attacker pretends to be coming from a legitimate address. If it were used for securing access then it is trivially by-passable. In the case of this plugin this can also lead to SQL injection and persistent XSS.

Conclusion

There are many things that plugin authors can do wrong! The number one rule to remember is that the user is not to be trusted. Always validate and escape anything that could be controlled by a user.

I hope that participants found this to be a useful exercise and that this review post has helped your understanding of plugin security.

#security

Review an intentionally vulnerable plugin

Imagine that another plugin author has asked you to look at a plugin that is currently in development to check for security flaws and help them fix any that are present. Would you know what to look for and how to fix the problems? Well, a fun challenge has arrived that will test, and hopefully improve, your knowledge in this crucial area of plugin development. I have developed a small, bug ridden plugin that requires a rigorous security review and suggestions for fixes.

The code is available from https://gist.github.com/joncave/5348689.

This is an incomplete plugin that aims to log any failed login attempts. Unfortunately, it actually harms the security of a site rather than enhancing it. All of the interesting parts are in vulnerable.php, so you should focus your review there. Please remember not to run this plugin on any server that is accessible to the internet!

If you spot a vulnerability whilst reviewing the code then make a note of the problem, where it’s located and what the problem is. Then come up with a patch that would solve the problem. It might also be beneficial to create a request that would demonstrate the vulnerability which can then be used to test your fix. I hope that this process will help you understand more about vulnerabilities, what sorts of things to look for when reviewing your own code, how to go about coding securely, and how to fix any problems in your own plugins if a flaw is found.

If you would like individual feedback on your finding and solutions, and to provide me with some information on which bugs people found and fixed, you can submit them via this survey. Please refrain from posting any spoilers in the comments for now.

In a week or so I will write another post detailing each of the vulnerabilities present in the code and how to fix them.

Bonus challenge: with access to a subscriber level account can you find any ways of extracting the data from an option named secret_option?

#security

Rewrite endpoints API

During the development of WordPress 3.4 I have spent some time working on improving the rewrite API. One of the tickets this involved was #16303: “Improve documentation and usability of WP_Rewrite Endpoint support”. Endpoints are a really cool feature of the rewrite API, but unfortunately also little known and misunderstood. So, with this post my aim is to get more plugin developers to read and understand the new and improved endpoint documentation.

What are endpoints?

Using endpoints allows you to easily create rewrite rules to catch the normal WordPress URLs, but with a little extra at the end. For example, you could use an endpoint to match all post URLs followed by “gallery” and display all of the images used in a post, e.g. http://example.com/my-fantastic-post/gallery/.

A simple case like this is relatively easy to achieve with your own custom rewrite rules. However, the power of endpoints shines for more complex situations. What if you wanted to recognise URLs for posts and pages ending with “gallery”? What if you wanted to be able to catch multiple different archive URLs, e.g. day, month, year and category archives, with “xml” appended in order to output an XML representation of the archive? For these situations endpoints are very useful as they allow you to add a string to the end of multiple rewrite structures with a single function call.

How to use them

There is one function for interacting with endpoints: add_rewrite_endpoint(). It takes two parameters $name and $places.

$name is a string and is, wait for it… the name of the endpoint. $name is what is used in the URL and is the name of the query variable that the endpoint URL will be rewritten to. For example, an endpoint named “print” added to post permalinks would use a URL like http://example.com/my-awesome-post/print/.

$places is an integer value which represents the locations (places) to which the endpoint will be added, e.g. posts, pages or year achives. To understand $places you need to learn about the endpoint mask constants.

In wp-includes/rewrite.php (browse wp-includes/rewrite.php on Trac) a number of constants are defined all with names beginning with “EP_”:

define('EP_NONE', 0);        // 0000000000000
define('EP_PERMALINK', 1);   // 0000000000001
define('EP_ATTACHMENT', 2);  // 0000000000010
define('EP_DATE', 4);        // 0000000000100
define('EP_YEAR', 8);        // 0000000001000
// ...
define('EP_PAGES', 4096);    // 1000000000000
define('EP_ALL', 8191);      // 1111111111111

These are the endpoint masks which describe sets of URLs; post permalinks are described by EP_PERMALINK, year archives are EP_YEAR, etc. They should be thought of in terms of their binary values (see the comment I’ve added to the end of each line). Every EP_* mask, except for EP_ALL, is a different power of two and so has a different bit set to one. This allows us to build up combinations of endpoint masks by using the bitwise OR operator:

// all posts or attachments
EP_PERMALINK | EP_ATTACHMENT // 0000000000011

// all full dates (yyyy/mm/dd), years or pages
EP_DATE | EP_YEAR | EP_PAGES // 1000000001100

$places should also be thought of as a binary number. It should be set to one of the EP_* constants or a combination of them using the bitwise OR operator. If we wanted to add our endpoint to all post permalinks we would use EP_PERMALINK. For both posts and pages: EP_PERMALINK | EP_PAGES. For posts, pages, and categories: EP_PERMALINK | EP_PAGES | EP_CATEGORIES. There is also a special value to add an endpoint to all URLs that support endpoints: EP_ALL.

NB: The values of the EP_* constants are not guaranteed to stay the same which is why you must say $places = EP_PERMALINK and not $places = 2. This is particularly important for EP_ALL which will change every time a new endpoint mask is added.

It’s time to put this information into practise. The running example will be a plugin that adds JSON representations of our content using a new rewrite endpoint called “json”. So, the goal is to get URLs such as http://example.com/about/json/ to return a JSON response that gives information about the “about” page. To add the “json” endpoint to post and page rewrite structures:

add_rewrite_endpoint( 'json', EP_PERMALINK | EP_PAGES );

This is called in a function hooked into the init action:

function makeplugins_add_json_endpoint() {
    add_rewrite_endpoint( 'json', EP_PERMALINK | EP_PAGES );
}
add_action( 'init', 'makeplugins_add_json_endpoint' );

Now we want to act on requests for JSON content. This is done by hooking into template_redirect. We want to detect appropriate requests and include our custom template for serving up posts and pages in JSON format:

function makeplugins_json_template_redirect() {
    global $wp_query;

    // if this is not a request for json or a singular object then bail
    if ( ! isset( $wp_query->query_vars['json'] ) || ! is_singular() )
        return;

    // include custom template
    include dirname( __FILE__ ) . '/json-template.php';
    exit;
}
add_action( 'template_redirect', 'makeplugins_json_template_redirect' );

And we’re done. For a full example plugin see https://gist.github.com/2891111.

How do they work?

The best way to understand how anything works is to take a look at the source, so let’s do that. Endpoints are added with the add_rewrite_endpoint() function in wp-includes/rewrite.php:

/**
 * Add an endpoint, like /trackback/.
 *
 * Adding an endpoint creates extra rewrite rules for each of the matching
 * places specified by the provided bitmask. For example:
 *
 * <code>
 * add_rewrite_endpoint( 'json', EP_PERMALINK | EP_PAGES );
 * </code>
 *
 * will add a new rewrite rule ending with "json(/(.*))?/?$" for every permastruct
 * that describes a permalink (post) or page. This is rewritten to "json=$match"
 * where $match is the part of the URL matched by the endpoint regex (e.g. "foo" in
 * "/json/foo/").
 *
 * A new query var with the same name as the endpoint will also be created.
 *
 * When specifying $places ensure that you are using the EP_* constants (or a
 * combination of them using the bitwise OR operator) as their values are not
 * guaranteed to remain static (especially EP_ALL).
 *
 * Be sure to flush the rewrite rules - flush_rewrite_rules() - when your plugin gets
 * activated and deactivated.
 *
 * @since 2.1.0
 * @see WP_Rewrite::add_endpoint()
 * @global object $wp_rewrite
 *
 * @param string $name Name of the endpoint.
 * @param int $places Endpoint mask describing the places the endpoint should be added.
 */
function add_rewrite_endpoint( $name, $places ) {
      global $wp_rewrite;
      $wp_rewrite->add_endpoint( $name, $places );
}

So this is just a wrapper for the add_endpoint() method of the WP_Rewrite class. Although the (excellent!) documentation gives us some clues as to what it does we’ll have to dig deeper to find the how:

/**
 * Add an endpoint, like /trackback/.
 *
 * See {@link add_rewrite_endpoint()} for full documentation.
 *
 * @see add_rewrite_endpoint()
 * @since 2.1.0
 * @access public
 * @uses WP::add_query_var()
 *
 * @param string $name Name of the endpoint.
 * @param int $places Endpoint mask describing the places the endpoint should be added.
 */
function add_endpoint($name, $places) {
    global $wp;
    $this->endpoints[] = array ( $places, $name );
    $wp->add_query_var($name);
}

Another very short and simple function. All it does is append the two parameters passed to it to the private $endpoints property of the WP_Rewrite class and also add a new query variable using WP::add_query_var().

Okay, so that’s still not useful for a full understanding of endpoints. All we know is that the arguments you pass to add_rewrite_endpoint() are stored in a private array of the $wp_rewrite global. To find out more we’ll have to search wp-includes/rewrite.php for “>endpoints” (i.e. code accessing the WP_Rewrite::$endpoints property). There are only three references to this: WP_Rewrite::add_endpoint() we have seen, WP_Rewrite::init() is boring (initialising the array), and the third is WP_Rewrite::generate_rewrite_rules():

$ep_query_append = array ();
foreach ( (array) $this->endpoints as $endpoint) {
    //match everything after the endpoint name, but allow for nothing to appear there
    $epmatch = $endpoint[1] . '(/(.*))?/?$';
    //this will be appended on to the rest of the query for each dir
    $epquery = '&' . $endpoint[1] . '=';
    $ep_query_append[$epmatch] = array ( $endpoint[0], $epquery );
}

// ... a lot of code removed ...

foreach ( (array) $ep_query_append as $regex => $ep) {
    //add the endpoints on if the mask fits
    if ( $ep[0] & $ep_mask || $ep[0] & $ep_mask_specific )
        $rewrite[$match . $regex] = $index . '?' . $query . $ep[1] . $this->preg_index($num_toks + 2);
}

In the code above the first foreach is looping through the defined endpoints and building a new array called $ep_query_append. This new array uses regular expressions that match a specific endpoint as keys and the values are the endpoint $places and $epquery which is a partial query string to append to a full query. So, for our JSON endpoint example we would get:

$ep_query_append[ 'json(/(.*))?/?$' ] = array( EP_PERMALINK | EP_PAGES, '&json=' );

The second loop generates the final rewrite rules for our endpoint. It loops through $ep_query_append checking if the current permastructure being generated has an endpoint mask, $ep_mask, that matches any of the endpoints. If the bitwise AND produces a non-zero value then there’s a match and the endpoint rewrite rules should be added to this permastructure.

For our JSON example, if WP_Rewrite::generate_rewrites_rules() has been called for the posts permalink structure then $ep_mask = EP_PERMALINK and $ep[0] = EP_PERMALINK | EP_PAGES. The bitwise AND of these values produces 1, therefore a new entry is added to $rewrite. Assuming that the post permalink structure is “/%postname%/” it would look something like:

$rewrite[ '([^/]+)/json(/(.*))?/?$' ] = 'index.php?name=$1&json=$3'

This is the final rewrite rule for our JSON endpoint applied to post permalinks. It matches a request for “post-slug/json/” and sets up the appropriate query variables “name” and “json”. Our template_redirect hook now picks this up and produces the required response.

And you made it to the end, phew! Time for a drink…

Conclusion

I hope that after all of that you understand how to use endpoints and how they work. If you have any questions please don’t hesitate to ask them in the comments. Always remember that the best way to understand a function is to look at the source and follow its execution.