Formal Inspections

Formal code inspections are done in meetings by a review team (involving the author of the code, a code reader who walks through the code, one or more reviewers, and a moderator or a coach) sitting around a table carefully looking at code printouts or code projected onto the wall. They are still done by some teams, especially in high-risk, life-critical systems development. But they are an expensive and inefficient way to find problems in code.

There are several recent studies which prove that setting up and holding formal code inspection meetings significantly adds to development delays and costs without adding significant value. While it can take hours to do all the planning, paperwork, and follow up, and weeks to schedule a code inspection meeting, less than 5% of defects are actually found in the meeting itself. The rest are all found by reviewers looking through code on their own while preparing for the meeting.¹

Rubber Ducking or Desk Checking

The term “rubber ducking” is based on the idea that if you don’t have anyone else to review your code, walking through the code and explaining it to an imaginary other person (or a rubber duck) is better than not reviewing the code at all.

Self-reviews don’t meet governance, compliance, transparency, and information sharing requirements. But if they are done in a disciplined way, self-reviews can still be an effective way to find defects, including security vulnerabilities.

In one study on code reviews at Cisco Systems, developers who double-checked their work found half of the defects that other reviewers found, without any help.

Pair Programming (and Mob Programming)

Pair programming, where two developers write code together, one at the keyboard driving, and the other navigating and assisting (like a couple on a road trip), is a fundamental practice in Extreme Programming (XP). Pair programming provides immediate, continuous code reviews: developers work together closely, share ideas, look at problems, and help each other write better code. Organizations such as Pivotal Labs are famous for their success with pair programming.

Pair programming is about joint problem-solving, driving, and navigating toward a solution. It’s a great way to bring a new team member up to speed or to debug a tricky problem. Pairing tends to result in cleaner, tighter code, with clearer abstractions, and fewer logical and functional errors. But unless you are pairing developers with a security expert or a technically strong experienced developer, pair programming won’t necessarily result in more secure code.

Pair programming also comes with costs. While two heads are better than one, it’s obviously more expensive to have two developers working on the same piece of code together. Pairing is also an intensive, highly disciplined practice, which many people find socially straining, mentally exhausting, and difficult to sustain. Few developers pair up more than a few hours a day or a few days in a week.

A more extreme version of pair programming is mob programming, where an entire team works together on the same piece of code, using a single computer. This approach emphasizes collaboration, team problem-solving, and learning. Only a small number of organizations have had success working this way, but the benefits should include complete transparency and even better quality code.

Code Audits

While pair programming and peer reviews are generally done as part of the day-to-day job of writing code, code audits are separate and outside of development. In a code audit, an expert security reviewer (or a small team of reviewers) from outside of the team examines the code base for security vulnerabilities, or at least as much of the code base as they can within the time allowed. These reviews are often required for compliance reasons or to manage important risks.

Code audits usually take several days of reviewer time, as well as time from the team to help the reviewer(s) understand the design and context of the system and the structure of the code, and then more time from the team to understand what the reviewers found (or think that they found) and to put together a plan to address the findings.

Code auditors bring specialized security knowledge or other expertise that most developers won’t have. But because this work can be so mentally fatiguing, and because reviewers usually have limited time and limited familiarity with the code, they may miss important issues. A successful audit depends on the reviewer’s experience, understanding of the language and technology platform, ability to come up to speed with what the system does and how it does it, and mental stamina.

We’ll look more at code audits in Chapter 12, External Reviews, Testing, and Advice.

Automated Code Reviews

Code scanning tools can be used to automatically review code for bad coding practices, and common coding mistakes and vulnerabilities. At a minimum, automated reviews using code scanning tools act as a backstop to manual reviewers, by catching careless and often subtle errors that are hard to see in the code.

We will look at the strengths and weaknesses of automated code reviews in more detail later in this chapter.

What Kind of Review Approach Works Best for Your Team?

To be effective and sustainable in a fast-moving environment, code reviews need to be Agile: lightweight, practical, inexpensive, and fast.

Formal inspections are rigorous, but they are expensive and too slow to be practical for most Agile teams.

As we’ve seen, getting developers to carefully review their own work can catch bugs early. But self-reviews do not meet compliance or governance requirements, they don’t provide increased transparency into changes or help with sharing information and ideas across the team, so this approach will not “do,” even if it is better than not doing a review at all. Unless of course there’s only one of you.

Code audits sit outside of the development cycle: they are a point-in-time risk assessment or compliance activity. You can’t rely on them as part of day-to-day development.

Pair programming isn’t for everyone. People who like it, like it a lot. People who don’t like it, don’t like it one bit. While pair programming is a great way for developers to share ideas and solve hard problems together, mentor new team members, and refactor code on the fly, it is not necessarily an effective way to find and prevent security issues.

This leaves peer reviews. Practiced properly, asking team members to review each other’s code is probably the most effective approach for Agile environments. This kind of review can be done often, in small bites, without adding significant delays or costs. We’ll focus on how to incorporate peer reviews into development, and how to include security checking into peer reviews, in this chapter.

Before Code Changes Are Committed

The most natural and the most valuable time for code reviews is before the change is committed to the code mainline. This is how many Agile teams and open source teams, especially teams using collaborative code review tools like Gerrit, work today.

Modern source code management systems such as Git make this easy to do. In Git, engineers create a pull request when they want to push code changes to a source repo. The pull request tells the rest of the team about the change and gives them an opportunity to review and discuss the change before it can be merged. Repo administrators can enforce rules to require approvals and set other contributing guidelines.

In many environments, enforcing code reviews up front is the only way to ensure that reviews get done at all: it can be difficult to convince developers to make code changes after they have already checked code in and moved on to another piece of work.

This is also a good place to add some automated checking. For example, ThoughtWorks has built a pre-commit tool for teams using Git, called Talisman, which looks for suspicious things in code, like secrets, and blocks the code from being checked in. You could extend this tool (or build your own) to implement other checks that are important in your environment.

Gated Checks Before Release

Security and compliance can insist that code reviews are completed at least for certain stories, or for high-risk fixes before the the code can be considered done. This means that the team cannot move forward and can’t deploy changes until these reviews have been completed and any issues found during the reviews are resolved.

Your goal should be to work with the team and keep it moving in a safe way. Instead of leaving these checks to the end as a final check before release, build steps into the team’s workflow so that the security team is immediately notified when these reviews are required and can do them as early as possible. Force the team to stop work only when necessary to keep risks under control.

Postmortem and Investigation

Another important time to do code reviews is in a postmortem situation, after a security breach or outage, or if nasty surprises are found in an external audit or a pen test. We will look more at postmortems and how to do them properly in Chapter 13, Operations and OpSec.

A postmortem code review is usually done by senior members of the team and may include outside experts, depending on how bad the problem was. The goals of these reviews are to ensure that you understand what went wrong, that you know how to fix it, and to help with root cause analysis—to dig deep into why the problem happened and find a way to prevent problems like this in the future.

There will also be some kind of paperwork required, to prove to somebody—senior management, auditors, regulators—that the code review was done properly, and to document follow-up actions. This can be expensive, serious work. Make sure that people learn as much as they can from the “opportunity” that a bad problem presents.

Take Advantage of Coding Guidelines

Coding guidelines are important in Agile teams to support collective code ownership, the principle that anybody on the team should be able to take on any coding assignment or fix any bug (within his or her ability), and everyone can refactor each other’s code. In order to do this effectively, the code has to be consistent and follow a common style and conventions.

The team’s code guidelines should prescribe surface issues like element naming and code indentation and formatting, as well as more important underlying issues: proper use of frameworks and libraries, auditing and logging patterns that need to be followed, and banned practices and banned function calls.

There are some good freely available coding guidelines that you can use:

• Google’s coding guidelines for various languages

• CERT coding standards for C, C++, Java, Android, Java, and Perl

• Microsoft’s secure .NET coding guidelines

• Oracle’s Java SE coding guidelines

• OWASP Secure Coding Practices

• Mozilla’s Web Application Secure Coding Guidelines

Even if your team and your code are wildly outside compliance with these guides, it’s worth picking one as a target and defining a plan to iterate toward that target. This will be easier than trying to create your own guide from scratch.

By following one of these guides, you should end up with cleaner, more consistent code that is easier to understand and review, and code that is more secure by default.

