Malice or Stupidity or Inattention? Using Code Reviews to Find Backdoors

The temptation to put a backdoor into a product is almost overwhelming. It’s just so dang convenient. You can go into any office, any lab, any customer site and get your work done. No hassles with getting passwords or clearances. You can just solve problems. You can log into any machine and look at logs, probe the box, issue commands, and debug any problem. This is very attractive to programmers.

I’ve been involved in several command line interfaces to embedded products and though the temptation to put in a backdoor has been great, I never did it, but I understand those who have.

There’s another source of backdoors: infiltration by an attacker.

We’ve seen a number of backdoors hidden in code bases you would not expect. Juniper Networks found two backdoors in its firewalls. Here’s Some Analysis of the Backdoored Backdoor. Here’s more information to reaffirm your lack of faith in humanity: NSA Helped British Spies Find Security Holes In Juniper Firewalls. And here are a A Few Thoughts on Cryptographic Engineering.

Juniper is not alone. Here’s a backdoor in AMX AV equipment. A Secret SSH backdoor in Fortinet hardware found in more products. There were Backdoors Found in Barracuda Networks Gear. And The 12 biggest, baddest, boldest software backdoors of all time. Who knows how many backdoors are embedded in chips? Security backdoor found in China-made US military chip. And so on.

By now we can pretty much assume backdoors are the rule, not the exception.

Backdoors are a Cheap form of Attack

The problem is two fold: backdoors are very economical to insert and backdoors are almost impossible to test for.

Backdoors are economical because all you need is a human asset on the inside to insert code into a code base or circuits into a netlist. Or perhaps an attacker can corrupt the tools that produce the code and the chips? Or perhaps an attacker can corrupt the manufacturer who is spinning your ASIC?

You Can Never Know who You are Hiring

Is that incredibly talented candidate someone who really wants to work for you? Or are they a plant? I know I’ve wondered this when hiring people. Isn’t that person a little too perfect to want to work here?

In most shops it would be trivially easy to insert malicious code into a code base. Employees are trusted. As they must be for a team of people to get quality work done.

Trojan employees are not the only attack vector. Maybe one of your loyal longstanding employees has been compromised. Perhaps they are being blackmailed into inserting a backdoor?

Yes, this is the way we have to think now. It sucks.

Testing Only Tests What You Expect to be There

How would you know a backdoor has been installed? Testing usually only tests for positive assertions of facts. Does system Y do X?

How would you test for a backdoor that you don’t know is there? You might only know if the backdoor itself caused an externally visible signal. Perhaps it interferes with some other code? Perhaps it makes an image suspiciously large? Perhaps it draws too much power? Perhaps it makes something a tick slower than it should be and that causes an investigation that leads to discovery.

Lots of Eyeballs are Not Enough

Have you ever had a ponder of programmers all stand around a monitor looking for a bug in a short seemingly clear piece of code...and nobody sees the bug?

The glibc remote hacking vulnerability existed since 2008.

We now know for sure that a Bazaar full of eyes is not enough to ensure code quality. Simple quantity is not enough. We have to go for quality and that means code reviews.

Yuck, code reviews, I know.

Perspective Based Code Reviews

When I worked on an embedded software system for a Five 9s core optical switch, code quality was of the highest importance. This thing could never fail. Really. So I implemented a code review system that required every line of code be reviewed by two or more developers before it was checked-in. It was a cool system. All automated and clean. This is an addition to a battery of systems tests, regression tests, unit tests, customer acceptance tests, etc. Quality is a defense-in-depth sort of situation.

What’s important is not just that code is reviewed, but how it’s reviewed.

The benefit of code reviews is undeniable. Yet, code reviews don't happen in most software development organizations. And for good reason. The way code reviews are typically handled uses an enormous amount of people and time resources. Even with system quality improvements developers may not feel the effort is worth the cost.

