What is it and why should I care?
Code reviews are an important process whereby developers have their code systematically examined by another set(s) of eyes in order to find defects. It’s a simple concept (double-check my work), but surprisingly effective. Studies show that you can detect 20-75% of defects with code review (range varies widely depending upon level of rigor applied). That is incredibly powerful, especially once you combine it with other quality processes.
Note: Since this series is specifically about security, I’m going to consider security focused code reviews, but the term is used commonly to refer to the generic defect finding reviews performed by developers.
The only thing that differentiates security code reviews from standard code reviews is that we’re looking for a specific set of issues. The same would be true of a “performance” code review where you only look for performance issues. This means that while a general code review might turn up plenty of poor programming practices that have no bearing on security, a security code review would usually ignore those. It’s just a matter of focus, not of different methods.
What should I do about it?
If we believe the studies (and I do based on my own experience) that tout the effectiveness of code review, what do we do now? We follow their advice and do code reviews! Specifically, here are the steps I think have a place in the security code review eco-system.
Define the Plan
As with any process, you need to set goals. These are obviously specific to an organization, but a few examples might be:
- Reducing vulns found in QA/Prod (basically find issues sooner)
- Ensure compliance with corporate standards or legal requirements
- Train developers on security (can be done with the process of review by close interaction with development team)
- Enhance overall security posture
Quite simple, but not always easy. As with all other code review types, it’s very common for people to either a) not do reviews, or b) do “rubber stamp” reviews. Neither are helpful, and both are common. Code reviews must be setup and executed on a regular basis to provide the desired value.
Don’t Over-Do It
There’s certainly a point of diminishing returns here. Reviewing low-value code is often not worth the effort, particularly from a security perspective (hint: reviewing java beans is usually a waste of time). It’s often fairly obvious (assuming you understand the app) what parts of the application are security-sensitive, so focus on those areas. In addition, you can also add in processes to detect changes to the security-relevant portions of the code and trigger automatic code reviews.
Automate What You Can
There are tools like static and dynamic analysis that can help you find some classes of issues. Use them. Automation should be preferred whenever it’s accurate and available. However, automation does NOT find everything. You also need to have manual reviews, especially of certain classes of vulnerabilities. This means you have to know the capabilities of your tools. If there’s a coverage gap, you’ll need manual review to cover it. Balancing this manual/automated approach is the key to security with scalability. Different organizations are going to slide along that scale a bit, but it’s important to know that the decision must be made, and to be conscious of the choices you’re making and why you’re making them.
In addition to using tools to help you find issues, there are tools that help you do manual reviews. You don’t want to just read all the code. You’ll want to look at diffs, view versioning comments, make notes for the developer, etc. and there are tools that help you do that, so use them.
Iterate and improve
Any good process has a feedback loop. Implement your first version, then evaluate what worked and what didn’t. Then remove things that are broken, and try new things. The point is that you want a holistic process to catch security issues. I usually view it similar to unit testing. I try to think of every valid test I can, and add them to my test suite. This makes my code better than if I had no tests. Invariably though, I find a bug that covers something I didn’t consider, so I add a test to cover that scenario, and now I’ve improved my code and test suite. That’s the way it should be with the code review process. Honestly evaluate the value of the steps in the process, then a) keep those that work, b) throw away those that don’t, and c) add to the process when you need to cover new issues.
I personally find that it’s helpful to do an evaluation/review of the process every 6 months to determine if everything’s still useful, and if anything needs to be added.
In conclusion, code review is a necessary process if you really want to improve your code. Security focused code review looks specifically at security related issues in the code, and gives you a double-check from another set of eyes. This process has a high likelihood of success given you perform it regularly.