Security Code Review

Introduction

Many organizations have switched in recent years to performing some form of code review. This trend is absolutely great as I am a big believer in the code review as part of early defect detection strategies. During the course of the normal code review, developers are looking for things such as failure to free memory, uncaught exceptions, null referenced pointers, logic flaws. I am not here to argue for the inclusion of code reviews in your organization. I am here to argue for and outline how a security code review should work.

A security review should focus more on the security vulnerable areas of the application. Good developers tend to write secure code however, good developers also make mistakes.

Most code reviews rely on a single engineer looking at another engineer’s code & providing feedback. A security review can work the same way, however I believe it’s best to have 2 security aware engineers reviewing the code during a security code review.

Method

The method for a security should be a multi pass approach review, and  should look at these 5 areas:

1. Environment
2. Low Hanging Fruit
3. Technical Control
4. Returns
5. Pointers

Environment

The first thing that reviewers want to do is start with a high level review understand the environment, data structures, and how objects are initialized. It’s also a good time to consult the requirements to gain a deeper understanding of exactly what this code is trying to solve and how it went about solving it. Essentially, you’re building a model of the code during this pass of the code review. The better you understand the code, the easier it will be for you to identify issues later on.

Low Hanging Fruit

This is exactly what the title says, when thinking about the low hanging fruit, you’re looking for things that you know are security vulnerabilities & they’re easy to spot. Examples of this might be simple XSS, unsafe functions, dangerous APIs, any off by 1 errors in calculations.

Low Hanging Fruit Examples

The Buffer overrun. The code below will cause a security vulnerability in C++ because the copy data function allocates a buffer size of 10, however the function blindly copies the input userId into the buffer. The input we are actually copying in has a size of 11. Classic buffer overrun issue.

void copyData(char *userId) {
   char  smallBuffer[10]; // size of 10
   strcpy(smallBuffer, userId);
}
int main(int argc, char *argv[]) {
char *userId = "01234567890";
copyData (userId);
}

Here is an issue of command injection in .NET. Basically, we start a process from the command line & then immediately launch another process to start something and pass the arguments arg1 through without validating anything. I have seen this pattern emerges in web services also, where the web service acted like a controller for a set of processes and the caller, made HTTP requests to the web service which would in turn kick of some other application on the server to do some work for it.

static void Main(string[] args)
{
String arg1=args[0];
System.Diagnostics.Process.Start("doStuff.exe", arg1);
}

This is probably my favourite classic SQL injection, no explanation needed here, the author simply pulls the user name & password form the HTTP Request and passes them onto the database.

Connection connection = DriverManager.getConnection(url, usrName, pw);
String Username = request.getParameter("usr");
String Password = request.getParameter("pw"); // From HTTP request
int uId= -1;
String logUser= "";
String statement= "SELECT User_id, 
Username FROM USERS WHERE Username = '" +Username + "' AND Password = '" + Password + "'";

Similar to the SQL injection, the author just pulls the user name and password directly from the Request without any form of validation.

Response.Write "Please confirm your name is " & Request.Form("UserFullName")

There are many other examples which would classify as low hanging fruit, however these are some of my favourites, also some of the most dangerous. In our first two passes, we’ve understood the environment and reviewed the code for any low hanging fruit. During the low hanging fruit pass, I also identify areas that require closer observation. These areas fall under the concept of technical control.

Technical Control

During the technical control pass, one wants to consider more complicated items of security, these generally include items like:

1. Authentication
2. Authorization
3. Session Management
4. Input Validation

Authentication

How is the user authenicated? Do you simply check to ensure the user name and password are not null or do you validate against a backend database? What types of passwords does the user require to have a strong password? What type of password reset functionality have you included? How often does the user have to change his/her password?

These are all the questions I try to ask & have answered during a security code review, admittedly some of them may be out of your hands.

Authorization

How is authorization managed? Does your application require different user permissions to do different things? If so, are those permissions well enforced and managed?

Session Management

This is a good place to review session variables and how the session is managed, things like how often does the session time out, Session transport (HTTP/HTTPS), Session IDs should all be reviewed when considering how the session is managed. How are session identifies passed? Via the query string – such that it would be easy to fake & steal a session or through some other mechanism such as a secure cookie? Does log out simply kill the browser window, or do they actually kill the session? Such that the user would be required to go through the whole log in process again.

Input Validation

If you’ve caught all the low hanging fruit, this part should be a breeze, however you still want to take time to ensure that you do not trust your inputs and you have performed input validation. You should also validate input against what makes sense according to business rules. However, if you’ve done a good job in other code reviews, this is not a huge deal.

Returns

This is straight forward and should be self explanatory however, I’ll mention it here, as part of the security code review, the author should check the error & return values of their code. Or at the very least, check to ensure the operation has succeeded. Imagine a situation where the developer is performing a strncpy into a buffer to write out to a file. It’s not enough to just read some data and write it. IF this operation fails, and the developer is comparing the size equal to 0, what will end up happening is they’ll attempt to write more data than they really expected, which may or may not crash the application. When performing an operation and the function returns a value, always check the return before attempting to proceed. Another clear example of where folks get into trouble is the Impersonation functions in .NET.

Pointers

If you’re working in the .NET world, you’re largely safe from pointer issues, unless you’re working on some Interop to unmanaged code or you’re working with unsafe code. However, pointers need to be given extra attention during security code reviews and as such I have left them to last. One needs to ensure that pointers have been allocated properly, their memory hasn’t been freed more than once, and any use of a pointer or pointer arithmetic doesn’t result in an unintentional buffer overflow or a pointer going off into the middle of nowhere causing issues. I always try to have extra folks look for any issues that involve pointers.