Fix vulnerability in shutil.unpack_archive() in ZIP files

Use ZipFile.extractall() to sanitize file names and extract files.

Files with invalid names (e.g. absolute paths) are now extracted with
different names instead of been skipped or written out of the destination
directory.

Files containing ".." in the name are no longer skipped.

Author: serhiy-storchaka

Lib/shutil.py

@@ -1314,27 +1314,8 @@ def _unpack_zipfile(filename, extract_dir):
     if not zipfile.is_zipfile(filename):
         raise ReadError("%s is not a zip file" % filename)
 
-    zip = zipfile.ZipFile(filename)
-    try:
-        for info in zip.infolist():
-            name = info.filename
-
-            # don't extract absolute paths or ones with .. in them
-            if name.startswith('/') or '..' in name:
-                continue
-
-            targetpath = os.path.join(extract_dir, *name.split('/'))
-            if not targetpath:
-                continue
-
-            _ensure_directory(targetpath)
-            if not name.endswith('/'):
-                # file
-                with zip.open(name, 'r') as source, \
-                        open(targetpath, 'wb') as target:
-                    copyfileobj(source, target)
-    finally:
-        zip.close()
+    with zipfile.ZipFile(filename) as zip:
+        zip.extractall(extract_dir)
 
 def _unpack_tarfile(filename, extract_dir, *, filter=None):
     """Unpack tar/tar.gz/tar.bz2/tar.xz/tar.zst `filename` to `extract_dir`

Lib/test/test_shutil.py

@@ -2110,8 +2110,6 @@ def test_make_zipfile_rootdir_nodir(self):
     def check_unpack_archive(self, format, **kwargs):
         self.check_unpack_archive_with_converter(
             format, lambda path: path, **kwargs)
-        self.check_unpack_archive_with_converter(
-            format, FakePath, **kwargs)
         self.check_unpack_archive_with_converter(format, FakePath, **kwargs)
 
     def check_unpack_archive_with_converter(self, format, converter, **kwargs):
@@ -2168,6 +2166,58 @@ def test_unpack_archive_zip(self):
         with self.assertRaises(TypeError):
             self.check_unpack_archive('zip', filter='data')
 
+    def test_unpack_archive_zip_badpaths(self):
+        srcdir = self.mkdtemp()
+        zipname = os.path.join(srcdir, 'test.zip')
+        abspath = os.path.join(srcdir, 'abspath')
+        with zipfile.ZipFile(zipname, 'w') as zf:
+            zf.writestr(abspath, 'badfile')
+            zf.writestr(os.sep + abspath, 'badfile')
+            zf.writestr('/abspath2', 'badfile')
+            if os.name == 'nt':
+                zf.writestr('C:/abspath3', 'badfile')
+                zf.writestr('C:\\abspath4', 'badfile')
+                zf.writestr('C:abspath5', 'badfile')
+                zf.writestr('C:/C:/abspath6', 'badfile')
+            zf.writestr('../relpath', 'badfile')
+            zf.writestr(os.pardir + os.sep + 'relpath2', 'badfile')
+            zf.writestr('good/file', 'goodfile')
+            zf.writestr('good..file', 'goodfile')
+
+        dstdir = os.path.join(self.mkdtemp(), 'dst')
+        unpack_archive(zipname, dstdir)
+        self.assertTrue(os.path.isfile(os.path.join(dstdir, 'good', 'file')))
+        self.assertTrue(os.path.isfile(os.path.join(dstdir, 'good..file')))
+        self.assertFalse(os.path.exists(abspath))
+        self.assertTrue(os.path.exists(os.path.join(dstdir, 'abspath2')))
+        if os.name == 'nt':
+            self.assertTrue(os.path.exists(os.path.join(dstdir, 'abspath3')))
+            self.assertTrue(os.path.exists(os.path.join(dstdir, 'abspath4')))
+            self.assertTrue(os.path.exists(os.path.join(dstdir, 'abspath5')))
+            self.assertTrue(os.path.exists(os.path.join(dstdir, 'C_', 'abspath6')))
+        self.assertFalse(os.path.exists(os.path.join(dstdir, '..', 'relpath')))
+        self.assertTrue(os.path.exists(os.path.join(dstdir, 'relpath')))
+        self.assertFalse(os.path.exists(os.path.join(dstdir, os.pardir, 'relpath2')))
+        self.assertTrue(os.path.exists(os.path.join(dstdir, 'relpath2')))
+
+        dstdir2 = os.path.join(self.mkdtemp(), 'dst')
+        os.mkdir(dstdir2)
+        with os_helper.change_cwd(dstdir2):
+            unpack_archive(zipname, '')
+            self.assertTrue(os.path.isfile(os.path.join('good', 'file')))
+            self.assertTrue(os.path.isfile('good..file'))
+            self.assertFalse(os.path.exists(abspath))
+            self.assertTrue(os.path.exists('abspath2'))
+            if os.name == 'nt':
+                self.assertTrue(os.path.exists('abspath3'))
+                self.assertTrue(os.path.exists('abspath4'))
+                self.assertTrue(os.path.exists('abspath5'))
+                self.assertTrue(os.path.exists(os.path.join('c_', 'abspath6')))
+            self.assertFalse(os.path.exists(os.path.join('..', 'relpath')))
+            self.assertTrue(os.path.exists('relpath'))
+            self.assertFalse(os.path.exists(os.path.join(os.pardir, 'relpath2')))
+            self.assertTrue(os.path.exists('relpath2'))
+
     def test_unpack_registry(self):
 
         formats = get_unpack_formats()