SoftwareSecurity2014/Group 2/Code Scanning

Fortify

When running Fortify on the OwnCloud core repository, we were presented with the following issues:

• 621 Critical
• 3378 High
• 8 Medium
• 500 Low
• 4507 Total

Issues Related to V6

There are 9 critical and 5 medium issues related to V6.

Some of them demonstrate an interesting feature of the dataflow analyzer. When the same call graph reflects data through different code paths, these are grouped together in one issue and can be inspected separately in the Analysis Evidence window.

In the first four XSS issues the GET parameter `app` is reflected through more than 16 steps into a piece of generated JavaScript code. Among these steps is a call to the function `sanitizehtml`. If we understand correctly, Fortify complains because user input that ends up in JavaScript code must be sanitized for JavaScript, not for HTML. (Erik: That might be the explanation. But note that javascript is a subset of HTML, as the HTML tags <script> </script> mark JavaScript; so HTML-escaping the keyword <script> should render the javascript harmless. Another explanation could be that Fortify does not recognize that sanitizehtml is a sanitisation routine; it might only know the standard sanitisation in PHP libraries.)

In the fifth issue there is a reflected POST parameter in the value attribute of a HTML input field:

<form class="searchbox" action="#" method="post">
    <input id="searchbox" class="svg" type="search" name="query"
        value="<?php if(isset($_POST['query'])) {p($_POST['query']);};?>"
        autocomplete="off" x-webkit-speech />
</form>

The function `p` HTML-escapes and then prints its input, but here the call to `p` occurs inside the quotes of an HTML attribute. A different method of escaping is probably required.

These first five critical issues seem to be in relation with the five medium ones. The medium issues are all Poor Validation warnings: the escaping method used may not be sufficient for the place where it is used. In all of these cases though, in our opinion it should be sufficient to use HTML escaping only. All these cases occur inside double quotes, in either JavaScript code or HTML attribute values. The only way to break out from there is to have double quotes in the value, but double quotes are escaped by HTML escaping.

Issue number six reflects the GET parameter `mime`, but through some mangling to turn it into a filename. No escaping is performed. This could be a problem.

The last three issues complain that values from the PHP variable `_SERVER` are reflected. The context where this happens is an XML file. No escaping, but some mangling to form a URL is performed. The PHP manual states that values in the `_SERVER` variable are under control of the server. We aren't sure if these may still originate from user input, say from an earlier request. A more thorough code review regarding this issue is required.

Critical priority issues

Most of these issues (Erik: You mean the other 621 minus 9 critical issues?) are not related to output escaping. There are some interesting ones though, which we discuss briefly.

More than half of the critical issues are password warnings. The vast majority of these are hardcoded (Erik: I guess you mean 'POSSIBLY hardcoded', as I expect the large majority to be false positives)passwords: Fortify has a rule to look for the string literal "password", and variations thereof, in the source code. Almost all of these issues in ownCloud come from the translation files, some come from captions of input fields, and a few are actually hardcoded passwords (Erik: Nice to hear that these warnings about passwords, which people typically note as an annoying source of many false positives, actually produces some true positives, albeit in the unit tests) , but in unit tests.

The other password-related issues concern passwords in HTML forms. The flagged line of code is the following, which populates the password field in an HTML form with a previous value, and Fortify doesn't like that. (Erik: is Fortify right not to like it? I'd say not)

<input type="password" name="adminpass" data-typetoggle="#show" id="adminpass" placeholder="" value="<?php p($_['adminpass']); ?>"

The next big class of issues, 250 of them, are dataflow warnings, where the user controls input to a system function like `mkdir`. These are interesting because they demonstrate Fortify's dataflow analyzer. By selecting one of these issues, Fortify's source code viewer jumps to the line in question which most of the time looks rather innocuous, for example

mkdir($cacheDir, 0770, true);

