"Secure coding" audit checkers and Kerberos

[Greg Hudson]

On Tue, 2008-10-14 at 23:08 -0500, Nicolas Williams wrote:
> +1.  I don't see the benefit of memcpy() vs. strcpy() unless you're
> effectively building a set of strl*()-like utility functions.

I'm hearing that the external-to-MIT Kerberos developer community likes
strlcpy.  I will probably accede to this preference since it's not
really important, but I want to go around on it once more because
there's also significant anti-strlcpy sentiment in parts of the C
programming community.

Ulrich Drepper wrote, in one of his more tactful moments:

        Correct string handling means that you always know how long your
        strings are and therefore you can you memcpy (instead of
        strcpy).

To expand on his point: you cannot judge the security of string
manipulation code by the individual statements involved.  Either the
code is correctly considering the lengths of the strings involved or
it's not.  If you replace a bad strcpy() or memcpy() call with an
unchecked strlcpy() call, you will simply replace a potential buffer
overrun with a potential truncation, which is still a bug and possibly a
security bug.

Using memcpy() is not a guarantee of safety, but it does indicate that
you're considering the lengths of the strings you're copying.  The main
benefit over strcpy, though, is an artificial one: it does not get
flagged by lint and similar tools.

Part of the disconnect within the C community is that the pro-strlcpy
crowd typically envisions checked usages while the anti-strlcpy crowd
typically envisions unchecked usages.  One glibc maintainer went so far
as to call strlcat() inefficient because it has to measure the length of
the source string for a "rarely-used return value" in the overflow case.

If we were to use strlcpy and strlcat in Kerberos, I would expect to
have unchecked usages when it is easy to tell that an overflow can't
occur.  Most of our string manipulation code uses allocated buffers, so
I would expect to see code like:

  buflen = strlen(s1) + strlen(s2) + strlen(s3) + 1;
  buf = malloc(buflen);
  if (!buf) return ENOMEM;
  (void) strlcpy(buf, s1, buflen);
  (void) strlcat(buf, s2, buflen);
  (void) strlcat(buf, s3, buflen);

Adding an unreachable, untestable error path after each strl*() call in
this situation would expand the code size, could force the use of
goto-style exception handling in functions which don't otherwise need
it, and would inevitably introduce phantom resource leaks which take
time to fix.

On the other hand, some code which uses fixed-length buffers might
actually get prettier if we use strl*() length checks instead of an
explicit length check prior to the string operations.

(In case anyone is concerned, the vast majority, possibly all, of the
Kerberos uses of strcpy() and strcat() are safe for one reason or
another.  But lint and similar tools have no idea whether this is true,
so they just flag all uses of it.)