SoftwareSecurity2013/Group 9/Code Scanning

Determining scope

Software

For our security analysis, we used the most recent version (1.5.3) of FluxBB, which can be downloaded here. We ran Fortify Static Code Analysis version 5.15.0.0060 and used Audit Workbench version 3.80.0060 to analyze the results. We also ran RATS version 2.3, which can be downloaded here. Finally, we have used Checkmarx, which can be found here.

OWASP ASVS

Our goal is to analyze the HTML part of Verification Requirement 6. The following detailed verification requirements are stated in the ASVS for a Level 1B/2B analysis.

V6.1

Verify that all untrusted data that are output to HTML (including HTML elements, HTML attributes, javascript data values, CSS blocks, and URI attributes) are properly escaped for the applicable context.

V6.2

Verify that all output encoding/escaping controls are implemented on the server side.

V6.3

Verify that output encoding/escaping controls encode all characters not known to be safe for the intended interpreter.

V6.4

Verify that all untrusted data that is output to SQL interpreters use parameterized interfaces, prepared statements, or are escaped properly.

V6.5

Verify that all untrusted data that are output to XML use parameterized interfaces or are escaped properly.

V6.6

Verify that all untrusted data that are used in LDAP queries are escaped properly.

V6.7

Verify that all untrusted data that are included in operating system command parameters are escaped properly.

V6.8

Verify that all untrusted data that are output to any interpreters not specifically listed above are escaped properly.

Our project scope will only entail V6.1 and partly V6.2 and V6.3, because the rest has little to do with HTML.

Fortify

In the Security Auditor View, 2417 issues are found, of which 254 are marked as "critical", 1172 as "high", 123 as "medium" and 868 as "low". This substantial number points out the flaws, bugs and potential vulerabilities exist in the entire FluxBB code. Using the default categories, we can point out a few that will definitely be interesting for our verification requirements, namely "Cross Site Scripting: Persistent", "Cross Site Scripting: Poor Validation" and "Cross Site Scripting: Reflection". These three categories together contain 1473 vulnerabilities according to Fortify. Additionaly, we checked the "System Information Leak" and "Code Correctness" sections, because we happened to find some things there that could be relevant to our verification requirements. The scanner identifies 28 different categories that contain all 2417 issues, so our research scope seems to contain a disproportionally large amount (61%) of vulnerabilites. We have investigated the reported vulnerabilities in these sections because they are relevant to the issue of correctly escaped and encoded HTML.

Each section contains a brief introduction of the security problem that Fortify has detected, and a more detailed analysis of one or more specific issues in this category of vulnerabilities.

Cross Site Scripting: Persistent

627 issues are placed in this category, of which 525 have a high risk and 102 a critical risk. These issues are caused by sending unvalidated data originating from an untrusted source - the user - to the database. Conversely we also run the risk of having unvalidated information being sent to the browser, by in this case a MySQL(i), MySQL(i) InnoDB, PostreSQL or SQLite database. If one assumes that the data in the database only comes from the FluxBB web application and that the data is escaped (HTML), sanitized and validated before it is entered in the database, one could think that these warnings are meaningless. However, this also assumes that the database software does not have any security vulnerabilities itself. Because we do not know this, we can not trust the data from the database to be valid, even if we validated the data before writing it to the database. The other concern is that another (possibly compromised) application might have access to the database.

Looking at this category of issues, we can easily see that most issues are actually duplicates. Since there are six different configurable database types you can use with FluxBB, every issue is duplicated six times, once for every database implementation. For some reason the SQLite issues are indicated as critical risk and the issues for the other databases are high risk. Also there are 102 issues for SQLite, while the other implementations have 105 issues each. Disregarding this small discrepancy, we can say that there are roughly a hundred places in the code where unvalidated data originating from the database is sent to a browser directly.

These issues are directly related to V6.1 of the verification requirements.

