SoftwareSecurity2014/Group 10/Code Scanning

Case study

The piece of software we decided to focus on is Piwigo: an open-source PHP library for creating and managing photo galleries. It boast a huge amount of features: including different albums, the ability to comment on photo's, user management, admin panels, user-interfaces for uploading photo's etc. This is precicely the type of functionality where a lot can go wrong from a security standpoint, rendering Piwigo an interesting case study.

Code scanning:

Scanning results

Fortify found a large number of vulnerabilities in the Piwigo source:

• 1051 Critical
• 2430 High
• 81 Medium
• 343 Low

Verification requirements

Because of the amount of vulnerabilities, and the scope of the project, we will only look at the critical vulnerabilities

Our focus will be on the following verification requirements:

• V2/V3. Authentication and session management (including a bit of V7. Cyrptography)
• V5. Input validation
• V6. Output Encoding/Escaping

Fortify does group the vulnerabilities by category, so we can match these to the verification requirements we are investigating. The following categories are critical, and are related to V5 and V6.

• Command Injection
• Cross-Site Scripting
• Dangerous File Inclusion
• Code Injection
• Open Redirect
• Path Manipulation
• SQL Injection
• System Information Leak

For V2 and V3 we will make less use of the output of the tool, but look at the places where authentication and session management functionality is present.

Results

Fortify

Input Validation & Output Encoding/Escaping

Fortify finds a large amount of potential input validation and output escaping problems, spread over various categories. These categories are as follows:

• Command Injection
• Cross-Site Scripting
• Dangerous File Inclusion
• Code Injection
• Open Redirect
• Path Manipulation
• SQL Injection
• System Information Leak

All these errors are very relevant from an input validation and output encoding viewpoint.

Cryptography

Fortify manages to find several issues related to cryptographic functions. It detected both weak encryption and weak cryptographic hashes based on encryption standards that have proven to be faulty. For the indepth review, we will not use the output of the tool, but look at the only place where cryptography is prevalent: The password hashing scheme.

RATS

Input Validation

RATS finds 54 potential Input Validation vulnerabilities, where the function accepts user input without verifying that it does not contain anything dangerous. No additional information is given except the location.

Output Encoding/Escaping

RATS has found 4 places wherein potentially dangerous output was sent to an external mail server. Apart from this, no other errors were found.

Cryptography

RATS does not appear to find any errors related to faulty cryptographic functions.

In depth result: SQL Injection

(Erik: Fine to zoom in on SQL injections, as you do here - because there is typically too much to look at and it's better to look one issue in-depth that at all very superficially. It would be good to point out that you make a choice here to focus on SQL injections, and give any motivation for this - if there is any, maybe you just choose to look at SQL injection without any particular reason,.)

Fortify revealed 182 critical SQL injection vulnerabilities in the source for Piwigo 2.6. These vulnerabilities were checked to see whether they are exploitable. A total of 82 of these might be used in an actual external SQL injection.

The other 100 cases of the user input that might lead to SQL injections were validated checked in the Piwigo source. We can define a number of different input validation classes:

is_numeric

In 23 cases, the user input is checked whether it is a numeric value before passing it to the SQL query. Since the data is only checked by this function, but not actually processed into something guaranteed safe. It is entirely dependent on the current implementation of is_numeric.This implementation might change in the future, or it might return True for some unexpected values, like hex, octa or binary strings. Some versions of MySQL do not support octal notation, but is_numeric passes this value as True.

Part of array

In 7 cases, the user input is checked whether it is part of a predefined array. This is a reasonable solution, but it would be better to pre-process the input data before passing it to the query.

check_input_parameter

Piwigo defines it's own input checking function. This basically checks if the parameter is not empty, and whether it holds to a given regular expression. It is good that Piwigo checks the user input to some degree, but with only checking and no preprocessing(Erik:not clear to me what you mean by `pre-processing'), you can never be certain that SQL injections are not possible. This variation is used in 45 cases.

