Issue 19883: Integer overflow in zipimport.c

MSVC complains about "conversion from 'Py_ssize_t' to 'long', possible loss of data" in zipimport.c. header_offset is a Py_ssize_t but fseek() only takes a long. On 64bit Windows Py_ssize_t is a 64bit data type but long is still a 32bit data type.

It's safe to assume that the header will be smaller than 2 GB for the next couple of years. :)

read_directory() uses fseek() and ftell() which don't support offset larger than LONG_MAX (2 GB on 32-bit system).  I don't know if it's an issue. What happens if the file is longer?

"header_offset += arc_offset;" can overflow or not? This instuction looks weird.

    header_position = ftell(fp);
    ...
    header_offset = get_long((unsigned char *)endof_central_dir + 16);
    arc_offset = header_position - header_offset - header_size;
    header_offset += arc_offset;

If I computed correctly, the final line can be replaced with:

    arc_offset = header_position - header_offset - header_size;
    header_offset = header_position - header_size;

(It is weird to reuse header_position for two different values, a new variable may be added.)

Instead of checking that "header_offset > LONG_MAX", it may be safer to check that:

 - header_size >= 0
 - header_offset >= 0
 - header_offset + header_size <= LONG_MAX ---> header_offset <= LONG_MAX - header_size
 - arc_offset >= 0 ---> header_position >= header_offset + header_size
 - header_offset > 0 ---> header_position >= header_size

If all these values must be positive according to ZIP format, get_long() may be replaced with get_ulong() to simplify these checks.

See also zipfile.py which is probably more correct than zipimport.c: zipfile uses for example "L" format for struct.unpack (*unsigned* long) to decode header fields.

Yes, these fields are unsingned.

zipimport.c makes no attempt to support zip files larger than 2GiB or zip64 files.

Here is a work-in-progress patch.

PyMarshal_ReadShortFromFile() and PyMarshal_ReadLongFromFile() are still wrong: new Unsigned version should be added to marshal.c. I don't know if a C cast to unsigned is enough because long can be larger than 32-bit (ex: on Linux 64-bit):
#if SIZEOF_LONG > 4
        /* Sign extension for 64-bit machines */
        x |= -(x & 0x80000000L);
#endif

I didn't test my patch. Anyone interested to finish the patch?

comments added to the patch.

Ping.

Here is revised patch. It addresses Gregory's comments, uses properly integer types and converters for all values, and adds additional checks for integer overflows and ZIP file validity. As a side effect the performance can be increased due to less memory allocations and IO operations.

Sorry, the patch contained parts of the advanced patch that will be submitted in separate issue.

Synchronized with current sources.

Updated patch addresses Victor's comments and adds (mandatory now) parenthesis. Thank you Victor.

Serhiy Storchaka added the comment:
> Updated patch addresses Victor's comments and adds (mandatory now) parenthesis. Thank you Victor.

Do  you mean braces {...}?

The new patch looks good to me, thanks for taking all my comments in account ;-)

New changeset 687be1cbe587 by Serhiy Storchaka in branch '3.5':
Issue #19883: Fixed possible integer overflows in zipimport.
https://hg.python.org/cpython/rev/687be1cbe587

New changeset f4631dc56ecf by Serhiy Storchaka in branch 'default':
Issue #19883: Fixed possible integer overflows in zipimport.
https://hg.python.org/cpython/rev/f4631dc56ecf

New changeset cebcd2fd3e1f by Serhiy Storchaka in branch '2.7':
Issue #19883: Fixed possible integer overflows in zipimport.
https://hg.python.org/cpython/rev/cebcd2fd3e1f

This seems to be causing an infinite loop in 2.7, in test_cmd_line_script.CmdLineTest.test_module_in_package_in_zipfile():

test_module_in_package_in_zipfile (test.test_cmd_line_script.CmdLineTest) ... ^C
Test suite interrupted by signal SIGINT.
1 test omitted:
    test_cmd_line_script
[Exit 1]
[vadmium@localhost cpython]$ sudo strace -p 11412 2>&1 | head
Process 11412 attached - interrupt to quit
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
lseek(3, 1024, SEEK_SET)                = 1024
[vadmium@localhost cpython]$ ls -l /proc/11412/fdtotal 0
lr-x------ 1 vadmium users 64 Jan 29 22:23 0 -> pipe:[1249749]
l-wx------ 1 vadmium users 64 Jan 29 22:23 1 -> pipe:[1249750]
l-wx------ 1 vadmium users 64 Jan 29 22:23 2 -> pipe:[1249750]
lr-x------ 1 vadmium users 64 Jan 29 22:23 3 -> /tmp/tmpHMiuOm/test_zip.zip (deleted)
[vadmium@localhost cpython]$ kill 11412

New changeset 82ee3c24bb86 by Serhiy Storchaka in branch '2.7':
Fixed an infinite loop in zipimport caused by cebcd2fd3e1f (issue #19883).
https://hg.python.org/cpython/rev/82ee3c24bb86

Thank you Martin.

components: + Extension Modules
versions: - Python 3.4
keywords: + needs review
nosy: + benjamin.peterson

messages: + msg258797

messages: + msg254351

messages: + msg254348

messages: + msg205544

messages: + msg205210
title: overflow in zipexport.c -> Integer overflow in zipimport.c