All issues point to the query and fetch functions in the DBLayer class, which has various implementations for the different database back-ends. The query function performs a query on the database and the fetch functions retrieve the result of the query in some form. Fortify reports that the results of the fetch functions are not being validated before being used as browser output. Because there are too many issues to go over each one individually, we looked only at a subset of the reported issues.

Issue 1

Description: Line 249 of index.php sends unvalidated data to a web browser, which can result in the browser executing malicious code.

Analysis: This line outputs a list of users that are currently online. It retrieves this list of users from the database and turns it into a string of usernames with optional links to their profiles. The usernames are validated using pun_htmlspecialchars, but the user IDs used to generate the links are not validated. However, these IDs appear to be simple integers, which should generally never result in a string that could be considered HTML mark-up. Also, quickly looking over the registration form code shows that the IDs are auto-generated by the database in the DBLayer's insert_id function and should thus be valid. We come to the conclusion that this issue is a false positive.

Severity: Harmless. Fortify indicates critical/high.

Result: False positive.

Issue 2

Description: Line 391 of viewtopic.php sends unvalidated data to a web browser, which can result in the browser executing malicious code.

Analysis: This line outputs a HTML link for subscribing to a forum topic. The link is generated using unvalidated data from the database. However, again we are dealing with an ID here, namely the topic ID. Looking over the code in post.php we find out that the ID is again generated using the DBLayer's insert_id function and should thus be safe.

Severity: Harmless. Fortify indicates critical/high.

Result: False positive.

Issue 3

Description: Line 647 of search.php sends unvalidated data to a web browser, which can result in the browser executing malicious code.

Analysis: This line outputs part of the header for a search result in the post view mode. Amongst some other info it prints the forum title and topic title of the posts matching the search criteria. It also adds links to these titles pointing to the respective forum/topic. The data is fetched from the database. The titles are escaped using pun_htmlspecialchars. The links to the forum and topic use the forum ID and topic ID respectively and are not validated. Again, the IDs are simple numbers generated by the database and do not need to be checked.

Severity: Harmless. Fortify indicates critical/high.

Result: False positive.

Issue 4

Description: Line 874 of admin_users.php sends unvalidated data to a web browser, which can result in the browser executing malicious code.

Analysis: This line prints a user's e-mail address as a clickable mailto link. The e-mail address is retrieved from the database without validation. An e-mail address is entered during user registration. By looking at register.php we can see that e-mail addresses are checked against a regular expression to see if they are valid. This regular expression rules out certain characters such as less-than and greater-than symbols and double quotes, but is far from safe. Firstly, it does not rule out single quotes. This means that when a string containing a single quote is echoed into a singly-quoted HTML attribute, it will terminate the quote prematurely and allows for the attacker to add additional attributes to an HTML tag. Secondly, the first part of an e-mail address (before the @-character), can be fully enclosed in double quotes. It even allows any characters (except for double quotes) in between the quotes. This allows an attacker to break out of a double-quoted HTML attribute and inject any HTML code, by entering an e-mail address like the following: ">html-code-here"@dodgy.hacker. On the line in question double quotes are used. Also, it displays e-mail addresses regardless of whether they are verified or not. So, non-verified fake/harmful e-mail addresses are displayed as well.

Severity: Critical. Fortify indicates critical/high.

Result: True positive.

Solution: Escape the e-mail address before output or perform proper validation when the address is entered into the database.

Conclusion

Requirement V6.1 is not met, because there is at least one situation (see issue 4) where untrusted data (from a database) is not properly escaped.

Cross Site Scripting: Reflected

77 issues are placed in this category and all of them have a critical risk. Non-persistent cross site scripting attacks are common and hard to detect without using automated tools such as Fortify. These issues are caused by echoing (possibly transformed) data from browser requests/posts back to the browser without validating the data. An attacker could abuse this behaviour by sending a URL to a vulnerable site to an unsuspecting victim. The URL could contain malicious code that would be sent back to the victim and executed in their browser. The link itself could look relatively harmless, as it points to a trusted source, but will also contain the malicious XSS vector which executes the harmful (Java)script from another source.

