SoftwareSecurity2014/Group 5/Code Scanning

Fortify

We ran Fortify on the source code of SMF. This produced 7298 issues:

• 3148 Critical
• 2809 High
• 422 Medium
• 919 Low

6173 of these are related to user input validation. With 37,082 lines of code, this is about one issue per 5 lines. We will show some results of the analysis it did.

Critical

The number of critical issues Fortify raises is impressive, you must be mad to install SMF on your server! (Erik: or mad to use Fortify :-) ) The 3148 critical issues break down into:

• Command injection: 106
• Cross-Site Scripting: Reflected: 1601
• Dangerous File inclusion: 207
• Dangerous Function: 1
• Dynamic Code evaluation, Code injection : 100
• Open Redirect: 147
• Password management, Hardcoded password: 4
• Path manipulation: 968
• Privacy Violation: 14

A closer look to those issues shows that it is not so bad.

Command injection

The command injection issues end up in 3 lines of Subs.php: 3736,3738 and 3754. Those lines look similar, for example line 3736:

$test = @shell_exec('host -W 1 ' . @escapeshellarg($ip));

Each line has 22 issues and issues can have multiple paths. The possible input via the $ip variable is escaped in PHP so that it all ends up in a single argument to the host function. Fortify does not know anything about the program that is called and whether any user taint in that single argument can be dangerous. (Erik: Fortify might know about what shell_exec is and whether escapeshellarg is a correct sanitisation for it's argument. Is it clear from Fortify's feedback whether (a) Fortify knows about shell_exec and escapeshellarg, but it thinks/knows that this is not adequate as sanitisation, or (b) whether it does not know about these functions and simply complains to be on the safe side? )

Cross-Site Scripting: Reflected

Fortify found 869 issues in this category. All except one are in the Subs.php file. There are 4 lines in Subs.php with each 127 issues associated. Those 4 lines described a debug functionality. You can expect some dangerous code there.

Another point with lots of issues was Subs.php:2831. This line of code contains:

			echo $context['insert_after_template'];

Fortify has 160 issues about this line of code. This line of code shows a common problem that we had with fortify and SMF: when some code changes something in the $context array ($context is a global) and you can reach this line from there Fortify reports it as an issue. A quick `grep` through all the code, searching for insert_after_template, shows that this key of the array is barely used and looks safe. The problem though is that there exist constructions where you can change the insert_after_template key of this array without directly referring to insert_after_template. Since the usage of global arrays in SMF is so common we get so many issues.

Dangerous Function

There is one dangerous function found by Fortify, it is in this code:

if (!function_exists('mysql_real_escape_string'))
{
	function mysql_real_escape_string($string, $connection = null)
	{
		return mysql_escape_string($string);
	}
}

Fortify is correct here, it is dangerous to simply replace mysql_real_escape_string by mysql_escape_string. The mysql_real_escape_string really escapes the input and mysql_escape_string is not a valid replacement.

Open Redirect

There are 145 issues from Fortify on one single line of Subs.php where it redirects to another page. In the surrounding function the input is checked. For example, if the input does not start with http, it puts the board url in front of it. So one thing we need to check about this line is that the input either never starts with http or the input must be validated. Due to the globals and array problem we described before we get so many issues.

Password Management

There were only 4 password management issues found, these were all false positives. Fortify looks for hardcoded passwords, but in these cases it was only it complained about the following two lines of code:

$txt['password'] = 'Password';

and:

'password' => true,

Both cases are not actually hardcoded passwords, they just contain the word password as a key. This is not relevant though for input validation.

Path manipulation

Fortify finds many of these issues to be caused by opening or evaluation a file with the name $filename, which is a paramater in the function template_include. Fortify sees that the variable $filename is not validated in the function and it also does not know if it is checked before being used as a parameter for the function template_include, it will complain that there is a path manipulation issue. Searching the code for calls to the method template_include shows that this variable is always safe, mostly a constant. (Erik:Maybe Fortify's data flow analysis simply gives up for variables? For constants you could expect the tool to do better, I'd say.)

Privacy Violation

This gives 14 issues in a special debug function.

High

The following categories of issues were detected:

• Code Correctness: Regular Expressions Denial of Service: 35
  • In the first affected line of code, Fortify fails to spot that the data that is coming from user input is checked using the in_array function.
• Command Injection: 70
  • The issues about command injection are about the same 3 lines of code it gave in the critical section. Each line got another 20 issues. It is unclear why those cases got a lower priority from Fortify.
  • We found some exploitable cases where SMF unserialized user input.
