gzip memory corruption

[rrl]

Hi all,

> uname -a
FreeBSD XXXXX 7.2-RELEASE FreeBSD 7.2-RELEASE #1: Wed Jun 24 10:19:42 EAT 2009     XXXXXXXXX:/usr/obj/usr/src/sys/GENERIC  i386

I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name with the -S option.
> gzip -S `perl -e 'print "A"x1200'` dummy_file
Memory fault (core dumped)

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 problem here is that outfile points to a local buffer from the
function handle_file which calls file_compress.  And given that we
give a very long suffix, memcpy does in fact write to memory location
out of outfile, overwriting the return address of file_compress.
Here's a possible fix:

--- /usr/src/usr.bin/gzip/gzip.c 2009-05-17 12:00:16.000000000 +0300
+++ gzip.c 2009-07-08 20:27:22.000000000 +0300
@@ -1219,10 +1219,15 @@ file_compress(char *file, char *outfile,
 
  /* Add (usually) .gz to filename */
  if ((size_t)snprintf(outfile, outsize, "%s%s",
- file, suffixes[0].zipped) >= outsize)
+ file, suffixes[0].zipped) >= outsize &&
+ (unsigned int)suffixes[0].ziplen < outsize)
  memcpy(outfile - suffixes[0].ziplen - 1,
  suffixes[0].zipped, suffixes[0].ziplen + 1);
-
+ else {
+ maybe_warnx("filename too long %s%s", file, suffixes[0].zipped);
+ close(in);
+ return -1;
+ }
 #ifndef SMALL
  if (check_outfile(outfile) == 0) {
  close(in);

Cheers,
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Eygene Ryabinkin-3]

Wed, Jul 08, 2009 at 10:33:39PM +0300, rrl wrote:

> I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name
> with the -S option.
> > gzip -S `perl -e 'print "A"x1200'` dummy_file
> Memory fault (core dumped)
>
> 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.
--
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #

---

