threads / discuss / 1828

sprintf security holes?

Subject: sprintf security holes?

## tl;dr

5 messages between Sep 16, 2005 and Sep 19, 2005.

replies: 4people: 5as markdown or json

Randal L. Schwartz· Sep 16, 2005, 14:56 UTC · lore

As I was trying to (unsuccessfully) compile git for OpenBSD, I noticed a number of occurances of sprintf(), because OpenBSD thoughtfully yells at programmers for using that.

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?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Morten Welinder· Sep 16, 2005, 16:11 UTC · re: Randal L. Schwartz · lore

Re: sprintf security holes?

> 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 assume you mean make them into snprintf calls.

I don't think that really buys you anything, i.e., there would still be far too many places where character pointers are derefenced and assigned to. You would probably have to create a small (and thus auditable) generic strings library and do all string creation within that. That is certainly feasible, but evidently not Linus style.

There really is nothing wrong with sprintf that couldn't be said to be wrong with snprintf, strcpy, strncpy, *p++ = 0, etc. If you don't have the right amount of memory allocated, you lose. (Yes, that goes for the "n" versions too, although in many cases you could syntactically check that a "sizeof" was used. Then you just get to worry whether random truncation introduced other security problems.)

Morten

Peter Eriksen· Sep 16, 2005, 21:36 UTC · re: Randal L. Schwartz · lore

[Was Re: sprintf security holes?] Building on OpenBSD

On Fri, Sep 16, 2005 at 07:56:05AM -0700, Randal L. Schwartz wrote:

> 
> As I was trying to (unsuccessfully) compile git for OpenBSD, I noticed
> a number of occurances of sprintf(), because OpenBSD thoughtfully
> yells at programmers for using that.

I've got git mostly working on OpenBSD.  Here is what I changed:

diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -87,7 +87,7 @@ SCRIPT_PYTHON = \

 # The ones that do not have to link with lcrypto nor lz.
 SIMPLE_PROGRAMS = \
-       git-get-tar-commit-id git-mailinfo git-mailsplit git-stripspace
        \
+       git-get-tar-commit-id git-mailsplit git-stripspace \
        git-daemon git-var

 # ... and all the rest
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -50,7 +50,7 @@ test_expect_success \

 test_expect_success \
     'validate file modification time' \
-    'TZ=GMT tar tvf b.tar a/a |
+    'TZ=GMT gtar tvf b.tar a/a |
      awk \{print\ \$4,\ \(length\(\$5\)\<7\)\ ?\ \$5\":00\"\ :\ \$5\} \
      >b.mtime &&
      echo "2005-05-27 22:00:00" >expected.mtime &&
 

The two issues showing up are
1) missing strcasestr() which is used in mailinfo.c
2) OpenBSD tar tvf doesn't format the date as we expect.

There were talks about making a directory compat/ with system 
specific code, where an implementation of strcasestr.c could go?

With those changes all I did to compile was: 

gmake CFLAGS+='-I/usr/local/include/ -L/usr/local/lib'
gmake test
gmake install


Regards,

Peter

Junio C Hamano· Sep 19, 2005, 07:42 UTC · re: Randal L. Schwartz · lore

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?

Sven Verdoolaege· Sep 19, 2005, 12:42 UTC · re: Junio C Hamano · lore

Re: sprintf security holes?

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

Show 6 quoted lines

> 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