classification
Title: zipfile: fix arcname with leading '///' or '..'
Type: security Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: duplicate
Dependencies: Superseder: zipfile.ZipFile overwrites files outside destination path
View: 6972
Assigned To: Nosy List: amaury.forgeotdarc, serhiy.storchaka, zhigang
Priority: normal Keywords: patch

Created on 2011-01-14 12:03 by zhigang, last changed 2013-01-26 18:16 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
python-zipfile-fix-arcname.patch zhigang, 2011-01-14 12:03
Messages (6)
msg126254 - (view) Author: Zhigang Wang (zhigang) Date: 2011-01-14 12:03
We only support arcname with one leading '/', but not more. This patch fixes it.

We don't support arcname with '..' well. The default behavior of unzip and 7z is to ignore all '..'. This patch does the same.

Also updated the doc. If there are other security related issues exist, we should revise the doc.

Please review.
msg126255 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2011-01-14 12:20
What happens when the archive contains both 'foo' and '../foo'? They seem to be extracted at the same place.
msg126258 - (view) Author: Zhigang Wang (zhigang) Date: 2011-01-14 13:20
$ unzip -l t.zip 
Archive:  t.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        3  01-14-2011 21:11   ../foo
        3  01-14-2011 21:11   foo
---------                     -------
        6                     2 files
[zhigang@localhost tmp]$ unzip -d aa t.zip
Archive:  t.zip
warning:  skipped "../" path component(s) in ../foo
 extracting: aa/foo                  
replace aa/foo? [y]es, [n]o, [A]ll, [N]one, [r]ename: A
 extracting: aa/foo                  


$ 7za x -oaa t.zip 

7-Zip (A) 9.13 beta  Copyright (c) 1999-2010 Igor Pavlov  2010-04-15
p7zip Version 9.13 (locale=en_US.UTF-8,Utf16=on,HugeFiles=on,1 CPU)

Processing archive: t.zip

file aa/foo
already exists. Overwrite with 
../foo?
(Y)es / (N)o / (A)lways / (S)kip all / A(u)to rename all / (Q)uit? A
Extracting  ../foo
Extracting  foo

Everything is Ok

Files: 2
Size:       6
Compressed: 198
msg126259 - (view) Author: Zhigang Wang (zhigang) Date: 2011-01-14 13:29
Yes, in zipfile, we just overwrite it. Actually, ZipFile.extract() overwrite existing files already. If we want it more powerful, we can add a 'overwrite' parameter. But turning zipfile a full featured zip/unzip tool needs much more extra work...
msg173478 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-21 20:16
Some comments to patch.

+        arcname = os.path.sep.join([x for x in arcname.split(os.path.sep)
+                                    if x != '..'])

File names in zip archive should use '/' as separator, not os.path.sep. '../spam' will be not cleaned by this code.

+        while arcname[0] in (os.sep, os.altsep):
+            arcname = arcname[1:]

It will not save from filenames containing drive letter: 'C:/Windows/python.exe'.
msg173525 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-22 14:44
I'm going to close this issue as a duplicate of issue6972. Issue6972 is older and has a larger discussion.

Thank you for patch and research, Zhigang Wang. I will use it for the new patch.
History
Date User Action Args
2013-01-26 18:16:31serhiy.storchakasetstatus: pending -> closed
stage: resolved
2012-10-22 14:44:47serhiy.storchakasetstatus: open -> pending
superseder: zipfile.ZipFile overwrites files outside destination path
resolution: duplicate
messages: + msg173525
2012-10-21 20:16:28serhiy.storchakasetmessages: + msg173478
2012-04-07 19:17:25serhiy.storchakasetnosy: + serhiy.storchaka
2011-01-14 13:29:47zhigangsetmessages: + msg126259
2011-01-14 13:20:50zhigangsetmessages: + msg126258
2011-01-14 12:20:30amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg126255
2011-01-14 12:03:45zhigangcreate