From eb1935dacf3b181d8b11cae721d24329a817261a Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Mon, 24 Jan 2022 22:34:41 +0000 Subject: [PATCH 01/11] bpo-39064: make ZipFile raise BadZipFile instead of ValueError when reading a corrupt file --- Lib/zipfile.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/zipfile.py b/Lib/zipfile.py index 8e9325b9343260..2c086dc36a11e1 100644 --- a/Lib/zipfile.py +++ b/Lib/zipfile.py @@ -1366,7 +1366,10 @@ def _RealGetContents(self): print("given, inferred, offset", offset_cd, inferred, concat) # self.start_dir: Position of start of central directory self.start_dir = offset_cd + concat - fp.seek(self.start_dir, 0) + try: + fp.seek(self.start_dir, 0) + except ValueError as e: + raise BadZipFile("Bad offset for central directory") data = fp.read(size_cd) fp = io.BytesIO(data) total = 0 From dc87047c68a509095f91b027f14b458f57e42851 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sun, 3 Apr 2022 19:32:09 +0100 Subject: [PATCH 02/11] Test that BadZipFile is raised when central directory offset is negative --- Lib/test/test_zipfile.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index df48fabff951d8..72fb5b2dcca7f8 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1727,6 +1727,17 @@ def test_empty_file_raises_BadZipFile(self): fp.write("short file") self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, TESTFN) + def test_negative_central_directory_offset_raises_BadZipFile(self): + # Zip file containing an empty EOCD record + b = [80, 75, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] + + # Set the size of the central directory bytes to become 1, + # causing the central directory offset to become negative + b[12] = 1 + + f = io.BytesIO(bytes(b)) + self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, f) + def test_closed_zip_raises_ValueError(self): """Verify that testzip() doesn't swallow inappropriate exceptions.""" data = io.BytesIO() From f97f327b2b4c972ef4d49d0c5ea9e6a522f25492 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sun, 3 Apr 2022 19:44:24 +0100 Subject: [PATCH 03/11] Add news entry --- .../next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst diff --git a/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst b/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst new file mode 100644 index 00000000000000..70ed47ae95f960 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst @@ -0,0 +1,2 @@ +:class:`zipfile.ZipFile` now raises :exc:`zipfile.BadZipFile` when reading a +corrupt zip file in which the central directory offset is negative. From 9fe2e98cd5ff423624a18d71983dd81c0f965ed7 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sun, 3 Apr 2022 21:30:44 +0100 Subject: [PATCH 04/11] Change variable name --- Lib/test/test_zipfile.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index 72fb5b2dcca7f8..d7894d26ab947e 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1729,13 +1729,13 @@ def test_empty_file_raises_BadZipFile(self): def test_negative_central_directory_offset_raises_BadZipFile(self): # Zip file containing an empty EOCD record - b = [80, 75, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] + buffer = [80, 75, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] # Set the size of the central directory bytes to become 1, # causing the central directory offset to become negative - b[12] = 1 + buffer[12] = 1 - f = io.BytesIO(bytes(b)) + f = io.BytesIO(bytes(buffer)) self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, f) def test_closed_zip_raises_ValueError(self): From f73ebecfeb228792bf3a1dbd63f81f7eb7bd0b80 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Mon, 4 Apr 2022 12:17:05 +0100 Subject: [PATCH 05/11] Update Lib/zipfile.py Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> --- Lib/zipfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/zipfile.py b/Lib/zipfile.py index 2c086dc36a11e1..994ab33355bb84 100644 --- a/Lib/zipfile.py +++ b/Lib/zipfile.py @@ -1369,7 +1369,7 @@ def _RealGetContents(self): try: fp.seek(self.start_dir, 0) except ValueError as e: - raise BadZipFile("Bad offset for central directory") + raise BadZipFile("Bad offset for central directory") from e data = fp.read(size_cd) fp = io.BytesIO(data) total = 0 From a26de91ac44800039d67647c1e5d282218cb28c8 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sat, 23 Apr 2022 13:09:47 +0100 Subject: [PATCH 06/11] Use byte-strings to write malformed zipfile Co-authored-by: Serhiy Storchaka --- Lib/test/test_zipfile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index d7894d26ab947e..ec8fc66dba4c4a 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1730,6 +1730,7 @@ def test_empty_file_raises_BadZipFile(self): def test_negative_central_directory_offset_raises_BadZipFile(self): # Zip file containing an empty EOCD record buffer = [80, 75, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] + buffer = bytearray(b'PK\x05\x06' + b'\0'*18) # Set the size of the central directory bytes to become 1, # causing the central directory offset to become negative From 4e358370f7abe309564d44d9617fdb1ea5ad9831 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sat, 23 Apr 2022 13:10:01 +0100 Subject: [PATCH 07/11] Remove rendundant call to bytes Co-authored-by: Serhiy Storchaka --- Lib/test/test_zipfile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index ec8fc66dba4c4a..4e98415de2171e 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1737,6 +1737,7 @@ def test_negative_central_directory_offset_raises_BadZipFile(self): buffer[12] = 1 f = io.BytesIO(bytes(buffer)) + f = io.BytesIO(buffer) self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, f) def test_closed_zip_raises_ValueError(self): From ab1afb0798007b69d9feaa682b4f0feb0881f8ee Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sat, 23 Apr 2022 13:11:08 +0100 Subject: [PATCH 08/11] Update news entry Co-authored-by: Serhiy Storchaka --- .../next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst b/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst index 70ed47ae95f960..34d31527e332dc 100644 --- a/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst +++ b/Misc/NEWS.d/next/Library/2022-04-03-19-40-09.bpo-39064.76PbIz.rst @@ -1,2 +1,2 @@ -:class:`zipfile.ZipFile` now raises :exc:`zipfile.BadZipFile` when reading a +:class:`zipfile.ZipFile` now raises :exc:`zipfile.BadZipFile` instead of ``ValueError`` when reading a corrupt zip file in which the central directory offset is negative. From bf8d1670c9726301c77beb00323d62eaa478a8a6 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sat, 23 Apr 2022 13:22:16 +0100 Subject: [PATCH 09/11] Use offset value to determine whether to raise BadZipFile --- Lib/zipfile.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/zipfile.py b/Lib/zipfile.py index 994ab33355bb84..fe3900cdb26c6b 100644 --- a/Lib/zipfile.py +++ b/Lib/zipfile.py @@ -1366,10 +1366,9 @@ def _RealGetContents(self): print("given, inferred, offset", offset_cd, inferred, concat) # self.start_dir: Position of start of central directory self.start_dir = offset_cd + concat - try: - fp.seek(self.start_dir, 0) - except ValueError as e: - raise BadZipFile("Bad offset for central directory") from e + if self.start_dir < 0: + raise BadZipFile("Bad offset for central directory") + fp.seek(self.start_dir, 0) data = fp.read(size_cd) fp = io.BytesIO(data) total = 0 From 05d6057b8e883273d11922aa17928e0bb009af2b Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Sun, 24 Apr 2022 00:19:36 +0100 Subject: [PATCH 10/11] Remove duplicated lines --- Lib/test/test_zipfile.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index 4e98415de2171e..d3ffe05ebb43dd 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1729,14 +1729,12 @@ def test_empty_file_raises_BadZipFile(self): def test_negative_central_directory_offset_raises_BadZipFile(self): # Zip file containing an empty EOCD record - buffer = [80, 75, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] buffer = bytearray(b'PK\x05\x06' + b'\0'*18) # Set the size of the central directory bytes to become 1, # causing the central directory offset to become negative buffer[12] = 1 - f = io.BytesIO(bytes(buffer)) f = io.BytesIO(buffer) self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, f) From f0ee1c7dceabdc5815881d4a07c6c92a8f0d5c48 Mon Sep 17 00:00:00 2001 From: Sam Ezeh Date: Mon, 23 May 2022 17:03:48 +0100 Subject: [PATCH 11/11] Run BadZipFile test for all 32 bit central directory record sizes Co-authored-by: Serhiy Storchaka --- Lib/test/test_zipfile.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_zipfile.py b/Lib/test/test_zipfile.py index d3ffe05ebb43dd..a830f834c6e114 100644 --- a/Lib/test/test_zipfile.py +++ b/Lib/test/test_zipfile.py @@ -1733,10 +1733,10 @@ def test_negative_central_directory_offset_raises_BadZipFile(self): # Set the size of the central directory bytes to become 1, # causing the central directory offset to become negative - buffer[12] = 1 - - f = io.BytesIO(buffer) - self.assertRaises(zipfile.BadZipFile, zipfile.ZipFile, f) + for dirsize in 1, 2**32-1: + buffer[12:16] = struct.pack('