Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150210141159.22729e84@redhat.com>
Date: Tue, 10 Feb 2015 14:11:59 +0100
From: Tomas Hoger <thoger@...hat.com>
To: mancha <mancha1@...o.com>
Cc: oss-security@...ts.openwall.com, sms@...inode.info, cve-assign@...re.org
Subject: Re: CVE Request: Info-ZIP unzip 6.0

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.

-- 
Tomas Hoger / Red Hat Product Security

Powered by blists - more mailing lists

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.