is_zipfile false positives #72680

thomaswaldmann · 2016-10-20T21:08:49Z

BPO	28494
Nosy	@Yhg1s, @gpshead, @vstinner, @serhiy-storchaka, @ThomasWaldmann, @jjolly, @miss-islington
PRs	gh-72680: Fix false positives when using zipfile.is_zipfile() #5053 bpo-28494: Test existing zipfile working behavior. #15853 [3.8] bpo-28494: Test existing zipfile working behavior. (GH-15853) #15891 bpo-28494: install ziptestdata to fix install bot. #15902 [3.8] bpo-28494: install ziptestdata to fix install bot (GH-15902) #15912
Files	isz_fail.py isz_fail_fix.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = None
created_at = <Date 2016-10-20.21:08:48.959>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'is_zipfile false positives'
updated_at = <Date 2019-09-11.11:09:56.949>
user = 'https://github.com/ThomasWaldmann'

bugs.python.org fields:

activity = <Date 2019-09-11.11:09:56.949>
actor = 'miss-islington'
assignee = 'gregory.p.smith'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2016-10-20.21:08:48.959>
creator = 'Thomas.Waldmann'
dependencies = []
files = ['45162', '45164']
hgrepos = []
issue_num = 28494
keywords = ['patch']
message_count = 16.0
messages = ['279084', '279088', '279089', '280341', '281805', '281817', '309245', '311281', '334595', '351723', '351749', '351750', '351751', '351755', '351802', '351823']
nosy_count = 9.0
nosy_names = ['twouters', 'gregory.p.smith', 'alanmcintyre', 'vstinner', 'serhiy.storchaka', 'Thomas.Waldmann', 'mryan1539', 'jjolly', 'miss-islington']
pr_nums = ['5053', '15853', '15891', '15902', '15912']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28494'
versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

thomaswaldmann · 2016-10-20T21:08:49Z

zipfile.is_zipfile has false positives way too easily.

I just have seen it in practive when a MoinMoin wiki site with a lot of pdf attachments crashed with 500. This was caused by a valid PDF that just happened to contain PK\005\006 somewhere in the middle - this was enough to satisfy is_zipfile() and triggered further processing as a zipfile, which then crashed with IOError (which was not catched in our code, yet).

I have looked into zipfile code: if the usual EOCD structure (with empty comment) is not at EOF, it is suspected that there might be a non-empty comment and ~64K before EOF are searched for the PK\005\006 magic. If it is somewhere there, it is assumed that the file is a zip, without any further validity check.

Attached is a failure demo that works with at least 2.7 and 3.5.

https://en.wikipedia.org/wiki/Zip_(file_format)

thomaswaldmann · 2016-10-20T22:26:45Z

patch for py2.7

The EOCD structure is at EOF.

It either does not contain a comment (this is what the existing code checks first) or it contains a comment of the length that is specified in the structure.

The patch checks consistency specified length vs. real length (end of fixed part of structure up to EOF). If this does not match, it is likely not a zip file, but just a file that happens to have the magic 4 bytes somewhere in its last 64kB.

thomaswaldmann · 2016-10-20T22:35:23Z

Note: checking the first bytes of the file (PK..) might be another option.

But this has the "problem" that a self-extracting zip starts with an executable that has different first bytes.

So whether this is an option or not depends on whether is_zipfile() should return truish for self-extracting ZIP files.

serhiy-storchaka · 2016-11-08T19:45:17Z

The problem is that the zipfile module supports even not well-formed archives, with a data appended past a comment, and with truncated comment. There are special tests for this, and the proposed patch breaks these tests: test_comments, test_ignores_newline_at_end, test_ignores_stuff_appended_past_comments. See bpo-10694 and bpo-1622.

thomaswaldmann · 2016-11-27T00:18:41Z

Well, if you have a better idea how to fix is_zipfile, go on.

I even suggested an alternative, how about that?

It is a miserable state when the is_zipfile function in the stdlib detects random crap as a zip file.

serhiy-storchaka · 2016-11-27T10:29:36Z

No, checking the first bytes of the file is not appropriate option. zipfile should support the Python zip application format [1].

I see two options:

Make is_zipfile() more strict that the ZipFile constructor. The later supports ZIP files with a data past the comment or with truncated comments, but the former should reject them.
Make both is_zipfile() and the ZipFile constructor more robust. They should check not just the EOCD signature, but check the Zip64 end of central directory record (if exists) and the first central file header signature (if the ZIP file is not empty).

