## Security Code Review Guidelines

### Adam Shostack

    $Id: review.html,v 1.7 1998/11/24 21:26:42 adam Exp $

### Executive Summary

Before programs may be placed in the firewall system, the source code is reviewed for deficiencies in the areas of security, reliability and operations. This document is dual purposed; first it is a guideline and checklist for security groups performing the code review; second, it is an attempt to provide development teams with information about what we look for in a review.

This is an early draft of the guidelines; it is being distributed in the hopes of providing a more transparent and predictable code review process. We do not mean to imply that the things listed here are the only issues we will raise in a review. We will attempt to keep this document up to date, so that over time, it becomes a more useful guide.

#### Table of Contents

1. Overview
2. Documentation
3. Logging
4. Filesystem issues
5. Code (Security)
6. Code (Reliability)
7. Code Handover
8. Miscellaneous
9. References

#### Appendixes

1. Language specific notes
2. Application specific notes
3. Change Log

####
1. Overview

This document describes the code review portion of getting a machine approved for connectivity outside of Acme Widgets. The full process is described in ().

When networking computers that can access information of financial or personal value that Acme Widgets has entrusted to a computer system, it is essential that we consider information security. When network connections pass through networks outside of Acme Widgets's control, we are exposed to attacks from a variety of sources. Those sources are known to include vandals ("hackers") who will make changes to information as a means of gaining prestige in the underground community, or as a matter of revenge or disruption, and amateur thieves, who will attempt to steal. There may also be attacks by professional criminals or criminal groups who have access to substantial resources of both time and money, and can perform an in-depth attack. Acme Widgets is a large, high visibility target...

As such, the firewall architecture group is tasked with providing a state of the art Internet firewall that will protect us from all these threats, and simultaneously help Acme Widgets provide service to our customers over the Internet. We are interested in making it possible to deploy new applications that work over the internet, however, we must make sure that those applications are safe.

This means that we need to resist a variety of attacks, and we need to do so better than in our phone or mail businesses. Why better? Because on the phone, there is a person in the loop. In the mail room, there is a person in the loop. Online, it may be possible to scale an attack from annoying to disastrous, or from a small scam to a huge theft, because the entire attack might be automated.

So it is with a healthy paranoia that we approach screening new components to the firewall. We will examine the authentication and authorization methods that you use. We will look for confidentiality and integrity controls. We will look at administrative issues such as the documentation that the firewall support group needs, and how the program logs.

This document, in addition to making the process transparent, is intended to speed the code review process, by letting you know what we'll be looking for, so you can have it ready when we do the review. It also explains some of the coding practices and common mistakes that we will insist be changed, so you can come to us with code thats closer to being installable.

This is a requirements document. In some places things will be marked 'optional', or 'preferred.' This refers to a judgment call on the part of the Firewall Architecture group, with input from corporate security, the group whose code is being reviewed, and other participants in the code review process. The word optional may be construed to mean that we will not require a change, although we may strongly recommend it. Other things will be marked 'should,' and those things are open to discussion, if they are documented design decisions. In other words, the documentation must explain the choice at the start of the code review. Otherwise, we will require that the code be changed to confirm with accepted security practices.

The code review participants should remain constant from review to review. Participation by representatives from many groups is required for a successful code review. Those groups include, but are not limited to, (Firewall Operations), (Firewall Architecture) and Corporate Security. The code must be presented by a developer or group member who is qualified and able to answer technical questions about the code and design. There must be a scribe and a coordinator appointed. At the end of a review, all present must agree on an appraisal of the code. In the event of irreconcilable disagreements, the least positive appraisal that everyone can agree to will be the appraisal. At reviews beyond the first, copies of the diffs and the minutes of all previous meetings should be available, along with a full copy of the current code.

####
2. Documentation

The code must be accompanied by documentation. The documentation allows the review team to do a proper review, and provides the support people with information that they will need to run the code in the firewall environment. Templates for the architectural overview, code overview, and functional summaries will be made available.

#####
1. Architectural Overview

The review team will start with the architectural overview. This overview will include a diagram of the system being implemented, and the place of the code under review in the system. The diagram should show the client, the proxy and server, all in relation to the Acme Widgets Firewall system. Data flow from (Corporate Network) to the server should be documented, as should how the client might relate to a firewall at the remote site. The overview should also contain a textual description of the system, including a functional overview. The functional overview must include information about what threats the code is expected to deal with, and how it will deal with them. The use of cryptography for confidentiality, integrity, etc should be outlined here.

#####
2. Code Overview

Code reviews can be long and tedious. Having to figure out how the code is designed, what modules will be compiled in, and other such information, obvious to the developer, will slow the process of code review while we wait for the overview information, and thus, deployment will be delayed.

#####
3. Comments in code

We expect the code to have a reasonable number of comments. As a guide, each file should have a comment at the start, explaining what the code does, possibly a comment at the start of each function, and comments as needed to explain complex or obfuscated code. (Incidentally, a compilation of the per module header files might make a good basis for an overview document, although it will not be a complete overview.)

#####
4. Functional summary

There must be a functional summary for the firewall support group. This documentation is expected at the time of review, and will be evaluated as part of the review.

1. Installation Requirements & Environment
The install procedure must be documented. Can we copy a binary and a configuration file? Do we need to run any scripts to set up directory hierarchies? What will the permissions and ownership be on the installed files?

2. How to invoke.
   Does it run from the command line? Inetd? Cron? Rc2.d/S99Acme? Are there arguments that the program expects? Does it expect environment variables to be set? (We'd prefer they be moved to a configuration file. We might mandate some options be moved to a configuration file for reliability reasons.)

3. Signals
   Does the program react to any signals other than SIGKILL and SIGTERM? If it does, it should use SIGUSR1 to activate debugging, and SIGUSR2 to turn it off. In any event, the programs signal handling must be documented.

4. Options & Configuration Files
   A complete description of all command line options is required. A complete description of the configuration files is required.

####
3. Logging

All programs in the firewall must call syslog to report a variety of things. Syslog is our standard way of getting log information from the firewall to the analysis machines. Information to be logged includes, but is not limited to: Invocation, normal use (including which machines and what is being done), problems, and the program's termination, normal or abnormal. Logging should be configurable via a signal. We strongly suggest use of SIGUSR1 to turn on debugging, and SIGUSR2 to turn it off.

Programs must be careful to avoid logging passwords, cryptographic keys, and other sensitive data. In addition, be careful about the amount of user supplied data that syslog gets.

Passing debugging information to syslog is optional.

Log facility and levels to be used will be provided by (Firewall Architecture) on request. In general, use LOG_INFO for information, LOG_DEBUG for debug, and LOG_ALERT, LOG_CRIT, and LOG_ERR when those are reasonably called for.

####
4. Files

#####
1. Configuration files

The program should read as much as is feasible from configuration files; not have things such as host names hard-coded into it. (Hostnames, incidentally, should always be fully qualified.) The file should be in a standard location (/usr/local/etc/acme/program is strongly suggested), should have its ownership and permission checked before reading, and should not be writable by the uid the program is running under. The config file should also specify the log facility. The config file should be treated like other incoming data, on the chance that its been corrupted.

#####
2. Core files

Code on the firewall should attempt not to dump core if it might have sensitive data in memory that could be retrieved. Cores might be made retrievable by an information (ftp, gopher, or web) server running on the machine. If the machine is a proxy server, we can discuss the disposition of core files.

#####
3. Chroot()

The chroot() call irreversibly changes a programs view of the filesystem, creating a new 'root,' usually within a tightly controlled 'jail' filesystem or partition. This denies many useful tools to the attacker who has broken in, and is attempting to gain privledges. Code that will need filesystem access should be able to run under a chrooted environment. We will be invoking it from chrootuid. (Note that if your code runs with root privleges, or if there are buggy setuid tools available in the chroot area, an attacker may be able to break out.

#####
4. NFS

Just say No File Security.
NFS uses 'client side' security, and has a host of security problems associated with the portmapper, the way file handles are generated, and thus should be avoided.

#####
5. Use Of setuid/setgid Permissions

Giving code that runs on the firewall any privileges is strongly discouraged, and use of privilege bits will cause much more extensive scrutiny of the privileged code. If you need privileges, call seteuid() and setruid(). Also, consider putting all the privilege in a single small program that does its one thing without taking any arguments or user input. (Example, a program that binds to port 23 could be called port23, and execv(/usr/sbin/ftpd) after setting its uid to nobody.)

####
5. Code (Security Issues)

#####
1. Libraries

There are a number of security related libraries that can provide some of the functions described below. () is in the process of reviewing these libraries, and may be able to make a recommendation.

#####
2. Command Line

The command line arguments of a program should be checked carefully, especially if the code is running with, or might be invoked with, privileges.

#####
3. Data Checking

Data coming in to Acme Widgets should be checked very carefully for appropriateness. This check should be to see if the data is what is expected (length, characters). Making a list of bad characters is not the way to go; the lists are rarely complete. A secure program should know what it expects, and reject other input. (For example, if you are looking for a host name, don't check to see if it contains a semi-colon or a newline, check to see if it contains anything other than a [A-Za-z0-9-] followed by a dot (period), followed by a hostname [A-Za-z0-9-].) See Appendix B for some comments on email address parsing.

It is not appropriate to assume that a connection is coming from the client that you expect, unless you take strong measures. It very difficult to ensure that you are not talking to a bad guy. Doing so requires that a connection use strong authentication up front and be encrypted and authenticated throughout its lifetime.

Even when this is done, there is a chance that a user might find it worth attacking through an authenticated connection. Consider verify the sanity of inbound data.

#####
4. Confidentiality

Data leaving the firewall should be checked for confidential Acme Widgets information, and most likely encrypted for confidentiality.

#####
5. System Calls

All system or library calls must have their return values checked, and errors handled. Not checking the results of a system or library call is unacceptable. The sole exception to this is that class of calls which are designed to either work or exit, and thus can not return failure.

Certain library calls have historically, been found to be associated with security problems, because they do no checking, and user input is often passed to them. Use of these calls is strongly discouraged. Those calls include but are not limited to,

system
  If there's user input in the arguments, the user might be able to run a command.
exec, popen
Similar to system. Use execl or execv, but be careful not to pass user data to them without strong sanity checking.
setuid and setgid
  Programs shouldn't be able to use these calls, because they shouldn't have any privileges.
strcpy, strcat, sprintf
  don't check the length of the strings they're working with. Use strncpy, strncat.
getenv
  Can produce buffer overflows. Also, watch for a variable set two (or more!) times.
gets, scanf
  Improper bounds checking. Use read, fgets
gethostbyname, gethostbyaddr
  These, and other calls that get data from the DNS, may return malicious data under the control of a bad guy. Getting a DNS server is pretty easy, as is having it return 64k of data for your hostname, or returning a Acme Widgets address instead of the real one. Any time you're getting data from the DNS, consider doing whats referred to as a double-reverse lookup, and again, don't trust the data returned will be in a safe format, or correct. Code that correctly checks the information returned by the dns can be found in the logdaemon package.
syslog
  If syslog is passed information derived from user input, be careful to not overflow syslog's buffers. The maximum buffer size to pass is 1024 bytes.

6. ##### Atomicity

   Many security holes are related to programmer expectations that the flow of their program is uninterrupted. File access should be atomic. Temporary files, if they exist, should be created with tmpfile().

####
6. Code (Reliability Issues)

1. System calls should have their return status checked.
2. Signals should be caught and handled.
3. Multithreaded code should use mutex() calls on globals.
4. C++ classes should have a constructor, destructor, (??)
5. Configuration information should be in a configuration file, not hardcoded into the program. See the section on configuration files.
6. The year 2000 is real soon now. Leap years happen every four years. Daylight savings may move the clock back an hour annually.
7. Functions should perform bounds checking.
8. umask should be set to a restrictive value.

####
7. Testing

During the process of writing code, it should be tested for functionality and security. These tests should be made available to the review team, preferably in the form of a script. Tests should look for things such as buffer overflows, proper dataflows, resistance to unusual input (control characters, for example), off-by-one loop errors, possible unterminated loop situations, etc.

#####
1. Compiler Checking

The code must be run through available tools for code quality checking, such as lint or perl -w. See the language notes.

#####
2. Testing Tools

We will work with the code's authors to produce a set of stress tests.

####
8. Miscellaneous

#####
1. Code size

The programs running in the firewall should be kept to a minimum of size and functionality, because all code, even when we're done reviewing it, has bugs. The more code there is, the more bugs there will be. The more bugs there are, the more security related bugs there will be. Thus, we want the smallest code that is reasonable for the job. The code should also be kept simple and straightforward.

#####
2. Revision Control

The code must be under revision control. RCS or similar log messages and version numbers must be in the code and available from strings(1). Revision based differential information must be available.

#####
3. Code Formatting

All code will be run through a standard formatter before review, and printed with file names, line and page numbers on each page. Lines will be formatted to wrap at a reasonable length. We suggest the use of the unix command "fmt FILE | vgrind | lpr" or "pr -n | mpage -2 -f | lp". The presenter of the code is responsible for providing the coordinator with the code in paper and electronic form at least one full day before a review.

#####
4. Code ownership

(Firewall Architecture) and (Firewall Operations) will take ownership of the code once it is frozen. It will be kept in an archive in a buildable form. The binaries from this code will be installed as per the install instructions. Cryptographic checksums of the code should be recorded with code version numbers.

#####
5. Strong Authentication

Strong authentication should be used on all connections. Contact (Firewall Architecture) for a list of supported authentication methods. All authentication should be managed by the TIS authmgr.

9. #### References

   #####
   1. Web Security

   - WWW Security FAQ.
   - Writing Secure CGI, a primer.

   #####
   2. Code Review

   Nuclear Power Plant Code Review Guidelines
   AusCert's writing secure UNIX code Guidelines
   The Software Inspections and Review Organization has some very useful things.

   Its worth noting that even small bugs should be fixed. Read this to see an example of a small bug that was identified and blew up in the designers faces.

   #####
   3. Logdaemon

   a properly paranoid set of UNIX daemons. ftp://ftp.win.tue.nl/pub/security/logdaemon.tar.gz

   #####
   4. Code testing.

   This paper details what happens when programs were fed arbitrary data.

   #####
   5. Cops

   Cops includes a man page entitled setuid(7) with much useful advice.

   #####
   6. Coding Standards

   The GNU Coding standards are a set of coding standards that produce some of the consistently best code that is freely available.

   FreeBSD has a page of Security Do's and don'ts for programmers.

   #####
   7. Misc. References

   Ross Anderson's papers on Why Cryptosystems Fail, and Robustness Principles for Public Key Cryptosystems are well worth reading.

   The ACM Forum on Risks to the Public (news:comp.risks) is full of great background.

   Matt Bishop's Writing Secure SetUID Programs page. Several well done papers that are well worth reading.

#### Acknowledgements

Simson Garfinkel & Gene Spafford's Practical UNIX security, 2nd edition contains useful notes on writing secure code. The section has been made available as an AUSCERT publication. Marcus Watts offered excellent insights into parsing user data. Mark O. Aldrich pointed me towards the SSECMM web site. Bernd Eckenfels, Ge' Weijers, Igor Chudov and others offered advice on parsing email addresses. Peter M Allan caught many large errors in the details, most notably a suggestion to use execve. He also pointed out the setuid man page.

### Appendices

###
1. Language Notes

#####
1. C

The code must lint(1) cleanly. It must be compilable with -Wall or equivalent without warnings. It should pass an extended memory checker, such as Purify or Electric Fence. Flexlint should also be used for checking.

#####
2. C++

As C.

#####
3. Perl

The code must run cleanly under perl -Tw. The T option directs perl to use its taint checking mode, and -w is warnings. Also, the directive "use strict" causes perl to catch more problems in the code.

###
2. Application Notes

#####
1. cgi-bin

Lincoln Stein form library

#####
2. Mail addressing

There are a wide variety of legal email address formats. We strongly advise that a paranoid, minimalist approach in choosing what to accept. This will cut off some subset of customers, but most people have available to them Internet style (user@host.net) addressing. The issue of how to handle unusual email address is a design decision. If an unusual address is passed, it may have repercussions later when some other bit of code expects internet addressing. You might consider some form of verification for bizarre addresses.

Legitimate address formats include:

         user_name@company.com
         alex+@pitt.edu
         mi%aldan.UUCP@algebra.com
         user%host.domain@anon.penet.fi
         host1!host2!user
         /G=JABYML/S=MASONS/O=CUSTOMER/ADMD=FOOBAR/C=KZ/@gateway.sprint.com
         "JE Jones"@foo.x.400.net

#####
3. nsapi

#####
4. Proxy

If you choose to write a new proxy, we recommend using a process model if the typical session will last more than a few seconds. The speed gain from threading is outweighed by the increased complexity, and that gain is amortized over the life of a process.

#####
5. DCE

We like DCE's strong authentication. DCE RPCs without authentication and authorization are not useful.

#####
6. Cryptography

- You must use standard algorithms. No algorithm not subjected to extensive public peer review will be accepted.
- You should use standard protocols wherever possible. PKCS message formats are good.
- Use standard implementations, don't recode algorithms or protocols.
- Understand the difference between authentication, privacy, and non-repudiation.

###
- Change Log

    $Log: review.html,v $
    Revision 1.7  1998/11/24 21:26:42  adam
    fixed system call/library call confusion pointed out by alert reader
    Olof Johansson

    Revision 1.6  1998/10/29 23:28:44  adam
    added that system calls must have their return status checked

    Revision 1.5  1998/09/02 22:44:03  adam
    David Landgren (david@landgren.net) provided improvements to Perl
    section about -T and strict.

    Revision 1.4  1998/07/24 12:58:39  adam
    added freebsd dos & don'ts

    Revision 1.3  1998/04/11 19:03:36  adam
    clarified chroot, added references to Matt Bishop's papers.

    Revision 1.2  1997/08/29 16:06:20  adam
    fixed link

    Revision 1.1  1996/10/05 05:19:31  adam
    Initial revision

    Revision 1.8  1996/09/04 17:09:26  adam
    Added changelog section

    revision 1.7   1996/09/04 17:06:14 adam
    Added comments about possible syslog buffer overflows.
    Added a few lines about the unavailability of NFS
    Changed execve to exec
    moved email example to appendix, used hostnames instead.
    Moved most of compiler checking to appendix, left platitude about using available
    e tools
    Added requirement for paper, electronic copies from presenter a day
      before a review, included command line to print with.
    Pointers to SIRO, Cops' setuid man page
    Acknowledgements
    Filled out appendixes on proxies, dce, and cryptography

    revision 1.6  1996/08/29 17:51:07    adam
    mail addressing expanded
    acks
    spell checking

    revision 1.5  1996/08/15 21:03:24     adam
    rearranged acknowledgements, fixed font sizing

    revision 1.4  1996/08/15 20:55:21     adam
    added appendixes
    Fixed html for references
    Added some of JB's comments

    revision 1.3  1996/08/12 20:19:59     adam
    Made changes as per JB's large suggestions.

    revision 1.2  1996/08/07 19:08:30     adam
    Finished htmlizing docs

    revision 1.1   1996/08/07 18:44:48    adam
    Initial revision