Integer overflow in zipimport.c

[tiran]

BPO	19883
Nosy	@brettcannon, @gpshead, @vstinner, @tiran, @benjaminp, @ericsnowcurrently, @vadmium, @serhiy-storchaka
Files	zipimport_header_offset.patch zipimport_int_overflow.patch zipimport_int_overflow_2.patch zipimport_int_overflow_3.patch zipimport_int_overflow_4.patch

^{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/serhiy-storchaka'
closed_at = <Date 2016-01-29.17:14:48.329>
created_at = <Date 2013-12-04.09:34:05.578>
labels = ['extension-modules', 'build']
title = 'Integer overflow in zipimport.c'
updated_at = <Date 2016-01-29.17:14:48.328>
user = 'https://github.com/tiran'

bugs.python.org fields:

activity = <Date 2016-01-29.17:14:48.328>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-01-29.17:14:48.329>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2013-12-04.09:34:05.578>
creator = 'christian.heimes'
dependencies = []
files = ['32970', '33044', '40982', '41687', '41697']
hgrepos = []
issue_num = 19883
keywords = ['patch', 'needs review']
message_count = 17.0
messages = ['205209', '205210', '205211', '205380', '205504', '205544', '205597', '231272', '254348', '254351', '258797', '258858', '258862', '259153', '259165', '259168', '259169']
nosy_count = 10.0
nosy_names = ['brett.cannon', 'gregory.p.smith', 'vstinner', 'christian.heimes', 'benjamin.peterson', 'python-dev', 'eric.snow', 'martin.panter', 'serhiy.storchaka', 'superluser']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue19883'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[tiran]

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. :)

[vstinner]

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.

[vstinner]

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.

[serhiy-storchaka]

Yes, these fields are unsingned.

[gpshead]

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

[vstinner]

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?

[gpshead]

comments added to the patch.

[serhiy-storchaka]

Ping.

[serhiy-storchaka]

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.

[serhiy-storchaka]

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

[serhiy-storchaka]

Synchronized with current sources.