gzip memory corruption

[Xin LI]

-----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 at 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-----
-------------- next part --------------
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) {