oss-sec mailing list archives
Re: LZW decompression issues
From: Solar Designer <solar () openwall com>
Date: Wed, 28 Sep 2011 19:42:03 +0400
Hi, On Wed, Aug 10, 2011 at 08:22:20PM +0200, Tomas Hoger wrote:
We've recently came across an issue in commonly re-used LZW decompression implementations - original BSD compress and GIF reader written by David Koblas. Due to an insufficient input checking, invalid LZW stream can create a loop in the decompression table, leading to the decompression stack buffer overflow. Following bugzillas list various code bases that were checked for the issue and if they are affected or not: https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2011-2895 https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2011-2896
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). The latest security fix in upstream gzip is from January 2010, for CVE-2010-0001: http://git.savannah.gnu.org/gitweb/?p=gzip.git;a=commitdiff;h=a3db5806d012082b9e25cc36d09f19cd736a468f (no idea if that issue was present in FreeBSD's gzip or not). 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. 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? 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. 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). 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)? ...looking at similar changes to usr.bin/compress/zopen.c in the same patch, I guess this may be how those changes were made to the gzip code as well - due to similarities between the two codebases. However, even if so this does not fully address my questions above. Colin - any comments? Thanks, Alexander
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 Solar Designer (Sep 28)