Five Things That Do Not Belong In A Review Checklist

This is the second half of an article I posted about using a checklist to prevent security errors. There, I said that you have 15 checklist items max, and you shouldn't waste any of those questions on silly things like "Does the code follow the coding standard?".

Jason Cohen pointed to an article of his in which he said "List no items that can be automated." (His emphasis, but I second the motion.)

In the context of the SANS paper, let's look at five items that should be automated so that no human has to find the defects:

  1. CWE-665: Improper Initialization. As the write-up for CWE-665 suggests, you could use a programming language that forces you to explicitly initialize all variables before use. Or, if you're stuck with something like C, make sure you turn on the warnings in your compiler, or use a static analysis tool (e.g. lint) to verify that all variables are initialized before being used. Tools like Coverity can be very sophisticated in their static analysis.
  2. CWE-119: Failure to Constrain Operations within the Bounds of a Memory Buffer. Same goes for this one. First, use a programming language that doesn't require attention to every tiny little detail of string handling. Failing that, apply static analysis. It's also worth performing runtime analysis (e.g. electric fence, Purify) with appropriate test cases to verify that you've avoided buffer overflows.
  3. CWE-404: Improper Resource Shutdown or Release. Even garbage collected languages can have problems with resource release. First, some GC systems have problems with circular references. Second, GC systems are typically worried about releasing memory. You also have to worry about database handles, file handles, and sockets. Configure your static analysis tool to track resources like these so that you don't have to do it manually.
  4. The write-up for CWE-404 also mentions in passing that you should "wash your garbage before you dispose of it". This is useful for two reasons: first, an attacker won't have access to the contents of previously used memory, and second, it often makes debugging memory problems easier if you write a known value into freed memory. (I modified malloc() at my last job to write 0xcacacaca into freed memory so that we would know right away when someone stepped on a turd, um, I mean used freed memory.) Configure your libraries to shred objects before you free them and you won't have to worry about it on a line-by-line basis in the code you review.
  5. CWE-362: Race Condition. I'm typically worried less about security than the nightmare of isolating and debugging race conditions. I haven't seen any tools that detect race conditions well, but Coverity does a decent job of telling you when you've mishandled a race condition in a couple of cases: first, it warns you when you fail to release a lock, and second, it warns you when you access a resource that is statistically accessed from within a lock. So you still have to beware of race conditions (at the design level is the best place to find these), but there is an option for finding tedious errors in the mechanics of dealing with races.

And now I'm going to throw out everything above. Remember that the tools are built to detect common errors. If it is really important that you find a certain class of bug, put it on your checklist. In fact, if it's super critical, make it a checklist of length one. For example, I've gone over codebases with the single-minded focus of finding race conditions. When you perform a review like this, it's amazing how many defects you might find in areas that you never suspected.

Posted on 2009-01-27 by brian in reviews .
Comments on this post are closed. If you have something to share, please send me email.