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 . > > 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
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Powered by Openwall GNU/*/Linux - Powered by OpenVZ