SoftwareSecurity2014/Group 6/Code Scanning

RATS

The output that RATS prduced can be found here: RATS output

Manual check of the code locations:

• High risk warning regarding the mail function of PHP might be an actual risk. However, this has more to do with requirement V5.
• High risk warning regarding system function seems to be a false positive, since everything user-supplied is escaped before passing it to system. The same warning is produced by Fortify.
• High risk warnings about fopen seem to be false positives, as filenames are all fixed in the code or read from the configuration and never come from a user.
• High risk warning about plugins/password/helpers/chgdbmailusers.c actually depict a security vulnerability. However, as the program seems to be used only by administrators that already have superuser rights, it probably can't be exploited for malicious intentions.
• plugins/password/drivers/chpasswd.php should not be vulnerable to the warning that RATS outputs, as the password command is probably safe enough to accept all kind of input. This is similar for the other warnings about popen. However, in this file that implements password changing, the old password of the user is not checked before it is changed. This is bad practice.
• Warnings about is_readable can be neglected, as the implementation seems secure enough and in any case does not pose a threat to security (especially not to V4/V9). The same goes for the warnings about is_writable and is_dir.
• Warnings about fsockopen do not apply since everything seems to be escaped correctly.
• There is a bunch of warnings at the end of the RATS output regarding file, readfile, read, fgets, getenv, .... None of these seem to be problematic.

RIPS

We also ran RIPS v0.54 on the Roundcube source code. RIPS generates an HTML page to display the results of a scan. The complete page that resulted from our scan is available on http://rips.orgfree.com/rips_output.html (Erik:this link is dead)

RIPS offers different scanning options. Important ones are the "verbosity level" and the "vuln type". For the "verbosity level" we have specified "1. user tainted only", so that RIPS will only analyze code that includes tainted user data, which generally consists of input that the user is able to modify. For the "vuln type" we have specified "All", which means that RIPS will check for all possible vulnerabilites. For a list of vulnerabilites that RIPS is able to identify, refer to http://rips-scanner.sourceforge.net/#features

Below is a summarizing overview of the scan results.

Although the current version of RIPS does not support object-oriented code, the scanning tool was still able to identify a total of 27 possible vulnerabilites. We will briefly discuss the results for each type of vulnerability.

Header injection (3)

It is reported that there are 2 possible header injection vulnerabilities in the file installer/test.php. This has to do with the mail() function of PHP, which can be used for spamming purposes when the additional_headers parameter that is passed to this function contains user input which is insufficiently sanitized. This actually seems to be the case, because the additional_headers parameter is consructed from user input that is only sanitized by using idn_to_ascii() and trim(), which does not prevent an attacker to enter a value in the form of \nbcc:spam@address.com\n... to send spam emails to multiple addresses using the application's server. We have not tried to exploit this possible vulnerability. One of the reasons that we did not look into this further is that the installer/test.php is within the installer directory, which should be removed as soon as the application has been installed according to the official documentation [1].

The third location of a possible header injection is within the file plugins/password/drivers/directadmin.php. RIPS reports a possibly vulnerability when the get() function, which is part of the HTTPSocket class, is called. The get() function will then subsequently call the query() function of the same class, providing a parameter $location, which was initially passed to get(). Because the contents of this variable are used in a call of socket_write(), improper sanitization of the contents of this variable can lead to unwanted contents being passed to the socket. Because it is unclear where the socket is used for and because we could not find any HTTPSocket instances being created within the application's code we did not investigate this further.

File Disclosure (4)

The first report of a possible file disclosure vulnerability is for a piece of code in the file plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php. However, this is a false positive as the temporary file name that is provided to file_get_contents() is safely checked with the function is_uploaded_file(). By using this checking function it can be ensured that the file being operated on was indeed an uploaded file, and an attacker did not trick the application into reading the contents of some important system file.

The second and third possible file disclosures are in program/steps/mail/import.inc and they are of identical form. Although there is a whitelist check on the content/mime type of the uploaded file, which is 'guessed' by the file_content_type() function of the rcube_mime class, there is no sanitization on the filename. When the name of a sensitive file is provided and the whitelist check fails due to an incorrect guess of the mime type, it may be possible to save the file as a message in the IMAP folder, as the description of the import.inc file indicates. This possible vulnerability is applicable to OWASP ASVS V4.3, which states: "Verify that users can only access data files for which they possess specific authorization.". However, for a level 1B verification this requirement does not need to be verified, so we did not investigate this further.

