SoftwareSecurity2014/Group 10/Verdict

LEVEL 2B - Code Review (Partial Manual Verification)

V3. Session management

Nr	Verification Requirement	Pass/Fail/N/A	Comment
V3.1	Verify that the framework’s default session management control implementation is used by the application.	Pass	PHP's standard session management mechanism is used, with session ID's being stored in the database rather than on disk. A few useless wrappers are added around this standard functionality, but these do not cause issues. (Erik: If these wrappers are useless, I would say they SHOULD be removed, and I'd qualify r this as an "almost Pass". NB it is important to get rid of any spurious code that only obfuscates was is happening, unless it makes sense as `defence in depth`, but I don't think that's the case here. )
V3.2	Verify that sessions are invalidated when the user logs out.	Pass	This is handled properly.
V3.3	Verify that sessions timeout after a specified period of inactivity.	Pass	Sessions time out after one hour by default. It can be argued that this period could have been shorter, but an hour shouldn't be too much.
V3.5	Verify that all pages that require authentication to access them have logout links.	Pass	A logout button is present in the site header, which appears to be present on all pages.
V3.6	Verify that the session id is never disclosed other than in cookie headers; particularly in URLs, error messages, or logs. This includes verifying that the application does not support URL rewriting of session cookies.	Fail	In admin/include/photos_add_direct_prepare.inc.php, the session identifier is directly included in a Javascript template send to the client (meaning it is accessible by any injected malicious scripts on the same page, despite the session cookie having HTTPOnly set). Furthermore, CSRF tokens are deterministically generated (using a hash function) from the session identifier and a server-side secret. While this hash can not be inverted, if one were to obtain this secret (which is the same for all users), it would become possible to perform an off-line test to see whether a certain session ID is correct.
V3.7	Verify that the session id is changed on login.	Pass	This is the case. The programmer even added a comment stating that this was for security reasons and provided a link to a paper describing the session fixation attack.
V3.8	Verify that the session id is changed on reauthentication.	Pass	The routine that refreshes the session ID seems to be invoked in any situation where the user is authenticated.
V3.9	Verify that the session id is changed or cleared on logout.	Pass	This is handled properly.

Further comments

• Session variables may contain objects that are frequently deserialised. If an attacker could change the content of such a session variable, they would be able to achieve remote code execution. Of course, session variables are stored on disk and can be assumed to be trusted. Nonetheless, this is not desirable from a defense in depth-standpoint.
• HTTPS is not required (and might not be even supported). So session cookies are not protected in transit.

V5. Input validation

Nr	Verification Requirement	Pass/Fail/N/A	Comment
V5.1	Verify that the runtime environment is not susceptible to buffer overflows, or that security controls prevent buffer overflows.	Pass	The OWASP Wiki (https://www.owasp.org/index.php/Buffer_Overflows) mentions PHP as a runtime environment which is not susceptible to buffer overflows as long as no external programs or extensions are used. Since no external programs or extensions could be found, this requirement is considered to be met.
V5.2	Verify that a positive validation pattern is defined and applied to all input.	Fail	This requirement is not met. Considering the following: Several places (themes.php line 34, updates.php line 34 as examples), use the $_GET method and use its contents without checking them. This means that the code is vulnerable against File Inclusion.
V5.3	Verify that all input validation failures result in input rejection or input sanitization	Pass	Piwigo has some emergency doors which are a good thing to implement in the code. Example: if (!in_array($page['tab'], $conf['LocalFilesEditor_tabs'])) die('Hacking attempt!'); The die() function behaves just like the exit() function, and while this method is a poor choice for several reasons, it does mean the hacker is incapable of manipulating the url in his favor.
V5.4	Verify that a character set, such as UTF-8, is specified for all sources of input	Pass	Input validation should be done by defining a set of valid (permissible) characters in a set, and then checking each character of the input against the valid set. If a character from the input is not found in the valid set, the application should return an error page and state that invalid characters were found in the input. We found following output which results in verifying that character set is specified for all sources of input. Example:- if (preg_match('/^[a-zA-Z0-9-_]+$/', $file )) { //Perform some actions; }
V5.5	Verify that all input validation is performed on the server side	Fail	Server side validation (Watermarking). PIWIGO creates a watermark image while uploading a file in the Server's Database. This action is performed on Server side. We checked for vulnerability where PIWIGO creates temporary file with random name in the directory. client side validation Client side verification is implemented mainly using Javascript. If something is wrong, the alarm can be triggered upon submission of the form. Example:- onsubmit="return this.q.value!= && this.q.value!=qsearch_prompt;"> creates client side validation for Search in menu bar.
V5.6	Verify that a single input validation control is used by the application for each type of data that is accepted	Fail	There seems to be no centralized method or place in the code for input validation, meaning that each place where data of the same type is accepted has its own ways of dealing with it.
V5.7	Verify that all input validation failures are logged.	Fail	As previously discussed, the die() method is used to reject invalid urls. (See Admin.php line 41 and Themes.php line 26 for examples). This method prevents the error from being logged, and thus, there is no way of telling when input validation has failed or when a user is trying to hack the site.

Discussion

1. Other than verification requirements, we have also found that PIWIGO executes the code on the left of '.' during file uploading. Though instead of simply checking the extension string present in the filename, PIWIGO is extracting the file extension searching for the ‘.’ character in filename, and extracting the string after the dot character. This is one of the basic vulnerabilities where :-‘filename.php.123’ is executed as 'filename.php'. This only works if the last extension (like .123), is not specified in the list of mime-types known to PIWIGO's server.

2. Another popular way of securing file upload forms is to protect the folder where the files are uploaded using .htaccess file. The idea is to restrict execution of script files in this folder. .htaccess file typically contains the code which is used to restrict attacker to upload the files or execute file in UPLOAD_FOLDER.

V6. Output Encoding/Escaping

Nr	Verification Requirement	Pass/Fail/N/A	Comment
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.	Fail	Piwigo relies on self-written verification methods that are not used consistently throughout the entire project (The same is true for input validation with SQL injection). Therefore not all output can be guaranteed to be checked correctly. OWASP has a nice overview for output-encoding related to XSS https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#Output_Encoding_Rules_Summary, but those rules are not followed here
V6.2	Verify that all output encoding/escaping controls are implemented on the server side.	Pass	While output encoding might not be implemented correctly in all cases, it does rely on server sided scripting, and is not performed on the client side.
V6.3	Verify that output encoding/escaping controls encode all characters not known to be safe for the intended interpreter.	Fail	Piwigo only passes HTML Entity encoding (Convert ' to &#x27 etc) and HTML Attribute encoding (Escape all non-ascii characters with the HTML Entity &#xHH). URL Encoding, JavaScript Encoding and CSS Hex Encoding are only used in some places.
V6.4	Verify that all untrusted data that is output to SQL interpreters use parameterized interfaces, prepared statements, or are escaped properly.	Fail	There is not a single instance of parameterized queries in Piwigo. Also, often untrusted data is passed to SQL interpreters (See our section on SQL injection about this)
V6.5	Verify that all untrusted data that are output to XML use parameterized interfaces or are escaped properly.	Fail	Piwigo's makes use of their own XML encoding throughout the application. This encoding seems to be adequate. But there are 6 libraries in use by Piwigo that are using XML, and these are not checked.
V6.6	Verify that all untrusted data that are used in LDAP queries are escaped properly.	Not applicable	Piwigo makes no use of LDAP queries.
V6.7	Verify that all untrusted data that are included in operating system command parameters are escaped properly.	Fail	In functions.inc Piwigo reads the $_POST['text'] parameter and 'evals' this after only using a 'stripslashes' function on it. This is bad, but the functionality is only available to a user with webmaster autorization. Therefore the risk is mitigated.

Further comments:

Both Piwigo's input and output validation are not secure. The use of a few centralized functions, and Parameterized queries that are used everywhere in Piwigo would increase the security a lot.

V7. Cryptography

Three forms of cryptography appear to be available in Piwigo:

• Password hashing.
• A custom random-number generator.
• (Ab)using MD5 to obtain additional "random" identifiers.

Nr	Verification Requirement	Pass/Fail/N/A	Comment
V7.1	Verify that all cryptographic functions used to protect secrets from the application user are implemented server side.	Pass	The cryptographic functions are only present server-side.
V7.2	Verify that all cryptographic modules fail securely.	Pass?	It does not appear that the password hasher and random number generator have dangerous fail states. However, since many PHP standard functions have a tendency to fail in inconsistant manners for weird reasons we are not very sure of this.
V7.3	Verify that access to any master secret(s) is protected from unauthorized access (A master secret is an application credential stored as plaintext on disk that is used to protect access to security configuration information).	Pass	There is a single 'secret key' in the configuration file which appears to be adequately protected from unauthorized access. This key should not really be there, though, as it is only used to deterministically generate values which should be random (see below).
V7.4	Verify that password hashes are salted when they are created.	Pass*	Password hashes are indeed salted, but that fact alone does not make them secure password hashes. See the notes.
V7.5	Verify that cryptographic module failures are logged.	Fail	No failures are ever logged when it comes to these cryptographic components.
V7.6	Verify that all random numbers, random file names, random GUIDs, and random strings are generated using the cryptographic module’s approved random number generator when these random values are intended to be unguessable by an attacker.	Fail	Random numbers intended to be unguessable are generated in a variety of ways, none of which are secure. See below.

Bad random numbers

The session management module has a function called generate_key which is intended to generate random byte strings. It is (luckily) no longer being used for generating session identifiers, but it is employed to obtain password restoration tokens and to generate random (and only six characters long) administrator passwords.

This function is a very bad custom PRNG seeded by a timestamp: it invokes a non-cryptographic Mersenne-Twister seeded by a hash of microtime() (which can only have 10 million different outputs) and then incorrectly draws a sequence of characters from it that is heavily biased.

Furthermore, a lot of identifiers that are supposed to be unpredictable (such as temporary file names) are generated by simply calculating the MD5 hash of something like microtime() or, even worse, rand().

Cryptographic hash functions are also used in places where actually random numbers are needed. CSRF tokens and 'remember me' tokens are deterministically generated with a hash function from values that are expected to be secret (such as user passwords and a static application-wide secret key not used for anything else). It is quite likely that there are many issues with these custom constructions.

What Piwigo really needs is to utilize a secure random generator and use that instead of generate_key and all these other custom random identifier generators. Unfortunately PHP does not directly provide that, so they would need to rely on extensions (such as OpenSSL) or the operator system (/dev/urandom or the Win32 CryptoAPI) for this.

Password hashing

An important aspect of password security is proper password hashing: applying a one-way function to passwords in order to obtain a digest against which you can compare passwords, but from which you can't obtain a password unless a brute-force attack is mounted. Since passwords can have a high value for attackers (especially when combined with e-mail addresses) it is beneficial to properly hash them so the impact of database compromise (through a SQL injection, for example) is reduced.

It should only be possible to attack a proper password hashing function through a brute-force attack, and these attack should be made as difficult as possible by designing the function is such a way that they are slow to compute (but not so slow that they become a potential denial-of-service vulnerability). Furthermore, since password hashing is a form of cryptography, it is preferable to use an established library rather than too try rolling out custom schemes.

Fortify did not gave results directly related to password hashing, although it did complain about the use of the weak cryptographic cipher MD5. In my opinion, any code using any cryptographic hash function (whether a broken one like MD5 or a strong one such as SHA3) is suspect since it probably represents an attempt of the authors to implement a custom cryptographic scheme. In the case of Piwigo, hash functions are used for password hashing, so that code deserves a close look.

Piwigo's password hashing

To perform password hashing, Piwigo uses phpass. It is configured to always use portable hashes (iterated MD5 with salt), even if a newer version of PHP is available (in which case, phpass supports a much preferable bcrypt-based hashing scheme). Since Piwigo 2.6 requires at least PHP 5.2, this should always be the case.

The essence of phpass's MD5-based scheme can be found in lines 132-135 of include/passwordhash.class.php:

   $hash = md5($salt . $password, TRUE);
               do {
                   $hash = md5($hash . $password, TRUE);
               } while (--$count);

Basically the hash function is iterated $count times, while constantly adding the password again in each iteration. This approach is far from best practice: even when MD5 is the only available cryptographic primitive, the standardized PBKDF construction (which has been specifically designed by cryptographers for this purpose) would be highly preferable to this custom scheme.

The iteration count used by Piwigo is 262144.

Strength of phpass-MD5

The MD5-configuration of phpass is widely used in older versions of Wordpress. Because of this, it has become an interesting target for password crackers. It is, for instance, used to benchmark the oclHashcat tool. In this benchmark, more than 6 million of these password hashes can be generated per second by just using the GPU of a consumer PC. Since Wordpress uses just 8192 iterations (Piwigo uses 32 times as many), it is probably safe to say it is achievable to test 180 thousand Piwigo hashes per second.

The number of hash executions per second can not directly be translated to the number of passwords that are actually cracked every second, since that depends on the complexity of these passwords. Still, it is probably safe to assume that an attacker which obtains the password hashes from a Piwigo database (through a SQL injection, for instance) will be able to retrieve a considerable amount of plaintext passwords in little time by using standard tools such as Hashcat.

Fix

The strength of Piwigo's password hashes can be improved considerably by simply changing two lines of code: namely lines 1029 and 1087 of include/functions_user.inc.php. In these lines the constructor of phpass's PasswordHash class is invoked with the $portable_hashes parameter set to true (forcing the weak MD5 construction) (Erik: Note that MD5 is weak when it come to finding collisions, but finding collisions is not an issue with hasing password files. Using MD5 for your passwords is fine, as long as the numbers of iterations is high enough to make life hard for password crackers.). By changing setting this value to false, phpass will instead start using a much stronger Bcrypt construction (which is always available in PHP versions supported by Piwigo).

Since phpass is designed to still support verification of hashes through the weaker scheme, changing this should (probably) not break compatibility with existing installations. On the other hand, this will mean that anyone registering to a Piwigo site before the application of this fix would still have their password hashed by the insecure scheme. Therefore it is recommended to implement rehashing of old passwords whenever users log in (a similar fix seems to have already been put in place when an older version of Piwigo moved from plain MD5 hashes to phpass).

Furthermore, the other paramater passed to the PasswordHash constructor determines the iteration count. When moving to the new scheme, this number should be tweaked to a reasonable default (as high as possible, without causing too severe performance loss or a denial-of-service opportunity). This parameter should also be configurable by the library user.