Re: [Nmh-workers] OpenBSD added to the buildbot cluster

[Ingo Schwarze]

Hi,

Robert Elz wrote on Mon, Dec 16, 2013 at 09:25:59AM +0700:

> There is absolutely nothing unsafe about the strcpy() API.
> It is possible to misuse it, as it is possible to misuse just about
> anything [...]

Of course there are correct uses of strcat() - though it is harder to
use that API correctly - and of course you can misuse strlcat() -
though that will usually be easier to distinguish from correct usage.
Anyway, no argument here, the linker warning doesn't say "strcat()
is abuse in and by itself, and strlcat() is always safe".

> It is absurd to assume that an API will be misused, just because
> it sometimes is, or could be.

Well, the warning is not so much about "could" or "might" (in theory),
but about *practical* experience of code auditors.  It reads:

  warning: strcat() is almost always misused, please use strlcat()

Which is intended to say:  Almost any substantial project using it
usually contains at least some misuse, and strlcat would be much
easier to use correctly and, in particular, to audit, thus - in
practice - safer.  It is an invitation for audit, and for using
audit-friendly idioms.

Take, for example, the file nmh-1.5/sbr/mts.c, function LocalName().
Assume

  flag == 0
  *localname != '\0'
  *localdomain != '\0'

Then the code effectively goes:

  /* Off by one alarm in the next line, should be sizeof()-1.
   * Luckily, doesn't matter, strlen(localname) is < BUFSIZ.
   * Ironically, strcpy() would be correct here, strncpy()
   * is just confusing and has no additional effect. */
  strncpy (buf, localname, sizeof(buffer0));
  /* But wait, _now_ the buffer may be _nearly_ full, right? */
  strcat (buf, ".");
  strcat (buf, localdomain);  /* Ouch! */

Effectively, buf is static char[BUFSIZ].
Both localname and localdomain come from the config file,
and reading function mts_read_conf_file(), both can become
nearly BUFSIZ in length, minus strlen("localdomain") and a bit.

Now you may argue that a config file plays no part in an attack
vector (arguably; not even sure about that).  Anyway, as an Admin,
i prefer an error message over a buffer overflow when inadvertently
feeding invalid data into my program's config file.

In this case, overflow prevention is not even attempted, which is
typical (rather than the exception) for strcat use in the wild.
Try adding some code to catch the overflow, log it and abort the
program in a controlled manner.  You will see that the resulting
code will end up being hard to audit.  Use strlcat, and it will
become easier to write and to audit, hence safer in practice.

Yours,
  Ingo