The Analysis Evidence window of Fortify then displays a series of dataflow steps where you see that some user input is at least part of the parameter passed to the function. In the case above the name of the current user, coming from a POST parameter, is part of the directory name. For some issues this series of steps is longer than a dozen.

Other critical issues involve things like passwords in HTML forms being submitted over unencrypted connections, file inclusions where the filename is partly controlled by POST parameters, and others unrelated to V6.

High priority issues

Among the high priority issues are things like the following.

Insecure Randomness. Fortify finds these issues in the jQuery library, which is an indicator that this is probably a false positive. Upon closer inspection, it flags every usage of the 'Math.random' Javascript function. This is an issue for cryptography, but definitely not in general. There is no reason to assume it is currently being used for crypto here.

Again, we have strings that contain the word "password". More than 3000 of the medium issues are of this kind. These alerts are all about strings that are used in the user interface. OwnCloud is translated into various languages and each of these strings occurs in dozens of translation files, triggering hundreds of alerts.

Lines of codes that are flagged in this issue are for example (including encoding bug):

"%s password reset" => "R√©initialisation de votre mot de passe %s",
"New password" => "Nouveau mot de passe",
...
"Password protected" => "Durch ein Passwort geschützt",
...

The Hardcoded Password issue shows one different type of flag; the word 'password' is also used in the jQuery library file.

None of the high priority issues are related to output encoding or escaping.

Medium priority issues

Five of the medium priority issues are related to output encoding, and are discussed above. The others are warnings that file upload functionality is often implemented wrong, but it doesn't say if that's the case here.

Low priority issues

Fortify reports 500 low priority issues. Some of them seem to be more critical than some of the critical and high priority issues listed above.

The first three issues that it finds are related to cookie security. These all concern the same cookie, which seems to be used to track and/or prevent session timeouts. It is unclear at this point if this cookie is revealing actual session information, or is just used as a token.

Fortify finds many CSRF vulernabilities, nearly all of them in Javascript files. It appears to have flagged get and post requests that are submitted without including a nonce or a user-specific secret.

Other issues include hidden form fields (as programmers might trust these easily-manipulated values), the leaking of error messages that might help an attacker and the use of the deprecated md5 cryptographic hash function. In this particular case, the md5 hash is used to generate random noise that is then used to generate an image placeholder.

As with the other issue types, the strict matching on the literal 'password' string makes another appearance. At various points in the code, the documentation contains the word 'password' in the comments. The vulnerability details provide general reasons for not hardcoding passwords, but even Fortify has a hard time explaining why the word 'password' would be a problem when it appears as part of the documentation.

RIPS

Link to complete results: https://dl.dropboxusercontent.com/u/3018741/Software%20Security/RIPS%20-%20A%20static%20source%20code%20analyser%20for%20vulnerabilities%20in%20PHP%20scripts.html

The first type we ran RIPS with the setting 'User input'. It found only 9 errors and 5500+ 'sensitive sinks' (places where possibly tainted user input could trigger vulnerabilities). Results summary:

Most of these sinks are obvious false positives, as they do not actually operate on user input. We checked the 9 errors by hand, but they all turned out to be false positives as well. This is because RIPS only looks at the code at a local level: it will flag a line as a positive when it finds a combination of keywords, without looking at the context.

When we ran RIPS with the 'check for all errors' setting, it returned 2800+ errors. We also got a notification that there may be some false negatives, since the code is object-oriented and RIPS is not made to deal with OO-code.

As we got such a huge amount of errors, we mainly looked at the cross-site-scripting errors since they would probably be relevant to V6. Again, we found mainly the same error over and over again: RIPS would complain of a sensitive sink because it had found relatively normal keywords. Upon further inspection, the larger part of the errors again turned out to be false positives. For example: RIPS will flag any line with required_once or the combination of $ GET and exit(). When we ask RIPS for an explanation, you get a general explanation about the error:

