SoftwareSecurity2013/Group 5/Verdict

ASVS Level 2B Report

Scope of the assessment & sampling strategy

For this ASVS evaluation level, a manual review of the code is required, in order to check that all the code present in the application complies with all the relevant items. Within the scope of this review is all code needed to run the application, regardless of the origin of the code (i.e. 3rd party source code is also under scope). However, since the codebase is so large that it is unfeasible for a three-person group to evaluate all the codebase, we have decided to base our analysis on a sample of all the codebase. For this code sampling, we have mainly used three different strategies, always having in mind that we have to check for compliance against ASVS requirements V3, V10 and V11: grepping, fortify reports from previous assessment and doxygen navigation. Additionally, it should be stated that we also googled for the requirements + the "mediawiki" string(Erik:Not quite clear to me what `the requirements+ the mediawiki string' means here. ) (Rafael Boix Carpi:we meant the keywords of the requirements concatenated with the mediawiki string, e.g. "http cookie mediawiki" ) , especially for V10.

Grepping

Since our requirements refer to session management, communication security and http security, we have performed a filename search containing related keywords: http, cookie, session, log, user, request, response, header and so on. With this strategy, we got a list of files to be explored, and after manual inspection we checked that the sampled codebase by this method was actually related to V3 and/or V11 reqs. It should be stressed that we found nothing for requirement group V10, though.

Fortify findings

Since we have the fortify report available from the previous level1B assessment, we actually know some files that were not highlighted by the grepping but were highlighted by Fortify. In addition, since Fortify actually classifies the possible findings into different categories, we have a better idea on what requirements to check against inside these files.

Doxygen navigation

On top of these two sampling techniques, we noticed that the codebase has support for automatically generating a report which contains the description of the semantics of the codebase, the code structure and data/functional hierarchy of the codebase by using the Doxygen tool. We ran this tool over the entire codebase, and obtained an 800(!!)MB report which, fortunately, contains not only the previously mentioned data, but also some nice function call graphs and groups of related files; however, the generated report is still quite large for a complete manual inspection. As a side comment, this was a really nice feature, because e.g. one of these groups was HTTP.

Google

Googling a bit revealed that the code related to V10 for this application is not in the provided codebase, since it belongs to the webserver. The typical setup would require to additionally review the apache2 web server source code (which is insanely complex and large).

Reviewed files and short description of their functionality

HttpFunctions.php

Generally speaking, it is a wrapper class for handling HTTP requests. Whenever a HTTP request arrives, it is either forwarded to cURL or passed on to internal PHP functions if necessary for handling internal requests. It parses the HTTP requests with all the accompanying headers, sets the variables and options respectively, fetches necessary information and sends back a response. It is closely related to "CookieJar.php" and "Cookie.php". First the headers, including the HTTP status code and any Set-Cookie headers are parsed. Afterwards it fetches the newest url after all redirections, parses the cookie into the response header and saves it into the cookie jar.

HttpFunctions.old.php

Deprecated file, as specified in the file comments.

Cookie.php

Custom definition of the Cookie class and methods for reading/setting values inside cookies

CookieJar.php

Custom definition of the CookieJar class and methods for cookie management.

User.php (With special focus on calls of Cookie::setCookie)

The file user.php contains a User class to work with user data. A big amount of user information is stored in several arraylists of this class. The User class interacts with more classes (also contained in this list) for management of cookies and sessions. NB: most of the functions are contained in WebRequest.php and WebResponse.php, whether HTTP headers are processed, and also Cookie.php for cookie handling.

Webrequest.php

This file contains a WebRequest class, which is a wrapper of the HTTP request in the form of a global singleton, which is then accessed by instances that refer to it. Among the methods of this class there are those who manage the HTTP headers, field sanitization, etc...

Webresponse.php

The counterpart to Webrequest.php, this file contains the Webresponse class, which is an object accessible via Webrequest::response(). The functionality of this class is to manage the HTTP response that is being sent, along with cookie and HTTP headers management.

SeleniumWebSettings.php

The purpose of this file is to implement functionality for modifying MediaWiki settings based on the current session & cookie settings in a testing framework called Selenium. The purpose of this framework is to test the integration of mediaWiki in a production environment, and although it is still present in the current codebase, it is set to phase out and be removed.

FauxResponseTest.php

This file contains code for testing the FauxResponse class (it is a dummy HTTP response, present in Webresponse.php).

WebStart.php

This does the initial setup for the webrequest singleton; namely, it does some security checks for the global variables not being overwritten oustide of the scope of the application, and additionally starts the profiler and loads the Mediawiki app configuration.

GlobalFunctions.php

The name says it all: a collection of functions that are used everywhere. Very large file, with almost 4k lines of functions with very different purposes.

V3 - Session Management Verification Requirements

Goal of this requirement group

The Session Management Verification Requirements define a set of requirements for safely using HTTP requests, responses, sessions, cookies, headers, and logging to manage sessions properly. NB that the important idea here is the safe use.

V3.1

Definition

Verify that the framework’s default session management control implementation is used by the application.

Verdict for this requirement

PASS

Motivation for the verdict

For the assessment of this requirement, we based our analysis on the OWASP guidelines found at: https://www.owasp.org/index.php/Session_Management_Cheat_Sheet An example of related code can be found on Mediawiki/include/user.php, in User::newFromSession() Overall, the sampled code is working OK with sessions. The user class is using sessionID or user token to keep sessions. Those tokens or sessionIDs must follow some steps as: session id length, session id Entropy, session id name fingerprinting and content. OWASP recommends the use of cryptographic functions as SHA1, but mediawiki uses md5 to generate those tokens. Despite this, the recommended steps are correctly done. If mediawiki uses MD5 function, they should remove unsalted validations. In the code we observed three types: unsalted, old-style and salted. Next steps are also achieved, such as entropy, name fingerprinting and length using cryptographic functions. Some points present in the provided link are not applied. Those aspects are discussed in SetCookie() and SetCookies() later. We present some code snippets

    public static function newFromSession( WebRequest $request = null ) {
        $user = new User;
        $user->mFrom = 'session';
        $user->mRequest = $request;
        return $user;
    }
  

Some aspects about sessions management at mediawiki. These are some extracted lines from the code (they are not contiguous in the file PHP).

  define( 'USER_TOKEN_LENGTH', 32 ); // it requires at least 16 bytes! OK! 
  $token = MWCryptRand::generateHex( 32 );
  $hash = md5( $token );  // it uses crypto. MD5 at least, not predictable.
  

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.2

Definition

Verify that sessions are invalidated when the user logs out.

Verdict for this requirement

PASS

Motivation for the verdict

The code under review (User::doLogout() ) correctly performs this operation.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.3

Definition

Verify that sessions timeout after a specified period of inactivity.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Mediawiki depends on PHP session management for this timeout; PHP session expiration should be checked for an assessment on this requirement.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.5

Definition

Verify that all pages that require authentication to access them have logout links.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

The codebase is too large for actually giving an actual assessment; so far we did not find suitable code to give an assessment on this requirement.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

(Erik: Note that for this requirement is probably more natural to check it using a dynamic analysis - ie looking at pages of an mediawiki site and seeing if there are logout links - rather than inspecting the source code)

V3.6

Definition

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.

Verdict for this requirement

PASS

Motivation for the verdict

The code under review (User::matchEditToken() ) correctly performs this operation.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.7

Definition

Verify that the session id is changed on login.

Verdict for this requirement

PASS

Motivation for the verdict

The relevant functions in files includes/GlobalFunctions.php and includes/specials/SpecialUserlogin.php enforce this session id change.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.8

Definition

Verify that the session id is changed on reauthentication.

Verdict for this requirement

PASS

Motivation for the verdict

The code under review (User::clearInstanceCache() ) correctly performs this operation.

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V3.9

Definition

Verify that the session id is changed or cleared on logout.

Verdict for this requirement

NEEDS REVISION

Motivation for the verdict

This ASVS level verifies if sessions are correctly cleared, and a bug was found in their Bugzilla website (https://bugzilla.wikimedia.org/show_bug.cgi?id=44327#c0 ). The issue here is that since anonymous users have cookies with a very long validity timespan, it could enable fingerprinting of anonymous user (e.g. sharing these cookies with google would enable to fingerprint users and send ads with an specific profile). The code we are reviewing is 1.20.3, but this bug has been fixed as of version 1.20.5. It should be noted, though, that we did not spot the security flaw by ourselves during the code review.

Comments and/or relevant findings

The functions we checked (e.g. doLogout() in Mediawiki/includes/User.php), perform a correct clearing of the sessionID.

Suggested mitigation / action to be taken (if applicable)

Apply the appropriate bugfix or update the codebase.

V10 – Communication Security Verification Requirements

Goal of this requirement group

The Communication Security Verification Requirements define a set of requirements that can be used to verify that all communications with an application are properly secured.

Global comment

We were unable to find code relevant to this requirement, because most of the communication security-related processing is done outside of the given codebase (namely in the Apache2 configuration if Apache is used as web server); for an actual assessment we would need to check the actual deployment or the web server to be used, in order to further check these requirements.

V10.1

Definition

Verify that a path can be built from a trusted CA to each Transport Layer Security (TLS) server certificate, and that each server certificate is valid.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V10.3

Definition

Verify that TLS is used for all connections (including both external and backend connections) that are authenticated or that involve sensitive data or functions.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V10.4

Definition

Verify that backend TLS connection failures are logged.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V10.5

Definition

Verify that certificate paths are built and verified for all client certificates using configured trust anchors and revocation information.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V10.6

Definition

Verify that all connections to external systems that involve sensitive information or functions are authenticated.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V10.7

Definition

Verify that all connections to external systems that involve sensitive information or functions use an account that has been set up to have the minimum privileges necessary for the application to function properly.

Verdict for this requirement

NOT CHECKED

Motivation for the verdict

Did not find relevant code for this requirement

Comments and/or relevant findings

Suggested mitigation / action to be taken (if applicable)

V11 – HTTP Security Verification Requirements

Goal of this requirement group

The HTTP Security Verification Requirements define a set of requirements that can be used to verify security related to HTTP requests, responses, sessions, cookies, headers, and logging. NB that the important idea here is the proper implementation of the HTTP communication.

V11.1

Definition

Verify that redirects do not include unvalidated data.

Verdict for this requirement

PASS

Motivation for the verdict

Redirects are disallowed by default. This is documented by the authors of the code.

Comments and/or relevant findings

Evidence can be found in file:

• Mediawiki/includes/HttpFunctions.php Line 52

Suggested mitigation / action to be taken (if applicable)

V11.2

Definition

Verify that the application accepts only a defined set of HTTP request methods, such as GET and POST.

Verdict for this requirement

FAIL

Motivation for the verdict

There are two functions generating requests objects:

• Http::Request
• UploadFromUrl::reallyFetchFile

The latter violates the rule, because it bypasses the Http::Request function and does not require any predefined HTTP request method.

Comments and/or relevant findings

Evidence:

• File Mediawiki/includes/HttpFunctions.php Line 59

Call graph of the two functions

Suggested mitigation / action to be taken (if applicable)

Either fix this behavior by using the default http::request function, or if for safety/security reasons this function has been overriden, then fix the outer http::request function to support all these object requests.

V11.3

Definition

Verify that every HTTP response contains a content type header specifying a safe character set (e.g., UTF-8).

Verdict for this requirement

FAIL

Motivation for the verdict

The content-type written in the HTTP request is passed on to the HTTP response. There is no content-type validation done for HTTP requests/responses, except for a POST 1.0 request.

Comments and/or relevant findings

Evidence can be found here:

• File Mediawiki/includes/HttpFunctions.php MWHttpRequest:parseHeader(), MWHttpRequest:getHeaderList()

MWHttpRequest::parseHeader() sets the response header with what is saved in $headerList, which is again generated in MWHttpRequest:getHeaderList() from the request header.

Suggested mitigation / action to be taken (if applicable)

Manual code review and correction for enabling a correct content-type field in the HTTP responses.

V11.4

Definition

Verify that the HTTPOnly flag is used on all cookies that do not specifically require access from JavaScript.

Verdict for this requirement

FAIL

Motivation for the verdict

Cookies are created from a call in User.php to Webresponse.php. However, this function does not set neither HTTPonly nor security flags anywhere. With a quick code review, it can be seen that a cookie with those flags could be created with this line (webrequest.php): setcookie( $name, $value, $expire = 0, $prefix = null, $domain = null, $optional ) being optional any of those flags if they are needed!

Comments and/or relevant findings

Evidence:

• File Mediawiki/includes/User.php User::setCookie($name,$value,$exp)
• File Mediawiki/includes/WebResponse.php WebResponse::setcookie($name, $value, $expire = 0, $prefix, $domain)

Suggested mitigation / action to be taken (if applicable)

Modify the code for setting this flag.

Function call graph for the code to be corrected

V11.5

Definition

Verify that the secure flag is used on all cookies that contain sensitive data, including the session cookie.

Verdict for this requirement

FAIL

Motivation for the verdict

Function in class 'User' set a cookie without setting the 'secure' flag.

Comments and/or relevant findings

Evidence:

• File Mediawiki/includes/User.php User::setCookie($name,$value,$exp)
• File Mediawiki/includes/WebResponse.php WebResponse::setcookie($name, $value, $expire = 0, $prefix, $domain)

Suggested mitigation / action to be taken (if applicable)

Modify the code for setting this flag.

V11.6

Definition

Verify that HTTP headers in both requests and responses contain only printable ASCII characters.

Verdict for this requirement

FAIL

Motivation for the verdict

No ASCII-encoding validation function could be found in some files. Webrequest.php performs data sanitization, but e.g. HTTPfunctions.php does not.

Comments and/or relevant findings

• File Mediawiki/includes/HttpFunctions.php

Suggested mitigation / action to be taken (if applicable)

Consider the input as tainted and act accordingly could be an option, but we consider it is preferable to sanitize the HTTP headers first (we could not find sanitization for non-printable ASCII characters).