Don’t Make These Mistakes

There are some common mistakes and anti-patterns that you need to avoid when it comes to code reviews:

1. Believing that senior people don’t need their code reviewed. Everyone on the team should follow the same rules, regardless of seniority. In many cases, senior team members are the ones who take on the most difficult coding tasks and problem-solving work, and this is the most important code to review.

2. Not reviewing legacy code. A study on the honeymoon effect in software development proves that there is a honeymoon period after new software features and changes are deployed before attackers have the chance to understand them and attack them. Most successful attacks are made against code that has been out long enough for bad guys to identify vulnerabilities and to find a way to exploit them. This is especially the case for popular third-party and open source libraries and platform code. Once attackers have found a vulnerability in this code and learned to fingerprint it, the risk of compromise increases significantly.²

   So while you need to review changes as they are being made, because it is much easier and cheaper to fix the mistakes that you find right away, it is still important to look at older code, especially if you have code written before the team got training on secure development.

3. Relying only on automated code reviews. Automated code scanning tools can help you to find problems in code and identify where code needs to be cleaned up, but as we’ll show later, they are not a substitute for manual code reviews. This is a case of AND, not OR.

Avoiding these fundamental mistakes will help you improve the security and quality of your code.

Review Code a Little Bit at a Time

Another common mistake is forcing team members to review large change sets. Experience backed up by research proves that the effectiveness of code reviews negatively correlates with the amount of code reviewed. The more files and lines of code that a reviewer has to look at, the more tired he will get, and the less he will be able to find.

A reviewer who is forced to look at 1,000 lines of code changes might comment on how hard it is to understand or how something might be made simpler. But she won’t be able to see all the mistakes that are made. A reviewer who only has to look at 50 or 100 lines of code will have a much easier job of finding bugs, especially subtle mistakes. And he will be able to do this much faster.

In fact, research shows that the effectiveness of code reviews starts to drop off after around 200 lines of code, and that the number of real defects that reviewers find falls off sharply when they are asked to review code for more than an hour (see the “Modern Code Review” chapter of Making Software [O’Reilly]).

This is an unavoidable problem in code audits, where auditors need to scan through thousands of lines of code each day, often for several days at a time. But it should be much less of a concern for Agile teams, especially teams following continuous integration, because they tend to make small, iterative changes, which can be reviewed immediately on each check-in.

What Code Needs to Be Reviewed?

The team needs to agree on what code has to be reviewed, and who needs to be involved in code reviews.

In a perfect world, all code changes should be reviewed for maintainability, for correctness, and for security. This demands a high level of engineering discipline and management commitment.

If you aren’t ready for this yet, or can’t convince your management or your customer to give you the time to review all code changes across the board, you can still get a lot of benefit by taking a pragmatic, risk-based approach. Most changes in continuous integration are small and incremental, and carry limited security risk, especially code that will be thrown away quickly when the team is experimenting and iterating through design alternatives.

For many of these changes, you can lean on automated code scanning tools in the developer’s IDE and in continuous integration to catch common coding mistakes and bad coding practices. As we’ll see later in this chapter, there are limitations to what these tools can find, but this might be good enough to contain your risks and help you to meet your compliance requirements (regulations like PCI DSS require all changes to be reviewed, but also allow for automating code reviews).

But some code, such as the following, needs to be looked at more closely and carefully, because the risk of making and missing a mistake is too high:

• Security features like authentication and access control, crypto functions, and code that uses crypto functions

• Code that deals with money, or with private or confidential data

• APIs with other systems that deal with money or with private or confidential data

• Web and mobile UIs (i.e., big changes or new workflows)

• Public network-facing APIs and file imports from external sources (i.e., code that is frequently exposed to attack)

• Framework code and other plumbing

• First-of code using a new framework or design approach, until the team gets comfortable with how to do this properly

• Code written by new team members (at least if they aren’t pairing up with a more experienced team member) to make sure that they follow the team’s coding patterns and guidelines

Changes to frameworks or security features or public APIs should also be reviewed in design, as part of threat modeling, which is discussed in Chapter 8, Threat Assessments and Understanding Attacks.

Although it will usually be obvious to the team what code changes carry risk, here are some steps to identify and track code that should be reviewed carefully:

