From 14f0e395f90946b2ae54205c42b892d52b6c940d Mon Sep 17 00:00:00 2001
From: Daniel Hillier <daniel.hillier@gmail.com>
Date: Tue, 9 Jul 2019 14:17:10 +1000
Subject: [PATCH 1/3] bpo-36993: Improve error reporting for zipfiles with bad
 zip64 extra data.

---
 Lib/test/test_zipfile.py | 183 +++++++++++++++++++++++++++++++++++++++
 Lib/zipfile.py           |  12 +++
 2 files changed, 195 insertions(+)

diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py
index 0c8ffcdbf14afe..d6a160ea7a2cd2 100644
--- a/Lib/test/test_zipfile.py
+++ b/Lib/test/test_zipfile.py
@@ -773,6 +773,189 @@ def test_too_many_files_append(self):
             self.assertEqual(content, "%d" % (i**3 % 57))
         zipf2.close()
 
+    def make_zip64_file(
+        self, file_size_64_set=False, file_size_extra=False,
+        compress_size_64_set=False, compress_size_extra=False,
+        header_offset_64_set=False, header_offset_extra=False,
+    ):
+        """Generate bytes sequence for a zip with (incomplete) zip64 data."""
+        actual_size = 8
+        actual_header_offset = 0
+        local_zip64_fields = []
+        central_zip64_fields = []
+
+        file_size = actual_size
+        if file_size_64_set:
+            file_size = 0xffffffff
+            if file_size_extra:
+                local_zip64_fields.append(actual_size)
+                central_zip64_fields.append(actual_size)
+        file_size = struct.pack("<L", file_size)
+
+        compress_size = actual_size
+        if compress_size_64_set:
+            compress_size = 0xffffffff
+            if compress_size_extra:
+                local_zip64_fields.append(actual_size)
+                central_zip64_fields.append(actual_size)
+        compress_size = struct.pack("<L", compress_size)
+
+        header_offset = actual_header_offset
+        if header_offset_64_set:
+            header_offset = 0xffffffff
+            if header_offset_extra:
+                central_zip64_fields.append(actual_header_offset)
+        header_offset = struct.pack("<L", header_offset)
+
+        local_extra = struct.pack(
+            '<HH' + 'Q'*len(local_zip64_fields),
+            0x0001,
+            8*len(local_zip64_fields),
+            *local_zip64_fields
+        )
+
+        central_extra = struct.pack(
+            '<HH' + 'Q'*len(central_zip64_fields),
+            0x0001,
+            8*len(central_zip64_fields),
+            *central_zip64_fields
+        )
+
+        central_dir_size = struct.pack('<Q', 58 + 8 * len(central_zip64_fields))
+        offset_to_central_dir = struct.pack('<Q', 50 + 8 * len(central_zip64_fields))
+
+        local_extra_length = struct.pack("<H", 4 + 8 * len(local_zip64_fields))
+        central_extra_length = struct.pack("<H", 4 + 8 * len(central_zip64_fields))
+
+        zip64_contents = (
+            # Local file header
+            b"PK\x03\x04\x14\x00\x00\x00\x00\x00\x00\x00!\x00\x9e%\xf5\xaf"
+            + compress_size
+            + file_size
+            + b"\x08\x00"
+            + local_extra_length
+            + b"test.txt"
+            + local_extra
+            + b"test1234"
+            # Central directory:
+            + b"PK\x01\x02-\x03-\x00\x00\x00\x00\x00\x00\x00!\x00\x9e%\xf5\xaf"
+            + compress_size
+            + file_size
+            + b"\x08\x00"
+            + central_extra_length
+            + b"\x00\x00\x00\x00\x00\x00\x00\x00\x80\x01"
+            + header_offset
+            + b"test.txt"
+            + central_extra
+            # Zip64 end of central directory
+            + b"PK\x06\x06,\x00\x00\x00\x00\x00\x00\x00-\x00-"
+            + b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00"
+            + b"\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00"
+            + central_dir_size
+            + offset_to_central_dir
+            # Zip64 end of central directory locator
+            + b"PK\x06\x07\x00\x00\x00\x00l\x00\x00\x00\x00\x00\x00\x00\x01"
+            + b"\x00\x00\x00"
+            # end of central directory
+            + b"PK\x05\x06\x00\x00\x00\x00\x01\x00\x01\x00:\x00\x00\x002\x00"
+            + b"\x00\x00\x00\x00"
+        )
+        return zip64_contents
+
+    def test_bad_zip64_extra(self):
+        """Missing zip64 extra records raises an exception.
+
+        There are 4 fields that the zip64 format handles (the disk number is
+        not used in this module and so is ignored here). According to the zip
+        spec:
+              The order of the fields in the zip64 extended
+              information record is fixed, but the fields MUST
+              only appear if the corresponding Local or Central
+              directory record field is set to 0xFFFF or 0xFFFFFFFF.
+
+        If the zip64 extra content doesn't contain enough entries for the
+        number of fields marked with 0xFFFF or 0xFFFFFFFF, we raise an error.
+        This test mismatches the length of the zip64 extra field and the number
+        of fields set to indicate the presence of zip64 data.
+        """
+        # zip64 file size present, no fields in extra, expecting one, equals
+        # missing file size.
+        missing_file_size_extra = self.make_zip64_file(
+            file_size_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_file_size_extra))
+        self.assertIn('file size', str(e.exception).lower())
+
+        # zip64 file size present, zip64 compress size present, one field in
+        # extra, expecting two, equals missing compress size.
+        missing_compress_size_extra = self.make_zip64_file(
+            file_size_64_set=True,
+            file_size_extra=True,
+            compress_size_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_compress_size_extra))
+        self.assertIn('compress size', str(e.exception).lower())
+
+        # zip64 compress size present, no fields in extra, expecting one,
+        # equals missing compress size.
+        missing_compress_size_extra = self.make_zip64_file(
+            compress_size_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_compress_size_extra))
+        self.assertIn('compress size', str(e.exception).lower())
+
+        # zip64 file size present, zip64 compress size present, zip64 header
+        # offset present, two fields in extra, expecting three, equals missing
+        # header offset
+        missing_header_offset_extra = self.make_zip64_file(
+            file_size_64_set=True,
+            file_size_extra=True,
+            compress_size_64_set=True,
+            compress_size_extra=True,
+            header_offset_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_header_offset_extra))
+        self.assertIn('header offset', str(e.exception).lower())
+
+        # zip64 compress size present, zip64 header offset present, one field
+        # in extra, expecting two, equals missing header offset
+        missing_header_offset_extra = self.make_zip64_file(
+            file_size_64_set=False,
+            compress_size_64_set=True,
+            compress_size_extra=True,
+            header_offset_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_header_offset_extra))
+        self.assertIn('header offset', str(e.exception).lower())
+
+        # zip64 file size present, zip64 header offset present, one field in
+        # extra, expecting two, equals missing header offset
+        missing_header_offset_extra = self.make_zip64_file(
+            file_size_64_set=True,
+            file_size_extra=True,
+            compress_size_64_set=False,
+            header_offset_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_header_offset_extra))
+        self.assertIn('header offset', str(e.exception).lower())
+
+        # zip64 header offset present, no fields in extra, expecting one,
+        # equals missing header offset
+        missing_header_offset_extra = self.make_zip64_file(
+            file_size_64_set=False,
+            compress_size_64_set=False,
+            header_offset_64_set=True,
+        )
+        with self.assertRaises(zipfile.BadZipFile) as e:
+            zipfile.ZipFile(io.BytesIO(missing_header_offset_extra))
+        self.assertIn('header offset', str(e.exception).lower())
+
     def tearDown(self):
         zipfile.ZIP64_LIMIT = self._limit
         zipfile.ZIP_FILECOUNT_LIMIT = self._filecount_limit
