Re: sprintf security holes?

From: Junio C Hamano <junkio@cox.net>
Date: Sep 19, 2005, 07:42 UTC
Message-ID: <7voe6pv6zz.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <86zmqd5aey.fsf@blue.stonehenge.com>

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?