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


Me, elsewhere

GitHub
parseword
Miscellaneous public code

Twitter
@parseword
I don't tweet much

XMPP chat
xmpp@shaunc.com
(Pidgin, Miranda, Swift, etc.)

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

📰 Generating vanity DNSSEC key tags

📰 DDoS involving forged packets from 23.225.141.70

📰 Website integrity monitoring through version control

📰 SpamAssassin 3.4.2 fixes security problems, adds HashBL and phishing plugins

📰 Bug or turf war? ICQ via Pidgin now fails with "startOSCARSession: Request Timeout"

📰 🎂

📰 SFSQuery, a PHP class to query the StopForumSpam API and DNSBL

📰 Resolving portmaster error "pkg-static: automake-1.16.1 conflicts with automake-wrapper-20131203"

📰 Resolving LibreNMS error "RuntimeException: The only supported ciphers are AES-128-CBC and AES-256-CBC with the correct key lengths"

📰 1.1.1.1: Fast, but not so accurate (yet)

📰 autodiscover.xml as an Indicator of Attack

📰 Blocking Facebook's Tracking and Surveillance: A Comprehensive Approach

📰 Let's Encrypt Readies for Certificate Transparency with Embedded SCTs

📰 Evaluating DNSBL Effectiveness with Postfix Logs

📰 Resolving subversion error E145001: Node has unexpectedly changed kind

▲ Back to top | Permalink to this page