gh-51067: Default ZipFile.repack() to strict_descriptor=True

Locating an unsigned data descriptor for a STORED or encrypted entry
requires a byte-by-byte scan that can be ~100-1000x slower and is a
potential denial-of-service vector on untrusted input. Default
strict_descriptor to True so repack() detects only the signed data
descriptors written by Python and other modern tools, leaving the slow
scan as an opt-in.

Also explain data descriptors and the signed/unsigned distinction in
the docs, and add coverage for the new default (unit, encrypted, and an
end-to-end repack test across all compression methods).

Author: gpshead

Doc/library/zipfile.rst

@@ -576,7 +576,7 @@ ZipFile objects
 
 
 .. method:: ZipFile.repack(removed=None, *, \
-                           strict_descriptor=False[, chunk_size])
+                           strict_descriptor=True[, chunk_size])
 
    Rewrites the archive to remove unreferenced local file entries, shrinking
    its file size.  The archive must be opened with mode ``'a'``.
@@ -587,12 +587,22 @@ ZipFile objects
    locate and remove local file entries that are no longer referenced in the
    central directory.
 
-   When scanning, setting ``strict_descriptor=True`` disables detection of any
-   entry using an unsigned data descriptor (a format deprecated by the ZIP
-   specification since version 6.3.0, released on 2006-09-29, and used only by
-   some legacy tools), which is significantly slower to scan—around 100 to
-   1000 times in the worst case. This does not affect performance on entries
-   without such feature.
+   When scanning, *strict_descriptor* controls how entries written with an
+   unsigned *data descriptor* are handled.  A data descriptor is an optional
+   record holding an entry's CRC and sizes, stored just after the entry's data;
+   it is used when the archive is written to a non-seekable stream, and is
+   *signed* when it begins with a marker signature or *unsigned* otherwise.
+   Unsigned descriptors have been deprecated by the `PKZIP Application Note`_
+   since version 6.3.0 (released in 2006) and are written only by some legacy
+   tools; signed descriptors—written by Python and other modern tools—are always
+   detected.  When *strict_descriptor* is true (the default), only signed data
+   descriptors are detected, so an unreferenced entry written with an unsigned
+   descriptor is not located and its space is not reclaimed by the scan.
+   Setting ``strict_descriptor=False`` additionally detects unsigned
+   descriptors, at the cost of a significantly slower scan—around 100 to 1000
+   times in the worst case—which may be exploitable as a denial-of-service
+   vector on untrusted input.  This does not affect entries without a data
+   descriptor, and is not needed when *removed* is provided.
 
    *chunk_size* may be specified to control the buffer size when moving
    entry data (default is 1 MiB).

Lib/test/test_zipfile/test_core.py

@@ -2134,6 +2134,52 @@ def test_repack_overlapping_blocks(self):
                     with self.assertRaisesRegex(zipfile.BadZipFile, 'Overlapped entries'):
                         zh.repack()
 
+    def test_repack_scan_unsigned_data_descriptor(self):
+        """By default (strict_descriptor=True) the scan does not reclaim an
+        unreferenced entry written with an unsigned data descriptor, but keeps
+        the archive valid; strict_descriptor=False reclaims it."""
+        removed_name, removed_data = self.test_files[1]
+        remaining = [n for n, _ in self.test_files if n != removed_name]
+
+        # Build an archive whose entries use *unsigned* data descriptors by
+        # writing to an unseekable stream with the descriptor signature stripped.
+        buf = io.BytesIO()
+        with mock.patch.object(struct, 'pack', side_effect=struct_pack_no_dd_sig):
+            with zipfile.ZipFile(Unseekable(buf), 'w', self.compression) as zh:
+                for file, data in self.test_files:
+                    with zh.open(file, 'w') as fh:
+                        fh.write(data)
+        archive = buf.getvalue()
+
+        # sanity: the removed entry really uses a data descriptor (flag bit 3);
+        # it is unsigned by construction above
+        with zipfile.ZipFile(io.BytesIO(archive)) as zh:
+            self.assertTrue(zh.getinfo(removed_name).flag_bits & 0x08)
+
+        # default repack(): strict_descriptor=True does not locate the unsigned
+        # data descriptor, so the local data is preserved (not reclaimed).
+        fz = io.BytesIO(archive)
+        with zipfile.ZipFile(fz, 'a', self.compression) as zh:
+            zh.remove(removed_name)
+            zh.repack()
+        default_size = len(fz.getvalue())
+        with zipfile.ZipFile(fz) as zh:
+            self.assertEqual(zh.namelist(), remaining)
+            self.assertIsNone(zh.testzip())
+
+        # strict_descriptor=False: the unsigned data descriptor is detected, so
+        # the local data is reclaimed and the archive shrinks.
+        fz = io.BytesIO(archive)
+        with zipfile.ZipFile(fz, 'a', self.compression) as zh:
+            zh.remove(removed_name)
+            zh.repack(strict_descriptor=False)
+        strict_false_size = len(fz.getvalue())
+        with zipfile.ZipFile(fz) as zh:
+            self.assertEqual(zh.namelist(), remaining)
+            self.assertIsNone(zh.testzip())
+
+        self.assertLess(strict_false_size, default_size)
+
     def test_repack_removed_basic(self):
         """Should remove local file entries for provided deleted files."""
         ln = len(self.test_files)