• Tagging user stories for security features or business workflows which handle money or sensitive data.

• Grepping source code for calls to dangerous function calls like crypto functions.

• Scanning code review comments (if you are using a collaborative code review tool like Gerrit).

• Tracking code check-in to identify code that is changed often: code with a high rate of churn tends to have more defects.

• Reviewing bug reports and static analysis to identify problem areas in code: code with a history of bugs, or code that has high complexity and low automated test coverage.

• Looking out for code that has recently undergone large-scale “root canal” refactoring. While day-to-day, in-phase refactoring can do a lot to simplify code and make it easier to understand and safer to change, major refactoring or redesign work can accidentally change the trust model of an application and introduce regressions.

Netflix has an interesting way to identify high-risk code. It wrote a tool called Penguin Shortbread, which maps out call sequences for microservices. Services that are called by many other services or that fan out to many other services are automatically tagged as high-risk dependencies that need to be reviewed.

At Etsy, as soon as high-risk code is identified through code reviews or scanning, its developers hash the code and create a unit test that automatically alerts the security team when the code hash value has been changed through a check-in.

Finally, you can make the team responsible for watching out for risks in code, and encourage developers to ask for a code review whenever they think they need help.

How Many Reviewers?

Reviews (and pair programming) are based on the reasonable principle that two people will find more problems than one person can on her own. So if two heads are better than one, why not have more reviewers, and get even more eyes on the code?

At Google and Microsoft, where they’ve been doing code reviews successfully for a long time, experience shows that two reviewers seems to be the magic number. Most teams in these organizations require two reviews, although there are times when an author may ask for more input, especially when the reviewers don’t agree with each other.

Some teams at Microsoft specifically ask for two different kinds of reviews to get maximum value from each of the reviewers:

1. A review before the code is checked in, focused more on readability and clarity and maintainability

2. A second review (done before or after check-in) to look for risks and bugs

Studies have found that a second reviewer will only find half as many new problems as the first reviewer. Beyond this point, you are wasting time and money. One study showed no difference in the number of problems found by teams of three, four, or five individuals, while another showed that two reviewers actually did a better job than four.⁴

This is partly because of overlap and redundancy. More reviewers means more people looking for, and finding, the same problems (and more people coming up with false positive findings that the author has to waste time sifting through). You also encounter a “social loafing” problem. Complacency and a false sense of security set in as you add more reviewers: because each reviewer knows that somebody else is looking at the same code, they are under less pressure to find problems on their own.

But what’s even more important than getting the right number of reviewers is getting the right people to review your code.

What Experience Do Reviewers Need?

A new team member will learn a lot by having an experienced team member review his code, reinforcing his understanding of how the system and the team work. By reviewing code for other people on the team, a new developer will gain exposure to more of the code base and learn something about the system. But this is an inefficient way to learn. And it misses the main point of asking for a code review, which is to find and correct as many problems with the code as early as possible.

Research backs up what should be obvious: effective code reviews depend heavily on the reviewer’s skill and familiarity with the problem domain and platform, and her ability to understand the code. A study on code reviews at Microsoft found that reviewers from outside of the team, or who were new to the team and didn’t know the code or the problem area, could only do a superficial job of finding formatting issues or simple logic bugs. Like other areas in software development, the range of individual performance can be huge: top performers are 10 times more effective in finding problems and providing valuable feedback.

This means that your best, most experienced developers will spend a lot of time reviewing code, and they should. You need reviewers who are good at reading code and good at debugging, and who know the language, frameworks, and problem area. They will do a much better job of finding problems and can provide much more valuable feedback, including suggestions on how to solve the problem in a simpler or more efficient way, or how to make better use of the language and frameworks. And they can do all of this much faster.

If you want new developers to learn about the code and coding conventions and architecture, it will be much more effective to partner them up with an experienced team member in pair programming or pair debugging exercises than asking them to review somebody else’s code. If you have to get inexperienced developers to review code, lower your expectations. Recognize that you will need to depend a lot more on automated static analysis tools and testing to find real problems in the code.

Different Tools Find Different Problems

For most environments you have a choice of static analysis tools, designed to look for different problems.

