oss-security - Re: CVE Request: Info-ZIP unzip 6.0

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 11 Feb 2015 12:27:06 +0000
From: mancha <mancha1@...o.com>
To: oss-security@...ts.openwall.com
Cc: sms@...inode.info, cve-assign@...re.org, thoger@...hat.com
Subject: Re: CVE Request: Info-ZIP unzip 6.0

On Tue, Feb 10, 2015 at 02:11:59PM +0100, Tomas Hoger wrote:
> On Mon, 22 Dec 2014 18:14:58 +0000 mancha wrote:
> 
> > OOB access (both read and write) issues exist in test_compr_eb
> > (extract.c) that can result in application crash or other
> > unspecified impact.
> > 
> > This vulnerability can be triggered via crafted zip archives with
> > extra fields that advertise STORED method compression (i.e. no
> > compression) and have uncompressed field sizes smaller than the
> > corresponding compressed field sizes.
> > 
> > This issue is different from CVE-2014-8140 [1].
> 
> FWIW, those issues are not entirely different, as the oCERT-2014-011
> reproducer triggered the same issue your patch addresses - memcpy()
> buffer overflow when using STORED compression and the uncompressed
> size field value smaller than the rest of the data in the extra field
> block.  In case of the oCERT-2014-011 report, the size was special -
> 0.  Your check, however, would prevent overflow on the test case.
> 
> The check to reject uncompressed size of 0 is still needed to avoid
> bypassing check if extra field block still has enough data for the
> compression header.  That makes it possible to bypass your check and
> trigger integer underflow in memextract() when EB_CMPRHEADLEN (2 + 4)
> is subtracted from srcsize, which leads to memcpy() with size close to
> SIZE_MAX.
> 
> Your patch, unzip-6.0_overflow2.diff, which is what got applied
> upstream, seems to perform an incorrect check.  It ensures that
> eb_ucsize is equal to eb_size - compr_offset.  The latter value
> includes compression header length (EB_CMPRHEADLEN), which is not
> included in eb_ucsize AFAICT (based on what I could find in
> extrafld.txt or os2/os2zip.c in Zip 3.0 sources).  It seems the check
> should be:
> 
>   (eb_size - compr_offset - EB_CMPRHEADLEN != eb_ucsize)
> 
> Can you or upstream confirm?
> 
> This problem would not be a security problem, but a bug that could
> cause well-formed extra fields to be rejected as invalid.

You're right. The identity that should hold in stored method is:

  eb_size - compr_offset - EB_CMPRHEADLEN == eb_ucsize

What must have thrown me for a loop when churning out the patch is the
weird wording in unzpriv.h:

  #define EB_OS2_HLEN    4  /* size of OS2/ACL compressed data header */

Thanks for catching that.

I've removed the buggy patch from sf and replaced it with:

http://sf.net/projects/mancha/files/sec/unzip-6.0_overflow3.diff

--mancha

Content of type "application/pgp-signature" skipped

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.