It turns out you don't need a high overhead code review process to get results. My initial impression of code reviews is that they were heavy and bloated and a waste of time. I was wrong on all counts. Here are some interesting facts I found in the research:

Using two inspectors is as effective as using four or more.

Using an asynchronous process is as effective as a meeting based process for code reviews. For design reviews a meeting based approach may be more effective.

One review session is as effective as multiple inspection sessions.

Scenario or perspective based reviews were more effective than add-hoc and check-list based reviews.

Inspections are effective.

Inspecting upstream process like requirements and designs is very effective.

The implication is that the complexity and overhead of Fagan like code inspections are not needed and a semi-automated method will yield improved system quality. That’s where  perspective based code reviews come in.

A perspective based code review isn’t just a few arbitrary people glancing at code.

From Are the Perspectives Really Different?

Perspective-Based Reading (PBR) is a scenario-based inspection technique where several reviewers read a document from different perspectives (e.g. user, designer, tester). The reading is made according to a special scenario, specific for each perspective. The basic assumption behind PBR is that the perspectives find different defects and a combination of several perspectives detects more defects compared to the same amount of reading with a single perspective.

A perspective-based review is intended to:

focus the responsibility of each inspector

minimize the overlap among reviewers

provide a subject matter “expert”

The idea is simple. Every line of code belongs to a module. The module scope defines a set of perspectives that must reviewed for any code in that module. A set of stakeholders qualified to review on that perspective are defined for each module. Module scope defines the minimum number of total reviewers and the minimum number of reviewers for each perspective. This is all easily automatable with a little tooling.

A module should be some coherent body of code, like device drivers, front-end, database, etc.

A perspective is some quality the code must possess to be considered acceptable to be included in the code base. In my example, since it was a real-time embedded system written in C++, the perspectives include topics like semaphore usage and memory management.

This is a very freeing approach in a way. You aren’t told to look at a chunk of code and “find a problem.” That’s almost impossible. There are too many things to keep in mind. You are told to look at a chunk of code from a very particular perspective and only that perspective. And you are told what to look for. So you can concentrate on finding very specific problems, which people are pretty good at.

Here’s an example.

Example Semaphore Code Review

When a person--not necessarily a developer, you can imagine PMs and even managers are included for some reviews--is assigned to perform a semaphore perspective review on a code change, here is what they would consider when looking at the change.

Semaphore Code Review Guidelines:

Ensure the semaphore is required. If it is not used to provide mutual exclusion or synchronization, consider removing it.

The Semaphore should be associated with a data structure or set of related data structures and be named appropriately.

Ensure that all critical sections are correctly protected by the Semaphore.

Minimize the scope of the critical region.

Ensure the semaphore can never be left locked inadvertently. Consider the appropriate use of LockGuard.

Check for external calls from within a critical region. Be thorough, as this might not be readily apparent. A code browser is extremely useful when analyzing unfamiliar code. If an external call is unavoidable, the implications must be understood and should be acceptable. They include:

The possibility of blocking on attempted acquisition of external Semaphores.

The possibility of acquiring external Semaphores and possibly blocking other tasks. In general, subsystems should be decoupled as much as possible.

Check for calls into the subsystem which may acquire a semaphore. Is the potential duration of the blocking a known and acceptable behavior? This may not be readily apparent. A code browser is recommended for the analysis of unfamiliar code.

Here are the Semaphore Usage Guidelines:

Granularity. A mutex is intended to provide exclusive access to a specific data structure. If it protects multiple structures or no apparent structure, consider refactoring. In general, the finer the granularity, the better.

Scope. The scope of a mutex should be as narrow as possible. It should protect only the critical section. In general, the smaller the scope, the better.

• Coupling. Making an external call while holding a mutex is perilous. Examples of external calls include persistence requests, calls to a messaging system (especially synchronous calls), and direct invocation of a method on another subsystem. Holding a mutex while making such a call introduces opportunities for the simultaneous acquisition of multiple mutexes, long and indeterminate blocking times and, possibly, priority inversion. The resultant tight coupling of tasks can create run-time behavior that is undesirable and very difficult to reproduce and correct.

