homepage

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

I'll update my patch to work on the current 3.x head later tonight.

> + # make sure the zip file isn't traversing out of the path
> + if not targetpath.startswith(basepath):
Check is insufficient. basepath='/etc/asd', member.filename='../asdfgh'.
The issue10905 has relations with this issue.
P. S. Viewing patches in this issue is not working.

To clarify what Serhiy said about the patches, the link to the patch works, but the Reitveld review button isn't working. I get 'No issue exists with that id (6972)'.

Here is a patch based on patch for issue10905. Test included (I have removed some old tests as new one supersede them). Please test on Windows.
".." components, leading slashes, drive letter, etc are just dropped, as in unzip or 7-Zip. Thanks Zhigang Wang for original patch and research.

Oh, I forgot docs.

Patch updated. Fixed case '../C:/foo' on Windows.

Can anyone review the patch?

the patch looks good, thanks! one minor comment in a test but i'll take care of that as i submit.

Feel free to change the patch as you see fit.

New changeset 0c5fa35c9f12 by Gregory P. Smith in branch '3.2':
Fixes Issue #6972: The zipfile module no longer overwrites files outside of
http://hg.python.org/cpython/rev/0c5fa35c9f12
New changeset 483488a1dec5 by Gregory P. Smith in branch '3.3':
Fixes Issue #6972: The zipfile module no longer overwrites files outside of
http://hg.python.org/cpython/rev/483488a1dec5
New changeset 249e0b47b686 by Gregory P. Smith in branch 'default':
Fixes Issue #6972: The zipfile module no longer overwrites files outside of
http://hg.python.org/cpython/rev/249e0b47b686
New changeset 4d1948689ee1 by Gregory P. Smith in branch '2.7':
Fixes Issue #6972: The zipfile module no longer overwrites files outside of
http://hg.python.org/cpython/rev/4d1948689ee1 

does anyone know if the same issue has been fixed in the tarfile module?

yes, tarfile appears to have the same problem. http://bugs.python.org/issue17102 filed.

New changeset c3ab8a698d2f by Serhiy Storchaka in branch '2.7':
Fix translating of illegal characters on Windows (issue #6972).
http://hg.python.org/cpython/rev/c3ab8a698d2f 

The patch introduced a Cyrillic "C" into the docs, see below. This makes the LaTeX build fail.
+ ``foo/bar`` on Unix, and ``С:\foo\bar`` becomes ``foo\bar`` on Windows.
 ^^

There are different test fails on Windows:
http://buildbot.python.org/all/builders/x86%20XP-5%203.3/builds/405/steps/test/logs/stdio
======================================================================
ERROR: test_extract_hackers_arcnames (test.test_zipfile.TestsWithSourceFile)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "D:\Buildslave3円.3.moore-windows\build\lib\test\test_zipfile.py", line 585, in test_extract_hackers_arcnames
 writtenfile = zipfp.extract(arcname, targetpath)
 File "D:\Buildslave3円.3.moore-windows\build\lib\zipfile.py", line 1212, in extract
 return self._extract_member(member, path, pwd)
 File "D:\Buildslave3円.3.moore-windows\build\lib\zipfile.py", line 1253, in _extract_member
 os.makedirs(upperdirs)
 File "D:\Buildslave3円.3.moore-windows\build\lib\os.py", line 269, in makedirs
 mkdir(name, mode)
FileNotFoundError: [WinError 3] The system cannot find the path specified: 'target\\subdir\\subsub\\foo'
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.3/builds/428/steps/test/logs/stdio
======================================================================
FAIL: test_extract_hackers_arcnames (test.test_zipfile.TestsWithSourceFile)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "C:\buildbot.python.org3円.3.kloth-win64\build\lib\test\test_zipfile.py", line 586, in test_extract_hackers_arcnames
 self.assertEqual(writtenfile, correctfile)
AssertionError: 'target\\subdir\\subsub' != 'target\\subdir\\subsub\\foo\\bar'
- target\subdir\subsub
+ target\subdir\subsub\foo\bar
? ++++++++

New changeset b6b707063991 by Serhiy Storchaka in branch '2.7':
Fix a Cyrillic "C" inroduced into the docs by patch for issue #6972.
http://hg.python.org/cpython/rev/b6b707063991
New changeset ede0f27988f2 by Serhiy Storchaka in branch '3.2':
Fix a Cyrillic "C" inroduced into the docs by patch for issue #6972.
http://hg.python.org/cpython/rev/ede0f27988f2
New changeset 785b8b49c3bf by Serhiy Storchaka in branch '3.3':
Fix a Cyrillic "C" inroduced into the docs by patch for issue #6972.
http://hg.python.org/cpython/rev/785b8b49c3bf
New changeset 25294188c4ea by Serhiy Storchaka in branch 'default':
Fix a Cyrillic "C" inroduced into the docs by patch for issue #6972.
http://hg.python.org/cpython/rev/25294188c4ea 

> The patch introduced a Cyrillic "C" into the docs, see below.
Thank you. Fixed.

Here are patches which possible fixes some of this failures.

New changeset ebef003a2acd by Serhiy Storchaka in branch '2.7':
Fix the test and remove trailing dots on Windows for issue #6972.
http://hg.python.org/cpython/rev/ebef003a2acd 

New changeset 5a68052b52ea by Serhiy Storchaka in branch '2.7':
Preserve backslashes in malicious zip files for testing issue #6972.
http://hg.python.org/cpython/rev/5a68052b52ea 

New changeset ab4b8da79a5f by Serhiy Storchaka in branch '2.7':
Fix test for issue #6972.
http://hg.python.org/cpython/rev/ab4b8da79a5f 

New changeset 434b50c7bbed by Serhiy Storchaka in branch '3.2':
Fix the test for issue #6972.
http://hg.python.org/cpython/rev/434b50c7bbed
New changeset 8b33f3a4a200 by Serhiy Storchaka in branch '3.3':
Fix the test for issue #6972.
http://hg.python.org/cpython/rev/8b33f3a4a200
New changeset 7a47fd7f2fdc by Serhiy Storchaka in branch 'default':
Fix the test for issue #6972.
http://hg.python.org/cpython/rev/7a47fd7f2fdc 

I believe this is all done after Serhiy's fixes.

There are 2 issues with the documentation changes introduced by these patches.
1. for 2.7, the note added by the doc patch is in the wrong place, at the setpassword method instead of the extract or extractall method
2. for 3.x the "Never extract archives from untrusted sources..." warning got removed but it's still useful for users that read the documentation online and therefore get the updated docs but haven't updated Python to the latest patch release and therefore don't have the fix. For example, anybody reading the docs for 3.2 or 3.3 today doesn't see that extractall is dangerous and there is no released Python containing the fix so by all practical means extractall is still dangerous today.
To address point 2, I think the warning should be kept with an extra mention regarding exact version where it got fixed so that, when reading the documentation, everybody can assess exactly whether extractall is safe for them to use or not.
I can't reopen the bug since I don't have tracker privileges but since it's a security issue I think it's important for these to get addressed.

reopening as documentation mixups remain to be fixed.

New changeset d73fb6b06891 by Gregory P. Smith in branch '2.7':
Issue #6972: fix the documentation mis applied patch.
http://hg.python.org/cpython/rev/d73fb6b06891
New changeset 1c2d41850147 by Gregory P. Smith in branch '3.2':
Issue #6972: keep the warning about untrusted extraction and mention
http://hg.python.org/cpython/rev/1c2d41850147
New changeset 5fbca37de9b1 by Gregory P. Smith in branch '3.3':
Issue #6972: keep the warning about untrusted extraction and mention
http://hg.python.org/cpython/rev/5fbca37de9b1
New changeset f5e3f2f0fe79 by Gregory P. Smith in branch 'default':
Issue #6972: keep the warning about untrusted extraction and mention
http://hg.python.org/cpython/rev/f5e3f2f0fe79