• Dangerous File Inclusion: 417
  • There is a line where it loads cached template files. Fortify thinks it is unvalidated user input because the user is able to input a theme id. That user input traces all the way down to some cache function. This already gives 144 issues. The user input is cast to an int so it is unlikely that this is exploitable. There more issues regarding the templates/themes.
  • Another point is a global called $sourcedir. It contains the path to the SMF installation on disk. There are lots of include statements in the code that load use this variable and prepend a hardcoded filename. Those issues show that the analysis evidence trace it gives is not perfect. In the traces the $sourcedir is not changed so we wonder why it reports this as an issue.
• Dynamic Code Evaluation: Code Injection: 42
  • SMF creates some functions through code. We have analysed a few cases and found that in the current state of the code it is not dangerous (we checked the usages of the relevant functions)
• Header Manipulation: 753
  • In the section with critical issues we already had 147 issues about an open redirect. Since this redirection is done through PHP's header function, Fortify also complains about header manipulation. The header manipulation issues in the redirect part were not able to add additional headers because PHP restricts that.
  • There are also lots of issues about putting the character set in the header. The character set is stored in the $context variable, a global array where user input is also stored. As the character set is echo-ed in a lot of places we see issues about this variable in lots of places
• Header Manipulation: Cookies: 37
• Insecure Randomness: 99
• Log Forging: 11
• Password Management: Empty Password: 3
• Password Management: Hardcoded password: 69
  • Almost all issues come from the language files. When a variable name contains password Fortify seems to create an issue about it.
• Path Manipulation: 1268
  • The cache system of SMF sometimes decides that some cache file is not needed anymore. Most of the issues in this category point to a single line with each line about 100~150 issues.
• Weak Encryption: 5

We skipped categories that are not about user input: Insecure Randomness, Log Forging, Password Management and Weak Encryption.

Medium

• Cross-Site Scripting: Poor Validation: 416
• Often Misused: File Upload: 6

Low

• Command Injection: 2
• Cookie Security: HTTPOnly not Set: 9
• Cookie Security: Persistent Cookie: 9
• Cross-Site Request Forgery: 2
• Header Manipulation: 551
  • Almost all issues are about the redirect function in Subs.php we described earlier.
• Header Manipulation: Cookies: 28
• JavaScript Hijacking: Ad Hoc Ajax: 1
• Open Redirect: 84
• Password Management: Password in Comment: 106
• Resource Injection: 2
• Weak Cryptographic Hash: 123
• Weak Cryptographic Hash: Hardcoded Salt: 2

One of the problems we found in SMF is that system commands are not called with an absolute path. If an attacker can change the PATH in the environment, an attacker can execute malicious executables. We did not find any location that allows an attacker to change the environment. Fortify gives lots of issues with this. An example (manageserver.php:417):

$modSettings['load_average'] = @`uptime`) !== null

Another repeating issue was the use of $_REQUEST[‘theme’]. In the code this was checked to be an integer. Fortify gives warnings whenever this variable was part of a path to files that are being modified. This is not an issue because the worst thing that can happen is that a file from the wrong theme is being modified, which can also be done within the system (from the admin panel).

We also found that it was possible to alter theme files from the admin panel. This also allows altering php files and thus for instance print the database password or call external php files.

RATS

RATS gives a lot of warnings about TOCTOU (Time Of Check, Time of Use). This is caused by the use of is_dir and is_writable and other file manipulations. These methods were called from the admin location and it is very unlikely that 2 admins modify a file at the same time. Nevertheless it is possible that such a problem occurs. We did not investigate this any futher because we do not consider this as user input.

RATS gives a 'High' warning with the use of the eval method. In SMF the eval method is mostly used to execute PHP source files that were first opened with fopen. In one case eval was used with an argument that seemed to be coming from the database. However this piece of code was never called within the code that we have. It is possible though that it is called from an extended package or theme.

RATS also gave a warning about fsockopen. In SMF it is possible to open a FTP connection from the web based admin panel to servers containing packages. Rats complains that the url is not verified. This is true. It is assumed that the admin area is well protected and that an admin will only enter verified urls. At another location SMF opens a socket to what it seems, retrieve the user's avatar image. This can indeed be an untrusted source because it can be entered by any user of the forum.

RIPS

We analysed SMF using the default RIPS settings.

There are 86 issues found by RIPS.

Some of these issues are identical to those found with Fortify. One of the issues: "Userinput reaches sensitive sink when function template_include() is called", which is the same problem we had in Fortify where the $filename variable was thought to be untrustworthy/unvalidated.

In the end we did only a fast scan of the issues and realize that most of the issues seemed familiar after using Fortify. We did not thoroughly check all issues found by RIPS.