The final possible file disclosure is in roundcubemail-1.0.0/program/steps/addressbook/import.inc. The uploaded file is used to import contacts into the address book and this functionality will only work when this is a CSV file or a vCard. After code inspection we concluded that there does not seem to be any possibility to exploit this functionality.

File Manipulation (1)

There is one report for a possible file manipulation vulnerability and it is similar to the third report of a possible header injection, which was described before. Instead of the supposedly tainted data that is passed to socket_write(), as was the case for the header injection, in this case similar data is passed to the fwrite() function, in which case RIPS apparently qualifies the vulnerability as 'file manipulation' instead of 'header injection'. For the reasons that were mentioned in the related header injection report we did not investigate this report further.

Cross-Site Scripting (14)

The first report of a possible XSS vulnerability is in the file installer/test.php. There is a function call of show() on an instance of html_inputfield with an unsanitized POST parameter. However, when analyzing the show() function, we observe that the POST parameter is passed through some other functions and in the end it go through the attrib_string() function before the value is displayed. This function filters the value based on a whitelist and when the value is not in the whitelist it will still be passed to the quote() function before the value will be displayed. The latter function will sanitize the value with htmlspecialchars(), which will rule out any XSS possibility. Therefore we conclude that this is a false positive.

There are 10 other reports of possible XSS vulnerabilities, which are identical to the first report in that they all display values based on tainted user data that goes through the show() function. Therefore these are all false positives. Apparently the developers have put some effort into centralizing the input and output validation functionality and using it consistenly through the application.

There are, however, a few remaining reports of possible XSS in which the data does not go through this function. The first one is in installer/test.php. In this case the tainted data goes through the Q() function, which subsequently calls the rcube_utils::rep_specialchars_output() function with the tainted parameter. This function sanitizes the data by using a functionality that is similar to a direct call of htmlspecialchars(). In this case the inner translation table of htmlspecialchars() is retrieved with get_html_translation_table(HTML_SPECIALCHARS) and this value is passed as the replace_pairs parameter to strtr(), together with the tainted data. This function will then replace characters in the tainted data just like htmlspecialchars(), except that it will not replace the question mark character, which was removed from the replace pairs in the code. This, however, will not affect the sanitization that defends against XSS. Therefore this report of possible XSS is a false positive.

Another report of a possible XSS vulnerability related to data that does not go through show() function is for the file /skins/larry/svggradient.php. However, in this case the value will be "sanitized" with preg_replace(), which in this case removes characters that do not occur in the provided whitelist, which consists of the following characters: a-f0-9,;%. This will make it hard, if not impossible, to execute a successful XSS attack on this piece of code.

The final report of possible XSS is for the file program/steps/mail/get.inc. In this case the tainted data will go through the Q() function, so that the data will eventually be sanitized, as we described earlier. We conclude that all reports of possible XSS vulnerabilities are false positives.

HTTP Response Splitting (5)

This type of vulnerability is present when tainted user data is included in the parameter that is passed to PHP's header() function. An attacker can abuse this to create malicious redirects and may even include javascript in the header to initiate an XSS attack. The first report of a supposed vulnerability of this type is in the file index.php. In this case the $_SERVER['REQUEST_URI'] is directly included in the header string. Since an attacker can alter the contents of this variable this may indeed be a valid vulnerability. Since this type of vulnearbility is out of the scope of the verification requirements that we investigate, we did not analyze this further.

The second report is for the file program/include/rcmail_output_html.php. This seems to be a false positive, since the data that is included in the header parameter is (not even partly) provided by the user. Apparently RIPS fails to notice that the related data is not tainted, possibly because there are some extensive constructions of variable modifications that do include parameters that are tainted.

The third and fourth report are for the file program/steps/mail/get.inc and are similar to the previous false positive. However, the final report in the same file is similar to the supposedly valid vulnerability that we found in index.php, because $_SERVER['REQUEST_URI'] is included in the header string unsanitized.

FORTIFY

