classification
Title: is_zipfile false positives
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Thomas.Waldmann, alanmcintyre, gregory.p.smith, jjolly, miss-islington, mryan1539, serhiy.storchaka, twouters, vstinner
Priority: normal Keywords: patch

Created on 2016-10-20 21:08 by Thomas.Waldmann, last changed 2019-09-11 11:09 by miss-islington.

Files
File name Uploaded Description Edit
isz_fail.py Thomas.Waldmann, 2016-10-20 21:08
isz_fail_fix.diff Thomas.Waldmann, 2016-10-20 22:26
Pull Requests
URL Status Linked Edit
PR 5053 open jjolly, 2017-12-30 18:37
PR 15853 merged gregory.p.smith, 2019-09-10 13:36
PR 15891 merged miss-islington, 2019-09-10 22:23
PR 15902 merged gregory.p.smith, 2019-09-11 09:48
PR 15912 merged miss-islington, 2019-09-11 10:31
Messages (16)
msg279084 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2016-10-20 21:08
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)
msg279088 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2016-10-20 22:26
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.
msg279089 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2016-10-20 22:35
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.
msg280341 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-08 19:45
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 issue10694 and issue1622.
msg281805 - (view) Author: Thomas Waldmann (Thomas.Waldmann) Date: 2016-11-27 00:18
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.
msg281817 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-11-27 10:29
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:

1. 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.

2. 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
msg309245 - (view) Author: John Jolly (jjolly) * Date: 2017-12-30 19:28
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.
msg311281 - (view) Author: John Jolly (jjolly) * Date: 2018-01-30 16:33
Is there any chance that this will make it into 3.7 or is my reminder too late?
msg334595 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-01-30 21:23
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.
msg351723 - (view) Author: Thomas Wouters (twouters) * (Python committer) Date: 2019-09-10 16:14
New changeset 3f4db4a0bab073b768fae958e93288bd5d24eadd by T. Wouters (Gregory P. Smith) in branch 'master':
bpo-28494: Test existing zipfile working behavior. (GH-15853)
https://github.com/python/cpython/commit/3f4db4a0bab073b768fae958e93288bd5d24eadd
msg351749 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-10 22:29
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
msg351750 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-10 22:37
The new ziptestdata/ subdir appears to not be part of the install that make install does. :/
msg351751 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-09-10 22:44
> 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.
msg351755 - (view) Author: miss-islington (miss-islington) Date: 2019-09-10 22:57
New changeset 74b0291b03db60dd244d31e9c97407cccb8d30dd by Miss Islington (bot) in branch '3.8':
bpo-28494: Test existing zipfile working behavior. (GH-15853)
https://github.com/python/cpython/commit/74b0291b03db60dd244d31e9c97407cccb8d30dd
msg351802 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-09-11 10:31
New changeset c37447481ec8f6d0e49d0587ec0de3f9e7d56b28 by Gregory P. Smith in branch 'master':
bpo-28494: install ziptestdata to fix install bot (GH-15902)
https://github.com/python/cpython/commit/c37447481ec8f6d0e49d0587ec0de3f9e7d56b28
msg351823 - (view) Author: miss-islington (miss-islington) Date: 2019-09-11 11:09
New changeset 7acb22e6e9061f85988c0c6c5ee25ebdf2950841 by Miss Islington (bot) in branch '3.8':
bpo-28494: install ziptestdata to fix install bot (GH-15902)
https://github.com/python/cpython/commit/7acb22e6e9061f85988c0c6c5ee25ebdf2950841
History
Date User Action Args
2020-10-28 08:02:36serhiy.storchakalinkissue42096 superseder
2019-09-11 11:09:56miss-islingtonsetmessages: + msg351823
2019-09-11 10:31:48miss-islingtonsetpull_requests: + pull_request15553
2019-09-11 10:31:39gregory.p.smithsetmessages: + msg351802
2019-09-11 09:48:05gregory.p.smithsetpull_requests: + pull_request15543
2019-09-10 22:57:59miss-islingtonsetnosy: + miss-islington
messages: + msg351755
2019-09-10 22:44:59vstinnersetmessages: + msg351751
2019-09-10 22:37:33gregory.p.smithsetmessages: + msg351750
2019-09-10 22:29:13vstinnersetnosy: + vstinner
messages: + msg351749
2019-09-10 22:23:02miss-islingtonsetpull_requests: + pull_request15532
2019-09-10 16:14:14twouterssetmessages: + msg351723
2019-09-10 13:36:27gregory.p.smithsetpull_requests: + pull_request15502
2019-01-30 21:23:57gregory.p.smithsetassignee: serhiy.storchaka -> gregory.p.smith
messages: + msg334595
2019-01-30 21:09:30gregory.p.smithsetversions: + Python 3.8, - Python 3.5, Python 3.6
2019-01-30 20:48:14mryan1539setnosy: + mryan1539
2018-01-30 16:35:26jjollysetnosy: + gregory.p.smith
2018-01-30 16:33:55jjollysetmessages: + msg311281
2017-12-30 19:28:09jjollysetnosy: + jjolly
messages: + msg309245
2017-12-30 18:37:19jjollysetstage: patch review
pull_requests: + pull_request4934
2016-11-27 10:29:35serhiy.storchakasetmessages: + msg281817
2016-11-27 00:18:41Thomas.Waldmannsetmessages: + msg281805
2016-11-08 19:45:16serhiy.storchakasetmessages: + msg280341
2016-11-03 13:47:15serhiy.storchakasetassignee: serhiy.storchaka
2016-10-20 22:35:23Thomas.Waldmannsetmessages: + msg279089
2016-10-20 22:26:45Thomas.Waldmannsetfiles: + isz_fail_fix.diff
keywords: + patch
messages: + msg279088
2016-10-20 21:30:44serhiy.storchakasetnosy: + twouters, alanmcintyre, serhiy.storchaka

versions: + Python 3.6, Python 3.7
2016-10-20 21:08:48Thomas.Waldmanncreate