Re: [PATCH] Unzip fails without optional extended local header signature

[Jim Meyering]

Michael Gray wrote:
> Did this patch make it into gzip 1.5? I took a 5-second look and it
> didn't seem like it was there. I can dig up the thread/patch/copyright
> assignment if you need it.
>
> Michael
>
> On Wed, Feb 8, 2012 at 12:59 AM, Jim Meyering <address@hidden> wrote:
>> Michael Gray wrote:
>>> Heard back from them today. Is this all you need?
>> ...
>>>   Your assignment/disclaimer process with the FSF is currently
>>>   complete; your fully executed PDF will be sent to you in a separate
>>>   email immediately following this one.
>>
>> Great.  Thanks for letting me know.
>> I've Cc'd the list, in case Paul finds time before I do.

Thanks for checking.
That had fallen off the radar.  Sorry about that.

I've applied your patch locally (the patch dated back to 2010, which was
before the TAB-to-spaces conversion, so the important part did not apply)
and confirmed that it causes no trouble with our current (rather small)
test suite.  I've included the result below, but it's incomplete.
We'll need at least a NEWS entry.  I'll look into it later this week.

I suppose you have an input or two that trigger failure in gzip
without this patch?  If so, please post them.  The smaller the better.
We'll have to add at least one new test to exercise this new code.

Better still, take a minimal zip file and simply excise that signature
to create the error-provoking input.

The best would be to have three sample inputs, one as constructed
above and another contrived to have a CRC value that happens to
match the optional signature: use it both with and without the
signature.


>From 17d7037f2d6ff7dc3501a9ff5c809292ff050094 Mon Sep 17 00:00:00 2001
From: Michael Gray <address@hidden>
Date: Tue, 19 Jun 2012 07:54:34 +0200
Subject: [PATCH] unzip fails without optional extended local header signature

As per the Section V.C of the .ZIP File Format Specification
(http://www.pkware.com/documents/casestudies/APPNOTE.TXT), data
descriptors (called the extended local header in the gzip source)
may or may not be preceded by a signature.  Gzip always assumes
this signature is present; if it is not, it reads the CRC and length
values 4 bytes further into the file than it should, and the CRC and
length checks fail even though the file is not corrupt.

I've included a patch that works around the problem.  First, it assumes
that the signature *is not present*, as it's possible that the
signature value is also a valid CRC, and no non-corrupt file should be
rejected if it's CRC just happens to match the signature value.  If the
CRC or length check fails assuming the signature is not present, the
signature is then checked for.  If present, 4 more bytes of input are
read, and the previously read values are shifted appropriately.  The
CRC and length checks then proceed as normal.

Here is the relevant text from the .ZIP spec:
"Although not originally assigned a signature, the value
0x08074b50 has commonly been adopted as a signature value
for the data descriptor record.  Implementers should be
aware that ZIP files may be encountered with or without this
signature marking data descriptors and should account for
either case when reading ZIP files to ensure compatibility.
When writing ZIP files, it is recommended to include the
signature value marking the data descriptor record.  When
the signature is used, the fields currently defined for
the data descriptor record will immediately follow the
signature."

* unzip.c: Fix CRC and length check bug when extended local header
does not contain the optional signature.
---
 unzip.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/unzip.c b/unzip.c
index 4002096..071b4f4 100644
--- a/unzip.c
+++ b/unzip.c
@@ -45,7 +45,8 @@
 #define LOCFIL 26               /* offset of file name field length */
 #define LOCEXT 28               /* offset of extra field length */
 #define LOCHDR 30               /* size of local header, including sig */
-#define EXTHDR 16               /* size of extended local header, inc sig */
+#define EXTHDR 12               /* size of extended local header, excl sig */
+#define EXTHDRSIG 4             /* size of optional extended local header sig 
*/
 #define RAND_HEAD_LEN  12       /* length of encryption random header */


@@ -111,7 +112,8 @@ int unzip(in, out)
     int in, out;   /* input and output file descriptors */
 {
     ulg orig_crc = 0;       /* original crc */
-    ulg orig_len = 0;       /* original uncompressed length */
+    ulg orig_ulen = 0;      /* original uncompressed length */
+    ulg orig_clen = 0;      /* original compressed length */
     int n;
     uch buf[EXTHDR];        /* extended local header */
     int err = OK;
@@ -123,7 +125,7 @@ int unzip(in, out)

     if (pkzip && !ext_header) {  /* crc and length at the end otherwise */
         orig_crc = LG(inbuf + LOCCRC);
-        orig_len = LG(inbuf + LOCLEN);
+        orig_ulen = LG(inbuf + LOCLEN);
     }

     /* Decompress */
@@ -164,10 +166,10 @@ int unzip(in, out)
             buf[n] = (uch)get_byte(); /* may cause an error if EOF */
         }
         orig_crc = LG(buf);
-        orig_len = LG(buf+4);
+        orig_ulen = LG(buf+4);

     } else if (ext_header) {  /* If extended header, check it */
-        /* signature - 4bytes: 0x50 0x4b 0x07 0x08
+        /* signature - 4bytes: 0x50 0x4b 0x07 0x08 (optional)
          * CRC-32 value
          * compressed size 4-bytes
          * uncompressed size 4-bytes
@@ -175,8 +177,29 @@ int unzip(in, out)
         for (n = 0; n < EXTHDR; n++) {
             buf[n] = (uch)get_byte(); /* may cause an error if EOF */
         }
-        orig_crc = LG(buf+4);
-        orig_len = LG(buf+12);
+        orig_crc = LG(buf);
+        orig_clen = LG(buf+4);
+        orig_ulen = LG(buf+8);
+
+        /* Try out the CRC and len assuming no signature
+         * as the signature value could be a valid CRC that
+         * we shouldn't skip over!
+         */
+        if (orig_crc != updcrc(outbuf, 0) ||
+              orig_ulen != (ulg)(bytes_out & 0xffffffff)) {
+          /* Check for the optional signature */
+            if(orig_crc == 0x08074b50UL) {
+              /* Signature match, read in rest of header and shift
+               * already read values to skip the signature
+               */
+              for (n = 0; n < EXTHDRSIG; n++) {
+                buf[n] = (uch)get_byte(); /* may cause an error if EOF */
+              }
+              orig_crc = orig_clen;
+              orig_clen = orig_ulen;
+              orig_ulen = LG(buf);
+            }
+        }
     }

     /* Validate decompression */
@@ -185,7 +208,7 @@ int unzip(in, out)
                 program_name, ifname);
         err = ERROR;
     }
-    if (orig_len != (ulg)(bytes_out & 0xffffffff)) {
+    if (orig_ulen != (ulg)(bytes_out & 0xffffffff)) {
         fprintf(stderr, "\n%s: %s: invalid compressed data--length error\n",
                 program_name, ifname);
         err = ERROR;
--
1.7.11