From 36bb164e221a3a76488f2ceaf808db14de6f8ca4 Mon Sep 17 00:00:00 2001
From: Tim Kientzle <kientzle@acm.org>
Date: Sun, 21 Aug 2016 09:25:00 -0700
Subject: [PATCH] Issue #770:  Be more careful about extra_length
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 libarchive/archive_read_support_format_zip.c | 40 +++++++++++++++++++---------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/libarchive/archive_read_support_format_zip.c b/libarchive/archive_read_support_format_zip.c
index 34ab04e..90c32bd 100644
--- 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