Re: sprintf security holes?

merlyn@stonehenge.com (Randal L. Schwartz) writes:

Since sprintf() can lead to buffer overflows from unprotected user
data, and you want to use git in server situtations, wouldn't it be
prudent to eliminate those in some near-ish timeframe?

I do not have any objection for a code audit, but what I do not
particularly like about the above statement is what to do after
you audited and know your sprintf() would not overrun any of
your allocated buffers.  I suspect OpenBSD will keep complaining
just because sprintf() is used.

Rewriting them all to use snprintf() is probably not a good
enough answer.  Later somebody will make mistakes while changing
our code, and if the modified format string (or any other change
that affects the condition under which snprintf() is called)
would now generate a string longer than allotted buffer, then
what happens?  Does OpenBSD nicely complain again, or keep mum
about the problem because it is happy that we are using
snprintf()?

We _might_ be protected from buffer overrun errors [*1*], but we
would then end up operating on a truncated data -- which would
lead to an equally unexpected behaviour.  I do not think there
is any guarantee that working with unexpectedly truncated data
leads to less severe security problems than buffer overruns with
careless sprintf().


[Footnotes]

*1* I do not think even that is guaranteed -- does OpenBSD
compare the xmalloc()'ed buffer length passed to snprintf
against the size limit?  What if we pass a statically allocated
buffer as a pointer to another function that uses snprintf?
Does it follow the flow of the data in such a way to take
notice?

On Mon, Sep 19, 2005 at 12:42:08AM -0700, Junio C Hamano wrote:

We _might_ be protected from buffer overrun errors [*1*], but we
would then end up operating on a truncated data -- which would
lead to an equally unexpected behaviour.  I do not think there
is any guarantee that working with unexpectedly truncated data
leads to less severe security problems than buffer overruns with
careless sprintf().

But you can _check_ whether the data has been truncated.
Just look at the return value of snprintf.

skimo