What Tools Are Good For, and What They’re Not Good For

Make sure that you and the team members understand what they are getting out of a static analysis tool and how much they can rely on them.

Some tools are much better at finding some problems than others, and this can depend a lot on your use of language, frameworks, and design patterns.

There are several good open source and commercial static analysis tools available today for mainstream languages like Java, C/C++, and C#, using common frameworks like Struts and Spring and .NET, and for other popular development environments like Ruby on Rails.

But it’s difficult to find good tool support for hot new languages such as Golang or Swift or F# or Jolie, and it’s especially difficult to find tools that can catch real problems in dynamically typed scripting languages like Javascript, PHP, and Python, which is where you need the best checking. Most code analyzers (at least the open source ones) for these languages are still limited to linting and basic checking for good practices, which helps to make for better code, but won’t ensure that your code is secure or even that it will run without crashing.

A static analysis tool can tell you when code makes unsafe library calls, but it can’t tell you if somebody forgot to make a call that he should have. Tools can tell you if you made a basic mistake in applied crypto, but can’t tell you if you forgot to encrypt or tokenize sensitive data, or forgot an ACL check, or if the code accidentally exposes sensitive data in exception handling. Only an experienced human reviewer can do this.

There is important research that can help us to understand the effectiveness, and limits, of automated static analysis tools.

In one study, researchers ran 2 different commercial static analysis tools against a large application that contained 15 known security vulnerabilities (found earlier in a structured manual audit done by security experts). The tools together found less than half of the known security bugs: only the simplest problems, the bugs that didn’t require a deep understanding of the code or the design.⁵

The tools also reported thousands of other issues that needed to be reviewed and qualified, or thrown away as false positives. Some of these findings included runtime correctness problems, null pointers, and resource leaks that probably needed to be fixed, and code quality issues (dead code, unused variables), but they did not uncover any other security vulnerabilities.

NIST has run a series of benchmarks to assess the effectiveness of static analysis tools called SAMATE. In its most recent analysis (2014) NIST tested 14 different static analysis tools on C/C++ and Java code with known vulnerabilities. This is what was found:

1. Over half of the vulnerabilities were not detected by any of the tools.

2. As the complexity of code increased, the ability of tools to find problems decreased significantly. Many tools simply gave up. This was also shown in the case of the Heartbleed bug in OpenSSL, which could not be found by any of the automated code analysis tools available, partly because the code was too complex.

3. NIST also found a significant and disappointing lack of overlap between tools: less than 1% of the vulnerabilities were found by all the tools.⁶

More recently, OWASP’s Benchmark Project was started to create a test suite with known security vulnerabilities, designed to evaluate the effectiveness of different static analysis and application scanners. This project scores tools by subtracting false positive findings from true positives. The average score for the commercial SAST tools that they evaluated was only 26%.

While tools continue to get better—more accurate, faster, easier to understand, and with better support for more languages and frameworks—they can’t replace reviews done by smart people. But they can act as an effective backstop to manual reviews by catching common mistakes; and, by enforcing good coding practices and consistency, they can make manual reviews easier and more effective.

Getting Developers to Use Automated Code Reviews

You want to reach a point where engineering teams treat static analysis findings like they do unit tests: when they check in a change, and a tool reports something wrong, they will fix it immediately, because they have learned to trust the tool and know that they can rely on it to find important problems for them.

It should be easier to introduce static analysis checking for security vulnerabilities into teams that have already had good experience with static analysis tools for code quality and have learned to depend on them. Start with getting the team to use a good bug-finding tool, and the team has accepted it and successfully made it part of its day-to-day work, introduce security checking.

Dropping off a report on a developer’s desk with thousands of static analysis warnings isn’t going to get buy in from the team. Try taking an incremental, low-friction approach.

Take some time to understand how the tool works and how to configure the rules and checkers properly. Many teams rely on the default setup, which is almost never the right thing to do. Some tools come with conservative defaults, which means that they don’t apply rules and checkers that could be important for your application. Many other tools want to be as thorough as possible, enforcing checks that aren’t relevant and flooding developers with false positives.

