How to Start a Code Review

A code security review should operate very similarly to a code functionality review. Functionality reviews are standard in almost every development organization, which makes the learning curve for code security reviews much shorter.

A first step in reviewing code for security is to pull the branch in question down to a local development machine. Some organizations allow reviews in a web-based editor (like provided by GitHub or GitLab)—but these online tools are not as comprehensive as the tools you can take advantage of locally.

Figure 19-1. GitHub Code Reviews—GitHub and it’s competitors (Gitlab, Bitbucket, etc) all offers web-based collaboration tools for making code reviews easier.

Here is a common local review flow which can be done from the terminal:

1. Checkout master git checkout master

2. Fetch and merge latest master git pull origin master

3. Checkout feature branch git checkout <username>/feature

4. Run a diff against master git diff origin/master...

The git diff command should return two things:

1. A list of files that differ on master and the current branch

2. A list of changes in those files between master and the current branch

This is the starting point for any code functionality review, and any code security review. The differences between the two start after this point.

Archetypical Vulnerabilities Versus Custom Logic Bugs

A code functionality review checks code to ensure it meets a feature spec and does not contain usability bugs.

A code security review checks for common vulnerabilities such as XSS, CSRF, injection, etc. but more importantly checks for logic level vulnerabilities that require deep context into the purpose of the code and cannot be easily found by automated tools or scanners.

In order to find vulnerabilities that arise from logic bugs, we need to first have context in regards to the goal of the feature. This means we need to understand the users of the feature, the functionality of the feature and the business impact of the feature.

Here we run into some differences in what we have primarily discussed throughout the book when we talk about vulnerabilities. Most of the vulnerabilities we have investigated are common archetypes of well known vulnerability. But it is just as possible that an application with a very specific use case has vulnerabilities that cannot be listed out in a book which is designed as a general education on software security.

Consider the following context regarding a new social media feature to be integrated into MegaBank—MegaChat:

“We are building an social media portal that allows registered users to apply for membership.

Membership is approved by moderators based on a review of the user’s activity prior to membership.

Users have limited functionality, but when upgraded to members they have increased functionality.

Moderators are automatically given members functionality, plus additional moderation capabilities.

Unlike users, who can only post text media—members can upload games, videos and artwork.

We gate the membership because hosting this type of media is expensive, and we wish to reduce the amount of low quality content as well as protect ourselves from bot accounts and freeloaders who are only looking to host their content.”

From this we can gather:

Users and Roles - The users are MegaBank customers - The users are split into three roles: user (default), member and moderator - Each user role has different permissions and functionality

Feature Functionality - Users, Members, Moderators can post text - Members, Moderators can post video, games and images - Moderators can use moderation features including upgrading users to members

Business Impact - The cost of hosting video, games and images is high - Membership comes at risk of freeloading (storage / bandwidth cost) - Membership comes at risk of bots (storage / bandwidth cost)

An archetypical vulnerability would be an XSS in a post made by a user. A custom logic vulnerability would be a specific API endpoint which is coding improperly and allows a user to send up a payload with isMember: true in order to post videos although they have not been granted the member functionality by a moderator.

The code review is where we will look for archetypal vulnerabilities, but also try to find custom logic vulnerabilities that require deep application context.

Where to Start a Security Review

Ideally, you should begin your code review with the highest risks components of an application. However, you may not always be aware of what those components are if you have been asked to perform a security review against an application you did not have a say in designing. This is frequent in consulting, or when working on existing products.

As a result, I propose a framework to simplify the security code review process and help you get started with a security review. This framework can be used until you are familiar enough with the given application to begin evaluating features of the application based on risk.

Imagine a basic web application with two components: a client in the browser, and a server that talks to that client.

We can be begin by reviewing the server-side code, sure. In fact there is nothing wrong with that. But—there may be functionality on the server which is not exposed to the client.

That means, that without having a good understanding of the functionality intended to be used by users (versus internal methods and such)—your effort may be accidentally focused into lower risk code when high risk code should be prioritized.

This is a confusing concept to grasp, but just like in the chapter on secure application architecture—we need to realize that in an ideal world every piece of application code would be equally reviewed.

Unfortunately, that reflects an ideal world—and in the real world there are often deadlines, timelines and alternate projects that require attention.

As a result of this, a good place to start in the actual source code is anywhere that a client (browser) makes a request to the server.

