(UPDATE: please note that official 3.6.4 version has just been published, so if you want the bug to be patched, simple update. If you want to learn about the bug itself, continue reading)

I personally don’t have the habit of using this kind of plugins (I harden Wordpress by hand using much simplier techniques with the less possible extra code involved), but since a third party developed Wordpress web has come into my hands and now I have to manage it, I decided to learn about this kind of things instead of just deleting them (the less Wordpress plugins you remove, the less the odds of screwing anything up).

So, I find Wordpress Better Security Plugin installed in that website, and trying to gather more information I ended up coming across this security bulletin explaining some security concerns.

First things first, I ask its coder @ChrisWiegman about the different concerns addressed in the bulletin. He tells me that he considers the bug definitions invalid (we didn’t elaborate on that any further), and anyway he’s already working on v4 series of the plugin which is being coded from scratch.

I fully get his point and I totally respect it (some so called concerns are actually misconceptions about plugin usage, indeed), but one of the concerns listed in bulletins is nothing more and nothing less that a persistent XSS which could allow a Wordpress site admin inject code into the browser of any other admin of the same site, and I don’t consider this as invalid so I prefer to patch the production version of BWPS 3.6.3 myself ASAP. BTW, I’m quite sure some people would be interested too, so let’s get into the PHP code in order to reach the offending pieces.

DISCLAIMER: I’m a digital bodyguard / forensic / computer crime investigator, not a PHP/MySQL/JavaScript pro. I just touch some code from time to time but it’s not my main task so forgive me should I state something unprecisely or contribute with ugly pieces of code.

So, let’s start the dirty work, but we must understand the details before. Quoting the text of the bulletin regarding the XSS:

2. Persisted XSS:
=================
The Better WP Security also suffers from stored XSS. In the premium box, by entering a valid key user can upgrade plugin to premium. Escaping is applied on the inputs.
However, it isn't sufficient to prevent occurring XSS. The key is stored in database within wp_options table. better-wp-security_licensekey holds the input's value. The plugin reads the key from database, then the key is shown to user and it's triggering the vulnerability. So, submiting

"><script>alert(1)</script>

would be a dimonstration.

Well, the heading image of this post demonstrates that this statement is absolutely true. After pasting "><script>alert(1)</script> into the license key input box and clicking validation button, the XSS detonates on every single WPBS plugin page load. This includes tab clicking.

This security concern can be discussed in many ways, but IMHO there are three key points here:

• Data should be sanitized on every write to the database
  (necessary)
• Data should be sanitized on every read from the database
  (recommended)
• The writing of user provided content to the database could be limited to valid license key storage only.
  (optional, unless we need to store invalid keys for any reason, in which case we can’t do that)

Now, let’s edit lib/bit51/foolic_validation_v1.1.php and discuss the suggested key points:

WRITES

At line 272 we can notice that the plugin stores the “license” string provided by the user directly into the WP options table.

Here we could focus on sanitizing the $license variable after getting its value from the $_REQUEST and before writing it with update_option. AFAIK, the update_option function provides protection against SQL injection (please someone correct me if I’m wrong) so we don’t have to worry about anything should the data stays in SQL context. But in this particular case, that data will end up into the HTML context almost instantly (plugin always renders the current key stored in database, even when its not valid), so we need more filtering.

changing the line

$license = get_option($this->plugin_slug . '_licensekey');

for

$license = htmlspecialchars(get_option($this->plugin_slug . '_licensekey'));

would prevent HTML tags to be stored in a way that could become real HTML code later.

READS

Applying the second key point, let’s imagine that we don’t just want to transform the user input. If we are paranoid enough, we could imagine that an attacker could modify the wp options table in the database using another attack and then he could spread code injections using unproperly sanitized HTML outputs. So, if we want to totally fix the XSS, we are also forced to sanitize data after being read from database, and before sending it to html output.

Here’s the code that shows the HTML output after retrieving the key from the DB:

We see a call to get_option at the line 42 of foolic_validation_v1.1.php That call assigns the value obtained from the database to the $license variable, which is later used in a concatenation of several variables to compose a big string of HTML output data (not fully seen in the capture).

again, a simple stripping of HTML special chars can eliminate the threat. Changing:

$license = $_REQUEST['license'];

for

$license = htmlspecialchars($_REQUEST['license']);

should do the trick.

After applying point two, we go to the WPBS plugin settings page and check that even the already “poisoned” config value stored in database becomes no threat once rendered in HTML context:

yay! ☺

LIMITATION OF WRITES TO VALID KEYS ONLY

The third key point.

Well, this point is unnecesary, and could interfere with the behaviour desired and designed by the coder. I certainly do not know if retaining every single license key input, even if invalid, is of any use.

But for the sake of security and speed, trying to avoid unnecessary database writes totally makes sense. The plugin is able to check the validity of a key using ajax (see ajax_validate_license at line 265), so writing to wordpress options table of the database could be limited just to the specific cases in which ajax query confirms that the provided input is a valid key.

Even more, that would provide a second layer of protection against badly formatted writes to the database, since a string with “<” never will be a valid license key, for example.

DOWNLOAD

Get the patched plugin from here

BOTTOM LINE

The article only wants to share the solutions to a problem I had to solve, hoping that many others would like to get help to solve it too. Consider it a small practical exercise of basic web security and web app troubleshooting.

In no way this article aims to be bad critizism against @ChrisWiegman nor @iThemes, even if Chris decided not to patch such a bug. I’m not a PRO coder and I’d never develop a plugin like WPBSP in a better way than he does. I know that people like Chris write lots of difficult code on a daily basis and nobody is 100% error free (anyway, nobody will beat the guiness record of bugs achieved by Apple’s TLS/SSL imeplementation LOL).

Despite the fact that I’m not a big fan of automatic security solutions/plugins/software/whatever for CMS, this issue provided me the opportunity to take a look at interesting code and appreciate the powerful features of WPBS; I think it’s a very good choice for people with lower level of tech skills and higher levels of paranoia. I will follow up the work of the guys at @iThemes from now on and I will consider testing the all new v4 series of the plugin in some WP sites once it becomes a production version.

Feedback and corrections welcome.

@AlisterWhitehat

PS: See Chris’s response to this article: