Chapter 20: Performing a Security Code Review

Chapter 20

Performing a Security Code Review

Although a security code review might seem to be much the same as an ordinary code review, which looks for ordinary flaws, like failure to free allocated memory or dereferencing a bad pointer, specific types of bugs ought to be examined more closely when doing a security review. That said, solid code is quite often secure code, assuming that there aren't higher level design issues. (For example, an absolutely correct implementation of telnet still passes username and password in the clear.) Careful, meticulous programmers don't tend to introduce as many bugs of any kind into their code. The very best programmers understand that they will make mistakes and ask for thorough reviews. It's estimated that a good reviewer will catch the great majority of the implementation bugs in a given piece of code.

An even better approach is to have a more formal code review. In A Guide to Code Inspections by Jack Ganssle from http://www.ganssle.com/Inspections.pdf, a formal process for code reviews is detailed. Although this paper is focused on embedded systems, where bugs are much harder to patch, the general approach is worth thinking about. You might want to strongly consider taking this approach for the most risky code network interfaces or code that runs as a highly privileged account. You might think that a rigorous code review would cost you a lot of time, but some studies have estimated that for every bug that is removed during an inspection, you've saved nine hours of testing, debugging, and fixing the code. Code reviews are also anywhere from 20 to 30 times more effective at finding bugs than relying only on testing.

Here's a synopsis of a formal code review process. Four roles are needed in addition to the reviewer: moderator, reader, recorder, and author. The moderator manages the meeting and follows up on issues found. The reader paraphrases the program flow and should never be the same person as the author; as with all authors, the author might read what he or she meant the program to do, not what it does. The recorder records each bug found, which frees the remaining team members to concentrate on the code. The author's role is to understand the problems found and illuminate unclear areas. The review should never become a personal critique of the person writing the code but instead should focus on the code itself. Although you might be tempted to reduce the number of people involved, there's evidence that a four-person team is much more effective than a three-person team. I'd tend to moderate this with the sensitivity of the code: the larger the team, the fewer bugs will escape, but you're using more person-hours per bug to find them. Embedded systems tend to have less code, but all of it is critical. Also, big groups tend to rat-hole, or spend time on unrelated issues, so keep the group size under control.

One of the most important aspects of conducting a security code review is understanding the functions being called and knowing the specific types of mistakes that can lead to security flaws. For example, I don't tend to write much RPC code, so I wouldn't be the best person to review RPC function calls. I do write a great deal of sockets code. Get someone to review your code who understands the functional areas you're using. This is one of the fallacies of the many eyes theory it doesn't do you any good if the people looking at your code won't recognize a problem when they see it. If the functional area is one you're not currently familiar with, see if one of the chapters in this book covers it and give it a quick read. If all else fails, be prepared to proceed very slowly and RTFM (Read The Fine Manual) for every API call. Pay particular attention to the remarks section this tends to be where the gotchas get documented.

When you look at someone's code, think about the assumptions implicit in the functions. Do we trust the caller to know how large his or her buffer is? Is the function easy to use? Does the caller need to know anything about the implementation of the function? If you get someone to give you a guided tour of the application, he or she will infect you with his or her assumptions. Sometimes a better approach is just to read it yourself while having the author handy to answer questions. Question these assumptions carefully always imagine that the caller of a given function is an evil entity controlled by your attacker who is bent on your destruction. If any of these assumptions are violated, can it possibly result in anything other than a graceful failure? If a developer indicates that the data is trusted, question why what event in the code made the data trusted and safe?

Rapid Peer Reviews

Less rigorous but still useful, another approach to performing code reviews is to have two developers peer review each other's code by sitting side-by-side with two computers and reviewing one source code file while asking questions of each other and questioning assumptions.