Install the tool and run a set of scans with different rules and settings. Measure how long the scans take, and how much CPU and memory the tool uses. Review the results, and look for the sweet spot between maximizing true positive findings and minimizing false positives. Find the rules and checkers that have the highest confidence. Turn the other rules off, at least to start.

Establish a baseline. Run a complete scan, go over the results with the team, or maybe just a couple of the strongest developers to start, to explain what the tool can do and show that it works. Then mark everything that the tool found so that these findings won’t show up in future scans by default. This is security debt that you will pay off later.

Get the team agree to take a zero bug tolerance approach to any findings moving forward: from this point on, if the tool finds any serious problems, the team will agree to review them and fix all true positives.

Instead of having to sift through pages of review results, team members will see only a handful of findings each day, or each time that they check in code—however often you decide to run the scanner. This shouldn’t add much to their work, and eventually will become just another part of their day-to-day workflow.

After the team has gained confidence in the tool and learned how to work effectively with the results, it can schedule time to go back and review and fix the problems found in the baseline scan.

Of course if you take this approach, you will be leaving some bugs in the system and vulnerabilities open for attack, at least for a while. But now at least you know what the problems are and where they are, and you can work with the Product Owner and the team and management on a plan to reduce the risks.

Self-Service Scanning

In many large enterprises, code scanning is done by the security team, in what Dr. Gary McGraw at Cigital calls a “scanning factory” model. The security team schedules and runs scans across different projects, reviews and triages the results, and reports the results back to development.

Working this way introduces unnecessary delays in feedback to the team. It can take days to complete the scans, review the results, and prepare reports. By the time that developers learn about problems, they may have already moved on to other features, which means that they have to stop what they are working on, switch back to their earlier work, restore the context, find the problem, fix it, test it, build it, and then switch back again. This is the kind of waste that Agile and Lean methods are intended to avoid.

Another disadvantage of this model is that it makes scanning “somebody else’s problem,” taking away any sense of ownership from the developers.

A more effective, more scalable, and more Agile way of working is to make code scanning available to developers on a self-service basis while they are working, in ways that make natural sense to them.

Instead of choosing standardized tools that are convenient for the security team to use across multiple projects, let the team choose tools that work best for its specific needs and that fit into its workflows.

The first place to include self-service scanning is in each developer’s IDE, using plug-ins or the IDE’s built-in rules to check for problems as they are coding, or when they save changes or compile code in their IDE. It’s especially important to have high-confidence, high-impact rules in place here, rules that highlight real problems; otherwise developers will quickly learn to ignore all alerts and highlighting.

But keep in mind that there’s no visibility to the rest of the team at this point into what problems were found, and no way to prove that developers are fixing these problems up front. So you will still need to add incremental code analysis and fast code scanning each time that the code is built, to catch any problems that might get past.

Most scanning tools have APIs that you can use to push results directly back into each developer’s IDE or automatically create bug reports in backlog tracking tools like Jira or VersionOne. Or you can use one of the application vulnerability management tools that we looked at in Chapter 6 to help with this.

Self-service, fast-feedback scanning requires tools that run quickly, and that provide clear context for each finding so that developers don’t need a security expert’s help to understand what the problem is, how serious it is, where it was found, how it was found, or how to fix it.

If you have the resources, you can still run deep, full code scans out of band in your security scanning factory, review and qualify the results, and feed them into the development backlog to get fixed. While this could take hours or days to run on large code bases, these scans should only pick up a small number of important exceptions that make it through the earlier, faster, high-certainty checking in development. So the cost of reviewing these issues and the impact on the team’s velocity to come back and deal with them should be minimal, and the risk of missing an important problem should be low.

Reviewing Infrastructure Code

Today’s systems are built using automated programmable configuration management tools like Ansible, Chef, Docker, and Puppet. Chef recipes, Puppet manifests, Ansible playbooks, Dockerfiles, and AWS CloudFormation templates should follow the same life cycle as your other code, from check-in, manual review, automated build checks, lint checks and other static analysis checks, and automated unit and integration and acceptance testing.

Code reviews for recipes and playbooks are done for many of the same reasons as application code:

• Ensure that all changes to system configuration have been checked by at least one person (governance).

• Check for maintainability and compliance with code conventions.

• Check for correctness.

• Make sure that test configurations are not accidentally pushed to production.

• Enforce operational and security policies.

Reviewers can use the same code review methods, workflows, and code review tools that application developers use, such as Gerrit or Review Board. They should pay attention to style and structure problems, proper separation of code and data, reuse, and readability. Reviewers should also check to make sure that there are good tests written for each module. But most important, they should be on the lookout for configuration mistakes and oversights that could leave the system vulnerable to attack.

Engineers can’t lean too heavily on static analysis tools for this type of code. Tools like Foodcritic for Chef or Puppet-lint for Puppet do basic syntax checks and look for common coding mistakes, which is important in these scripting languages, where you want to find problems before runtime. Out of the box they won’t find serious security issues for you. You will need to write your own custom rules to do this.⁷

We’ll look more at how and where security comes in when working with these tools in Chapter 13, Operations and OpSec.

Reviews Take Time

First, it takes time to review code properly. The team, including and especially the Product Owner, as well as management, all need to understand that code reviews will take up a significant part of each developer’s day, and that waiting for code reviews will slow down delivery.

At an organization like Microsoft, developers spend on average between two and six hours a week reviewing code changes. Developers wait a day on average for code review feedback. But in some cases, reviewers may take days, or even weeks, to get back with their findings. These code changes can’t be checked in to the mainline, can’t be tested, and can’t be factored into delivery timelines.

So it is critical that teams build up the discipline and a collaborative and supportive culture, ensuring that everyone is prepared to step up and help other team members by getting their reviews done. And it is even more important that they have the discipline to take reviews seriously, and are willing to put the necessary work in to do a good job. Because, as we’ll see, doing code reviews properly isn’t easy.

Understanding Somebody Else’s Code Is Hard

One of the main reasons that reviewers don’t find as many bugs as they should or could is because in order to find problems, they need to understand what the code does and how and why it was changed.

Understanding all of this takes most of a reviewer’s time, which is why most review comments are about readability (naming, commenting, formatting) and how to make code simpler or clearer, instead of about more fundamental problems.

If reviewers aren’t familiar with the code, they will also need to read more code to establish context, which means that they may run out of time before they can find anything materially wrong.

This is another reason why it’s important to have code reviews done by experienced team members. People who have worked with the code before—changed it themselves or reviewed it previously—have a clear edge over someone who has never seen the code before. They can work much faster, and provide more effective feedback.⁹

Finding Security Vulnerabilities Is Even Harder

Finding functional or logical defects in somebody else’s code is hard, as we’ve seen. A reviewer has to understand the purpose of the change, the context, and understand the code well enough to find mistakes in implementation or logic errors.

Finding security vulnerabilities is even harder. You start off with the same challenges—the need to understand the purpose and context, the structure and the coding style, and so on—and the reviewer also needs to understand secure coding and what to look out for.

Let’s look at some more research to see how hard reviewing for security vulnerabilities really is.

In a 2013 study, 30 PHP developers (including some security experts) were hired to do independent manual security code reviews of a small web app with 6 known vulnerabilities. The reviewers were given 12 hours to complete the reviews, and were not allowed to use static analysis tools.

None of the reviewers found all the known bugs (although several found a new XSS vulnerability that the researchers hadn’t known about), and 20% of the reviewers didn’t find any vulnerabilities at all. Reviewers with more coding experience didn’t necessarily find more security bugs—because even experienced developers don’t always understand what to look for in a security code review.¹⁰

We can see how difficult, and how important, code reviews are in security by looking closely at one of the most high-profile security vulnerabilities of the past few years: the OpenSSL Heartbleed vulnerability. This critical weakness in SSL handshaking was caused by not checking for a buffer over-read in C, a simple, low-level coding mistake.

What was surprising was that even though the original change was reviewed by someone who had worked on the OpenSSL code before, and the code was open source and available to anyone to read, it took more than two years before a security team at Google found the bug in a code audit.¹¹

The author of the change, the original reviewer, several static analysis tools, and almost everyone in the open source community who downloaded and used the code all missed the bug because the code was simply too complicated to understand, which also meant that it was too complicated to change safely. This should never be the case for security-critical code.

Understanding code, making it simpler and cleaner and clearer, emphasizing coding guidelines and continuous refactoring, isn’t just about making code easier to change for the next person. It’s also about making the code more secure.