Starting on the client is great because it will begin to give you a good idea of the surface area you are dealing with. From here you can learn what type of data is exchanged between the client and server, and if there are multiple servers being utilized rather than one.

Furthermore, you learn about the payloads being exchanged and how these payloads and being interpreted on the server.

After evaluating the client itself, you should follow the client’s API calls back to the server—and begin evaluating calls that connect the client and the server in the web application.

Once this is complete, you should probably consider tracing the helper methods, dependencies and functionality those API’s rely on it. This means evaluating databases, logs, uploaded files, conversion libraries, and anything else that the API endpoints call directly or via a helper library.

Next, cover the bases by looking over every bit of functionality that could be exposed to the client but isn’t directly called. This could be API’s built to support upcoming functionality, or perhaps just functionality that was accidentally exposed and should be internal.

Finally, after those major points in the codebase have been covered—then dedicate your time to the rest of codebase—determining the route taken via analysis of the business logic and prioritization based on the risks you envision such an application encountering.

To summarize, an effective way of determining what code to review in a security review of a web application is as follows:

1. Evaluate the client side code, to gain understanding of the business logic and understand what functionality users will be capable of using.

2. Using knowledge gained from the client review, begin evaluating the API layer—in particular the API’s you found via the client review. In doing this, you should be able to get a good understanding of what dependencies the API layer relies on to function.

3. Trace the dependencies in the API layer, carefully reviewing databases, helper libraries, logging functions, etc. In doing this, you will get close to having covered the majority of user-facing functionality.

4. Using the knowledge of the structure of the client-linked API’s, attempt to find any public facing API’s that may be unintentionally exposed or intended for future feature releases. Review these as you find them.

5. Continue on throughout the remainder of the codebase, this should actually be pretty easy because you will be familiar with the codebase having read through it in an organic method versus trying to brute-force an understanding of the application architecture.

This is not the only method of working your way through a security review, and certain applications with niche security requirements may require a different review path.

However, I suggest this path because it will grant you familiarity with the application at an organic pace and allow you to prioritize user-facing functionality while leaving potentially low risk functionality towards the end.

As you become more familiar with the secure code review process, and the particular applications you find yourself reviewing—you should be able to modify this set of guidelines to better suite your application and the risks your application faces.

Secure-Coding Anti-Patterns

Security reviews at the code level share some similarities with architecture reviews that occur prior to code being written.

Code reviews differ from architecture reviews as they are the ideal point in time to actually find vulnerabilities, whereas such vulnerabilities are only hypothetical if brought up during the architecture stage.

There are a number of anti-patterns to be on the lookout for, as you go through any security review. Many times, an anti-pattern is just a hastily implemented solution—or a solution that was implemented without the appropriate prior knowledge. But regardless of the cause—understanding how to spot said anti-patterns will really help speed up your review process.

The following anti-patterns are all quite common, but each of them can wreak havoc on a system if they make it into a production build.

const blacklist = ['http://www.evil.com', 'http://www.badguys.net'];

/*
 * Determine if the domain is allowed for integration.
 */
const isDomainAccepted = function(domain) {
 return !blacklist.includes(domain);
};

const whitelist = ['https://happy-site.com', 'https://www.my-friends.com'];

/*
 * Determine if the domain is allowed for integration.
 */
const isDomainAccepted = function(domain) {
  return whitelist.includes(domain);
};

Summary

When reviewing code for security, we need to consider more than just looking for common vulnerabilities (which we will discuss in the upcoming chapters)—but also need to consider anti-patterns in the application which may look like solutions but become problems later down the line.

Additionally, code security reviews should be comprehensive—that is, covering all of the potential area for vulnerabilities to be found.

While doing so, we need to consider the specific usage requirements of the application so that we can understand what logical vulnerabilities could be introduced that would not easily fit into a common predefined vulnerability archetype.

When starting a code review—we should take a logical path that allows us to gain understanding of the use cases for the application so that we can begin assessing and evaluating risk in the application.

In more established applications where high risk areas are well known, the majority of reviewing effort should be focused on those areas—with the remaining areas being reviewed in descending order of risk.

Ultimately, integrating security reviews into your code review pipeline will help you mitigate the odds of introducing vulnerabilities into your codebase if done correctly.

The code security review process should be part of any modern software development pipeline, and should be performed by security-knowledgeable engineers alongside the product or feature’s developer when possible.