The output that Fortify produced can be found here: Fortify output

Critical priority issues

• Path Manipulation
  • Roundcube creates a directory for gpg on the server. The warning comes from the construction of the directory name. However, this can only be manipulated by an entity that has direct access to the server.
• Cross-Site Scripting
  • Quite a few cross-site scripting warnings are generated due to roundcube logging to a web page. However, in all instances the critical data is either static or input validated.
• Open Redirect
  • Fortify complains about the "Host:" parameter from the HTTP request not being verified. With a misconfigured web server it might actually be possible to redirect to an arbitrary host.
• Dangerous File Inclusion
  • Fortify warns about an attacker being able to load arbitray files by setting their preferred language to the target path. This could be possible because the localization is loaded from files. However, the input is parsed and if no matching language file is found, roundcube defaults to the 'en_US' language file.
• Privacy Violation
  • Fortify warns about roundcube logging passwords. These are, however, all hashed and do not reveal any information on the passwords themselves.
  • Roundcube, though, does log the username together with information about a request when an error occurs.
  • Another warning stems from roundcube sending the username and password on to the IMAP server. However, this has to be done to be able to login and retrieve e-mails.

High priority issues

• The first group of issues (Dynamic Code Evaluation : Code Injection) is reported due to the create_function() function in PHP which can be exploited as it interprets arbitrary strings into code. But in our case these arbitrary strings are like return($value) or return(hash($value)). So again Fortify just scans for dangerous functions without checking the parameters.
• The second group of issues (Header Manipulation) has to do with possibly untrusted/unvalidated data sent to a user through an HTTP response header. Here the use of header() function might be a real and a not flase alarm in some cases.
• The third group of issues(Insecure Randomness) is due to the usage of functions rand(), mt_rand(), mt_srand(). Clearly a false positive.
• The fifth issue (password management:hardcoded password) could be of inteest for the spec requiement V9 (data protection). Apperently this seems to be a false positive ouput. The highlighted line of code just sets the character set of the password to ISO-8859-1 if the backend does not support UTF-8. Once again the word "password" is more the suficient to raise a false alarm.
• The sixth group of issues (Weak encryption) is of great importance to V9(Data protection) spec requirement. The reason for this alarm is the usage of crypt() function. It is stated that crypt() function should always be used with a salt parameter (even though it is optional) which determines the security of the function. In our case crypt() is always used with a salt parameter so even though we can say that this is a false positive, it is a good reminder for a programmer to always add a salt parameter.

Medium priority issues

• The second issue (often misused : file upload) is about the (mis)use of the move_uploaded_file() function which can be exploited by an attacker to inject malicious code. Here it is used to upload an attched file to an e-mail without being vulnerable at all since the uploaded file's content is used nowhere. The highlighting of this issue indicates that Fortify peforms a dump scanning for vulnerable functions. This issue is clearly a false-positive having nothing to do with V4/V9 requirements.

Low priority issues

• There are many warnings about command injections. However, all of the data that is passed to shell commands seems to be properly escaped. RATS actually also complained about some of these issues. This would be relevant to our verification goals if it weren't false positives.
• The cookie that is criticized by fortify does not contain sensitive data.
• Some of the cross site request forgery warnings might be actual issues. It would be necessary to look at them in more detail. This is definitely relevant to our verification requirements.
• The cross side scripting warnings are false positives, since data does not come from an untrusted source, but from the configuration file.
• The hardcoded domain issue might need fixing. It would probably be better not to include code from third party websites.
• The hidden field issue isn't an issue in this case, since it only occurs during the installation and does not come from a user.
• Fortify complains about many passwords in comments (107 warnings). However, none of these are actual passwords. They are just comments about password functionality. Fortunately it is easy to suppress such things in fortify. Otherwise this kind of stuff would be really annoying.
• The system information leak warning would require further inspection to see if this is an actual issue. In any case, this is not that relevant to verification requirements V4/V9 since no application-specific data is leaked.
• roundcube uses MD5 and SHA-1 hashes in some locations in the code. While at some points this obviously doesn't matter since nothing is protected with this hash, in other cases this might be a big security issue. We assume that this is in the code due to compatibility reasons. As this also happens as part of an authentication process, this is relevant to verification requirement V4.