Build on What the Team Is Doing, or Should Be Doing

If the team is already doing pair programming or peer reviews, help it understand how to include security in reviews, using the guidance in this book. Work with managers and the team’s Product Owner and Scrum Master to make sure that the team members are given enough time to learn, and time to learn about security as they are working.

Make sure developers are trained in the fundamentals of secure coding, so that they will write safer code, and so that they know what kinds of problems to look for when they are reviewing code. Take advantage of SAFECode’s free training courses and public information from OWASP. Be practical when it comes to training. You don’t need to train everyone on the team on the low-level details of how to do encryption properly. But everyone needs to understand when and how to correctly call crypto functions.

Don’t rely on reviews by inexperienced team members. Insist on experienced, qualified reviewers, especially for high-risk code.

If the team isn’t doing pair programming or peer reviews:

• Find people that the team trusts and respects, senior developers who care about writing good code and who like to take on challenges.

• Do everything you can to convince these people (and their managers) that it is an important and valuable use of their time to review code, starting with high-risk framework code and security features.

• Make sure that they have the information and training to know what problems to look for.

• If you work in a regulated industry, use compliance as a stick, if you have to.

Refactoring: Keeping Code Simple and Secure

Code that is clean and clear and consistent is less likely to have bugs (including security bugs) and easier and safer to change and fix. And much easier to review: if reviewers can’t understand the code, they will waste time trying to figure out what is going on. And they will miss the opportunity to find bugs.

This is why, for example, post-Heartbleed, the OpenSSL team spent months reformatting the code and deleting unused code and doing other simple cleanup work. The team needed to do this before it could take on more important, and more fundamental improvements.¹²

Fortunately, many Agile teams, especially teams following Extreme Programming, understand the importance of writing clean code and enforcing consistent coding guidelines. If you are working on one of these teams, your job as a reviewer will be much easier—and more valuable.

If not, you should encourage the team to take advantage of refactoring, another common Agile practice. Refactoring—a well-defined, consistent set of patterns and practices for cleaning up and restructuring code in small, incremental steps—is built in to most modern IDEs. Developers can rename fields and methods and classes, extract methods and classes, change method signatures, and make other, more fundamental structural changes safely and predictably—especially if they have a good regression test safety net in place, something we will look at more in the next chapter.

Using refactoring tools in their IDEs, reviewers can do their own quick-and-dirty, throwaway “scratch refactoring” until they understand the code better, suggest refactorings in review comments, or submit refactoring changes in a separate patch back to the author. But if reviewers need to reformat code in their IDE or refactor the code before they can review it, the team is doing something wrong.

Fundamentals Will Take You a Long Way to Secure, Safe Code

If you can get past the point of understanding, either because the code is clean, or because you cleaned it up yourself, or because you have worked with it enough that you can make sense of it, then you can start to review what the code actually does—or doesn’t do.

As part of code reviews, even without a strong understanding of secure coding, reviewers can also look out for:

1. Hardcoded passwords or other credentials, hardcoded paths, hardcoded magic numbers, and other hardcoded things which could be dangerous.

2. Test code or debugging code left in by accident.

3. Ensuring that sensitive data is encrypted or tokenized or otherwise handled safely—whether encryption is done properly is a security specialist problem, but whether it is done at all is a business problem that can be handled by anyone on the team who is familiar with the requirements.

4. Mistakes in error handling—good error handling is another part of defensive programming, and because error handling is hard to test and rarely tested, it is important to check that errors will be handled properly during code reviews.

5. Access control checks, making sure that they are applied and maintained correctly and consistently. Who can do what or see what in the system are business rules, not a security problem for expensive pointy-hat security wizards.

6. Consistent auditing for add/change/delete actions.

7. Consistent use of approved frameworks and standard libraries—especially when dealing with security functions like crypto or output encoding.

8. Thread safety, including time-of-check/time-of-use and deadlocks: these problems are hard to find in testing, so it is particularly important to look out for concurrency and timing and locking problems in code reviews.

None of the things in the list requires specialized security knowledge. You don’t need to bring in a security auditor to make sure that your team is writing clean, solid, and safe defensive code.