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