$this = (new Soapbox())->shout(array_map('strtoupper', $opinions)); //Shaun's blog

Secure PHP file inclusion based on query string parameters

Posted November 25, 2017 by shaun

This week, the Plugin Vulnerabilities site posted about a local file inclusion exploit in MailChimp's WordPress plugin for WooCommerce. My thoughts about WordPress are generally unenthusiastic, so I don't spend much time looking at its vulnerabilities, but from the standpoint of a mail server operator I was interested in the MailChimp angle. As it turns out, there's nothing of impact to mail admins, though I was a little intrigued by MailChimp's fix.

The exploit, in a nutshell, is that one of the plugin's scripts accepted a template name via the query string and then called include() on a filename built from that variable:

<?php if(isset($_GET['error_notice']) 
  && file_exists(__DIR__.'/errors/'.$_GET['error_notice'].'.php')): ?>
    <?php include(__DIR__.'/errors/'.$_GET['error_notice'].'.php'); ?>
<?php endif; ?>

Direct use of the $_GET parameter opens the door to directory traversal and file inclusion. Here, there's at least a barrier to arbitrary file inclusion, in that the target file's name will have to end in .php. But if an attacker knows (or can guess or create) the location of a .php file outside of the document root, he can cause it to be executed.

The fix was to eschew inclusion entirely in favor of passing hardcoded error messages back to the WordPress templating system. This certainly addresses the bug, but it's a bit of a heavy hammer to swing, as file inclusion based on user input can be accomplished safely.

There are multiple approaches to securing the original code while retaining the benefits of isolated template files. The easiest is to use a simple array containing a whitelist of allowed filenames:

$include_whitelist = ['missing_api_key', 'invalid_user', 'other_error'];

if (!empty($_GET['error_notice']) && in_array($_GET['error_notice'], $include_whitelist)) {
    @include(__DIR__ . '/errors/' . $_GET['error_notice'] . '.php');

A more paranoid technique is to create a map of query string tokens to filenames, concealing the names of your templates altogether. This is a moot point for an open source project, where your filenames are public by necessity, but I think it's worth the effort in proprietary code. (See: Obscurity is a Valid Security Layer).

$include_map = [
    'missing_api_key' => 'err_api_key_noexist.php',
    'invalid_user'    => 'err_user_noexist.php',
    'other_error'     => 'err_generic.php',

if (!empty($_GET['error_notice']) && !empty($include_map[$_GET['error_notice']])) {
    @include(__DIR__ . '/errors/' . $include_map[$_GET['error_notice']]);

Either of these approaches will let you keep the flexibility of partitioned templates, while preventing any unintended file inclusions.

As an aside, I'm a bit surprised that MailChimp doesn't appear to have disclosed this vulnerability. It's not mentioned in the commit message and I don't see any discussion in the support forum or the official MailChimp site. For an enterprise plugin boasting 30,000 users, especially one specifically targeted at ecommerce operators, silence is a bad look.

Recent articles

📰 Evaluating DNSBL Effectiveness with Postfix Logs

📰 Russian/Ukrainian Referer Spam Campaign IPs

📰 Resolving subversion error E145001: Node has unexpectedly changed kind

📰 Installing PHP 7.2 with pthreads on CentOS 6

📰 LocalStorage kills another site, or: Working around Zap2it's new interface

📰 A new DNS geolocation service from PowerDNS

📰 Firefox's privacy.resistFingerprinting option reports a very old User-Agent (50.0)

📰 Undefined symbol "Py_InitModule4_64" while upgrading harfbuzz

📰 ipid.shat.net is back online for now

📰 Implementing a report-uri endpoint for Expect-CT (and other headers)

📰 A curious UDAP packet from DirecTV hardware

📰 Secure PHP file inclusion based on query string parameters

▲ Back to top | Permalink to this page