Similarly to the persistent category, these issues are an indication that V6.1 might not be met.

About half of the vulnerabilities deal with admin-only pages, while the other half should be available to regular users. The vulnerabilities are similar in their concept:

Unvalidated $_GET[...] php calls retrieve the data which is (indirectly) used to output (echo) HTML. A malicious third party could have an unsuspecting user execute injected code. If the victim were to click on a link to a seemingly trusted site, but malicious code was appended to the link itself, the code would be executed in the browser of the victim due to the vulnerability in the website. An example of such a reflective XSS vulnerability is detected in the admin_bans.php file on lines 399 and 461 and a nearly identical vulnerability in admin_user.php on lines 157, 250, 830 and 899 . Most of the other reflective XSS vulnerabilities are very similar in concept. The first vulnerability is explained in more detail below.

Issue 1

Description: Line 399 of admin_bans.php sends unvalidated data to a web browser, which can result in the browser executing malicious code. Lines 333 and 334 contain $_GET['expire_after'] and $_GET['expire_before']. These inputs are not validated anywhere else later on.

Analysis: admin_bans.php is a file which deals with the interface and controls to ban and unban forum users. It takes a number of input parameters in its URL. The parameters are used, partly, to filter through the banned users. $_GET['expire_after'] and $_GET['expire_before'] are used to select a portion of the banned users whose ban will expire before or after a certain date. This input is completely unvalidated.

Attack: A hacker could craft a nifty url that contains malicious code. If the logged-in administrator of the FluxBB forum were to click on such a link it could compromise the security of the entire FluxBB forum. Injected code could contain Javascript to ban or unban certain users, it could contain code to send cookie information to the attacker, or it could render a different web page while pretending to be the trusted FluxBB forum and trick the victim into giving away passwords.

Solution: It could be fixed easily by validating and sanitizing the inputs for the two $_GET[] calls.

Severity: Critical. Fortify indicates critical/high.

Result: True positive.

Cross Site Scripting: Poor Validation

769 issues are placed in this category, of which 648 are classified as low risk and 121 as medium risk. Issues in this category are caused by data originating from an untrusted source - in this case either a database or from a browser request/post - that are sent to the browser after validation of insufficient quality. This category contains issues from both the persistent and reflected categories, but instead of the issues being caused by having no validation, they are caused by having poor validation. The issues from the reflected category (data originating from a browser request/post) are marked as medium risk, while the issues from the persistent category (data originating from a database) are marked as low risk. For the latter, issues are again duplicated for every database implementation.

So Fortify flags every single one case of improperly or poorly validated output as a possible cross site scripting vulnerability. In the case of a larger project such as FluxBB this results in a significant number of vulnerabilities. What Fortify fails to mention, is that all these 'bugs' could be fixed from a single location instead of in 648 different locations. So this makes quite a difference for someone doing a risk assessment when he is presented with 648 cross site scripting vulnerabilities, or just one or a few. And in the case of FluxBB most of the XSS vulnerabilities are in fact duplicates and could be considered a single cross site scripting error (occurring in multiple locations).

Most, if not all, poor validation issues use a beforementioned custom pun_htmlspecialchars function, which returns htmlspecialchars($str, ENT_QUOTES, 'UTF-8');. Fortify claims that using this function will prevent some, but not all cross site scripting attacks. However, due to the ENT_QUOTES and UTF-8 parameters, we think that all these issues are in practice false positives.

Verification requirement V6.3 is of importance for this category of issues.

Issue 1

Description: In viewforum.php (line 122), the program uses HTML, XML or other type of encoding that is not always enough to prevent malicious code from reaching the web browser.

Analysis: This line outputs the name of the forum that the user is currently visiting. It uses pun_htmlspecialchars to encode the name. It is the only text child node of a span node. span does not give any special meaning to its contents. Basic HTML encoding should therefore be sufficient.

Severity: Harmless. Fortify indicates low.

Result: False positive.

Issue 2

Description: In login.php (line 285), the program uses HTML, XML or other type of encoding that is not always enough to prevent malicious code from reaching the web browser.

