sprintf security holes?

sprintf security holes?

[Randal L. Schwartz]

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!

Re: sprintf security holes?

[Morten Welinder]

> 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

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

[Peter Eriksen]

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

Re: sprintf security holes?

[Junio C Hamano]

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?

Re: sprintf security holes?

[Sven Verdoolaege]

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