We decided to zoom in on the cross-site-scripting errors, because it was likely that these errors were relevant to our V6 requirements. When we checked a few of the positives, we found that these were also all false positives: RIPS flagged the lines of code because of certain keywords. It is unlikely that we could find anything relevant to our project.

(Erik:From your description, I get the impression that there is no obvious overlap between RIPS and Fortify's warnings, right?)

RATS

When we run RATS on the Owncloud project, it finds about 300 vulnerabilities, with fopen as the most critical one (which seems to be logical, since we analyse a file hosting service):

    145  High:   fopen
     72  Medium: is_dir
     21  Medium: is_file
     16  Medium: stat
      4  Medium: fsockopen
      4  Medium: chdir
      3  Medium: is_readable
      3  Medium: is_link
      2  Medium: open
      2  Medium: is_writable
      1  Medium: pfsockopen
      1  Medium: mkdir
      1  Medium: fileperms
      1  High:   popen
      1  High:   mail

We can categorize these results into different subgroups:

Untrusted input

Functions that use data that comes from an untrusted source without first verifying that it contains nothing dangerous. (fopen, fsockopen, pfsockopen, popen, mail)

Time Of Check, Time Of Use vulnerabilities

These are concurrency issues: when you check a property of a file, it could be changed later when you use this property. (is_dir, is_file, stat, is_readable, is_link, is_writable, fileperms)

File naming problems

This is a form of untrusted input, but specifically for file names, for example directory traversal could be a problem with file names. (chdir, open, mkdir)

We will now briefly analyse an example of untrusted input: the fopen-function. RATS lists the line numbers where this function is used in combination with an error message:

apps/files_sharing/lib/sharedstorage.php:320: High: fopen
apps/files_sharing/lib/sharedstorage.php:352: High: fopen
apps/files_external/3rdparty/php-opencloud/lib/OpenCloud/ObjectStore/Resource/DataObject.php:271: High: fopen

...
tests/lib/streamwrappers.php:52: High: fopen
tests/lib/files/filesystem.php:160: High: fopen
tests/lib/archive.php:91: High: fopen
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.

It matched the keyword fopen from the included vulnerability database:

  <Vulnerability>
    <Name>fopen</Name>
    <InputProblem>
      <Arg>1</Arg>
      <Severity>High</Severity>
    </InputProblem>
    <RaceUse>1</RaceUse>
  </Vulnerability>

The '1' in the Arg-tag is used to print the error message that is displayed. This error message comes from the source file report.c:

report.c-        case InputProblem:
report.c:            printf("Argument %d to this function call should be checked to ensure that it does not\n", ptr->data->InputProblem->Arg);
report.c-            printf("come from an untrusted source without first verifying that it contains nothing\n");
report.c-            printf("dangerous.\n\n");
report.c-            break;

Analysis of requirement V6 using Rats

Although Rats does only a simple keyword search, we could at least analyse our requirement (V6: Output Encoding/Escaping (HTML)) at the OWASP Level 1B. Since we only look at output encoding/escaping, we should just look at the keywords that are relevant here. We already categorized the results into three sub groups. From these sub groups, both Time Of Check, Time Of Use and File naming vulnerabilities are just at the inside or input side, and thus not relevant for our requirement (which only looks at the output side).

The other category, Untrusted input, could be interesting, since untrusted input could result in untrusted output. In untrusted input, we found fopen, fsockopen, pfsockopen, popen and mail. From these keywords are fopen (which opens a file) and popen (which connects to another process on your system) not relevant, since they have nothing to do with output.

fsockopen and pfsockopen are used to create sockets, which will output something. However, their output will not be sent back to the HTML-page of the user (instead it will make a connection to another listening process), so it falls out of our scope of output encoding.

The last keyword that Rats found is mail. Rats defines this as dangerous because it could be exploited to sent unauthorized e-mails. However, this not a XSS output filtering problems and thus we can also ignore this keyword in our analysis.