SoftwareSecurity2014/Group 8/Code Scanning

ASVS Verification requirements

In this project, the fifth ASVS verification requirement, Input Validation, will be highlighted. The set of requirements to be checked at level 1B are:

• V5.1 - Verify that the runtime environment is not susceptible to buffer overflows, or that security controls prevent buffer overflows.
• V5.2 - Verify that a positive validation pattern is defined and applied to all input.

Fortify

We ran the Fortify SCA on our Wordpress (3.8.1) code base and obtained the following results.

Of these critical vulnerabilities, the ones that are relevant for verification requirement V5 are the following: Command Injection, Cross Site Scripting (Persistent and Reflected), Dangerous File Inclusion, Path Manipulation and SQL Injection. (Erik: Isn't path manipulation also an input validation issue? as it involves (lack of) validation of input that is used in paths?)

It is not feasible to check all vulnerabilities manually. Therefore we will just look at the critical vulnerabilities reported by Fortify. We will show an example for each of the categories that are relevant to us.

Command injection

For the command injection category, Fortify found 54 critical vulnerabilities in Wordpress:

By inspecting the vulnerability reported at class-snoopy.php, we find that the function exec() is called in the following way:

exec($this->curl_path." -k -D \"$headerfile\"".$cmdline_params." \"".escapeshellcmd($URI)."\"",$results,$return);

Fortify reports this as a vulnerability, since this call can cause the program to execute malicious commands on behalf of an attacker. The parameters of this function are considered untrusted. (Erik: which parameters? Presumbaly not $URI because that is escaped? Or is that not recognised/regarded as safe sanitation by Fortify? (Erik: Is it clear to you whether this a false/true pos?)

Another example is the reported vulnerability of upgrade.php at line 1039. The function unserialize() is called with untrusted parameters.

if ( !@unserialize( $value ) )

However, Fortify shows that the data enters the application via the function mysql_query() at line 1213. Analysing this function shows that filters are applied to the actual query before returning it. This vulnerability is therefore considered a false positive.

Cross-Site Scripting: Persistent

Fortify finds 414 critical persistent cross-site scripting vulnerabilities. Examination of these vulnerabilities shows that Fortify flags all pieces of code where unvalidated data is sent to a web browser, since this could result in the web browser executing malicious code.

For us (regarding the verification requirement), it is most important to look at code where user input is handled, since a lot of the vulnerabilities reported by Fortify are in places that a user can never reach, or are only accessible by users with higher privileges, such as a site administrator.

An example of a vulnerability is found in user-edit.php which contains the following block of code:

<?php
	$new_email = get_option( $current_user->ID . '_new_email' );
	if ( $new_email && $new_email['newemail'] != $current_user->user_email && $profileuser->ID == $current_user->ID ) : ?>
	<div class="updated inline">
	<p><?php 
               printf( __('There is a pending change of your e-mail to <code>%1$s</code>. <a href="%2$s">Cancel</a>'), 
               $new_email['newemail'], esc_url( self_admin_url( 'profile.php?dismiss=' . $current_user->ID . '_new_email' ) ) ); 
        ?></p>
	</div>
	<?php endif; ?>

Line 409 contains the alleged vulnerability:

<p><?php 
       printf( __('There is a pending change of your e-mail to <code>%1$s</code>. <a href="%2$s">Cancel</a>'), 
       $new_email['newemail'], esc_url( self_admin_url( 'profile.php?dismiss=' . $current_user->ID . '_new_email' ) ) ); 
?></p>

This is something a user sees when trying to edit their profile. The problem here would be that the new email address is sent to the web browser, while this is unvalidated data. This is a case of real user-input. This is a false positive, however as the newmail value does not come from the database, and thus cannot contain persistent code to execute on the users side.

This seems to be the pattern in the cross-site scripting vulnerabilities that Fortify finds. The input is either sanitized, enclosed in html tags that ensure that it cannot be executed or is no real user input at all. Fortify reports a lot of false positives.

We cannot be sure that there is not a single vulnerability that really is a persistent cross-site scripting vulnerability, since we did not go through all 414 vulnerabilities by hand, but by looking at examples (one of which was shown above), we conclude that it is unlikely that Fortify really found a persistent cross-site scripting vulnerability.

Cross-Site Scripting: Reflected

Fortify identified no less than 1019 critical items in the cross-site scripting reflected section. They mostly consist of false positives as Fortify didn't correctly identify the custom escape functions that Wordpress has implemented. The most common ones are the esc_* functions as esc_html(), esc_attr() and esc_url(). (Erik: This raises an interesting questions, namely whether it is/should/could be possible to tune Fortify by telling it about such custom sanitisation functions.) For legacy code, also the function attribute_escape is still available. They often use a whitelist implemented using a regular expression to remove all the forbidden characters. For example, esc_url() uses:

$url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\\x80-\\xff]|i', '', $url);

During sing-up however, there is also some escaping done without the esc_* functions, but it uses a regular expression of its own which also results in a false positive:

$newblogname = isset($_GET['new']) ? strtolower(preg_replace('/^-|-$|[^-a-zA-Z0-9]/', '', $_GET['new'])) : null;

For numeric values the function number_format_i18n() is used or a sprinf() using the formatter %d. And therefore they are also false positives. For example:

$errmsg = sprintf(__('The plugin generated %d characters of <strong>unexpected output</strong> during activation. 
If you notice “headers already sent” messages, problems with syndication feeds or other issues, 
try deactivating or removing this plugin.'), $_GET['charsout']);

In 539 cases add_query_arg() is marked as unsafe but is a false positive. This function creates an url to another page. It therefore uses the current url which is seen as input by Fortify and is later on echoed as a link. In the cases that we have checked, the url is also escaped using esc_url().

Some code which is run during setup contains a strange piece of code which overwrites the function get_bloginfo() with wp_guess_url(). Wp_guess_url() guesses the url of the blog and therefore, again uses the current url retrieved with $_SERVER['REQUEST_URI'].

// Fetch or generate keys and salts.
$no_api = isset( $_POST['noapi'] );
if ( ! $no_api ) {
	require_once( ABSPATH . WPINC . '/class-http.php' );
	require_once( ABSPATH . WPINC . '/http.php' );
	/**#@+
	 * @ignore
	 */
	function get_bloginfo() {
		return wp_guess_url();
	}
	/**#@-*/
	$secret_keys = wp_remote_get( 'https://api.wordpress.org/secret-key/1.1/salt/' );
}

There are more functions to create url's which beside REQUEST_URL also PHP_SELF and HTTP_X_ORIGINAL_URL used depending on the executing web server.

Only in cases of an Ajax request, there are some examples of no escaping or any checks on the input. So hopefully this is done in the JavaScript (which we didn't check, so far).

echo json_encode( array(
	'success' => false,
	'data'    => array(
		'message'  => __( 'The uploaded file is not a valid image. Please try again.' ),
		'filename' => $_FILES['async-upload']['name'],
	)
) );

Although Fortify found a lot of critical problems, we consider them all false positives. Fortify can't check if escaping is correctly implemented, but so far Wordpress looks to be doing a good thing. Only the Ajax requests look suspicious as there are no checks on the server side, which is, in general, a real bad thing. You should never trust an user's input.

Dangerous File Inclusions

Fortify reports 19 critical Dangerous File Inclusions. However, in all these reported vulnerabilities the included file is checked first, in most cases by a call to the validate_file() function. The first two reported Dangerous File Inclusion can be found in the lines 242-245 of admin.php in the following code:

if ( file_exists(WPMU_PLUGIN_DIR . "/$plugin_page") )
	include(WPMU_PLUGIN_DIR . "/$plugin_page");
else
	include(WP_PLUGIN_DIR . "/$plugin_page");

Which would be dangerous if $plugin_page would be unchecked user input. However, on line 219-220 this variable is checked with the validate_file() function as follows:

if ( validate_file($plugin_page) )
	wp_die(__('Invalid plugin page'));

In class-phpmailer.php the following line can be found:

require_once $this->PluginDir . 'class-smtp.php';

Where $this->PluginDir is a public variable in the PHPMailer class. So if it would be possible to change this variable somewhere, this would be a vulnerability, but as far as we found, this is not possible.

Fortify also reports three vulnerabilities in plugin.php and plugins.php, which are similar. In plugin.php this is on line 540 in the code:

include_once(WP_PLUGIN_DIR . '/' . $plugin);

However, this $plugin is checked a few lines before with the validate_plugin() function, which in turn calls the validate_file() function.

In wp_admin/update.php, Fortify reports a vulnerability on line 87 in:

include(WP_PLUGIN_DIR . '/' . $plugin);

However, in line 71 is checked whether the page was referred to by another admin page with the check_admin_refer() function.

And the reported vulnerabilities in wp-settings.php are no vulnerabilities because they do not rely on user input.

Path Manipulation

Fortify reports 28 Path Manipulation vulnerabilities. An example of these reports are those found in file.php like on line 319:

@ chmod( $new_file, $perms );

However, this $new_file is a generated name which does not originate from user input.

Another Path Manipulation issue is found on line 82 in ms-files.php:

readfile( $file );

But this does not lead to a vulnerability because in the user input the .. are removed in line 26:

$file = rtrim( BLOGUPLOADDIR, '/' ) . '/' . str_replace( '..', '', $_GET[ 'file' ] );

SQL Injection

Fortify reports 16 critical SQL Injection vulnerabilities. All of these vulnerabilities amount to the same problem: Fortify reports that wp-db.php invokes a SQL query that is built using input coming from an untrusted source.

The danger of this is that an attacker could modify the statement's meaning or execute arbitrary SQL commands. This piece of code is called from 16 different places, hence the 16 vulnerabilities.

According to Fortify, the following line contains the vulnerability:

 $this->result = @mysql_query( $query, $this->dbh ); 

The problem is that $query is built using input coming from an untrusted source. However, the function query() that contains this code, applies a filter to the query that is passed along to make sure that is safe to execute it later on.

 $query = apply_filters( 'query', $query ); 

The apply_filters() function calls the callback functions that are hooked to a certain tag, which in this case is 'query'. All of these functions are then applied to the $query variable. The contents of the $query variable will be sanitized, after which the variable may be used in the application, which in this case results in a query on the database to be executed. Because of the fact that all of the queries that are executed on the database are routed via the above function, this means that all of the queries are actually sanitized properly. We consider the SQL Injection vulnerabilities that Fortify finds as false positives.

Other vulnerabilities found by Fortify

RATS

Running RATS on high severity (level 1) over the Wordpress 3.8.1 code base resulted in the following summarized results:

fopen - severity: high, 57 occurences

Argument 1 to this function call should be checked to ensure that it does not come from an untrusted source without first verifying that it contains nothing dangerous.

When looking at the results of RATS we see that a lot of these files are accessible by the administrator (or other privileged users) only. When looking through some of the reported files, we saw that the file paths that are being opened do not depend on input from users.

gzopen - severity: high, 2 occurences in /wp-admin/includes/class-pclzip.php

Argument 1 to this function call should be checked to ensure that it does not come from an untrusted source without first verifying that it contains nothing dangerous.

Both occurrences of the use of this function look like the following:

$v_gzip_temp_name = PCLZIP_TEMPORARY_DIR.uniqid('pclzip-').'.gz';
if (($v_file_compressed = @gzopen($v_gzip_temp_name, "wb")) == 0) {
  // some operations on the newly opened file
}

We see that the gzopen function creates an unique temporary file using the uniqid() function. PCLZIP_TEMPORARY_DIR is defined as follows:

if (!defined('PCLZIP_TEMPORARY_DIR')) {
  define( 'PCLZIP_TEMPORARY_DIR', '' );
}

Any new temporary file that is created will thus be placed in the Wordpress root folder.

mail - severity: high, 2 occurences in /wp-includes/class-phpmailer.php

Arguments 1, 2, 4 and 5 of this function may be passed to an external program. (Usually sendmail). Under Windows, they will be passed to a remote email server. If these values are derived from user input, make sure they are properly formatted and contain no unexpected characters or extra data.

The code is called from the following function definition:

/**
   * Calls actual mail() function, but in a safe_mode aware fashion
   * Also, unless sendmail_path points to sendmail (or something that
   * claims to be sendmail), don't pass params (not a perfect fix,
   * but it will do)
   * @param string $to To
   * @param string $subject Subject
   * @param string $body Message Body
   * @param string $header Additional Header(s)
   * @param string $params Params
   * @access private
   * @return bool
   */
  private function mail_passthru($to, $subject, $body, $header, $params) {
    if ( ini_get('safe_mode') || !($this->UseSendmailOptions) ) {
        $rt = @mail($to, $this->EncodeHeader($this->SecureHeader($subject)), $body, $header);
    } else {
        $rt = @mail($to, $this->EncodeHeader($this->SecureHeader($subject)), $body, $header, $params);
    }
    return $rt;
}

We see that the second parameter is actually being sanitized in the SecureHeader() function:

  /**
   * Strips newlines to prevent header injection.
   * @access public
   * @param string $str String
   * @return string
   */
  public function SecureHeader($str) {
    return trim(str_replace(array("\r", "\n"), '', $str));
  }

The code that calls the mail_passthru() function takes case of sanitizing the the $to parameter. The $body and $header parameters are constructed in the public CreateHeader() and CreateBody() functions respectively, which also use the SecureHeader() to sanitize the input. $params is set as follows:

    if (empty($this->Sender)) {
      $params = " ";
    } else {
      $params = sprintf("-f%s", $this->Sender);
    }

There may be a slight chance that using the $params variable in this way introduces a vulnerability, as it is not sanitized in the above code. In this way, any code that creates a PHPMailer object and after that sets the $Sender is itself responsible for sanitizing before using it.

popen - severity: high, 2 occurences in /wp-includes/class-phpmailer.php

Argument 1 to this function call should be checked to ensure that it does not come from an untrusted source without first verifying that it contains nothing dangerous.

if(!@$mail = popen($sendmail, 'w')) {
   throw new phpmailerException($this->Lang('execute') . $this->Sendmail, self::STOP_CRITICAL);
}
fputs($mail, "To: " . $val . "\n");
fputs($mail, $header);
fputs($mail, $body);
$result = pclose($mail);

Just before this code, there is actually some sanitization on the $sendmail variable:

if ($this->Sender != '') {
   $sendmail = sprintf("%s -oi -f%s -t", escapeshellcmd($this->Sendmail), escapeshellarg($this->Sender));
} else {
   $sendmail = sprintf("%s -oi -t", escapeshellcmd($this->Sendmail));
}

The $this->Sendmail and $this->Sender are equal to the following in the default case (but may be set to a different value on the PHPMailer object, if needed):

public $Sendmail          = '/usr/sbin/sendmail';
public $Sender            = '';

We see that both of these parameters are actually sanitized by the escapeshellcmd() and the escapeshellarg functions. The first one should be used to protect against a shell being tricked into executing arbitrary commmands. The second should be used to supply a single safe argument to a shell command (which in this case is the $this->Sendmail command). Both of the arguments may be set on a PHPMailer object when an email is to be sent, and are thus susceptible to malicious user input (if that is not sanitized before being fed to the PHPMailer object, at least).

RATS also found 63 and 591 issues on the medium and low severity level, respectively. These issues consisted mostly of TOCTOU (Time of Check, Time of Use) vulnerabilities.

Conclusions

The ASVS states in V5.1 that we should check the application runtime environment to not be susceptible to buffer overflows. The code scanning tools were not helpful to assess this requirement, as they are not able to check the actual running application. (Erik: I can believe your conclusion that these tools are no good at scanning for buffer overflows, but not your motivation: there is no reason why a tool might not be able to spot (some) buffer overflows without checking the running application. Just grepping for gets will do this :-). I guess you mean that any buffer overflows would be outside the PHP code, in underlying web server/php interpreter, which is out of scope in this evaluation?) Furthermore, we didn't install nor run the application, so we didn't have a runtime environment during the assignment. Of course we are aware that a PHP web application that is deployed on a web server may actually be vulnerable to buffer overflows, but they do not exist in the PHP web application itself, rather they may exist in the PHP compiler

V5.2 of the ASVS states that we should check that a positive validation pattern is defined and applied to all input. To find out whether this is indeed the case, we ran Fortify and RATS over the code. Fortify returned a lot of results, so we decided to focus on the 'critical' vulnerabilities it found. RATS returned less results, but we nevertheless focused on the 'high severity' issues it found during the scan, also.

Fortify found a lot of false positives, cases where Fortify thought that untrusted user input was handled that was actually validated. Going through these some of these vulnerabilities showed us that a positive validation pattern was defined and applied to the input: queries were sanitized, certain characters in input were escaped, all HTML output that included user input was properly escaped, etc. However, we were not able to check that a positive validation pattern was defined and applied to all input, since Fortify reported too many possible vulnerabilities for us to check. In the code, there are some functions that are reused throughout the whole program and that indeed use a positive validation pattern. The Ajax code, however, doesn't always check the input and in some cases returns input without any sort of validation. Hopefully this is correctly validated on the client side in JavaScript.