You can probably imagine a number of different perspectives for your kind of system.

There’s a lot more to be said on the subject, but I’m sure you’re tired of it by now.

Require a Security Perspective in Your Code Review

In retrospect I’m embarrassed to admit that I did not include a security perspective for any review. In my defense it was a more naive time. What’s clear now is each and every code change should be reviewed for security flaws.

Another admission: I have no idea what a security review would even look like. I imagine it would be module specific. A core developer isn’t likely to be an expert in SQL injection attacks, for example. Nor are most people going to be encryption experts. So considerable thought must go into what this means and how it works.

I’d like to hear other people’s ideas on the subject.

Reviewing an Existing Code Base

A code review process works for new changes, but what about the pile of code you’ve already developed?That is a problem.

One solution would be to review chunks of code, in some sort of priority order, over time. The automated system could schedule code reviews for existing unchanged code in such a way that complete code coverage could be achieved without out overwhelming developers.

Would it Work?

I think a security perspective review on each and every code change would be a huge deterrent to the problem of a lone attacker inserted into an organization. They would know the chances of passing a hack through the entire process would be low (assuming a tight process). And it would be much more difficult to corrupt a small group of people to let a backdoor through.

Something to think about anyway.

Code Review Standard

For the hard core among you here’s the code review standard I developed. It might be a good template to adapt for your situation.

Every Line of Code Must Be Reviewed Before it is Checked-In

Do not allow code to be checked-in unless it has been reviewed, fixed, and rereviewed. If you can't do this then your review process isn't fast or light enough. It can be done.

Reviewing code after it has checked-in is next to useless as everyone is exposed to the unreviewed code.

Code Must Be Integrated, Compiled, Unit Tested, and System Tested Before Review

Spending time on code that is just going to change or doesn't work is a massive waste of time. The entrance requirements for a code review are:

Code should compile without errors or warnings (according to coding standard).

Code should be already be integrated with its parent branch.

Code should be fully unit tested.

Code should already pass the system tests where possible.

Develop Perspective Based Reviews

For the issues that are important in your software consider developing a Perspective Review for each issue. Perhaps special hotspots for you are internationalization, memory corruption, memory usage, threads, semaphores, etc. An example can be found in Semaphore Perspective Code Review.

You can then use the perspectives to assign roles to review team members.

Use Meetingless Asynchronous Reviews

You don't need to hold a big meeting for a review. Everyone can review the code when they can find time. Simply have a done-by date when the review must be completed.

Everyone on the review must review the code. If a person can't perform the review then the team needs to elect someone else to perform a similar role in the review.

Meetingless asynchronous reviews are fast and light, yet they are still effective. With the right tool support you can easily review every line of code in your system.

Use Between 2 and 5 Relevant Reviewers

Too many reviewers wastes everyone's time. Keep the number of reviewers small and relevant to the code being reviewed.

Assign Reviewers Roles and Perspectives

It's almost impossible to review a lot of different issues in a lot of code. People get tired and they stop noticings things.

The way around the getting tired issue is to use perspective reviews. Create a perspective for each important issue category your are concerned about. Assign the perspectives to people on the review team. Because they are only reviewing for issues in their perspective they will do a better job because they can stay more concentrated. This doesn't mean they can't find other issues as well, but they responsible for their perspective.

For example, if using semaphores correctly is important in your software and the code has semaphores, then assign someone the role of reviewing semaphore usage. An example can be found in Semaphore Perspective Code Review.

All Review Communication Should Go to the Review List and be Logged

Part of the benefit of a review is that people learn about the system being reviewed. This learning feature is facilitated by broadcasting email communication between the review team and saving all communication so it can be read by other people later.

Do Not Redesign in the Review

Make a note and schedule design issues for for a later time.

