Full Disclosure mailing list archives
Re: new class of printf issue: int overflow
From: Pierre Habouzit <madcoder () debian org>
Date: Thu, 11 Jan 2007 12:00:24 +0100
On Thu, Jan 11, 2007 at 02:00:53AM +0100, Felix von Leitner wrote:
This is about two issues. First: abs within vasprintf. I just read some gnupg source code and stumbled upon their vasprintf implementation. Basically they make one pass over the format string to find out how much memory to malloc, and then they call sprintf on the malloced buffer. Here is an excerpt: if (*p == '*') { ++p; total_width += abs (va_arg (ap, int)); } I noticed this code because it calls abs on an int. A little known fact about abs is that it can return negative values. If the int is 0x80000000, for example, abs() will also return 0x80000000. So, I thought, mhh, if someone can control this value but not the format string, he can cause total_width to be very small but then write lots of stuff to it.
That's indeed a bug, and that should be fixed. MIN_INT is a value that should generate an error IMHO, that's not reasonable to use printf to write 2Go of data anyway, but YMMV.
But that got me thinking. *printf return an int, and it's supposed to be the number of chars written. So a typical idiom is size_t memory_needed=snprintf(NULL,0,format_string,...); char* ptr=malloc(memory_needed+1); sprintf(ptr,format_string,...);
that's not the sole braindead idiom that generate errors. In my software I use an xmalloc that returns NULL if its argument is <= 0, and as you're supposed to check if malloc returns NULL (theorically) then the idiom is: size_t needed; char *buf; needed = snprintf(NULL, 0, fmt, ...); buf = xmalloc(needed + 1); if (!buf) { // somehow barf here about beeing OOM } sprintf(ptr, format_string, ...); The error message will be inaccurate, but there is no security flaw. [...]
The question is: do we want to do something about it? What should printf do if it detects an int overflow? Return -1? Is there a good solution to this? Solaris apparently returns -1.
like said for your aprintf case, IMHO, MIN_INT for a '*' width specifier has to be taken as an erroneous value. At least, it really feels sensible.
PS: Does anyone understand why sprintf does not count the \0 at the end? I think that's pretty brain-dead.
That's everything but braindead. It allows you to point _on_ the final NUL rather than after it. Every string function should speak of the string length that it produces, because that's what the programmer needs most of the time. In fact, I really believe snprintf is the good API: - it returns the _length_ of the string that could have been written if enough space exist in the buffer ; - it takes the buffer and the buffer _size_ (opposed to the string length it contains) as an argument. Then you can _safely_ write things like: char buf[SOME_SIZE]; int pos = 0; pos += snprintf(buf + pos, sizeof(buf) - pos, fmt, ...); pos += snprintf(buf + pos, sizeof(buf) - pos, fmt, ...); pos += snprintf(buf + pos, sizeof(buf) - pos, fmt, ...); pos += snprintf(buf + pos, sizeof(buf) - pos, fmt, ...); without needing to check for overflows (I obviously suppose that the size can be negative, I use a wrapper around snprintf that takes a ssize_t rather than a size_t for this very reason). if snprintf returned the size of the buffer rather than the length, that would have been pretty akward to write, because you'd have to write sth like: pos += snprintf(buf + pos, sizeof(buf) - pos, fmt, ...) - 1; and if you forget the -1, then you don't really concatenate strings correctly. In fact, what is good with such an API is that you can create new functions that follow the very same API using bricks that use it, like that: ssize_t my_strfunc(char *dst, ssize_t dlen, ...) { ssize_t pos = 0; pos += my_strotherfunc(dst + pos, dlen - pos, ...); pos += my_strotherfunc2(dst + pos, dlen - pos, ...); return pos; } With that you inherit from the security from the functions you call, without needing any more checks. It generates very safe code, _and_ readable code too. Once again, if you return the size, the previous code would be: ssize_t my_strfunc(char *dst, ssize_t dlen, ...) { ssize_t pos = 0; pos += my_strotherfunc(dst + pos, dlen - pos, ...) - 1; pos += my_strotherfunc2(dst + pos, dlen - pos, ...) - 1; return pos + 1; } or worse: ssize_t my_strfunc(char *dst, ssize_t dlen, ...) { ssize_t pos = 0; pos += my_strotherfunc(dst + pos - 1, dlen - pos + 1, ...); pos += my_strotherfunc2(dst + pos - 1, dlen - pos + 1, ...); return pos; } it's way too error prone IMHO. -- ·O· Pierre Habouzit ··O madcoder () debian org OOO http://www.madism.org
Attachment:
_bin
Description:
_______________________________________________ Full-Disclosure - We believe in it. Charter: http://lists.grok.org.uk/full-disclosure-charter.html Hosted and sponsored by Secunia - http://secunia.com/
Current thread:
- new class of printf issue: int overflow Felix von Leitner (Jan 10)
- Re: new class of printf issue: int overflow Pierre Habouzit (Jan 11)
- Re: new class of printf issue: int overflow Felix von Leitner (Jan 11)
- Re: new class of printf issue: int overflow Pierre Habouzit (Jan 11)
- Re: new class of printf issue: int overflow Pierre Habouzit (Jan 11)
- Re: new class of printf issue: int overflow Felix von Leitner (Jan 11)
- Re: new class of printf issue: int overflow Pierre Habouzit (Jan 11)
- Re: new class of printf issue: int overflow Mihai Dontu (Jan 11)
- Re: new class of printf issue: int overflow Thomas (Jan 11)
- Re: new class of printf issue: int overflow Felix von Leitner (Jan 11)
- Re: new class of printf issue: int overflow Thomas (Jan 11)
- Re: new class of printf issue: int overflow Felix von Leitner (Jan 11)