String handling audit and new HACKING points

Russ Allbery rra at stanford.edu
Tue Aug 13 01:41:31 UTC 2002


Over the weekend, I finally found time to go through the (year-old!)
omnibus patch from Bear Giles to correct questionable uses of sprintf and
strcpy.  Most of that patch was mechanical replacements with snprintf and
strncpy; in applying it, I tried to instead change to concat, concatpath,
or xstrdup where appropriate.  There are also some cases where I left
sprintf or strcpy as is, such as when there was an explicit check
immediately before that code or where this change would not actually make
the code any more secure (because there is a deeper problem).

There are still quite a few places where one cannot be sure from casual
inspection that the string handling is safe, and several places where I'm
sure that it's not (but where it's not exploitable since all of the
information being worked with is either limited by accident by other parts
of INN or is under the administrator's control).  There are *so* many
places in INN that use static buffers and string functions that it's hard
to be sure that none of them are exploitable, but it should now be easier
to audit.

The code in innd is almost certainly all fine, due to careful use of
MaxLength and similar functions to limit the length of strings being
passed to sprintf, but I replaced the calls to snprintf where possible
anyway just because it's a lot easier to then tell at a glance that the
code is safe.  Most of that code in INN is in error handling, which badly
needs a better system for handling it; right now, there is a ton of
duplicated code and handling errors is decidedly awkward.

Yesterday evening and today, I coded some simple implementations of
strlcpy and strlcat so that we can start using those functions for string
handling in the future.  For those not familiar with them, see:

    <http://www.courtesan.com/todd/papers/strlcpy.html>

They are much easier to use in a safe manner, and native implementations
are available (at least) on Solaris and the BSDs.  They aren't available
natively on Linux in part due to philosphical objections (namely that the
functions are only really useful when using static buffers and GNU code
tries to avoid static buffers as a general design principle), but INN
already has so many static buffers that having these functions available
to handle them safely is preferrable in the medium term to saying that we
want to eliminate them all and not having time to do that for several
years.

I've added the following to HACKING, based on applying that patch:

  *  Use string handling functions that take counts for the size of the
     buffer whenever possible.  This means using snprintf in preference to
     sprintf and using strlcpy and strlcat in preference to strcpy and
     strcat.  Also, use strlcpy and strlcat instead of strncpy and strncat
     unless the behavior of the latter is specifically required, as it is
     much easier to audit uses of the former than the latter.  (strlcpy is
     like strncpy except that it always nul-terminates and doesn't fill
     the rest of the buffer with nuls, making it more efficient.  strlcat
     is like strncat except that it always nul-terminates and it takes the
     total size of the buffer as its third argument rather than just the
     amount of space left.)  All of these functions are guaranteed to be
     available; there are replacements in lib for systems that don't have
     them.

  *  Avoid uses of fixed-width buffers except in performance-critical
     code, as it's harder to be sure that such code is correct and it
     tends to be less flexible later on.  If you need a reusable,
     resizable memory buffer, one is provided in lib/buffer.c.

  *  Avoid uses of static variables whenever possible, particularly in
     libraries, because it interferes with making the code re-entrant down
     the road and makes it harder to follow what's going on.  Similarly,
     avoid using global variables whenever possible, and if they are
     required, try to wrap them into structures that could later be
     changed into arguments to the affected functions.

Please feel free to replace any additional misuses that you see in the
source tree, and feel free to start using strlcpy and strlcat.

I had a look at adding asprintf to the tree as well (sprintf except that
it allocates an appropriate amount of memory), but unfortunately to do
that well and make it cooperate with our handling of memory allocation
failures requires a lot more work than I have time to do, since it
basically requires getting very chummy with an sprintf implementation or
requires variadic macros (which we can't rely on).  At least as far as I
can tell.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.


More information about the inn-workers mailing list