Home page logo
/

oss-sec logo 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


  By Date           By Thread  

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