This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH][BZ #14771] Fortify tweak for snprintf et al.
- From: Florian Weimer <fweimer at redhat dot com>
- To: Rich Felker <dalias at aerifal dot cx>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 21 Oct 2013 10:31:57 +0200
- Subject: Re: [PATCH][BZ #14771] Fortify tweak for snprintf et al.
- Authentication-results: sourceware.org; auth=none
- References: <52611BBF dot 7000500 at redhat dot com> <20131020024756 dot GH20515 at brightrain dot aerifal dot cx> <5263B1F3 dot 4040808 at redhat dot com> <20131020161449 dot GJ20515 at brightrain dot aerifal dot cx>
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;
}