security goals re strcpy/strcat/sprintf (Re: "Secure coding" audit checkers and Kerberos)

[Nicolas Williams]

On Wed, Oct 15, 2008 at 08:06:32PM -0400, Tom Yu wrote:
> I acknowledge that the "unsafe" string functions can be used safely
> and that "safe" string functions can be used unsafely.  Regardless, I
> think there is a meaningful difference between these "safe" and
> "unsafe" functions.  It is easier to determine whether a particular
> use of "safe" function is actually safe than it is to determine
> whether a particular use of an "unsafe" function is safe, particularly
> if the pieces of compensating length checking code, etc. are
> replicated everywhere the "unsafe" function is used.  The
> vulnerabilities resulting from unsafe uses of the "safe" functions,
> which could be truncation-related problems, are less severe than those
> vulnerabilities resulting from unsafe uses of the "unsafe" functions,
> which are more likely to be buffer overflow type problems.

I very much agree with the above.

> strlcpy/strlcat are not standard, and I recall reading allegations
> that their behavior is not very consistent across implementations.

They are quite common now though.  I'll look at the alleged difference
between Solaris' and BSD' strlcat().

> memcpy is standard, but using it with null-terminated strings requires
> additional calls to strlen and such, which can further complicate
> automatic or manual inspection.

I agree.  It's not worth going there (unless avoiding locale issues is
important; see below).

> Known issues with the printf family on Solaris include some
> "interesting" interpretations of the precision field for %s
> specifiers, such as counting "column width" rather than bytes.  This
> can make dealing with gss_buffer_t and other such explicit-length
> string-like data structures problematic, depending on the current
> locale.  Nico or other Sun folks, any thoughts on this?

You use a precision field for %s?  Where?  In any case, the manpage for
Solaris' snprintf() says:

"
              If the  conversion  specifier  is  s,  a  standard-
              conforming  application  (see  standards(5)) inter-
              prets the field width  as  the  minimum  number  of
              bytes  to  be  printed;  an application that is not
              standard-conforming interprets the field  width  as
              the  minimum  number  of columns of screen display.
              For an application that is not standard-conforming,
              %10s  means  if  the  converted  value has a screen
              width of 7 columns, 3 spaces would be padded on the
              right.
"

and

"
              If the conversion specifier is s or S, a  standard-
              conforming  application  (see  standards(5)) inter-
              prets the precision as the maximum number of  bytes
              to   be   written;   an  application  that  is  not
              standard-conforming interprets the precision as the
              maximum number of columns of screen display. For an
              application that is not  standard-conforming,  %.5s
              would  print  only  the  portion of the string that
              would display in 5 screen  columns.  Only  complete
              characters are written.
"

The key here is "standard-conforming application."  Figuring out exactly
what that means requires looking at standards(5), and maybe some source.

Looking at standards(5) I see nothing that clarifies this.

I'll file a manpage bug -- the specific standard that applies really
should be documented in the *printf() manpage.

So looking at source I see:

                                /*
                                 * sec_display only needed if width
                                 * is specified (ie, "%<width>s")
                                 * Solaris behavior counts <width> in
                                 * screen column width.  (If XPG4
                                 * behavior,
                                 * <width> is counted in bytes.)
                                 */

and:

                                } else if (__xpg4 == 0 && MB_CUR_MAX > 1) {
					...
                                } else {
                                        /*
                                         * XPG4 behavior - count
                                         * precision as bytes.
                                         * We don't use strlen() because
                                         * the given char string may not
                                         * be null-terminated.
                                         */

and:

 * __xpg4 == 0 by default. The xpg4 cc driver will add an object
 * file that contains int __xpg4 = 1". The symbol interposition
 * provided by the linker will allow libc to find that symbol
 * instead.

In other words, compile and link using the XPG4 or XPG6 options and
you'll get the standard byte-counting, rather than column-counting
behavior.

I'm not sure what this means for _libraries_, however.  I'm not sure
whether libraries can have their own __xpg4 interposer that is local to
their link map group.  I'll inquire and let you know.  Having this
behavior selected by the application could certainly cause problems.

> Also, I suggest that we refine the description of our supported
> platforms to include some qualifiers like "includes an implementation
> of snprintf that is conforming or that is non-conforming in one of a
> small number of well-characterized ways".

+1

Nico
--