Thursday, December 29, 2011

Security Code Review vs. Code Review

Tomorrow I have a code review to lead...well participate in. I usually lead so tomorrow I really need to focus on listening as it is my code that is being reviewed. Not having written any code in a while I was doubly careful but still know that there are significant issues to be addressed (there always are). In thinking about tomorrows review, I remembered a discussion I had once with a PHP friend who asked what the difference was between a security code review and a regular code review.

The question struck me a bit and made me wonder, what is the difference? Was there a difference? Did someone just tack the word "security" on to make the code review sound more interesting? After a bit of research I came to a conclusion - security was the difference. This is not profound or deep, it is pretty obvious and became more and more obvious as I sat in on many code reviews.

Things that are the same
A code review (security or not) has a core focus: talking about the code. This makes Code Review one of the most important practices to building quality, secure code because it analyzes the one common output from a software project: code. Your review can be as structured or loose as what fits your environment but you need at least four roles (roles not people):
  • The Author: The person who wrote the code. They are here to learn and explain when the code cannot explain for them.
  • The Reader: This role reads the code as a narrative to explain to the reviewers what it is doing. This role cannot be held by the author as that will skew the review.
  • The Secretary: The role who records the notes of the meeting. They are also responsible to bring the notes to the team after the meeting.
  • The Moderator: The role who keeps the review positive, productive and actionable. This role also ensures the notes from the review are executed after the review.
These roles can be held by a single person (except author and reader) which can produce a code review team of 2 people. Sometimes these smaller groups can produce fast results but beware of always reviewing with the same people. A fresh perspective is always necessary as two reviewers who always work together could start to overlook issues due to a "oh that is their coding style" type mindset.

Another aspect that is the same is the focus on good design and execution not "how I would have done it". As soon as you hear someone say, "well, I would have..." stop right there. Dig in and find out why it should be done a different way. If there is not a good reason, such as a pre-existing standard, then agree to disagree. Code is art and critics rarely agree on what is good, great or horrible.

Things that are different
In a Security Code Review you want to discuss Attack Patterns. Is the code susceptible to an attack pattern such as injection? Does the code mix the Data Channel and Command/Control Channel (which can lead to injection)? If so, why? Is it necessary? As an example of this, anytime you see:

var sql = "SELECT * FROM tbl WHERE ID = " + id + " "

Ask why! What data type is id and how are you sure that you did not just open a SQL Injection attack vector. An excellent introduction to attack patterns can be found at https://buildsecurityin.us-cert.gov/bsi/articles/knowledge/attack/585-BSI.html. Keep a list of attack patterns handy and ask: "Does the code allow this?". Strong knowledge of attack patterns and the OWASP Top Ten will provide useful input to this section of the security code review.

Another activity is the examination of assumptions. Dig in to each assumption and ask what happens if that assumption is false. For instance, if the code assumes Request.Querystring["ID"] will be an integer (using int.Parse(Request.Querystring["ID"]) or something like that) what happens when it is not? A great thinking strategy for this is the concept of Equivalence Partitioning. Using Equivalence Partitioning, you break down the input into valid and invalid partitions. For our above example, Alpha characters would be an invalid partition. What happens when we encounter ?ID=A in the query string? Beyond assumptions of data type validate assumptions in logic. If the developer believes that ID will always be between 1 and 10, ask why and what happens when the business decides to allow ids between 11 and 20.

There is a lot more to a security code review but these two items will get you started in the right direction. As security becomes a larger focus for developers code reviews will become security code reviews (thus why it is important to focus on security education for your devs). Examining attack patterns and assumptions will enable you to produce more secure code. Just remember to dig deep, return to review often and work as a team to discover the most secure, highest quality solution.

No comments: