OpenTrade Security Analysis

Posted by Ben Hayward on Feb 4, 2019

OpenTrade Security Analysis

 

The Arcadia Group, In Coordination with Evimeria

Initial Discovery by: Minh Khai Do & Joel Farris

By: Ben Hayward

Foreword

This document outlines a range of vulnerabilities and weak points found within the OpenTrade system by 3s3s. Firstly a manual code review was conducted to establish any clear weak areas, and following that a range of static analysis and penetration testing tools were further used to augment the search. Advice on best practices is offered as well as all test data.

The equipment used in this test was one EC2 small instance on Amazon Web Services (AWS), running Ubuntu 16.4 LTS. Ports were opened up to allow all access solely to the main lab, running Manjaro 18.0.2, and TLS certificates on the machine were manually generated and self-signed.

Code Review

Data Validation & Sanitization:

In places in the system there are weak points in data validation, and sanitization. For example, when the user logs in, data is not thoroughly validated. It is checked on the client side at static_pages/js/login.js, testing whether the user is equal to null, and an authentication request is dispatched to the server through SSL. When received, the only server-side validation performed is to check both username and password have indeed been passed. It is best practice to thoroughly validate the data that you are expecting on the server-side.

This is generally bad practice as it can open up various vectors for an attacker to inject data into the system. It is imperative that before doing anything with the data on the server, it is validated to be in the expected shape, else an attacker can potentially turn off javascript on their browser, bypassing the client-side validation entirely and sending data through that the system is not prepared to handle.

 

Client side Validation Example

The client side should not be neglected either, though it is not a critical security concern; you can just take some load off the servers if you check that a user has entered their data in the expected format on the client-side, then re-check again on the server if the client has confirmed that it is valid. This means the client does not have to send a request to the server to check whether their password is enough characters . Ultimately under high server load requests like this on a smaller server could amount to an unintentional DOS.

If you want to avoid using a third party library, validation could be taken care of using regex, e.g. for password, something like:

“^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)(?=.*[@$!%*?&])[A-Za-z\d@$!%*?&]{8,}$”

Regex definition for a field with 8 chars minimum,

1 upper case letter, 1 lower letter, 1 symbol and a number

Alternatively, it is possible to use a third party library like express-sanitizer or express-validator to take care of the job, depending on your preference.

Data is also not always escaped before it is consumed on the server-side, which opens up a variety of Cross-site Scripting (XSS) vectors for attack, such as SQL Injections, compromising security of the database. This is another issue that can be solved using a library, such as express-validator.

An example of a lack of data validation & sanitization (static_pages/js/login.js)

Users Generated from SQL row number:

From the below code snippet, it is implied that the Users RowID is used in the system — this is generally bad practice, as it potentially reveals information to the end-user, that they should not have access to. The best solution for this would be to rework the ID system to generate a new GUID for the user on creation, and refer to them by that instead.

g_constants.dbTables['users']
    .selectAll('ROWID AS id, *', 'login="'+escape(user)
    +'" AND email="'+escape(email)+'"', '', (err, rows) => {

Static Analysis:

ZAProxy:

Testing using OWASPs ZAProxy returned a few issues, only one medium in severity, the rest low. This can be seen in the following table. Full CSV data is also available at:

https://cloudflare-ipfs.com/ipfs/QmZ8Zz1wKZnLeRmCdi9WjfPufuFsV5Li8LsG739XAdnQkW

Backup: https://drive.google.com/file/d/1JR8O9yU-M04_1xD1Jz9vL1y2ChuaGsNW/view?usp=sharing

 
 

The Anti-MIME-Sniffing header X-Content-Type-Options was not set to ‘nosniff’. This allows older versions of Internet Explorer and Chrome to perform MIME-sniffing on the response body, potentially causing the response body to be interpreted and displayed as a content type other than the declared content type. Current (early 2014) and legacy versions of Firefox will use the declared content type (if one is set), rather than performing MIME-sniffing.

LGTM:

Analysis was furthermore, a static analysis was ran using LGTM, which highlighted a

few different bugs and vulnerabilities, the full scope of which can be found here:

https://lgtm.com/projects/g/3s3s/opentrade/alerts/?mode=list

Summary of Vulnerabilities and Errors:

 

OWASP Dependency Check:

Dependencies for OpenTrade are scarce, but for the sake of completeness it made sense to check that no dependencies have any publicly known vulnerabilities. As expected this returned a clean sheet, which can be viewed below:

https://cloudflare-ipfs.com/ipfs/QmdXbC6DRKCRQvgswQwEhaTd2z2CmwHcdJqEp728V8XpFD

This was tested a second time using retire.js instead, which returned the same clean sheet.

Other Issues:

Other unrelated issues, that aren’t strictly in the codebase include the recommended node version of 8.0.0 — This is likely bad advice, and you’d be best informing users to remain on the LTS branch of node, such as v8.9.0 — As long term support is offered, this is a much safer alternative for production systems.

Comments in the codebase are quite scarce, this isn’t a direct security issue but it should be noted that in an open source project, you should be aiming for your code to be as legible as possible, so that users have an easier time understanding the internal workings, and can identify and highlight any issues.

Conclusion:

Tests have highlighted a few key areas that need work before OpenTrade is entirely production ready — Some of the highlighted vulnerabilities are likely false positives, but the most important areas are to mitigate the chance of DDOS by rate-limiting requests — This is something that can arguably dealt with deployment, but there’s no indicator to the user that it is required, and to make the code sturdier, it would be highly beneficial.

Another key area is to set an X-Frame-Options header, to reduce chances of a user being Clickjacked, this should be accomplishable with the cors package already imported. Furthermore attention should be paid to making sure strings are fully escaped, and data validation needs implementing on the Server-side, as there appears to be little done in a lot of cases, outside of the client.