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

Me, elsewhere

Miscellaneous public code

A PHP API client for Reddit

I don't tweet much

XMPP chat
(Pidgin, Miranda, Swift, etc.)

Perfect is the enemy of good enough.

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.

PHP logo by Colin Viebrock (http://php.net/logos) CC BY-SA 4.0, via Wikimedia Commons

Recent articles

📰 Caveat with Vantec SATA/IDE to USB 2.0 Adapter and Macrium software

📰 Jay Niffley, Man of Mystery

📰 Compiling Doxygen on FreeBSD without LaTeX and Ghostscript

📰 Introducing Snuze, a PHP client for the Reddit API

📰 jisusaiche: Java's installer telemetry

📰 BIND client log error "query_find: query_getdb failed"

📰 Resolving "The lang/perl5.24 port has been deleted: Has expired" portmaster error

📰 Armagaddon2 interim fix for Firefox 56 and other old versions

📰 Strange DNS queries: qname "miep", qtype ANY

📰 Undeliverable as addressed: A massive broken spam campaign?

📰 Using WITH_META_MODE and ccache for FreeBSD build boosts

📰 Resolving subversion error E000013: Unable to create pristine install stream

📰 Enhancements to SmokePing's AnotherDNS probe

📰 Generating vanity DNSSEC key tags

📰 DDoS involving forged packets from

▲ Back to top | Permalink to this page