freebsd security - Jul 2009 - gzip memory corruption

Hi all,
> uname -a
FreeBSD XXXXX 7.2-RELEASE FreeBSD 7.2-RELEASE #1: Wed Jun 24 10:19:42 EAT 2009  
XXXXXXXXX:/usr/obj/usr/src/sys/GENERIC  i386

I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name with
the -S option.
> gzip -S `perl -e 'print "A"x1200'` dummy_file
Memory fault (core dumped) 

The offending code lays in the function file_compress:
>		/* Add (usually) .gz to filename */
>		if ((size_t)snprintf(outfile, outsize, "%s%s",
>					file, suffixes[0].zipped) >= outsize)
>			memcpy(outfile - suffixes[0].ziplen - 1,
>				suffixes[0].zipped, suffixes[0].ziplen + 1);
The problem here is that outfile points to a local buffer from the
function handle_file which calls file_compress.  And given that we
give a very long suffix, memcpy does in fact write to memory location
out of outfile, overwriting the return address of file_compress.
Here's a possible fix:

--- /usr/src/usr.bin/gzip/gzip.c	2009-05-17 12:00:16.000000000 +0300
+++ gzip.c	2009-07-08 20:27:22.000000000 +0300
@@ -1219,10 +1219,15 @@ file_compress(char *file, char *outfile,
 
 		/* Add (usually) .gz to filename */
 		if ((size_t)snprintf(outfile, outsize, "%s%s",
-					file, suffixes[0].zipped) >= outsize)
+					file, suffixes[0].zipped) >= outsize && 
+			(unsigned int)suffixes[0].ziplen < outsize)
 			memcpy(outfile - suffixes[0].ziplen - 1,
 				suffixes[0].zipped, suffixes[0].ziplen + 1);
-
+		else {
+			maybe_warnx("filename too long %s%s", file, suffixes[0].zipped);
+			close(in);
+			return -1;
+		}
 #ifndef SMALL
 		if (check_outfile(outfile) == 0) {
 			close(in);

Cheers,

Wed, Jul 08, 2009 at 10:33:39PM +0300, rrl wrote:
> I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name
> with the -S option.
> > gzip -S `perl -e 'print "A"x1200'` dummy_file
> Memory fault (core dumped)
> 
> The offending code lays in the function file_compress:
> >		/* Add (usually) .gz to filename */
> >		if ((size_t)snprintf(outfile, outsize, "%s%s",
> >					file, suffixes[0].zipped) >= outsize)
> >			memcpy(outfile - suffixes[0].ziplen - 1,
> >				suffixes[0].zipped, suffixes[0].ziplen + 1);
The memcpy() call looks like a complete madness: it will write before
the beginning of the 'outfile', so it will be buffer underflow in any
case (unless I am terribly mistaken and missing some obvious point).

I'd change the above code to warn and return if snprintf will discard
some trailing characters, the patch is attached.
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gzip.c-fix-buffer-underflow.diff
Type: text/x-diff
Size: 609 bytes
Desc: not available
Url :
http://lists.freebsd.org/pipermail/freebsd-security/attachments/20090708/9f479154/gzip.c-fix-buffer-underflow.bin