Date: Thu, 9 Jul 2009 07:17:46 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: d@delphij.net Cc: rrl <endian.sign@gmail.com>, freebsd-security@freebsd.org Subject: Re: gzip memory corruption Message-ID: <LxW4OaFbQKVvB5FP5/FFtXkZd3U@%2BE41IXYRRzAjXLJbRTrYDjniL/s> In-Reply-To: <4A553458.70005@delphij.net> References: <20090708193339.GA4836@minerva.freedsl.mg> <qbNi6WaraP%2BYYd65ZtihTj0ewks@BpFm1zkZmHABxHH1eUOcQSRoWTc> <4A553080.5060205@delphij.net> <4A553458.70005@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
{_.-``-' {_/ #
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?LxW4OaFbQKVvB5FP5/FFtXkZd3U>