oss-sec mailing list archives
Re: LZW decompression issues
From: Tomas Hoger <thoger () redhat com>
Date: Wed, 28 Sep 2011 20:22:28 +0200
On Wed, 28 Sep 2011 19:42:03 +0400 Solar Designer wrote:
FreeBSD has just released an advisory for this: http://security.freebsd.org/advisories/FreeBSD-SA-11:04.compress.asc To my surprise, it lists gzip as affected (and provides a patch for it too), even though it was believed that neither CVE-2011-2895 nor CVE-2011-2896 affected current versions of gzip (at least per Tomas' off-list notification to distro vendors).
That info is also noted in public comment of our CVE-2011-2895 bugzilla.
Trying to match the changes to usr.bin/gzip/zuncompress.c in http://security.freebsd.org/patches/SA-11:04/compress.patch against code in gzip 1.4 tarball, it appears that FreeBSD's patch actually introduces more checks than gzip upstream has - although it is difficult to tell for sure because of other differences in the code.
Let me try to explain some.
For example gzip-1.4/unlzw.c has: if (maxbits > BITS) { FreeBSD now patches it as: - if (zs->zs_maxbits > BITS) { + if (zs->zs_maxbits > BITS || zs->zs_maxbits < 12) { Do we possibly want to add the "maxbits < 12" check as well? And does it matter for security?
I'm not aware of any security impact of that. Not sure if there's any spec that requires maxbits >= 12, if not, INIT_BITS (9) may be a safer lower bound.
Then there are non-obvious differences related to the "oldcode" variable. In one of those places, gzip-1.4/unlzw.c has: if (code >= free_ent) { /* Special case for KwKwK string. */ if (code > free_ent) { whereas the FreeBSD patch has: if (zs->u.r.zs_code >= zs->zs_free_ent) { + if (zs->u.r.zs_code > zs->zs_free_ent || + zs->u.r.zs_oldcode == -1) { + /* Bad stream. */ which adds an extra "or" condition.
This is consequence of the following change: - zs->u.r.zs_finchar = zs->u.r.zs_oldcode = getcode(zs); - if (zs->u.r.zs_oldcode == -1) /* EOF already? */ - return (0); /* Get out of here */ - - /* First code must be 8 bits = char. */ - *bp++ = (u_char)zs->u.r.zs_finchar; - count--; + zs->u.r.zs_oldcode = -1; That check ensures that the first code (in the stream or after CLEAR) is not >= 256.
Similarly, gzip-1.4/unlzw.c has: if ((code = free_ent) < maxmaxcode) { /* Generate the new entry. */ whereas the FreeBSD patch has: /* Generate the new entry. */ - if ((zs->u.r.zs_code = zs->zs_free_ent) < zs->zs_maxmaxcode) { + if ((zs->u.r.zs_code = zs->zs_free_ent) < zs->zs_maxmaxcode && + zs->u.r.zs_oldcode != -1) { (an extra "and" condition this time).
This part is due to a CLEAR code handling change. Note that gzip (and ncompress) has: free_ent = FIRST - 1; while Joerg's patch changes that to: zs->zs_free_ent = FIRST; The original version is quite confusing and probably another bug (unless it's a feature) in the original compress code. Consequence of the CLEAR re-setting free_ent to FIRST-1 is that suffix/prefix tables get modified at CLEAR position, but those positions should never get read though. However, some variants allowed accessing those tables at CLEAR when processing inputs with multiple subsequent CLEAR codes. I have created such reproducer for freetype upstream, it may work with (unpatched) BSD compress/gzip. https://savannah.nongnu.org/bugs/index.php?34139 gzip/ncompress resets free_ent to FIRST-1, so writes to CLEAR position, but never reads it AFAIK.
Are these differences only a result of other differences in the FreeBSD revision of gzip? Or are they generic hardening that could get into gzip proper and into other distros' revisions of gzip? Or are they even security fixes for issues known to FreeBSD (but presumably not to others yet, in gzip context)?
As far as I can tell, FreeBSD gzip had the issues I reported, even though my mails said gzip is not affected. If Savannah gzip git is to be trusted, gzip had those issue addressed in the oldest version available there (1.2.4, 1993): http://git.savannah.gnu.org/gitweb/?p=gzip.git;a=history;f=unlzw.c;hb=HEAD On Wed, 28 Sep 2011 19:53:29 +0400 Solar Designer wrote:
On Wed, Sep 28, 2011 at 07:42:03PM +0400, Solar Designer wrote:whereas the FreeBSD patch has: if (zs->u.r.zs_code >= zs->zs_free_ent) { + if (zs->u.r.zs_code > zs->zs_free_ent || + zs->u.r.zs_oldcode == -1) { + /* Bad stream. */Perhaps the FreeBSD "affected" statement for gzip was based on it missing the "zs->u.r.zs_code > zs->zs_free_ent" check prior to this patch. This check was already added upstream before gzip 1.4, which is why gzip was "not affected" this time for other distro vendors (the issue was patched years ago).
Without testing, I'd say it was possible to create and trigger prefix loop in that gzip versions via code > free_ent, first code >= 256, and possibly also subsequent CLEARs too. HTH -- Tomas Hoger / Red Hat Security Response Team
Current thread:
- LZW decompression issues Tomas Hoger (Aug 10)
- Re: LZW decompression issues Solar Designer (Sep 28)
- Re: LZW decompression issues Solar Designer (Sep 28)
- Re: LZW decompression issues Colin Percival (Sep 28)
- Re: LZW decompression issues Tomas Hoger (Sep 28)
- Re: LZW decompression issues Solar Designer (Sep 28)
- Re: LZW decompression issues Tavis Ormandy (Sep 28)
- Re: LZW decompression issues Solar Designer (Sep 28)
- Re: LZW decompression issues Tomas Hoger (Sep 29)
- Re: LZW decompression issues Tim Zingelman (Sep 29)
- Re: LZW decompression issues Joerg Sonnenberger (Sep 29)
- Re: LZW decompression issues Solar Designer (Sep 29)
- Re: LZW decompression issues Tavis Ormandy (Sep 29)
- Re: LZW decompression issues Solar Designer (Sep 28)