Re: gzip memory corruption

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 --

- 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.

- --
Xin LI <delphij@xxxxxxxxxxx>
FreeBSD - The Power to Serve!
Version: GnuPG v2.0.12 (FreeBSD)

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) {
freebsd-security@xxxxxxxxxxx mailing list
To unsubscribe, send any mail to "freebsd-security-unsubscribe@xxxxxxxxxxx"

Relevant Pages

  • Re: gzip memory corruption
    ... so it will be buffer underflow in any ... But the place with the memcpy() should be patched too. ... Two reasons: ... I am really worried about the usage of memcpy with underflow; ...
  • Re: using memcpy to copy classes
    ... > I have been using memcpy to copy one class to another of the same type. ... > There are reasons why I had to do this bug am getting some odd crashes and ... > Cheers, spoc ... If o contains strings, vectors, or any non-primitive ...
  • Re: copy size
    ... it's ok to call memcpy() with a size of 0. ... But it seems odd for three reasons: ... where it may make sense to derefence a pointer parameter ... const unsigned char *p2 = s2; ...