Analysis: On the login page, the referer URL is stored in a hidden form field. The URL is first escaped using pun_htmlspecialchars and then echoed to the value attribute of an input node. Because pun_htmlspecialchars calls htmlspecialchars with the special ENT_QUOTES parameter (which encodes quotes), it is safe to use it in most HTML attributes. On a side note, it is absolutely necessary to properly escape referer URLs as they can easily be spoofed.

Severity: Harmless. Fortify indicates low.

Result: False positive.

Conclusion

Fortify indicates a high probability of false positives in this category. This happens because Fortify is often unable to determine the context in which the output escaping is to be applied. All reported issues we checked are in the context of child nodes and attributes without special meaning outside of standard HTML. In these cases using htmlspecialchars with ENT_QUOTES should be sufficient. However, since we have not looked at every single issue (because of the large quantity of issues), we can not safely say whether V6.3 is met or not. We suspect that V6.3 is likely to be met. (Erik: Saying that you looked at n% of the source code/code files, or at x out of y potential trouble spots, randomly selected, would be useful ways to back up your claim that it is likely that it is met)

System Information Leak

17 issues are placed in this category, of which 1 is marked with a critical risk and the other 16 with a low risk. There are only three (shared sinks) that are relevant for our verification requirements. They are all within the error function on line 1414 in functions.php. The other risks deal primarily with the install script, installation error reporting and phpinfo calls. Fortify does find these information leaks, but they are used only in the install scripts which should be deleted after succesful installation. Error reporting during installation will 'leak' system information. But it would be unwise to disable it to protect the server from leaking system information before FluxBB is even installed. In any case these notifications about vulnerabilities do not belong to the scope of our research which is about verification requirement V6 (HTML).

Issue 1

Description: The standard error function for reporting errors is reportedly insecure:

function error($message, $file = null, $line = null, $db_error = false) { /*...*/ echo "\t\t".'Error: '.$message.'.'."\n"; /*...*/ }

Here, no output validation is being done, which makes this a dangerous function. If the user could supply the $message code there would be the potential to leak system information. However, it is not possible for users to supply their own $message anywhere. All error messages are statically defined all over the FluxBB code, and use this function call to report the error message to the user.

Analysis: Although these issues count as a system information leak, they do not violate our verification requirements. In this specific case it is dangerous still because even though the implementation is secure since there are only static error calls. This potential vulnerability is not documented in the code so an unsuspecting programmer might add his own functionalities, which could lead to a security issue in this piece of code.

Solution: Validate and sanitize the $message.

Severity: Harmless. Fortify indicates low.

Result: False positive.

Code Correctness

8 issues are placed in this category, all with a high risk. There are two shared sinks that allow untrusted data within a regular expression. Because there was a tiny chance that this might have an effect on output, we decided to take a look at these risks as well.

Issue 1

Description: In parser.php, line 595 is as follows and is reported insecure. $current = preg_replace('%\['.$current_tag.'=("|\'|)(.*?)\\1\]\s*%i', '['.$current_tag.'=$2]', $current);

Analysis: $current_tag may contain unvalidated user input which can be passed on to the regular expression. $current is then appended to a string that is returned by the preparse_tags function, which is (escaped and) stored in the database in edit.php. However, when the messed up text is printed on screen, everything is still properly escaped, so this example does not violate our verification requirements either.

Severity: Harmless with respect to V6. Fortify indicates high.

Result: It is important to note that there is still the possibility that the unvalidated user input that is passed on to the regular expression could potentially cause a thread to over consume CPU resources, and possibly crash. This vulnerability could be exploited in a (D)DOS attack. So while it is a true positive, it does not deal with security requirement V6.

Issue 2

Description: In db_update.php (which should be accessable only to users with administrator priviliges), this is line 343:

$db->alter_field($table, $cur_column['Field'], preg_replace('%'.$type.'%i', $types[$type], $cur_column['Type']), $allow_null, $cur_column['Default'], null, true) or error('Unable to alter field to binary', __FILE__, __LINE__, $db->error());

