zipfile.ZipFile overwrites files outside destination path

@loewis

BPO	6972
Nosy	@loewis, @birkenfeld, @gpshead, @amauryfa, @larryhastings, @benjaminp, @ned-deily, @bitdancer, @serhiy-storchaka
Files	extract-doc.diff: Update to extract() method documentation zipfile-6972-test.diff: test for directory escape zipfile-6972-patch-2.diff: patch to zipfile module for directory escape, with fix for case sensitive file systems zipfile_fix_arcname_3.patch: Strip leading absolute name part and ".." components zipfile_fix_arcname_4-2.7.patch zipfile_fix_arcname_4-3.x.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 2013-02-08.06:19:09.044>
created_at = <Date 2009-09-22.22:10:50.564>
labels = ['type-security', 'library', 'release-blocker']
title = 'zipfile.ZipFile overwrites files outside destination path'
updated_at = <Date 2013-02-08.20:04:05.696>
user = 'https://bugs.python.org/schmir'

bugs.python.org fields:

activity = <Date 2013-02-08.20:04:05.696>
actor = 'Arfrever'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2013-02-08.06:19:09.044>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2009-09-22.22:10:50.564>
creator = 'schmir'
dependencies = []
files = ['15004', '15005', '15009', '27686', '28932', '28933']
hgrepos = []
issue_num = 6972
keywords = ['patch', 'needs review']
message_count = 50.0
messages = ['93021', '93251', '93268', '93269', '93278', '93305', '93330', '93331', '93332', '93333', '93334', '93336', '93349', '93372', '93374', '93375', '93376', '93379', '93380', '93381', '93382', '93396', '93420', '93426', '157763', '157778', '157789', '173526', '173567', '173630', '180681', '181055', '181072', '181102', '181106', '181134', '181158', '181159', '181161', '181162', '181163', '181166', '181171', '181178', '181183', '181188', '181244', '181646', '181647', '181659']
nosy_count = 14.0
nosy_names = ['loewis', 'georg.brandl', 'gregory.p.smith', 'amaury.forgeotdarc', 'larry', 'schmir', 'benjamin.peterson', 'ned.deily', 'Arfrever', 'r.david.murray', 'twb', 'catalin.iacob', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue6972'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']

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

New changeset ebef003a2acd by Serhiy Storchaka in branch '2.7':
Fix the test and remove trailing dots on Windows for issue bpo-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 bpo-6972.
http://hg.python.org/cpython/rev/5a68052b52ea

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

New changeset 434b50c7bbed by Serhiy Storchaka in branch '3.2':
Fix the test for issue bpo-6972.
http://hg.python.org/cpython/rev/434b50c7bbed

New changeset 8b33f3a4a200 by Serhiy Storchaka in branch '3.3':
Fix the test for issue bpo-6972.
http://hg.python.org/cpython/rev/8b33f3a4a200

New changeset 7a47fd7f2fdc by Serhiy Storchaka in branch 'default':
Fix the test for issue bpo-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.

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
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 bpo-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 bpo-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 bpo-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 bpo-6972: keep the warning about untrusted extraction and mention
http://hg.python.org/cpython/rev/f5e3f2f0fe79

Navigation Menu

Repository navigation

zipfile.ZipFile overwrites files outside destination path #51221

33 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

zipfile.ZipFile overwrites files outside destination path #51221

Description

Activity

schmir commented on Sep 22, 2009

twb commented on Sep 29, 2009

schmir commented on Sep 29, 2009

schmir commented on Sep 29, 2009

amauryfa commented on Sep 29, 2009

twb commented on Sep 29, 2009

twb commented on Sep 29, 2009

twb commented on Sep 29, 2009

twb commented on Sep 29, 2009

bitdancer commented on Sep 29, 2009

gpshead commented on Sep 29, 2009

33 remaining items

python-dev commented on Feb 2, 2013

python-dev commented on Feb 2, 2013

python-dev commented on Feb 2, 2013

python-dev commented on Feb 2, 2013

gpshead commented on Feb 3, 2013

cataliniacob commented on Feb 7, 2013

gpshead commented on Feb 7, 2013

python-dev commented on Feb 8, 2013

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions