SoftwareSecurity2014/Group 2/Verdict

Verification requirements

V6.1 (PASS)

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.

OwnCloud has two wrappers for the built-in PHP print function, one that escapes HTML and one that doesn't. In the code, mostly the escaping print function is used, and only in selected places where it is necessary and safe, they use direct printing. This is good design, which they consistently apply, and furthermore Fortify didn't find instances where tainted data flows into the non-escaping print function.

We're pretty confident that ownCloud passes requirement 6.1.

V6.2 (PASS, pretty sure)

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

To implement output escaping on the client side, you have to do output on the client side in the first place. This is usually done with the javascript function document.write or by dynamically creating elements and adding them to the DOM either directly or using jquery.

A grep for the string "document.write" gave no results. We searched for all places in php code where javascript is generated, and checked if the code generates output differently. Doesn't seem so. Furthermore, ownCloud has 35000 lines of java script code in 67 .js files which are served to the client at various pages. Again it doesn't seem that there is output being done in these files, according to some grepping for the usual suspects.

We cannot rely on Fortify to check this requirement, because Fortify only detects when tainted data is being output but doesn't give warnings when generally output is done on the client side.

There are just so many ways to obfuscate generating output, how can you be sure? OwnCloud uses jquery, which can be used to insert elements in different hard to detect ways. At least it does seem like ownCloud performs all output escaping on the server side.

(Erik: Btw, note that even if you do find validation on the client side, you would still have to check if this valdiation is not repeated at the server side. Only when the validation happens ONLY at the client side is there a problem. Sometimes for user-friendliness and quick feedback there is client-side validation, which is then repeated server-side,because you cannot rely on client-side validation,of course. The phrasing of this ASVS requirement could be a bit clearer in this respect,I feel )

V6.3 (Don't know)

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

OwnCloud uses two functions for all its printing needs: p($string) and print_unescaped($string). The names of these functions can be clearly linked to their contents; one of them calls OC_Util::sanitizeHTML (see V6.5) on the argument before passing it to the build-in print function, while the other does not. The documentation above the unsafe variant gives due warning to whoever is using it: "Prints an unsanitized string - usage of this function may result into XSS. Consider using p() instead"

Unfortunately, print_unescaped is still used very often. At nearly 200 unique occurrences, it is not feasible to manually verify these calls at this point (especially since the content that is printed often originates from function arguments, further obscuring the origin of the values). Because of this, we cannot conclude that OwnCloud is escaping all characters that are not known to be safe. In fact, the very existence of such a function indicates that it strongly depends on context. In some situations, it appears to be necessary not to escape certain characters. Not only does this imply that OwnCloud does not satisfy requirement V6.3, it is also a potentially risky decision for the developer every time output is being generated.

As mentioned, the p-function relies on sanitizeHTML. The implementation is fairly simple. If the input is an array, it recursively applies sanitizeHTML to all the values. If not, the value is cast to string and htmlspecialchars is applied. ENT_QUOTES is set, so it also converts single quotes to ''' or ''' (depending on the specific PHP implementation). The encoding is redundantly specified to be UTF-8, which is what OwnCloud uses consistently throughout the codebase. The safety of this ENT_QOUTES choice is dubious, though, as this disables encoding of double quotes. It is impossible to manually verify consistent quotation use, and PHP does not make a difference. It would probably be wise to err on the side of safety and use htmlentities instead, encoding both sets of quotes.

V6.4 (PASS)

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

OwnCloud does not rely on a specific SQL implementation, but allows the user to choose which underlying database type should be used. During installation, it checks which options are available, and sets flags accordingly ('$hasSQLite', '$hasPostgreSQL', '$hasMySQL', etc.). OwnCloud then wraps the database interaction in an OC_DB class (defined in /lib/private.db.php).

When executing a query from anywhere in the codebase, OwnCloud calls OC_DB::prepare(query), after which the query can be parametrised upon execution using $query->execute(). This returns an array containing the results, equivalent to the built-in PHP database functions. The OC_DB class establishes a database connection, and (except for the setup), no other class directly calls any database functions without passing through an OC_DB object.

The OC_DB class relies on the Doctrine project to provide a database abstraction layer. Doctrine does not automatically guarantee safety from SQL injections, but OC_DB only calls the prepare function. However, it does expose the Doctrine connection object through OC_DB::getConnection. This is potentially risky.

The following files make use of this getConnection method to gain access to the Doctrine interface:

./apps/files/appinfo/update.php

This file uses the database connection to get a version number of the database installation. This is then used as a postfix for app installations.

./core/command/db/generatechangescript.php

Here, the Doctrine connection is used to initialise an MDB2SchemaManager object: new \OC\DB\MDB2SchemaManager(\OC_DB::getConnection()). Such an MDB2SchemaManager is used to generate database blueprint files, or change from the current database schema to a new schema, as specified in a db_schema.xml file. It does not further rely on user input.

./lib/private/legacy/preferences.php

The preferences class uses it to initialise /lib/private/preferences, where it is then used in several static or prepared queries. The data used for the preferences is of unknown origin (i.e. might be tainted), and is handled accordingly. The connection is not exposed to other classes from here on.

./lib/private/server.php

This file uses the connection in two different places; once to construct an AppConfig object, and once for a ConnectionWrapper. Other than prepared select statements, AppConfig uses several insert and delete statements directly on the Doctrine connection (so without preparing). However, none of these take tainted input; all the data that this class uses either comes from configuration files or was already in the database. The ConnectionWrapper does not further expose the Doctrine connection, and only provides an interface for several 'safe' operations (such as 'prepare', 'lastInsertID', 'rollback', etc.).

As mentioned above, though, lib/private/setup does not use the connection that is established via the Doctrine interface provided by OC_DB. It sets up a local connection, and directly calls PHP functions on that. Here, OwnCloud does not use prepared statements, and sets up their queries in a very unsafe manner:

$query="SELECT user FROM mysql.user WHERE user='$this->dbuser'"; // (MySQL / MariaDB)
...
$query = "CREATE LOGIN [".$this->dbuser."] WITH PASSWORD = '".$this->dbpassword."';";  // (Mssql)

However, as these values are submitted during the setup process, it would already require far-reaching permissions to get to the point where one can inject malicious content. Specifically, it would require that the user has signed in with administrator privileges, at which point it would be possible (and more convenient) to get to the entire database 'the regular way' as well, or modified the on-disk configuration files. (Erik: I understand your reasoning. Still, it's always better to be paranoid and check anyway :-) For example, a creative attacker might be create a username with some SQL commands in it, which might cause problems.)

In general, OwnCloud seems to have managed to successfully handle and escape the untrusted data before passing it to the SQL interpreter. The key point here was to create a database wrapper that does not allow direct queries, but always enforces prepared statements. By doing this, the various developers do not constantly have to take care of proper escaping, and any changes to the querying process can be done in one controlled place.

V6.5 (PASS)

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

It seems that the bulk of the functions that have to do with XML-output are in ocs.php and api.php. Looking at the code, we can see that OwnCloud uses the class XMLWriter to write XML. A search on stackoverflow suggests that using this class is a way to output safe, escaped XML.

The OWASP wiki and StackOverflow recommend to use functions from the class DOMDocument (for XML and HTML) to validate XML and HTML. OwnCloud checks whether this class exists (in util.php), but doesn't seem to use any of its functions.

They don't seem to use any standard functions to sanitize HTML or XML (apart from the built-in escaping in XMLWriter). Instead, they use their own function sanitizeHTML, which can be found in lib/private/util.php. This function calls htmlspecialchars, which transforms <, >, ", ' and & to their UTF-8 equivalents.

public static function sanitizeHTML( &$value ) {
                if (is_array($value)) {
                        array_walk_recursive($value, 'OC_Util::sanitizeHTML');
                } else {
                        //Specify encoding for PHP<5.4
                        $value = htmlspecialchars((string)$value, ENT_QUOTES, 'UTF-8');
                }
                return $value;
        }

Finally we have found a function readData that processes user data. The function has "raw" as a default value for the input type. Whenever the input type is specified as "raw", the function will not sanitize the data. This means that by default, the input will not be sanitized, which seems a risky choice. However, it doesn't seem to be a problem, since the function is only called twice in the entire code, and always with parameter "input type is text", which leads to the input being sanitized.

public static function readData($method, $key, $type = 'raw', $default = null) {
...
    if ($type == 'raw') return $data;
    elseif ($type == 'text') return OC_Util::sanitizeHTML($data);
    elseif ($type == 'int')  return (int) $data;
    elseif ($type == 'float') return (float) $data;
    elseif ($type == 'array') return OC_Util::sanitizeHTML($data);
    else return OC_Util::sanitizeHTML($data);

V6.6 (Don't know)

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

Applied procedure:

1. Consult PHP documentation for a list of LDAP related library functions.
2. grep for calls to these functions

This procedure didn't turn up any hits. That's because ownCloud calls LDAP library functions in an odd way: they construct the names of LDAP functions using string concatenation and then invoke it using the PHP function call_user_func_array. This makes it impossible for Fortify to notice that the LDAP API is being used.

private function invokeLDAPMethod() {
  $arguments = func_get_args();
  $func = 'ldap_' . array_shift($arguments);
  if(function_exists($func)) {
     $result = call_user_func_array($func, $arguments);
     return $result;
}

This means we have to look for calls to `invokeLDAPMethod` throughout the source code. Calls to this function are wrapped in many other functions, but only invokeLDAPMethod calls ldap API functions. Tracking the call graph by hand quickly becomes infeasible. This is exactly the situation to write a custom rule telling Fortify that inputs to invokeLDAPMethod must not be tainted. (Erik: Great that you tried this - even if the problems below meant it did not work in the end)

The first stumbling stone towards our custom rule is that Fortify is case sensitive internally, but PHP identifiers are case insensitive. This isn't documented in Fortify's documentation, but members of the Fortify community forum knew that you can make patterns in custom rules case insensitive.

The second, still unsolved, problem is that Fortify's support for PHP namespaces is buggy: It doesn't give XSS warnings for class methods in namespaces. We were able to reproduce the issue in a minimal test case and wrote an email to the Fortify support team. They confirmed that this is a bug, and will address it in one of the future releases.

OwnCloud makes extensive use of PHP namespaces, so this means that the results of Fortify's data flow analyzer are unreliable. Furthermore the functions vulnerable to LDAP injection have names like "search" and "connect". There are 26 functions with the name "search" in ownCloud so it's hard to pinpoint calls to the ldap search function. We found 7 definitive call sites to the ldap "search". But tracking their arguments back quickly becomes infeasible.

We conclude that the way ownCloud handles ldap queries makes it too hard for the scope of this course to determine whether the requirement is fulfilled.

V6.7 (Don't know)

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

List of functions

To verify that all untrusted data that could possibly be passed to an operating system command is escaped properly, we first need to determine which commands are operating system commands. The PHP manual contains two interesting pages with such functions.

The first is about processes, and allows the programmer to connect and interact with processes or even execute their own on the operating system running on the server. This is obviously the most dangerous one, since this list also includes commands like exec(), system() and passthru(), which execute the given argument directly on the shell of the server with the privileges of the user.

The second one is about filesystem IO and is used to interact with files on the operating system. It can of course also be used to create and delete files with the permissions of the user running the webserver.

Interacting with processes

From the list of process functions, Owncloud only uses the exec() function with possibly untrusted input. However, when we grep for this function, we can see that all of the found calls are escaped when they are used with untrusted input. An example:

exec('command -v ' . escapeshellarg($program) . ' 2> /dev/null', $output, $returnCode);

escapeshellarg is a php built-in that escapes output to pass it safely to an argument of a shell function. Since this is used here to pass an argument to the shell function command, this example is escaped properly.

Another nice example we found while looking for this function is a blacklist check for 'bad' functions in third party addons. When you install an addon in Owncloud, it will first scan the code of the addon with a grep and the following blacklist:

$blacklist=array('exec(','eval('); // more evil pattern will go here later

At this moment it only looks for two functions, while there more dangerous commands. They will probably expand this list in a later version, as the comment there already suggests. (Erik:Interesting that they check add-on's like this.)

File IO

For the analysis of File IO, we need to scan the source code for a lot of potential dangerous function as listed on http://www.php.net/manual/en/ref.filesystem.php. Not all functions that are listed there are dangerous; the following functions are used by Owncloud but we do not consider them harmful or dangerous:

basename / dirname

Does nothing with File IO, only return base path of a link to a file / directory

clearstatcache

Clears the cache of file links, not harmful

disk_free_space, file_exists, filemtime, filesize, filetype

Returns only the amount of available free space on a file system. While this could be a minimal form of data leakage (ie an attacker could learn whether the server's is full or not), it is not dangerous.

glob

Glob finds pathnames matching a pattern. Since this functions only works on strings and not on files it is not considered dangerous in this scope.

disk_free_space, file_exists, filemtime, filesize, filetype, is_dir, is_executable, is_file, is_link, is_readable, is_uploaded_file, is_writable, is_writeable, pathinfo, stat

These functions returns some information about the properties of a file. An attacker could use this to gain more information about files on the file system. However, this is not enough information to actually do an attack and we can thus consider these functions not dangerous.

tmpfile

This function creates a temporary file and returns a file pointer. It does not take any arguments and thus cannot be used with untrusted data.

touch

Touch is used to change timestamps of a file. This could be used for an attacker, for example to delete its traces. However, this is still a minor problem and thus we consider this function not dangerous.

Then there are some functions that can alter files, but only if they are provided by a valid file pointer. Thus, we only need to check if the file pointer is safe (which can be created by fopen or .... These functions are used in Owncloud and work only on valid file pointers: fclose, feof, fflush, fgetc, fgets, flock, fpassthru, fread, fseek, fstat, ftell, ftruncate, rewind.

For the 'more' dangerous filesystem functions, they made an abstraction in the file core/lib/private/files/filesystem.php (which contains the Filesystem class), and thus they are able to do proper escaping. The following functions could be harmful if they are used with untrusted input.

copy

Their copy function can be found in core/lib/private/files/view.php, consists of almost 80 lines and takes also care of file locks. It does also the escaping using a the isValidPath function of the Filesystem class, and can be found in core/lib/private/files/filesystem.php This functions checks whether a path is valid and does not containt escape sequences like /../:

if (strstr($path, '/../') || strrchr($path, '/') === '/..') { return false; }

file_get_contents

file_get_contents is in Owncloud only used in the unit tests as test functions: there they compare the contents of the files read by the internet Owncloud filesystem abstractions classes with a read on the same file with the simple file_get_contents function. If they are the same, then the test passes. Since this function is only used for testing purposes, we can consider it safe.

popen

popen opens a proces file pointer to interact with a proces. This functions is used only once in Owncloud:

 $fp = popen("file -b --mime-type $path 2>/dev/null", "r"); 

where $path is escaped:

$path = escapeshellarg($path);

readfile

Readfile can be used to read arbitrary files and write them to the output buffer. Owncloud uses this function only on trusted data (ie not data from the user):

$view->readfile($versionName);

where $versionName is

'/'.$uid.'/files_versions/'.$filename.'.v'.$revision;

, where the variables used there come directly from the database.

Some dangerous functions were used very extensively, when we looked for them. We used the following find/grep expression to look for them in a manual way:

 find . -name \*.php -exec grep function_name\(\.*$.*,.*\) {} \;

This resulted in a lot of hits, too much to analyse manually in a reasonable time. Thus, for this functions is an automated scanner needed.

file_put_contents

The check for this function resulted in 142 occurences.

fopen

This function also resulted in 142 occurences.

mkdir

This function resulted in 97 occurences.

rename

This function resulted in 98 occurences.

rmdir

This function resulted in 51 occurences.

unlink

This function resulted in 202 occurences.

Summary

All the functions we completely examined in this section were either not dangerous or proper used in this project. However, there were some functions that we were not able to audit completely, simply because it was too much. Therefore, we can see that Owncloud partially passed this requirement.

V6.8 (PASS)

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

After searching through the OwnCloud codebase, it appears that it does not use any interpreters other than the ones already discussed. The only occurrences of 'perl' appeared in a list of mime-types for files the user can store using OwnCloud. In addition to the mime type list, Python also occurred in the documentation of a Javascript function that was ported from Python, but again there were no actual Python scripts to be found. The only matches for 'awk' and 'sed' appeared in cacert.pem files in the 3rd party addons - apparently these three characters names are sufficiently likely to appear in random data. Furthermore, we came across several shell scripts, but they were all aimed at setting up OwnCloud, and could not be executed from the web interface (such as the generation of internationalisation files).