Home page logo
/

oss-sec logo oss-sec mailing list archives

Re: LZW decompression issues
From: Tavis Ormandy <taviso () cmpxchg8b com>
Date: Wed, 28 Sep 2011 20:42:56 +0200

Solar Designer <solar () openwall com> wrote:

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

I believe I wrote that patch, I found a lot of vulnerabilities in gzip a few
years ago, and added lots of additional sanity checks. Most distributions
decided to take an alternative (smaller) patch written by SuSE.

I don't know why everyone preferred SuSE patches, they originally claimed I
was wrong and there were no bugs, until I downloaded their rpm's and
verified with the actual binaries they were shipping that the testcases I
sent them reproduced (!?!). This was on vendor-sec, I suppose smaller was
considered superior.

FreeBSD went with my patch, which I think was much safer.

At the time gzip was effetively unmaintained Jean-Loup had dissapeared, and
it wasn't until a year or two later that a new maintainer appeared and a new
official release was made. I guess they went with SuSE patch as well.

Tavis.


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






-- 
-------------------------------------
taviso () cmpxchg8b com | pgp encrypted mail preferred
-------------------------------------------------------


  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]
AlienVault