gzip memory corruption

[Eygene Ryabinkin]

Xin, good day.

Wed, Jul 08, 2009 at 05:05:44PM -0700, Xin LI wrote:
> >>> 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.
> 
> I have attached another possible fix, which catches the problem when
> parsing the command line.  The point is that, I think we really want to
> catch bad input as early as possible.

Yes, it is good to catch it here too.

> Index: gzip.c
> ===================================================================
> --- gzip.c	(?????? 195435)
> +++ gzip.c	(????????????)
> @@ -372,6 +372,8 @@
>  		case 'S':
>  			len = strlen(optarg);
>  			if (len != 0) {
> +				if (len >= PATH_MAX)
> +					errx(1, "incorrect suffix: '%s'", optarg);
>  				suffixes[0].zipped = optarg;
>  				suffixes[0].ziplen = len;
>  			} else {

But the place with the memcpy() should be patched too.  Two reasons:

 - suffix could not (yet) overflow PATH_MAX, but filename + suffix --
   can;

 - I am really worried about the usage of memcpy with underflow;
   I had tried to study the reasons for it via NetBSD CVS, but
   it just appeared one day and the reason for going to
   'outfile - suffixes[0].ziplen - 1' (with .gz its outfile - 4)
   are unknown; I am still taking this as the programming error.

So, unless you know why we're underflowing the passed pointer,
the memcpy block should be patched too, for future safety and
code correctness.
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #