ZipFile.extractall happily overwrites any file on the filesystem. One
can put files with a name like "//etc/password" in a zip file and
extractall will overwrite /etc/password (with sufficient rights).

The docs say:

ZipFile.extractall([path[, members[, pwd]]])

    Extract all members from the archive to the current working
directory. path specifies a different directory to extract to. members
is optional and must be a subset of the list returned by namelist(). pwd
is the password used for encrypted files.


I read that as: it will put all files into path or a subdirectory.
Using names like "../../../etc/password" also leads to files being
written outside that path directory.

Do people have an opinion as to whether this should be fixed with a 
docfix, fixed as default (with option to allow path traversal) or fixed as 
a non-default option? The same issue exists in ZipFile.extract, but in 
that case you're presumably passing a path you've already vetted.

Either way, I'll write a test case for it tomorrow.

I think this should clearly be fixed in the code. The current code tries
to handle absolute paths by removing the first slash (unfortunately not
the second), so it looks like it tries to be safe and only write to the
destination directory. That should be the default operation.
I even think that there should be *no* option to allow overriding files
outside the destination path (on unix one can always use / as
destination if he feels like overwriting his /etc/passwd)
The documentation should also mention that it's unsafe to use this
method in python <2.6.2.

The documentation should also mention that it's unsafe to use this
method in python <= 2.6.2. 2.6.2 is also unsafe.

The tarfile module solved this issue with a documentation warning:
http://docs.python.org/library/tarfile.html#tarfile.TarFile.extractall

My working solution is to iterate through members, and ensuring that
os.path.abspath(os.path.join(path, member)) always .startswith(path).
This seems like a better solution than trying to trap on a pattern in
the string. Presumably the same fix can be made to tarfile.

For what it's worth, OS X's BOMArchiveManager will place a file stored
as '../foo.txt' in the extract path, not the directory right outside it.

While we're on the topic, there may also be a bug in this, or the
tarfile package that would allow a malicious archive to extract a
symlink to an existing directory somewhere on the target machine, and
files extracted to that symlink. I haven't really thought that through,
but I'm sure that my fix won't correct that possible issue.

Uploading test.

Uploading patch. This actually should fix my theoretical symlink bug
since realpath() properly follows symlinks. The only thing that I
haven't been able to test is the behavior of realpath() on
case-insensitive operating systems. This should do the right thing, the
path should be normalized, but can someone confirm this?

As for the documentation, it might be a wise idea to up date the current
documentation to mention this issue, until the next release. I'm not
really sure what the process is for doing that, though...

Patches to the docs, just like patches to the code (the docs are in the
Doc subdirectory).  Once committed, they get auto-generated and uploaded.

Documentation note added (copied from tarfile) in trunk r75149, 
release26-maint r75150 (hopefully in time for 2.6.3 but thats up to 
Barry).

zf.extract() is unsafe for the same reason. My patch fixes this issue,
but we should mention the possible bug in the documentation there as
well. They do this for the similar bug in tarfile.

I've copy/pasted the mention in tarfile.extract() to zipfile.extract()
into the diff.

My apologies, I clicked the wrong button and deleted my test. There is no 
change in the newly uploaded one.

os.path.realpath() doesn't normalize case, so this could have issues on
Windows. The new patch should not.

The Mac version of os.path.normpath doesn't change the path, as per the
posix version, which isn't correct on HFS+, which is not case sensitive.
That's another bug for another ticket, though.

"The Mac version of os.path.normpath doesn't change the path, as per the
posix version, which isn't correct on HFS+, which is not case sensitive."

Not so. Case-sensitive vs case-insensitive behavior is chosen when 
initializing an HFS+ file system (since OS X 10.3).

Good point, I'd forgotten that case-sensitive file systems are an
option. I do know that it's not the default, though, and that as shipped
from Apple, at least the consumer machines are case-insensitive. Things
may be different in server-land.

For what it's worth, NTFS isn't entirely case-insensitive, either.
(http://support.microsoft.com/kb/100625)

Should the os.path.normcase() method be made smarter, and actually query
the filesystem about such things? (If I should start a new ticket for
this, somebody please let me know.)

Yes, as shipped from the factory, the default "root" file system is 
still case-insensitive but the user can change that.  There there are 
file systems on attached disk images and NFS-mounted file systems, etc 
etc.  More to the point, it's not a system attribute, rather it's a 
file-system attribute and, since a file system mount point can be almost 
anywhere in a directory structure, in general, you can't predict where 
you're going to encounter insensitive vs -sensitive behavior; it could 
vary from directory to directory.  But isn't dealing with case-
insensitive behavior just a special case of the general case of what 
extract/extractall do about overwriting existing files?  I don't see 
that addressed in the current docs.

A fair point. I was thinking that we could query the OS about whatever
filesystem the path is on, but this wouldn't work for a file that hasn't
been created yet.

The issue with extractall() isn't just that it can extract over existing
files, it's that it can write files anywhere on the filesystem, both by
exploiting symlinks and through path manipulation. The more I think
about it, though, the more I think the case sensitivity is a non-issue
here, since the trailing part of the extraction paths is built out of
the base path, which I then compare against. The capitalization will
therefore be consistent, and I don't need to worry about this. Does this
seem right?

The patch won't work if the target file already exists as a symlink.

I think that such a check is not a good idea. Using symlinks to extract files 
to somewhere else may be a feature, after all.  Specially if the symlink 
already exists before the operation. Some real-case example:
  /home/xxx/bin   --> symlink to /someotherpath/bin
  /home/xxx/lib   --> symlink to /someotherpath/lib
Now I want to extract "lib/libXXX.so" into "/home/xxx"

I suggest to only update the documentation with a warning, similar to the one 
for the tarfile module.

Adding a warning to the documentation is wrong. The intention of the
code clearly is to only create files in the destination directory (or
why remove the first slash then?) and that is also the impression I get
from reading the documentation.

Adding a warning to the documentation is not wrong, it is the only thing 
that is possible for the 2.6.3 release.  Its too late in the current 
release process to change code.

I'd rather have an extractall version which just throws a RuntimeError
than one which overwrites any file with any content on my filesystem if
I'm trying to unzip a zip file.  Then I at least know that I have to
write my own version. Adding a warning to the documentation instead of
fixing that bug was wrong for the tarfile module and is wrong for the
zipfile module. The "release process" disagrees with me here, but I'd
still call that wrong.

Even if we can't fix things for this release, presumably it's not too
late to fix things for 2.7, right?

Yes, there certainly are cases where you might want to have creative
usage of symlinks and stored paths to allow overwriting existing files,
and placing files outside of the extraction path, but that doesn't seem
like the default case to me. Would it be a decent compromise to add a
extract_under_path=True default option to extract() and extractall() for
2.7?

yes this will be fixed in 2.7/3.2.

as for creative uses where someone might want the out of supplied path 
overwriting behavior?  those people are insane and should be made to jump 
through extra hoops to get it. ;)