(Erik: The name of this function suggests that this is a general input validation check that is not specific to a context, ie whether the input is meant to be an email address, used in a SQL statement (and hence might cause SQL injection problems), or as HTML (and hence might cause XSS problems), etc. Is that indeed the case? Note that such a general input validation check is a bit suspicious.)

Part of the install process

In 5 cases, the user input is not validated before passing it to the SQL query. But this should not pose a security risk since these functions are only available during installation of the Piwigo application. It would still be better practice to secure these functions for the case that these ever become publicly available when the application is running.

Taint from DB, XSS

In 2 places, strings from the database are literally used in SQL queries. This might be tainted data, for example in the case of Cross site scripting. (Erik: This would not be stored XSS, but rather stored SQL injection :-) Of course, the basis root cause would be the same, namely that an attacker can stick dangerous data in the database. Note that finding out of an attacker could inject SQL into the database is not something that Fortify can easily tell; maybe user gets sanitised before it gets entered in the database.)

Magic quotes

The php magic quote function is used in 1 place. Just as the custom check_input function, this only makes SQL injections harder, but it does not guarantee that a attack string can not pass through. Worse, this is entirely dependent on the current PHP implementation of magic quotes which might be changed at any point in the future. (Erik: The fact that they only use this function in just 1 place suggests to me that the code does not have standard way of doing input sanitisation. Is that indeed the case? The fact that they also have this pwg_real_escape_string, mentioned below, only confirms this suspicion. Or are the magic quote and pwg_real_escape_string used for different purposes/in clearly different contexts?)

pwg_real_escape_string

Piwigo's pwg_real_escape_string suffers from the same issues that magic quotes do. But in this case t is more clear to the program what is actually happening. On the contrary, the checking for real escaped strings is limited by the Piwigo developers imagination. This happens in 9 locations.

Validated by a series of custom functions

In 8 cases a series of custom functions checks the user input. This suffers from the same issues as Magic quotes, but is spread over a number of places, therefore making it harder to verify the entire code. (Erik: Again, this suggests that they don't have a standard way to do input validation, right?)

Exploitable modules

The remaining 82 cases are not checked correctly, or not checked at all. Therefore these are most vulnerable. Refer to the page below for details:

• Exploitable SQL Injections

Mitigation

Prepared statements in php are much safer then only using custom checks on parameters for queries, but these are not used in Piwigo. The Piwigo developers are clearly concerned about security since they actually validate a lot of the input. But the problem is that this validation does not always happen, and it does not happen in a consistent way. (Erik: OOps, I see that here you answer several of my questions above :-)) We already discovered 8 different kinds of input validation (those that are listed here). But there might be more in parts of the code that Fortify did not mark as critical. Prepared statements with a small set of central validation and pre-processing functions would significantly diminish the risk of SQL injections.

Dynamic Code Evaluation

Reported as: Critical Real impact: Low

Fortify reports on five critical cases if Dynamic code Evaluation. Dynamic code evaluation is the ability to execute code based on the input received from the user. It is dangerous to let unauthorized users run code in the webapplication without proper validation.

As long as the user is logged in as a webmaster, he can visit use the $_POST['text'] parameter from functions.inc to execute arbitrary unchecked code. Piwigo does require the user to be logged in as webmaster, and the user account validation seems to be implemented correctly.

System Information Leak

Reported as: Critical Real impact: Low

Fortify reports on one critical issue of system information leak. Namely one call to phpinfo(). This information could help adversarys to form a plan of attack. This call is part of intro.php, and is served by a simple request in the form om: isset($_GET['action']) and 'phpinfo' == $_GET['action']

In general this is a bad thing to have. But this call is part of the admin section of Piwigo, and a check in the form of: check_status(ACCESS_ADMINISTRATOR) is performed before allowing this request to be served. Once an attacker has Administrator access in order to use this information, he will have not much use for the phpinfo call. Therefore the impact of this vulnerability is low.