diff --git a/Lib/zipfile.py b/Lib/zipfile.py
index 3c1f1235034a9e..6708500837806e 100644
--- a/Lib/zipfile.py
+++ b/Lib/zipfile.py
@@ -479,14 +479,26 @@ def _decodeExtra(self):
 
                 # ZIP64 extension (large files and/or large archives)
                 if self.file_size in (0xffffffffffffffff, 0xffffffff):
+                    if len(counts) < idx + 1:
+                        raise BadZipFile(
+                            "Corrupt zip64 extra field. File size not found."
+                        )
                     self.file_size = counts[idx]
                     idx += 1
 
                 if self.compress_size == 0xFFFFFFFF:
+                    if len(counts) < idx + 1:
+                        raise BadZipFile(
+                            "Corrupt zip64 extra field. Compress size not found."
+                        )
                     self.compress_size = counts[idx]
                     idx += 1
 
                 if self.header_offset == 0xffffffff:
+                    if len(counts) < idx + 1:
+                        raise BadZipFile(
+                            "Corrupt zip64 extra field. Header offset not found."
+                        )
                     old = self.header_offset
                     self.header_offset = counts[idx]
                     idx+=1

From 33e2d37544996c2cb46a51ea61a80cecdf439a12 Mon Sep 17 00:00:00 2001
From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com>
Date: Tue, 9 Jul 2019 05:44:41 +0000
Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?=
 =?UTF-8?q?rb=5Fit.?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 .../NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst

diff --git a/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst b/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst
new file mode 100644
index 00000000000000..f21f0c52eaa53b
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst
@@ -0,0 +1 @@
+Improve error reporting for corrupt zip files with bad zip64 extra data.
\ No newline at end of file

From 4b4cd5727ea7532aabfbc086b130486866f2bcb8 Mon Sep 17 00:00:00 2001
From: Daniel Hillier <daniel.hillier@gmail.com>
Date: Tue, 29 Oct 2019 10:52:45 +1100
Subject: [PATCH 3/3] bpo-36993: add zip64 error reporting review suggestions

---
 Lib/test/test_zipfile.py                      | 143 +++++++++++-------
 Lib/zipfile.py                                |   6 +-
 .../2019-07-09-05-44-39.bpo-36993.4javqu.rst  |   3 +-
 3 files changed, 96 insertions(+), 56 deletions(-)

diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py
index d6a160ea7a2cd2..b59719ed27426b 100644
--- a/Lib/test/test_zipfile.py
+++ b/Lib/test/test_zipfile.py
@@ -1,6 +1,7 @@
 import contextlib
 import importlib.util
 import io
+import itertools
 import os
 import pathlib
 import posixpath
@@ -773,12 +774,67 @@ def test_too_many_files_append(self):
             self.assertEqual(content, "%d" % (i**3 % 57))
         zipf2.close()
 
+    def tearDown(self):
+        zipfile.ZIP64_LIMIT = self._limit
+        zipfile.ZIP_FILECOUNT_LIMIT = self._filecount_limit
+        unlink(TESTFN)
+        unlink(TESTFN2)
+
+
+class StoredTestZip64InSmallFiles(AbstractTestZip64InSmallFiles,
+                                  unittest.TestCase):
+    compression = zipfile.ZIP_STORED
+
+    def large_file_exception_test(self, f, compression):
+        with zipfile.ZipFile(f, "w", compression, allowZip64=False) as zipfp:
+            self.assertRaises(zipfile.LargeZipFile,
+                              zipfp.write, TESTFN, "another.name")
+
+    def large_file_exception_test2(self, f, compression):
+        with zipfile.ZipFile(f, "w", compression, allowZip64=False) as zipfp:
+            self.assertRaises(zipfile.LargeZipFile,
+                              zipfp.writestr, "another.name", self.data)
+
+    def test_large_file_exception(self):
+        for f in get_files(self):
+            self.large_file_exception_test(f, zipfile.ZIP_STORED)
+            self.large_file_exception_test2(f, zipfile.ZIP_STORED)
+
+    def test_absolute_arcnames(self):
+        with zipfile.ZipFile(TESTFN2, "w", zipfile.ZIP_STORED,
+                             allowZip64=True) as zipfp:
+            zipfp.write(TESTFN, "/absolute")
+
+        with zipfile.ZipFile(TESTFN2, "r", zipfile.ZIP_STORED) as zipfp:
+            self.assertEqual(zipfp.namelist(), ["absolute"])
+
+    def test_append(self):
+        # Test that appending to the Zip64 archive doesn't change
+        # extra fields of existing entries.
+        with zipfile.ZipFile(TESTFN2, "w", allowZip64=True) as zipfp:
+            zipfp.writestr("strfile", self.data)
+        with zipfile.ZipFile(TESTFN2, "r", allowZip64=True) as zipfp:
+            zinfo = zipfp.getinfo("strfile")
+            extra = zinfo.extra
+        with zipfile.ZipFile(TESTFN2, "a", allowZip64=True) as zipfp:
+            zipfp.writestr("strfile2", self.data)
+        with zipfile.ZipFile(TESTFN2, "r", allowZip64=True) as zipfp:
+            zinfo = zipfp.getinfo("strfile")
+            self.assertEqual(zinfo.extra, extra)
+
     def make_zip64_file(
         self, file_size_64_set=False, file_size_extra=False,
         compress_size_64_set=False, compress_size_extra=False,
         header_offset_64_set=False, header_offset_extra=False,
     ):
