homepage

Creating a ZipFile object with a certain type of zip file can cause it
to go into an infinite loop. The problem is the new extra field parsing
routine. It unpacks integers as a signed value, which if they are
sufficiently large (over 32767), then it will loop forever.
There are many places in the zipfile module that incorrectly unpack
values as signed integers, but this is the only place that I can
determine that causes a serious problem.
Attached is a fix.

Based on the ZIP spec (I'm using the one here:
http://www.pkware.com/documents/casestudies/APPNOTE.TXT), I'm inclined
to agree. There's a general note that says "All fields unless otherwise
noted are unsigned and stored in Intel low-byte:high-byte,
low-word:high-word order." I can't find any explicit mention of any
fields that should be signed.
Since I find myself poking around in the ZIP file format today, I may
try changing all the format strings to unsigned, and writing up some
tests to check cases that can be run in a reasonable amount of time. If
I get anything usable I'll post a patch.

Some of this work has already been done, see issue 1189216.
You'll obviously need to keep the CRC unpack as signed because the
binascii module uses signed values.
Some header values are stored as 0xffffffff to denote the value is stored
in the 64-bit extended fields. There are a few places in the zipfile
module that use -1 to achieve this. This will cause a deprecation warning
when passed into the struct module:
DeprecationWarning: struct integer overflow masking is deprecated
The places that check for this appear to always check for both -1 and
0xfffffff, so that should be safe. Could potentially even remove the -1
check, not sure why it's checking for both. In general, you should
probably review the 64-bit support with a fine-toothed comb.
There are some other signed fields in the spec, but I don't think any of
them apply to the zipfile module. Some of them are timestamps in the
extra fields (Extended Timestamp and Info-ZIP Mac Extra fields).
I'd also keep an eye on other zipfile bugs that appear to be sign-related
(which you appear to have already looked at), such as issues 1189216,
1060, 1760357, 1526.

Alan and Eric, can I depend on you two to review each other's code and
let me know when you think the patch is ready to be submitted?

Well I can't promise it will be swift, since my winter vacation ended
today, but I'll keep on top of it as best I can. :)

Here's the first draft of a patch (zipfile-unsigned-fixes.diff) that
does a few things:
- Interpret fields as unsigned unless required (CRC, etc.)
- Corrects reading of ZIP files with large archive comments
- Replaces hard-coded structure sizes with module-level variables
(calculated using struct.calcsize)
- When writing a file in ZipFile.close(), if the ZipFile instance has a
comment value, this value is now written as the archive comment
- Renamed some of the module variables to better match the names used in
the ZIP file spec
- General cleanup & addition of comments to (hopefully) make the code a
little easier to follow
Test & doc changes are included, & this code currently passes regression
test suite with -uall on OS X.

Alan, your changes look good to me, but it is missing my patch in this bug
that fixes the sign issue in _decodeExtra. While you're there, you might
as well change the other 3 unpack lines to use a capital Q.
-Eric

Thanks for the reminder, Eric; I'll include that and post the updated patch.
As a side note, on OS X, running regrtest with -uall or -ulargefile
still skips test_zipfile4 for some reason. I'll have a look at that
before submitting the next version of the patch as well.

Currently the extra field data is only parsed when it contains Zip64
extended information. It could probably be broken up into a list of
(id, data) pairs at a minimum for all data types, since the spec says
that's the structure that "should" be used. 
I don't know whether the results of that parse should be kept in a new
slot; putting it in the ZipInfo.extra slot would probably be bad for
2.6, since users might be counting on the current behavior of a raw
chunk of data still being there. Does anybody else have any suggestions
for this?

Here's an updated patch (zipfile-unsigned-fixes2.diff) that contains
Eric's fixes. I also changed the structure for the Zip64 extension data
to be unsigned. I think this should cover all the deficiencies caused
by the improper use of unsigned values.
Note: if this patch is committed, then issue 1746 should be closed since
it is fixed with this patch.

I just noticed that my changes for issue 1526 are included in this
patch. Eric, if you have time could you have a look at that issue and
see if you think I addressed it properly? If not I can back them out
into a separate patch for that issue.

Eric: Can you review the latest version of this patch?

Sorry for the long delay. Yes, the latest patch looks very good to me.
-Eric

Is there anything else that needs to be addressed before this can be
committed? At the moment I don't know of anything, just wanted to make
sure somebody wasn't waiting on me. 
As a reminder, issues #1526 and #1746 should be closed if this patch is
committed.

Alan, it appears that the patch doesn't apply anymore, in r63553. Can
you please update it?

Sure, I'll look at it later today or over the weekend. I should
probably break out the parts that apply to other issues and update those
patches as well.

Here's an updated patch (zipfile-patch-1622.diff). I didn't get around
to separating the fixes for #1526 and #1746.
All the tests (including test_zipfile64) pass after applying this patch.

Thanks for the patch. This is now committed as r64688