It may be that PDF files contain PK\005\006 not accidentally, but because they contain embedded ZIP files (I don't know if this is a case). In that circumstances is_zipfile() returning True is correct.

[1] https://docs.python.org/3/library/zipapp.html

jjolly · 2017-12-30T19:28:10Z

Fix submitted that evaluates the ECD structure and validates the first CD entry. The fix also handles empty zipfiles.

IMO the purpose of this API is to *quickly* verify that the file is a valid zipfile. With this fix, the API only reads another 46 bytes of data (after a seek, of course). This should still qualify as "quick", especially after having potentially read 64k of data.

Perhaps a full zip validator would be appropriate in addition to is_zipfile. That would be more appropriate as a full feature rather than in this bugfix.

jjolly · 2018-01-30T16:33:55Z

Is there any chance that this will make it into 3.7 or is my reminder too late?

gpshead · 2019-01-30T21:23:57Z

it's a bugfix, it seems reasonable for 3.7 to me. I agree that the previous is_zipfile check is too lenient. I'll follow up on jjolly's PR for any specific concerns I have with the implementation.

Yhg1s · 2019-09-10T16:14:15Z

New changeset 3f4db4a by T. Wouters (Gregory P. Smith) in branch 'master':
bpo-28494: Test existing zipfile working behavior. (GH-15853)
3f4db4a

vstinner · 2019-09-10T22:29:14Z

x86 Gentoo Installed with X 3.x buildbot is unhappy:

https://buildbot.python.org/all/#/builders/103/builds/3051

======================================================================
ERROR: test_execute_zip2 (test.test_zipfile.TestExecutablePrependedZip)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2502, in test_execute_zip2
    output = subprocess.check_output([self.exe_zip, sys.executable])
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 411, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 489, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 845, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 1689, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'ziptestdata/exe_with_zip'

======================================================================
ERROR: test_execute_zip64 (test.test_zipfile.TestExecutablePrependedZip)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2509, in test_execute_zip64
    output = subprocess.check_output([self.exe_zip64, sys.executable])
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 411, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 489, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 845, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/subprocess.py", line 1689, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'ziptestdata/exe_with_z64'

======================================================================
FAIL: test_read_zip64_with_exe_prepended (test.test_zipfile.TestExecutablePrependedZip)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2496, in test_read_zip64_with_exe_prepended
    self._test_zip_works(self.exe_zip64)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2484, in _test_zip_works
    self.assertTrue(zipfile.is_zipfile(name),
AssertionError: False is not true : is_zipfile failed on ziptestdata/exe_with_z64

======================================================================
FAIL: test_read_zip_with_exe_prepended (test.test_zipfile.TestExecutablePrependedZip)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2493, in test_read_zip_with_exe_prepended
    self._test_zip_works(self.exe_zip)
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.9/test/test_zipfile.py", line 2484, in _test_zip_works
    self.assertTrue(zipfile.is_zipfile(name),
AssertionError: False is not true : is_zipfile failed on ziptestdata/exe_with_zip

gpshead · 2019-09-10T22:37:33Z

The new ziptestdata/ subdir appears to not be part of the install that make install does. :/

vstinner · 2019-09-10T22:44:59Z

The new ziptestdata/ subdir appears to not be part of the install that make install does. :/

It seems like Lib/test/eintrdata/ (for example) is installed using LIBSUBDIRS variable in Makefile.pre.in.

Note: The Windows installer copies recursively Lib/test/ and subdirectories: see <InstallFiles Include="$(PySourcePath)Lib\test\**\*" ...> in Tools/msi/test/test.wixproj.

miss-islington · 2019-09-10T22:57:59Z

New changeset 74b0291 by Miss Islington (bot) in branch '3.8':
bpo-28494: Test existing zipfile working behavior. (GH-15853)
74b0291

gpshead · 2019-09-11T10:31:40Z

New changeset c374474 by Gregory P. Smith in branch 'master':
bpo-28494: install ziptestdata to fix install bot (GH-15902)
c374474

miss-islington · 2019-09-11T11:09:57Z

New changeset 7acb22e by Miss Islington (bot) in branch '3.8':
bpo-28494: install ziptestdata to fix install bot (GH-15902)
7acb22e

vpv-csc · 2024-06-03T12:59:33Z

Any chance #5053 could be merged? A user of ours has a TIFF file, for which is_zipfile returns True, but calling the ZipFile constructor results in BadZipFile. I probably can not give out the file, but here's an example run:

Python 3.12.3 (main, Apr 17 2024, 00:00:00) [GCC 14.0.1 20240411 (Red Hat 14.0.1-0)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import zipfile
>>> zipfile.is_zipfile("testfile.tif")
True
>>> zipfile.ZipFile("testfile.tif")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.12/zipfile/__init__.py", line 1349, in __init__
    self._RealGetContents()
  File "/usr/lib64/python3.12/zipfile/__init__.py", line 1435, in _RealGetContents
    raise BadZipFile("Bad offset for central directory")
zipfile.BadZipFile: Bad offset for central directory

vstinner · 2024-06-03T13:16:36Z

@vpv-csc: test_ignores_newline_at_end() and test_ignores_stuff_appended_past_comments() of test_zipfile fail with attached isz_fail_fix.diff. Someone should design a fix which doesn't break the test suite, or the test suite should be updated as well.

We cannot directly use zipfile.is_zipfile function because it gives false positives. More info: (python/cpython#72680)

thomaswaldmann mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 20, 2016

serhiy-storchaka added the 3.7 (EOL) end of life label Oct 20, 2016

serhiy-storchaka self-assigned this Nov 3, 2016

gpshead added the 3.8 (EOL) end of life label Jan 30, 2019

gpshead assigned gpshead and unassigned serhiy-storchaka Jan 30, 2019

ezio-melotti transferred this issue from another repository Apr 10, 2022

serhiy-storchaka added this to @serhiy-storchaka's project python-zipfile May 8, 2022

serhiy-storchaka added this to Zipfile issues May 19, 2022

bedevere-bot mentioned this issue Feb 25, 2023

gh-72680: Fix false positives when using zipfile.is_zipfile() #5053

Open

arhadthedev removed 3.8 (EOL) end of life 3.7 (EOL) end of life labels Feb 25, 2023

lhoestq mentioned this issue Mar 15, 2023

Less zip false positives huggingface/datasets#5640

Merged

StefanBruens mentioned this issue Jan 1, 2024

obscpio containing zip file raises BadZipFile exception openSUSE/obs-service-set_version#87

Open

gpshead removed their assignment Jun 3, 2024

tovento pushed a commit to Digital-Preservation-Finland/file-scraper that referenced this issue Jul 1, 2024

TPASPKT-1337 Add is_zipfile function to utils

94bc6af

We cannot directly use zipfile.is_zipfile function because it gives false positives. More info: (python/cpython#72680)

picnixz mentioned this issue Jul 30, 2024

is_zipfile does not set the position to the file header #122356

Closed

mxmlnkn mentioned this issue Oct 7, 2024

Twine check tries to open a .tar.gz file as a zip leading to errors pypa/twine#1160

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

is_zipfile false positives #72680

is_zipfile false positives #72680

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

serhiy-storchaka commented Nov 8, 2016

thomaswaldmann mannequin commented Nov 27, 2016

serhiy-storchaka commented Nov 27, 2016

jjolly mannequin commented Dec 30, 2017

jjolly mannequin commented Jan 30, 2018

gpshead commented Jan 30, 2019

Yhg1s commented Sep 10, 2019

vstinner commented Sep 10, 2019

gpshead commented Sep 10, 2019

vstinner commented Sep 10, 2019

miss-islington commented Sep 10, 2019

gpshead commented Sep 11, 2019

miss-islington commented Sep 11, 2019

vpv-csc commented Jun 3, 2024

vstinner commented Jun 3, 2024

is_zipfile false positives #72680

is_zipfile false positives #72680

Comments

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

thomaswaldmann mannequin commented Oct 20, 2016

serhiy-storchaka commented Nov 8, 2016

thomaswaldmann mannequin commented Nov 27, 2016

serhiy-storchaka commented Nov 27, 2016

jjolly mannequin commented Dec 30, 2017

jjolly mannequin commented Jan 30, 2018

gpshead commented Jan 30, 2019

Yhg1s commented Sep 10, 2019

vstinner commented Sep 10, 2019

gpshead commented Sep 10, 2019

vstinner commented Sep 10, 2019

miss-islington commented Sep 10, 2019

gpshead commented Sep 11, 2019

miss-islington commented Sep 11, 2019

vpv-csc commented Jun 3, 2024

vstinner commented Jun 3, 2024