Click here to Skip to main content
Click here to Skip to main content

Tagged as

Go to top

Security Code Review

, 11 Feb 2013
Rate this:
Please Sign up or sign in to vote.
Intro 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

Intro

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 feed back. 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:

First thing that the 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 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 has low hanging fruit, however these are some of my favourite, 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 complicate 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 their 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 then 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 Introp 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 then 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 have extra folks look for any issues that involve pointers.

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

CdnSecurityEngineer
Engineer
Canada Canada
I am a Sr Engineer for a major security firm; I have been developing software professionally for 8 years now; I've worked for start ups, small companies, large companies, myself, education. Currently the company I work for has 7,000+ employees worldwide. I am responsible for our platform security, I write code, implement features, educate other engineers about security, I perform security reviews, threat modeling, continue to educate myself on the latest software. By night, I actively work to educate other developers about security and security issues. I also founded a local chapter of OWASP which I organize and run.
 
I cut my teeth developing in C++ and it's still where my heart is with development, lately I've been writing a lot of C# code & some java, but I do have a project or two coming out in C++ /DiectX 11 whenever I get the time.
 
When I am not developing code I am spending my time with my wife and daughter or I am lost deep in the woods some where on a camping trip with friends. If you can't find me with a GPS and a SPOT device then chances are I am on the Rugby pitch playing Rugby and having a great time doing so.
 

You can find more about me and My thoughts on security
Follow on   Twitter

Comments and Discussions

 
GeneralMy vote of 5 PinmemberCdnSecurityEngineer12-Feb-13 15:19 
QuestionRe: My vote of 5 PinmemberJohn Brett13-Feb-13 0:47 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Mobile
Web04 | 2.8.140926.1 | Last Updated 11 Feb 2013
Article Copyright 2013 by CdnSecurityEngineer
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid