Security Code Review Guidelines

Adam Shostack

First published online August, 1996.
$Id: review.html,v 1.17 2006/07/19 04:19:53 adam Exp $
Note that as of May, 2000, this document is not getting a lot of attention as to the latest tricks to avoid. Much of it is still valid, but I suggest that you look elsewhere for an ultamately up-to-date list.

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
    4. Calling open()

  1. Overview
  2. 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.

  3. Documentation

    1. 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.

    2. Architectural Overview
    3. 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.

    4. Code Overview
    5. 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.

    6. Comments in code
    7. 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.)

    8. Functional summary
    9. 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.

  4. Logging
  5. 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.

  6. Files
    1. Configuration files

    2. 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.

    3. Core files

    4. 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.

    5. Chroot()

    6. 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, a competent attacker can break out.

    7. NFS

    8. 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.
    9. Use Of setuid/setgid Permissions

    10. 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.)

  7. Code (Security Issues)
    1. Libraries

    2. 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.

    3. Command Line

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

    5. Data Checking

    6. 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.

    7. Confidentiality

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

    9. System Calls

    10. 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.

      realloc
      Realloc should not be used in crypto applications. If memory contiguous to that already allocated is not available, realloc will make a copy of the memory without zeroing the old bits. If this old memory contains keys, then you lose the pointer to the memory without destroying the information. This is generally considered a mistake.
      open
      The filesystem can change in ways you don't expect. There is sample code in an appendix for how to call open and be sure you avoid tmp races, linking games, and other things.
    11. 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().

  8. 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.
    9. UIDs should never be -1, which can sometimes happen if you have insufficient type strength, or otherwise try putting UID 65535 in a short.

  9. Testing
    1. 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.

    2. Compiler Checking
    3. The code must be run through available tools for code quality checking, such as lint or perl -w. See the language notes.
    4. Testing Tools
    5. We will work with the code's authors to produce a set of stress tests.

  10. Miscellaneous
    1. Code size
    2. 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.
    3. Revision Control
    4. 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.
    5. Code Formatting
    6. 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.
    7. Code ownership
    8. (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.
    9. Strong Authentication
    10. 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.

  11. References

    1. Web Security
    2. Code Review
    3. 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. More recently, the Linux Security Audit Project has a FAQ, and Kragen has a list of things to consider at Kragen.

    4. Logdaemon
    5. a properly paranoid set of UNIX daemons. ftp://ftp.win.tue.nl/pub/security/logdaemon.tar.gz
    6. Code testing.
    7. This paper details what happens when programs were fed arbitrary data.
    8. Cops
    9. Cops includes a man page entitled setuid(7) with much useful advice.
    10. Coding Standards
    11. 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.

    12. Misc. References
    13. 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.

      Rain Forest Puppy wrote an article in Phrack 55-07 about common failures in perl coding. If you're writing in perl, its worth reading.

      Simon Burr has a web page explaining how to break out of a chroot jail.

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
    2. 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. Initial revision
    3. C++
    4. As C.
    5. Perl
    6. 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.

      Also, be aware of the null byte issue when passing things to system calls: Perl allows nulls in strings, C does not, which can lead to interesting behavior. See Phrack 55-07 for details.

  2. Application Notes
    1. cgi-bin
    2. Lincoln Stein form library
    3. Mail addressing
    4. 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
      
    5. nsapi
    6. Proxy
    7. 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.
    8. DCE
    9. We like DCE's strong authentication. DCE RPCs without authentication and authorization are not useful.
    10. Cryptography

  • Change Log
  • $Log: review.html,v $
    Revision 1.17  2006/07/19 04:19:53  adam
    added note about Olaf Kirch's "Cryogenic Sleep" post to bugtraq.  Via
     Ilja van Sprundel.
    
    Revision 1.16  2004/11/13 17:17:21  adam
    added first published line.
    
    Revision 1.15  2001/11/06 20:47:32  adam
    updated for kragen
    
    Revision 1.14  2000/07/03 16:45:05  adam
    added pointer to chroot page.
    
    Revision 1.13  2000/05/02 18:30:55  adam
    added historical note.
    
    Revision 1.12  2000/05/02 18:27:52  adam
    added per zab
    
    Revision 1.11  1999/11/09 14:55:58  adam
    added pointers to RFP's 55-07 article
    
    Revision 1.10  1999/06/01 19:25:49  adam
    added open comments from Peter
    
    Revision 1.16  1999/06/01 19:14:23  adam
    added realloc comments (Thanks to pguttmann)
    
    Revision 1.15  1998/11/24 21:26:42  adam
    fixed system call/library call confusion pointed out by alert reader
    Olof Johansson
    
    Revision 1.14  1998/10/29 23:28:44  adam
    added that system calls must have their return status checked
    
    Revision 1.13  1998/09/02 22:44:03  adam
    David Landgren (david@landgren.net) provided improvements to Perl
    section about -T and strict.
    
    Revision 1.12  1998/07/24 12:58:39  adam
    added freebsd dos & don'ts
    
    Revision 1.11  1998/04/11 19:03:36  adam
    clarified chroot, added references to Matt Bishop's papers.
    
    Revision 1.10  1997/08/29 16:06:20  adam
    fixed link
    
    Revision 1.9  1996/10/05 05:19:31  adam
    VC System change
    
    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
    
    

  • Calling Open
  • It turns out that calling open() can be tricky. This code was provided by Peter Guttmann to handle cases where the filesystem is changing underneath you.

    18 July 2006: Note that this doesn't work quite as wella s you'd like. See http://www.security-express.com/archives/bugtraq/2000-01/0012.html for details."

    
    
        /* Under Unix we try to defend against writing through links, but this is
           somewhat difficult since the there's no atomic way to do this, and
           without resorting to low-level I/O it can't be done at all.  What we
           do is lstat() the file, open it as appropriate, and if it's an
           existing file ftstat() it and compare various important fields to make
           sure the file wasn't changed between the lstat() and the open().  If
           everything is OK, we then use the lstat() information to make sure it
           isn't a symlink (or at least that it's a normal file) and that the
           link count is 1.  These checks also catch other weird things like
           STREAMS stuff fattach()'d over files.
    
           If these checks pass and the file already exists we truncate it to
           mimic the effect of an open with create.  Finally, we use fdopen() to
           convert the file handle for stdio use */
    
        struct stat lstatInfo;
        char *mode = "rb+";
        int fd;
    
        /* lstat() the file.  If it doesn't exist, create it with O_EXCL.  If
           it does exist, open it for read/write and perform the fstat()
           check */
        if( lstat( fileName, &lstatInfo ) == -1 )
            {
            /* If the lstat() failed for reasons other than the file not
               existing, return a file open error */
            if( errno != ENOENT )
                return( -1 );
    
            /* The file doesn't exist, create it with O_EXCL to make sure an
               attacker can't slip in a file between the lstat() and open() */
            if( ( fd = open( fileName, O_CREAT | O_EXCL | O_RDWR, 0600 ) ) == -1 )
                return( -1 );
            mode = "wb";
            }
        else
            {
            struct stat fstatInfo;
    
            /* Open an existing file */
            if( ( fd = open( fileName, O_RDWR ) ) == -1 )
                return( -1 );
    
            /* fstat() the opened file and check that the file mode bits and
               inode and device match */
            if( fstat( fd, &fstatInfo ) == -1 || \
                lstatInfo.st_mode != fstatInfo.st_mode || \
                lstatInfo.st_ino != fstatInfo.st_ino || \
                lstatInfo.st_dev != fstatInfo.st_dev )
                {
                close( fd );
                return( -1 );
                }
    
            /* If the above check was passed, we know that the lstat() and
               fstat() were done to the same file.  Now check that there's
               only one link, and that it's a normal file (this isn't
               strictly necessary because the fstat() vs lstat() st_mode
               check would also find this) */
            if( fstatInfo.st_nlink > 1 || !S_ISREG( lstatInfo.st_mode ) )
                {
                close( fd );
                return( -1 );
                }
    
            /* On systems which don't support ftruncate() the best we can do
               is to close the file and reopen it in create mode which
               unfortunately leads to a race condition, however "systems
               which don't support ftruncate()" is pretty much SCO only, and
               if you're using that you deserve what you get ("Little
               sympathy has been extended") */
    #ifdef NO_FTRUNCATE
            close( fd );
            if( ( fd = open( fileName, O_CREAT | O_TRUNC | O_RDWR ) ) == -1 )
                return( -1 );
            mode = "wb";
    #else
            ftruncate( fd, 0 );
    #enndif /* NO_FTRUNCATE */
            }
    
        /* Open a stdio file over the low-level one */
        stream->filePtr = fdopen( fd, mode );
        if( stream->filePtr == NULL )
            {
            close( fd );
            unlink( fileName );
            return( -1 );  /* Internal error, should never happen */
            }
        }