--- usr.bin/gzip/gzip.c.orig 2009-07-09 00:03:03.000000000 +0400
+++ usr.bin/gzip/gzip.c 2009-07-09 00:21:40.000000000 +0400
@@ -1235,9 +1235,12 @@
 
  /* 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);
+ file, suffixes[0].zipped) >= outsize) {
+ warnx("Output file name '%s%s' is too long, exiting",
+      file, suffixes[0].zipped);
+ close(in);
+ return -1;
+ }
 
 #ifndef SMALL
  if (check_outfile(outfile) == 0) {

---

_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Xin LI-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eygene Ryabinkin wrote:

> Wed, Jul 08, 2009 at 10:33:39PM +0300, rrl wrote:
>> I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name
>> with the -S option.
>>> gzip -S `perl -e 'print "A"x1200'` dummy_file
>> Memory fault (core dumped)
>>
>> 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.

Nice catch!  I'll take a look at this as soon as possible.

Cheers,
- --
Xin LI <delphij@...> http://www.delphij.net/
FreeBSD - The Power to Serve!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (FreeBSD)

iEYEARECAAYFAkpVMIAACgkQi+vbBBjt66BkrgCePlsfN2Y8+yXkJiI39A2tEmRS
CKcAnipqLptYZx2BeuM+7piL0vBF1yzz
=9kvD
-----END PGP SIGNATURE-----
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Xin LI-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Xin LI wrote:

> Eygene Ryabinkin wrote:
>> Wed, Jul 08, 2009 at 10:33:39PM +0300, rrl wrote:
>>> I run Freebsd 7.2 and gzip doesn't handle correctly long suffix name
>>> with the -S option.
>>>> gzip -S `perl -e 'print "A"x1200'` dummy_file
>>> Memory fault (core dumped)
>>>
>>> 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.

If there is no objections I would request for approval from re@.

Cheers,
- --
Xin LI <delphij@...> http://www.delphij.net/
FreeBSD - The Power to Serve!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (FreeBSD)

iEUEARECAAYFAkpVNFcACgkQi+vbBBjt66AkuQCfSm79QmZC2jPwE8kSEaz5NvH7
V+8Al0zsIfe40Tv0Yu/LrtMpnEK5cok=
=OtC/
-----END PGP SIGNATURE-----

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 {

_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Eygene Ryabinkin-3]

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
    {_.-``-'         {_/            #
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Xin LI-2]

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

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@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Eygene Ryabinkin-3]

Xin, good day.

Thu, Jul 30, 2009 at 04:49:53PM -0700, Xin LI wrote:
> 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.

Yes, this patch looks much better, thanks!  One thing: I would expand
the error message here:
> + if (len >= SUFFIX_MAXLEN)
> + errx(1, "incorrect suffix: '%s'", optarg);

say to

> + errx(1, "incorrect suffix: '%s': too long", optarg);

I will be better, since the reason of incorrectness will be stated:
it is not very obvious why the suffix like
  '.barrhmumbojombofromthemightyuserwhoseemtogonecompletelymad'
isn't acceptable ;))
--
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Xin LI-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

After talking with Matthew Green (the author of NetBSD) it seems that it
would be more reasonable to fix the bug itself than breaking upon
receipt.  Here is the patch.

Regarding to the suffix prompt, give me some time to think about it.  At
the beginning I just matched GNU gzip's behavior, but they cover when
the -S is specified when decompressing, which we don't care about, so it
might be reasonable for us to explicitly say it's too long.

Cheers,
- --
Xin LI <delphij@...> http://www.delphij.net/
FreeBSD - The Power to Serve!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (FreeBSD)

iEYEARECAAYFAkpyhGoACgkQi+vbBBjt66Bk3wCfT0w2DQipG05hksUv9r/CPioo
s4IAni8otQHmNOxticY23JqzevzsDeBL
=JzTo
-----END PGP SIGNATURE-----

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,7 +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,
+ memcpy(outfile + outsize - suffixes[0].ziplen - 1,
  suffixes[0].zipped, suffixes[0].ziplen + 1);
 
 #ifndef SMALL

_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Eygene Ryabinkin-3]

Xin,

Thu, Jul 30, 2009 at 10:43:07PM -0700, Xin LI wrote:
> After talking with Matthew Green (the author of NetBSD) it seems that it
> would be more reasonable to fix the bug itself than breaking upon
> receipt.  Here is the patch.

You'll probably want to check that (outsize - suffixes[0].ziplen - 1)
is greater than zero.  Like this:
-----
  if ((size_t)snprintf(outfile, outsize, "%s%s",
  file, suffixes[0].zipped) >= outsize) {
  size_t sfx_start = outsize - suffixes[0] - 1;
  if (sfx_start > 0) {
                                memcpy(outfile + sfx_start,
                                  suffixes[0].zipped, suffixes[0].ziplen + 1);
                        } else {
                                errx(1, "Can't insert suffix: name buffer is too short");
                        }
                }
-----
Just now we can garantee that 'outsize' will fit any suffix because of
the suffix length check, but when Someone (TM) will modify the code,
this could no longer be true and a bug will arise again.  So it is
better to check this locally and fail loudly if we can't make it happen.

I should say that transforming the "/long-path/foo.gz" (that is
expected) into "/long-path/f.gz" isn't quite obvious for the end-user.
But if the absence of such a transformation will break anything that
relies on this behaviour (I can't think about any usages of this
behaviour, but who knows), then the code should keep it.  What were
Mattew's arguments for keeping the old behaviour?
--
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."

[Xin LI-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Eygene Ryabinkin wrote:
> Xin,
>
> Thu, Jul 30, 2009 at 10:43:07PM -0700, Xin LI wrote:
>> After talking with Matthew Green (the author of NetBSD) it seems that it
>> would be more reasonable to fix the bug itself than breaking upon
>> receipt.  Here is the patch.
>
> You'll probably want to check that (outsize - suffixes[0].ziplen - 1)
> is greater than zero.  Like this:
[...]
> Just now we can garantee that 'outsize' will fit any suffix because of
> the suffix length check, but when Someone (TM) will modify the code,
> this could no longer be true and a bug will arise again.  So it is
> better to check this locally and fail loudly if we can't make it happen.

We should probably add an assertion here (e.g. assert outsize >
suffixes[0].ziplen]), but no, I don't think it's the right thing to
re-check already sanitized input, it is not a good practice for
production code to check the same thing everywhere, it's something that
should happen during development and testing phase, these should be
assertions IMHO.

> I should say that transforming the "/long-path/foo.gz" (that is
> expected) into "/long-path/f.gz" isn't quite obvious for the end-user.
> But if the absence of such a transformation will break anything that
> relies on this behaviour (I can't think about any usages of this
> behaviour, but who knows), then the code should keep it.  What were
> Mattew's arguments for keeping the old behaviour?

Because GNU gzip do the truncation instead of reporting an error (I
think the original intention for the memcpy was to match this behavior
as well).

There are even worse cases for the problem you have raised, for instance
truncating /long/p/a/t/h.gz to /long/p/a/.gz .  But for now I think the
underflow issue is more serious than that, and it would be probably a
better idea if we address the stack underflow now, and have a clear mind
to re-think about how we should do it.  There is undergoing plan to
replace gzip with something more efficient as well, on the other hand.

Cheers,
- --
Xin LI <delphij@...> http://www.delphij.net/
FreeBSD - The Power to Serve!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.12 (FreeBSD)

iEYEARECAAYFAkpypNIACgkQi+vbBBjt66BVlQCdHJC1upV+z29Ex4pb86uDBoPc
PwsAni2t0pwuptNuP1uKKyX5LhjSSOKl
=Rf8c
-----END PGP SIGNATURE-----
_______________________________________________
freebsd-security@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "freebsd-security-unsubscribe@..."