This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

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;

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

  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;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]