Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<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 ]

Powered by blists - more mailing lists

Your e-mail address:

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

Powered by Openwall GNU/*/Linux - Powered by OpenVZ