Date: Thu, 30 Jul 2009 16:49:53 -0700 From: Xin LI <delphij@delphij.net> To: rea-fbsd@codelabs.ru Cc: rrl <endian.sign@gmail.com>, freebsd-security@freebsd.org, d@delphij.net Subject: Re: gzip memory corruption Message-ID: <4A7231A1.2050104@delphij.net> In-Reply-To: <LxW4OaFbQKVvB5FP5/FFtXkZd3U@%2BE41IXYRRzAjXLJbRTrYDjniL/s> References: <20090708193339.GA4836@minerva.freedsl.mg> <qbNi6WaraP%2BYYd65ZtihTj0ewks@BpFm1zkZmHABxHH1eUOcQSRoWTc> <4A553080.5060205@delphij.net> <4A553458.70005@delphij.net> <LxW4OaFbQKVvB5FP5/FFtXkZd3U@%2BE41IXYRRzAjXLJbRTrYDjniL/s>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------090909090002070501050501 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, Eygene, Eygene Ryabinkin wrote: [...] > 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. Sorry for the late response. I am busy recently. After carefully investigating the code, I have the following observations: - The usage of memcpy() here is wrong. It's definitely a bug. - The intention of it seems to be to make a pathname that fits into MAXPATHLEN, say, if we have /very/long/name which makes /very/long/name.gz longer than MAXPATHLEN, it might truncate it into, i.e. /very/long/n.gz instead. I feel really uncomfortable for the latter case, we should stop for that case instead of proceeding further. Having checked with GNU's gzip, it looks like that they arbitrarily set an upper limit of the suffix length to 30. This is unrelated to the memcpy bug but let's address it here as well. My revised patch would make the memcpy into a fatal errx, and reduce the allowed suffix length to 30 to match GNU behavior. Please let me know if this version looks better, I'll propose it to re@ and commit if they approved it. Cheers, - -- Xin LI <delphij@delphij.net> http://www.delphij.net/ FreeBSD - The Power to Serve! -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.12 (FreeBSD) iEYEARECAAYFAkpyMaAACgkQi+vbBBjt66DLngCgv+VoeLsZN1NM5qFHX5hc0lPM 5WgAnjTeMukfn8akGrDpz8ozDDG/7AdV =7ywC -----END PGP SIGNATURE----- --------------090909090002070501050501 Content-Type: text/plain; name="gzip.c-S-underflow.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="gzip.c-S-underflow.diff" Index: gzip.c =================================================================== --- gzip.c (revision 195945) +++ gzip.c (working copy) @@ -150,6 +150,8 @@ }; #define NUM_SUFFIXES (sizeof suffixes / sizeof suffixes[0]) +#define SUFFIX_MAXLEN 30 + static const char gzip_version[] = "FreeBSD gzip 20090621"; #ifndef SMALL @@ -372,6 +374,8 @@ case 'S': len = strlen(optarg); if (len != 0) { + if (len >= SUFFIX_MAXLEN) + errx(1, "incorrect suffix: '%s'", optarg); suffixes[0].zipped = optarg; suffixes[0].ziplen = len; } else { @@ -1236,8 +1240,7 @@ /* 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); + errx(1, "Path name too long"); #ifndef SMALL if (check_outfile(outfile) == 0) { --------------090909090002070501050501--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4A7231A1.2050104>