%includes; ]> &header;

• Never trust any source of input, i.e. command line arguments, environment variables, configuration files, incoming UDP packets, hostname lookups, function arguments, etc. If the length or contents of the data received is at all subject to outside control then the program or function should watch for this when copying it around. Specific security issues to watch for in this area are:
  • strcpy() and sprintf() calls from unbounded data. Use strncpy() and snprintf() when the length is known (or implement some other form of bounds-checking when it's not). In fact, never use gets(3) or sprintf(3), period.

  • strncpy() and strncat() calls. Be sure you understand how these work\! strncpy() might not append a terminating \\0 while strncat() always adds the \\0.

  • Watch for strvis(3) and getenv(3) abuse. strvis() is easy to get the destination string wrong for, and getenv() can return strings much longer than the user might expect - they are one of the key ways an attack is often made on a program, causing it to overwrite stack or variables by setting its environment variables to unexpected values. If your program reads environment variables, be paranoid!

  • Every time you see an open(2) or stat(2) call, ask yourself, "What if it's a symbolic link?"

  • All uses of mktemp(), tempnam(), mkstemp(), etc.; make sure that they use mkstemp() instead. Also look for races in /tmp in general, being aware that there are very few things can be atomic in /tmp:
    • Creating a directory. This will either succeed or fail.
    • Opening a file O_CREAT | O_EXCL
    mkstemp(3) properly handles this for you, so all temp files should use mkstemp to guarantee there's no race and that the permissions are right.

  • If an attacker can force packets to go/come from another arbitrary system then that hacker has complete control over the data that we get and *NONE* of it should be trusted.

  • Understand the differences between uid, euid and svuid in 2.1 and 2.2. We sure don't. [XXX but we should find out and fill this in after talking to Bruce]

  • Never trust that a config file is correctly formatted or that it was generated by the appropriate utility. If there is some chance for being sneaky, then some twisted cracker will try to be sneaky: Don't trust user input like terminal names or language strings to be free of '/' or ../../... embedded if there is any chance that they can be used in a path name. Don't trust *ANY* paths supplied by the user when you are running setuid root.

  • Look for holes or weaknesses in how data is stored. All temp files should be 600 permission.

  • Don't just grep for the usual suspects in programs which run at elevated privs. Look line by line for possible overflows in these cases since there are a lot more ways than strcpy() and friends to cause buffer overflows.

  • Just because you drop privs somewhere doesn't necessarily mean that no exploit is possible. The attacker may put the necessary code on the stack to regain them before execing /bin/sh.

• Do uid management. So drop privs as soon as possible, and really drop them. Switching between euid and uid is not enough. Use setuid() when you can.

• Never display configuration file contents on errors. A line number and perhaps position count is enough. This is true for all libs and for any SUID/SGID program.

• Tips for those reviewing existing code for security problems:
  • If you're unsure of your security fixes, send them to a reviewer with whom you've already made arrangements for a second glance over. Don't commit code you're not sure of since breaking something in the name of securing it is rather embarrassing. :)

  • Those without CVS commit privileges should make sure that a reviewer with such privileges is among the last to review the changes. That person will both review and incorporate the final version you would like to have go into the tree.

  • When sending changes around for review, always use context or unidiff format diffs which may be easily fed to patch(1). Do not simply send whole files! Diffs are much easier to read and apply to local sources (especially those in which multiple, simultaneous changes may be taking place). All changes should be relative to 3.0-current just so we can all be working from a common base, unless there is strong reason in a specific instance to do otherwise.

  • Always directly test your changes (e.g. build and run the affected module(s)) before sending them to a reviewer; no one likes being sent obviously broken stuff for review, and it just makes it appear as though the submitter didn't even really look at what he was doing (which is hardly confidence-building). If you need accounts on a 2.1, 2.2 or 3.0 machine in order to do proper testing, just ask - the project has such resources available for just such purposes.

  • For committers: Be sure to retrofit -current patches into the 2.2 and 2.1 branches as appropriate.

  • Do not needlessly rewrite code to suit your style/tastes - it only makes the reviewer's job needlessly more difficult. Do so only if there are clear technical reasons for it.

• Look out for programs doing complex things in signal handlers. Many routines in the various libraries are not sufficiently reentrant to make this safe.

• Pay special attention to realloc() usage - more often than not, it's not done correctly.

• When using fixed-size buffers, use sizeof() to prevent lossage when a buffer size is changed but the code which uses it isn't. For example: char buf[1024]; struct foo { ... }; ... BAD: xxx(buf, 1024) xxx(yyy, sizeof(struct foo)) GOOD: xxx(buf, sizeof(buf)) xxx(yyy, sizeof(yyy)) Be careful though with sizeof of pointers when you really want the size of where it points to\!

• Every time you see "char foo[###]", check every usage of foo to make sure it can't be overflowed. If you can't avoid overflow (and cases of this have been seen) then at least malloc the buffer so you can't walk on the stack.

• Always close file descriptors as soon as you can -- this makes it more likely that the stdio buffer contents will be discarded. In library routines, always set any file descriptors that you open to close-on-exec.

&footer