-        """Generate bytes sequence for a zip with (incomplete) zip64 data."""
+        """Generate bytes sequence for a zip with (incomplete) zip64 data.
+
+        The actual values (not the zip 64 0xffffffff values) stored in the file
+        are:
+        file_size: 8
+        compress_size: 8
+        header_offset: 0
+        """
         actual_size = 8
         actual_header_offset = 0
         local_zip64_fields = []
@@ -822,30 +878,33 @@ def make_zip64_file(
         )
 
         central_dir_size = struct.pack('<Q', 58 + 8 * len(central_zip64_fields))
-        offset_to_central_dir = struct.pack('<Q', 50 + 8 * len(central_zip64_fields))
+        offset_to_central_dir = struct.pack('<Q', 50 + 8 * len(local_zip64_fields))
 
         local_extra_length = struct.pack("<H", 4 + 8 * len(local_zip64_fields))
         central_extra_length = struct.pack("<H", 4 + 8 * len(central_zip64_fields))
 
+        filename = b"test.txt"
+        content = b"test1234"
+        filename_length = struct.pack("<H", len(filename))
         zip64_contents = (
             # Local file header
             b"PK\x03\x04\x14\x00\x00\x00\x00\x00\x00\x00!\x00\x9e%\xf5\xaf"
             + compress_size
             + file_size
-            + b"\x08\x00"
+            + filename_length
             + local_extra_length
-            + b"test.txt"
+            + filename
             + local_extra
-            + b"test1234"
+            + content
             # Central directory:
             + b"PK\x01\x02-\x03-\x00\x00\x00\x00\x00\x00\x00!\x00\x9e%\xf5\xaf"
             + compress_size
             + file_size
-            + b"\x08\x00"
+            + filename_length
             + central_extra_length
             + b"\x00\x00\x00\x00\x00\x00\x00\x00\x80\x01"
             + header_offset
-            + b"test.txt"
+            + filename
             + central_extra
             # Zip64 end of central directory
             + b"PK\x06\x06,\x00\x00\x00\x00\x00\x00\x00-\x00-"
@@ -956,53 +1015,33 @@ def test_bad_zip64_extra(self):
             zipfile.ZipFile(io.BytesIO(missing_header_offset_extra))
         self.assertIn('header offset', str(e.exception).lower())
 
-    def tearDown(self):
-        zipfile.ZIP64_LIMIT = self._limit
-        zipfile.ZIP_FILECOUNT_LIMIT = self._filecount_limit
-        unlink(TESTFN)
-        unlink(TESTFN2)
-
-
-class StoredTestZip64InSmallFiles(AbstractTestZip64InSmallFiles,
-                                  unittest.TestCase):
-    compression = zipfile.ZIP_STORED
-
-    def large_file_exception_test(self, f, compression):
-        with zipfile.ZipFile(f, "w", compression, allowZip64=False) as zipfp:
-            self.assertRaises(zipfile.LargeZipFile,
-                              zipfp.write, TESTFN, "another.name")
-
-    def large_file_exception_test2(self, f, compression):
-        with zipfile.ZipFile(f, "w", compression, allowZip64=False) as zipfp:
-            self.assertRaises(zipfile.LargeZipFile,
-                              zipfp.writestr, "another.name", self.data)
-
-    def test_large_file_exception(self):
-        for f in get_files(self):
-            self.large_file_exception_test(f, zipfile.ZIP_STORED)
-            self.large_file_exception_test2(f, zipfile.ZIP_STORED)
-
-    def test_absolute_arcnames(self):
-        with zipfile.ZipFile(TESTFN2, "w", zipfile.ZIP_STORED,
-                             allowZip64=True) as zipfp:
-            zipfp.write(TESTFN, "/absolute")
+    def test_generated_valid_zip64_extra(self):
+        # These values are what is set in the make_zip64_file method.
+        expected_file_size = 8
+        expected_compress_size = 8
+        expected_header_offset = 0
+        expected_content = b"test1234"
+
+        # Loop through the various valid combinations of zip64 masks
+        # present and extra fields present.
+        params = (
+            {"file_size_64_set": True, "file_size_extra": True},
+            {"compress_size_64_set": True, "compress_size_extra": True},
+            {"header_offset_64_set": True, "header_offset_extra": True},
+        )
 
-        with zipfile.ZipFile(TESTFN2, "r", zipfile.ZIP_STORED) as zipfp:
-            self.assertEqual(zipfp.namelist(), ["absolute"])
+        for r in range(1, len(params) + 1):
+            for combo in itertools.combinations(params, r):
+                kwargs = {}
+                for c in combo:
+                    kwargs.update(c)
+                with zipfile.ZipFile(io.BytesIO(self.make_zip64_file(**kwargs))) as zf:
+                    zinfo = zf.infolist()[0]
+                    self.assertEqual(zinfo.file_size, expected_file_size)
+                    self.assertEqual(zinfo.compress_size, expected_compress_size)
+                    self.assertEqual(zinfo.header_offset, expected_header_offset)
+                    self.assertEqual(zf.read(zinfo), expected_content)
 
-    def test_append(self):
-        # Test that appending to the Zip64 archive doesn't change
-        # extra fields of existing entries.
-        with zipfile.ZipFile(TESTFN2, "w", allowZip64=True) as zipfp:
-            zipfp.writestr("strfile", self.data)
-        with zipfile.ZipFile(TESTFN2, "r", allowZip64=True) as zipfp:
-            zinfo = zipfp.getinfo("strfile")
-            extra = zinfo.extra
-        with zipfile.ZipFile(TESTFN2, "a", allowZip64=True) as zipfp:
-            zipfp.writestr("strfile2", self.data)
-        with zipfile.ZipFile(TESTFN2, "r", allowZip64=True) as zipfp:
-            zinfo = zipfp.getinfo("strfile")
-            self.assertEqual(zinfo.extra, extra)
 
 @requires_zlib
 class DeflateTestZip64InSmallFiles(AbstractTestZip64InSmallFiles,
diff --git a/Lib/zipfile.py b/Lib/zipfile.py
index 6708500837806e..083708ce4caafe 100644
--- a/Lib/zipfile.py
+++ b/Lib/zipfile.py
@@ -479,7 +479,7 @@ def _decodeExtra(self):
 
                 # ZIP64 extension (large files and/or large archives)
                 if self.file_size in (0xffffffffffffffff, 0xffffffff):
-                    if len(counts) < idx + 1:
+                    if len(counts) <= idx:
                         raise BadZipFile(
                             "Corrupt zip64 extra field. File size not found."
                         )
@@ -487,7 +487,7 @@ def _decodeExtra(self):
                     idx += 1
 
                 if self.compress_size == 0xFFFFFFFF:
-                    if len(counts) < idx + 1:
+                    if len(counts) <= idx:
                         raise BadZipFile(
                             "Corrupt zip64 extra field. Compress size not found."
                         )
@@ -495,7 +495,7 @@ def _decodeExtra(self):
                     idx += 1
 
                 if self.header_offset == 0xffffffff:
-                    if len(counts) < idx + 1:
+                    if len(counts) <= idx:
                         raise BadZipFile(
                             "Corrupt zip64 extra field. Header offset not found."
                         )
diff --git a/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst b/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst
index f21f0c52eaa53b..009e07b92d22d3 100644
--- a/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst
+++ b/Misc/NEWS.d/next/Library/2019-07-09-05-44-39.bpo-36993.4javqu.rst
@@ -1 +1,2 @@
-Improve error reporting for corrupt zip files with bad zip64 extra data.
\ No newline at end of file
+Improve error reporting for corrupt zip files with bad zip64 extra data. Patch
+by Daniel Hillier.