Developers always think they can do stuff better. Take these issues off line unless the issue is that requirements are not being met. Requirements not being met is not the same as you could have done it better.

Do Not Cover Coding Standards Violations in the Review

Send violations via email or in person.

Talking about violations only gets everyone angry and is a waste of time.

Code Is Rereviewed Until it Passes

Code isn't reviewed once and then forgetten. Any changes made have to be rereviewed. If you think this is too slow then your process isn't light enough.

No reviewing all changes makes the process useless as people will just ignore suggestions or introduce new bugs in any changes.

All Issues Must be Fixed, Marked as Not an Issue, or Marked as Bug

Any issue brought up to a developer must be handled. A developer just can't ignore issues because they think it's stupid. Every issue must:

Fixed.

Marked as Not an Issue. If the developer and the reviewer can't decide between themselves if an issue should be fixed or not, then the review team gets to decide. If there is only one reviewer then bring a manager in or another team member.

Automate Your Code Review System

You can make your process light enough by building it into your build system. If your process isn't light enough work on until it is.

Review All Code on Private Branch Before a Merge

Code developed on a private branch doesn't need to reviewed during development. But before the code is merged into a parent branch all code changes must go through the complete review process. For this reason, development of a large scale feature, may still want to perform reviews on the private branch because that can speed up the merge process.

Of course, try not to have branches separate from the mainline, but for large features that take a long time to develop you will often need separate branches.

Review the Right Scope of Changes

You don't have to review every line of code in every module that has changed. Certainly if a module is new it must be completely reviewed. Other than that you may be able to just review the changed code. Though just reviewing changed code isn't always possible. If you are performing a semaphore perspective review, for example, then you will need to go look at the code within in the scope of the semaphore as well.

Stick to Reviewable Issues

Develop your list of what issues can be reviewed and how they are to be reviewed. Usually this is in the form of check lists and perspective reviews. Don't allow reviews on other items without changing what can be reviewed. Otherwise people spend endless time on off-topic arguments.

Review Team Responsible for Deciding Issues

If there is a conflict on any part of the review then the review team is responsible for handling it. That's the only way the review process will be light enough to work.

Keep it Cool

Nobody is perfect. The attitude of the review should never be personal, it should always be professional, with the goal of improving the system and the people building the system. Keep your tempers.

Don't blame people for bugs. Work together to make things better. No finger pointing! Not ever!

Meetingless reviews can help keep the anger down, but it can make it worse too. When people are in the same room anger can ramp up really quick. And we know in email it's very easy to say something that can be take wrong. Raise the awareness of these issues in your team.

A good rule is to Never Assume An Attack. If you find yourself getting angry, assume it's a misunderstanding, not an attack.

No Managers

Unless a manager has something to add to the reviews then they shouldn't be involved. Issues should be decided by the review team. Managers always have to run to a different meeting, they don't have time slots open for meetings, and they generally don't add technical input. So you don't need managers as part of the review process.

If a Bug Was Not Caught in a Review Figure Out Why

If a bug happens after a review then track down why each bug wasn't found and then change your development process somehow to try and prevent that bug in the future.

This is not always possible as running full tests are often impossible, but it should be mostly possible.

I would create a bug for each bug to track down why it wasn't caught. Because this issue is more serious than just the review. It means the unit test, the system test, and the review did not work.

Review Upstream Documents Too

Reviewing product requirement documents, specs, standards, etc can provide excellent return on value. Make sure those products are reviewed as well.

Feed Leasons Learned Into the Team and Documentation

If issues come up during the review that everyone in the team would benefit from, then have a way to make wisdom public.

I would recommend a development wiki where you can write documentation on anything useful that people come up with.

Plug Reviews into Your Source Code Control System

I have done this through change check-in comments. Each change has to be reviewed before it is checked-in. The submit comment must contain a review ID that points to some document containing the review status for the change that is about to be submitted. The code is prevented from being submitted without a valid passed review. If you are able to automate your code review system all this works quite quickly and painlessly.