Re: [PATCH][BZ #14771] Fortify tweak for snprintf et al.

[Florian Weimer <fweimer at redhat dot com>]

On 10/20/2013 06:14 PM, Rich Felker wrote:

> > Note that this is for fortify mode only.  Both ISO C and POSIX
> > require that you end up with sprintf behavior when you specify
> > (size_t)-1 as the buffer length.
>
> No, they don't. POSIX explicitly requires an error if you pass
> (size_t)-1 or any value larger than INT_MAX as the size argument.

True, make it INT_MAX instead.

It's also unspecified if anything is written to the buffer if snprintf 
fails.

> > (Neither standard requires that
> > the size argument is the length of the buffer.)  The fortify
> > implementation already aborts in that case (even if the result would
> > fit into the supplied buffer), so my proposal is less radical than
> > you might think.
>
> Then this should be fixed. It's a bug in fortify unless the point of
> fortify is to make applications crash instead of getting back an error
> return on the basis that you expect they don't check for the error
> return.

Okay, you're probably right that the patch as-is is the wrong approach. 
 I'll try to list relevant usage patterns.

(1) Return value ignored

  snprintf(buf, sizeof(buf), "format);
  process_string(buf); // assumes buf is NUL-terminated

(2) Return value as bytes written

  char *p = buf;
  int ret = snprintf(p, sizeof(buf) - (p - buf), "first part");
  if (ret < 0) goto error;
  p += ret;
  ret = snprintf(p, sizeof(buf) - (p - buf), "second part");
  if (ret < 0) goto error;
  p += ret;
  ret = snprintf(p, sizeof(buf) - (p - buf), "and so on");

(3) Return value as bytes written, unchecked

  char *p = buf;
  p += snprintf(p, sizeof(buf) - (p - buf), "first part");
  p += snprintf(p, sizeof(buf) - (p - buf), "second part");
  p += snprintf(p, sizeof(buf) - (p - buf), "and so on");

(4) Return value ignored, multiple parts

  char *p = buf;
  snprintf(p, sizeof(buf) - (p - buf), "first part");
  p += strlen(p);
  snprintf(p, sizeof(buf) - (p - buf), "second part");
  p += strlen(p);
  snprintf(p, sizeof(buf) - (p - buf), "and so on");
  p += strlen(p);

(5) Return value for buffer sizing

  int ret = snprintf(NULL, 0, "format");
  if (ret < 0) goto error;
  char *ptr = malloc(ret + 1);
  if (ret == NULL) goto error;
  int ret2 = snprintf(ptr, ret + 1, "format");
  if (ret2 < 0) goto error;

(1), (4) are unsafe on errors (because the resulting string might not be 
null-terminated).  (4) is also unsafe because the pointer arithmetic 
goes astray with a -1 return value.  (2) used to be completely correct 
(we have some older code that targets Single UNIX Specification, Version 
2), but C99 breaks it.  (2) defeats fortification (because GCC cannot 
infer the buffer length on subsequent calls) and the length checking 
(because of the wraparound).  (3) is slightly worse because -1 error 
returns cause the pointer to go totally wrong.  (5) is probably okay as 
it stands (although it is not completely race-free in multi-threaded 
programs).

Based on this analysis, I think we need to look at the following things.

We need to double-check that we always NUL-terminate the buffer (if the 
buffer length is positive), even in case of error.  We should do this 
even without fortification.  (A quick check using the attached program 
shows that we do this for some failures at least.)  This will cover (1) 
and (4).

Considering the history, we should make sure that fortification turns 
(2) and (3) into predictable crashes instead of potentially exploitable 
buffer overflows.  (2) might be covered accidentally by correct 
EOVERFLOW handling (see below), but (3) would not.  There is little we 
can do about the -1 return value in (3) due to memory allocation 
failures, but I think this example provides a case against return 
EOVERFLOW when fortify is enabled.

> > I'm open to tweaking the magic number, perhaps to ((size_t)-1)/2 -
> > 1024 or something like that.  But I do think that check makes sense,
> > as exemplified by Exim.
>
> INT_MAX is always less than ((size_t)-1)/2 - 1024, so the check would
> be completely redundant with the check against INT_MAX.

Hmm.  There is no up-front check to return EOVERFLOW in glibc, and the 
GCC folder doesn't know about it, either. :-(  EOVERFLOW is apparently 
returned only if the actually produced string is at least INT_MAX 
characters long (this is the fprintf etc. behavior).

--
Florian Weimer / Red Hat Product Security Team

#include <stdio.h>
#include <errno.h>
#include <printf.h>
#include <err.h>
#include <string.h>

static int
error_handler (FILE *stream, const struct printf_info *info,
	       const void *const *args)
{
  errno = ENOMEM;
  return -1;
}

int
arginfo_handler (const struct printf_info *info, size_t n, int *argtypes)
{
  if (n > 0)
    argtypes[0] = PA_POINTER;
  return 1;
}

int
main(void)
{
  if (register_printf_function('Y', error_handler, arginfo_handler) < 0)
    err(1, "register_printf_function");
  char buf[16];
  memset(buf, ' ', sizeof(buf));
  int ret = snprintf(buf, sizeof(buf), "prefix%Y", NULL);
  int err = errno;
  printf("return value: %d, errno: %d, length: %zu\n",
	 ret, err, strnlen(buf, sizeof(buf)));
  return 0;
}