Analysis: Here, $cur_column contains input that comes directly from the database, without any escaping or validation. However, this variable is the result of the query $db->query('SHOW FULL COLUMNS FROM '.$table), which will never contain any data that could be malicious.

Severity: Harmless. Fortify indicates high.

Result: False positive.

RATS

Here you can find the results of the most detailed RATS scan, using rats.exe -w 3 -i -r > ratsresults.txt. The outcomes can roughly be categorized as follows. None of them have anything to do with our verification requirements.

Check argument

These findings are due to the -i flag. The tool reports that functions like fopen and mail have arguments that should be checked to ensure that they do not come from an untrusted source. If the values are derived from user input, it has to be made sure that they are properly formatted and contain no unexpected characters or data. Fortify is better suited to find out if this is actually the case.

"Non-function call reference"

These findings are due to the -r flag. It is reported lots of times that a function call is not being made, but that a reference is being made to a name that is normally a vulnerable function. In all cases we manually checked however, the function call is actually being made. The error might be reported when a function call is being made inside another call, when the @-operator is being used, et cetera. We also noticed that the list of functions that are marked as being vulnerable, might be a bit outdated.

TOCTOU vulnerability

RATS can find five potential Time-of-Check-Time-of-Use vulnerabilities, of which in two cases no use is detected. Another is in admin_statistics.php on line 39, one in include/dblayer/sqlite.php on line 49 and the last is in include/functions.php on line 1983. The first reads from a file which could be made unaccessible or altered while reading. The second executes a chmod 0666. The third checks whether a file is writable. None of these should result in a real vulnerability.

Race condition

Lots of race conditions are detected, with function calls like file, basename, unlink, fopen and dirname. Normally calls to these function are supposed to be vulnerable only when a match check precedes it. No check was detected, however one could still exist that could not be detected. A manual check is required.

Checkmarx

Checkmarx is a SaaS product that runs in the cloud. After uploading the source code and running a full scan, it reports 86 high risks and 92 medium risks. These findings are further categorized as follows.

High Risk Vulnerabilities	(86)
Stored_XSS	43
Second_Order_SQL_Injection	23
Reflected_XSS_All_Clients	12
SQL_Injection	4
Code_Injection	2
Remote_File_Inclusion	2
Medium Risk Vulnerabilities	(92)
XSRF	59
Parameter_Tampering	21
Stored_Code_Injection	6
Open_Redirect	3
DB_Parameter_Tampering	2
Path_Traversal	1

A full report can be found here. These numbers are friendlier to work with than the thousands of Fortify. This is partly due to that Checkmarx does not check system information leaks like phpinfo(); or insufficent randomness, but what is remarkable is that all other issues are quite similar. Only the Stored_XSS and Reflected_XSS_All_Clients categories deal with our verification requirements.

Stored_XSS

When looking at the Stored_XSS vulnerabilities, we again find a few where pun_htmlspecialchars is used, usernames from the database and also the error function in functions.php, where the error message is not escaped and statically defined.

Issue 1

Description: Line 116 of admin_statistics.php prints unescaped data. $load_averages = fread($fh, 64); /*...*/ $load_averages = @explode(' ', $load_averages); $server_load = isset($load_averages[2]) ? $load_averages[0].' '.$load_averages[1].' '.$load_averages[2] : $lang_admin_index['Not available']; /*...*/ printf($lang_admin_index['Server load data']."\n", $server_load, $num_online);

Analysis: If a malicous user could put his HTML in the file pointed to by $fh, this would have been a true positiver. However, the file that is used is statically defined as /proc/loadavg, so an attacker would need root access to the server anyways.

Severity: Harmless. Checkmarx indicates high.

Result: False positive.

This is the only one not also mentioned by Fortify. The rest of the issues are all similar to issues already mentioned above.

Reflected_XSS_All_Clients

All issues that are found here, were also found by Fortify and mentioned above. Most are due to the pun_htmlspecialchars function.