@@ -2628,7 +2674,9 @@ def test_validate_local_file_entry_zstd(self):
         self._test_validate_local_file_entry(method=zipfile.ZIP_ZSTANDARD)
 
     def _test_validate_local_file_entry(self, method):
-        repacker = zipfile._ZipRepacker()
+        # strict_descriptor=False to exercise unsigned data descriptor scanning
+        # (the default is strict_descriptor=True, tested separately below)
+        repacker = zipfile._ZipRepacker(strict_descriptor=False)
 
         # basic
         bytes_ = self._generate_local_file_entry(
@@ -2799,7 +2847,9 @@ def test_validate_local_file_entry_zip64_zstd(self):
         self._test_validate_local_file_entry_zip64(method=zipfile.ZIP_ZSTANDARD)
 
     def _test_validate_local_file_entry_zip64(self, method):
-        repacker = zipfile._ZipRepacker()
+        # strict_descriptor=False to exercise unsigned data descriptor scanning
+        # (the default is strict_descriptor=True, tested separately below)
+        repacker = zipfile._ZipRepacker(strict_descriptor=False)
 
         # zip64
         bytes_ = self._generate_local_file_entry(
@@ -2870,7 +2920,9 @@ def _test_validate_local_file_entry_zip64(self, method):
         m_sddns.assert_not_called()
 
     def test_validate_local_file_entry_encrypted(self):
-        repacker = zipfile._ZipRepacker()
+        # strict_descriptor=False to exercise unsigned data descriptor scanning
+        # of an encrypted entry (the default strict_descriptor=True is tested below)
+        repacker = zipfile._ZipRepacker(strict_descriptor=False)
 
         bytes_ = (
             b'PK\x03\x04'
@@ -2903,6 +2955,21 @@ def test_validate_local_file_entry_encrypted(self):
         m_sddnsbd.assert_not_called()
         m_sddns.assert_called_once_with(fz, 38, len(bytes_), False)
 
+        # return None for the unsigned data descriptor if `strict_descriptor=True`
+        repacker = zipfile._ZipRepacker(strict_descriptor=True)
+        fz = io.BytesIO(bytes_)
+        with mock.patch.object(repacker, '_scan_data_descriptor',
+                               wraps=repacker._scan_data_descriptor) as m_sdd, \
+             mock.patch.object(repacker, '_scan_data_descriptor_no_sig_by_decompression',
+                               wraps=repacker._scan_data_descriptor_no_sig_by_decompression) as m_sddnsbd, \
+             mock.patch.object(repacker, '_scan_data_descriptor_no_sig',
+                               wraps=repacker._scan_data_descriptor_no_sig) as m_sddns:
+            result = repacker._validate_local_file_entry(fz, 0, len(bytes_))
+        self.assertEqual(result, None)
+        m_sdd.assert_called_once_with(fz, 38, len(bytes_), False)
+        m_sddnsbd.assert_not_called()
+        m_sddns.assert_not_called()
+
     def test_iter_scan_signature(self):
         bytes_ = b'sig__sig__sig__sig'
         ln = len(bytes_)

Lib/test/test_zipfile64.py

@@ -225,7 +225,9 @@ def _test_strip_removed_large_file_with_dd_no_sig(self, f):
 
         with zipfile.ZipFile(f, 'a') as zh:
             zh.remove(file1)
-            zh.repack()
+            # strict_descriptor=False to scan the unsigned data descriptor
+            # (scanning is disabled under the strict_descriptor=True default)
+            zh.repack(strict_descriptor=False)
             self.assertIsNone(zh.testzip())
 
     @requires_zlib()
@@ -255,7 +257,9 @@ def _test_strip_removed_large_file_with_dd_no_sig_by_decompression(self, f, meth
 
         with zipfile.ZipFile(f, 'a') as zh:
             zh.remove(file1)
-            zh.repack()
+            # strict_descriptor=False to detect the unsigned data descriptor
+            # (scanning is disabled under the strict_descriptor=True default)
+            zh.repack(strict_descriptor=False)
             self.assertIsNone(zh.testzip())
 
 

Lib/zipfile/__init__.py

@@ -1396,7 +1396,7 @@ def close(self):
 
 class _ZipRepacker:
     """Class for ZipFile repacking."""
-    def __init__(self, *, strict_descriptor=False, chunk_size=2**20, debug=0):
+    def __init__(self, *, strict_descriptor=True, chunk_size=2**20, debug=0):
         self.debug = debug  # Level of printing: 0 through 3
         self.chunk_size = chunk_size
         self.strict_descriptor = strict_descriptor