Issue #770: Be more careful about extra_length

Hanno Böck pointed out that the loop here computes
extra_length - 4 without first checking for possible underflow.

In addition to fixing this, I also added a bunch of error
checks so Zip parsing will fail if any Zip extra field
is malformed.  Among other things, this uncovered an
old bug that would skip a trailing extra field with
zero-sized data.

Note that we still simply ignore well-formed
extra fields that we don't understand.

diff --git a/libarchive/archive_read_support_format_zip.c b/libarchive/archive_read_support_format_zip.c
index 34ab04ecc9b84296a9900586a77ea27ea578f08e..90c32bd5e0562f544128abe9e12ee752b97e9293 100644 (file)
--- a/libarchive/archive_read_support_format_zip.c
+++ b/libarchive/archive_read_support_format_zip.c
@@ -418,18 +418,30 @@ zip_time(const char *p)
  *     id1+size1+data1 + id2+size2+data2 ...
  *  triplets.  id and size are 2 bytes each.
  */
-static void
-process_extra(const char *p, size_t extra_length, struct zip_entry* zip_entry)
+static int
+process_extra(struct archive_read *a, const char *p, size_t extra_length, struct zip_entry* zip_entry)
 {
        unsigned offset = 0;
 
-       while (offset < extra_length - 4) {
+       if (extra_length == 0) {
+               return ARCHIVE_OK;
+       }
+
+       if (extra_length < 4) {
+               archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT,
+                   "Too-small extra data: Need at least 4 bytes, but only found %d bytes", (int)extra_length);
+               return ARCHIVE_FAILED;
+       }
+       while (offset <= extra_length - 4) {
                unsigned short headerid = archive_le16dec(p + offset);
                unsigned short datasize = archive_le16dec(p + offset + 2);
 
                offset += 4;
                if (offset + datasize > extra_length) {
-                       break;
+                       archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT,
+                           "Extra data overflow: Need %d bytes but only found %d bytes",
+                           (int)datasize, (int)(extra_length - offset));
+                       return ARCHIVE_FAILED;
                }
 #ifdef DEBUG
                fprintf(stderr, "Header id 0x%04x, length %d\n",
@@ -715,13 +727,13 @@ process_extra(const char *p, size_t extra_length, struct zip_entry* zip_entry)
                }
                offset += datasize;
        }
-#ifdef DEBUG
-       if (offset != extra_length)
-       {
-               fprintf(stderr,
-                   "Extra data field contents do not match reported size!\n");
+       if (offset != extra_length) {
+               archive_set_error(&a->archive, ARCHIVE_ERRNO_FILE_FORMAT,
+                   "Malformed extra data: Consumed %d bytes of %d bytes",
+                   (int)offset, (int)extra_length);
+               return ARCHIVE_FAILED;
        }
-#endif
+       return ARCHIVE_OK;
 }
 
 /*
@@ -840,7 +852,9 @@ zip_read_local_file_header(struct archive_read *a, struct archive_entry *entry,
                return (ARCHIVE_FATAL);
        }
 
-       process_extra(h, extra_length, zip_entry);
+       if (ARCHIVE_OK != process_extra(a, h, extra_length, zip_entry)) {
+               return ARCHIVE_FATAL;
+       }
        __archive_read_consume(a, extra_length);
 
        /* Work around a bug in Info-Zip: When reading from a pipe, it
@@ -2691,7 +2705,9 @@ slurp_central_directory(struct archive_read *a, struct zip *zip)
                            "Truncated ZIP file header");
                        return ARCHIVE_FATAL;
                }
-               process_extra(p + filename_length, extra_length, zip_entry);
+               if (ARCHIVE_OK != process_extra(a, p + filename_length, extra_length, zip_entry)) {
+                       return ARCHIVE_FATAL;
+               }
 
                /*
                 * Mac resource fork files are stored under the