From 7c5ad11510c5d31e8f18fd61bef21638c61ed314 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 27 Feb 2026 12:11:15 +0200 Subject: [PATCH 1/4] 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. --- Lib/shutil.py | 23 ++---------------- Lib/test/test_shutil.py | 54 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 8d8fe145567822..c483320d4b2493 100644 --- a/Lib/shutil.py +++ b/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` diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index ebb6cf88336249..dead8f11258851 100644 --- a/Lib/test/test_shutil.py +++ b/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() From 8d0ade51ba0ed9642701fb4831c8f429fc923452 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 27 Feb 2026 17:20:17 +0200 Subject: [PATCH 2/4] Preserve the current behavior whether it is possible. --- Lib/shutil.py | 1 + Lib/test/test_shutil.py | 61 +++++++++++++++++++++++++---------------- Lib/zipfile/__init__.py | 21 ++++++++++---- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index c483320d4b2493..b7608f7edfc93c 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -1315,6 +1315,7 @@ def _unpack_zipfile(filename, extract_dir): raise ReadError("%s is not a zip file" % filename) with zipfile.ZipFile(filename) as zip: + zip._ignore_invalid_names = True zip.extractall(extract_dir) def _unpack_tarfile(filename, extract_dir, *, filter=None): diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index dead8f11258851..a1d61ed9dd19cf 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -2173,12 +2173,13 @@ def test_unpack_archive_zip_badpaths(self): 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('/abspath', 'badfile') + zf.writestr('C:/abspath', 'badfile') + zf.writestr('D:\\abspath', 'badfile') + zf.writestr('E:abspath', 'badfile') + zf.writestr('F:/G:/abspath', 'badfile') + zf.writestr('//server/share/abspath', 'badfile') + zf.writestr('\\\\server2\\share\\abspath', 'badfile') zf.writestr('../relpath', 'badfile') zf.writestr(os.pardir + os.sep + 'relpath2', 'badfile') zf.writestr('good/file', 'goodfile') @@ -2189,16 +2190,22 @@ def test_unpack_archive_zip_badpaths(self): 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, 'abspath'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'G_'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'server'))) + if os.name != 'nt': + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'C:', 'abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'D:\\abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'E:abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'F:', 'G:', 'abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, '\\\\server2\\share\\abspath'))) + if os.pardir == '..': + self.assertFalse(os.path.exists(os.path.join(dstdir, '..', 'relpath'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'relpath'))) + else: + self.assertTrue(os.path.isfile(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'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'relpath2'))) dstdir2 = os.path.join(self.mkdtemp(), 'dst') os.mkdir(dstdir2) @@ -2207,16 +2214,22 @@ def test_unpack_archive_zip_badpaths(self): 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('abspath')) + self.assertFalse(os.path.exists('C_')) + self.assertFalse(os.path.exists('server')) + if os.name != 'nt': + self.assertTrue(os.path.isfile(os.path.join('C:', 'abspath'))) + self.assertTrue(os.path.isfile('D:\\abspath')) + self.assertTrue(os.path.isfile('E:abspath')) + self.assertTrue(os.path.isfile(os.path.join('F:', 'G:', 'abspath'))) + self.assertTrue(os.path.isfile('\\\\server2\\share\\abspath')) + if os.pardir == '..': + self.assertFalse(os.path.exists(os.path.join('..', 'relpath'))) + self.assertFalse(os.path.exists('relpath')) + else: + self.assertTrue(os.path.isfile(os.path.join('..', 'relpath'))) self.assertFalse(os.path.exists(os.path.join(os.pardir, 'relpath2'))) - self.assertTrue(os.path.exists('relpath2')) + self.assertFalse(os.path.exists('relpath2')) def test_unpack_registry(self): diff --git a/Lib/zipfile/__init__.py b/Lib/zipfile/__init__.py index 51e0ce9fa36d7e..1e0cc5f6234f28 100644 --- a/Lib/zipfile/__init__.py +++ b/Lib/zipfile/__init__.py @@ -1410,6 +1410,7 @@ class ZipFile: fp = None # Set here since __del__ checks it _windows_illegal_name_trans_table = None + _ignore_invalid_names = False def __init__(self, file, mode="r", compression=ZIP_STORED, allowZip64=True, compresslevel=None, *, strict_timestamps=True, metadata_encoding=None): @@ -1890,21 +1891,31 @@ def _extract_member(self, member, targetpath, pwd): # build the destination pathname, replacing # forward slashes to platform specific separators. - arcname = member.filename.replace('/', os.path.sep) - - if os.path.altsep: + arcname = member.filename + if os.path.sep != '/': + arcname = arcname.replace('/', os.path.sep) + if os.path.altsep and os.path.altsep != '/': arcname = arcname.replace(os.path.altsep, os.path.sep) # interpret absolute pathname as relative, remove drive letter or # UNC path, redundant separators, "." and ".." components. - arcname = os.path.splitdrive(arcname)[1] + drive, root, arcname = os.path.splitroot(arcname) + if self._ignore_invalid_names and (drive or root): + return None + if self._ignore_invalid_names and os.path.pardir in arcname.split(os.path.sep): + return None invalid_path_parts = ('', os.path.curdir, os.path.pardir) arcname = os.path.sep.join(x for x in arcname.split(os.path.sep) if x not in invalid_path_parts) if os.path.sep == '\\': # filter illegal characters on Windows - arcname = self._sanitize_windows_name(arcname, os.path.sep) + arcname2 = self._sanitize_windows_name(arcname, os.path.sep) + if self._ignore_invalid_names and arcname2 != arcname: + return None + arcname = arcname2 if not arcname and not member.is_dir(): + if self._ignore_invalid_names: + return None raise ValueError("Empty filename.") targetpath = os.path.join(targetpath, arcname) From 79400843e5e0cfbd64d6b466bfdd9d1a6461dd1f Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 27 Feb 2026 17:20:17 +0200 Subject: [PATCH 3/4] Preserve the current behavior whether it is possible. --- Lib/shutil.py | 1 + Lib/test/test_shutil.py | 63 +++++++++++++++++++++++++---------------- Lib/zipfile/__init__.py | 21 ++++++++++---- 3 files changed, 56 insertions(+), 29 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index c483320d4b2493..b7608f7edfc93c 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -1315,6 +1315,7 @@ def _unpack_zipfile(filename, extract_dir): raise ReadError("%s is not a zip file" % filename) with zipfile.ZipFile(filename) as zip: + zip._ignore_invalid_names = True zip.extractall(extract_dir) def _unpack_tarfile(filename, extract_dir, *, filter=None): diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index dead8f11258851..6b87f52d6e0885 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -2173,12 +2173,13 @@ def test_unpack_archive_zip_badpaths(self): 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('/abspath', 'badfile') + zf.writestr('C:/abspath', 'badfile') + zf.writestr('D:\\abspath', 'badfile') + zf.writestr('E:abspath', 'badfile') + zf.writestr('F:/G:/abspath', 'badfile') + zf.writestr('//server/share/abspath', 'badfile') + zf.writestr('\\\\server2\\share\\abspath', 'badfile') zf.writestr('../relpath', 'badfile') zf.writestr(os.pardir + os.sep + 'relpath2', 'badfile') zf.writestr('good/file', 'goodfile') @@ -2189,16 +2190,23 @@ def test_unpack_archive_zip_badpaths(self): 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, 'abspath'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'G_'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'server'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'server2'))) + if os.name != 'nt': + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'C:', 'abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'D:\\abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'E:abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, 'F:', 'G:', 'abspath'))) + self.assertTrue(os.path.isfile(os.path.join(dstdir, '\\\\server2\\share\\abspath'))) + if os.pardir == '..': + self.assertFalse(os.path.exists(os.path.join(dstdir, '..', 'relpath'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'relpath'))) + else: + self.assertTrue(os.path.isfile(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'))) + self.assertFalse(os.path.exists(os.path.join(dstdir, 'relpath2'))) dstdir2 = os.path.join(self.mkdtemp(), 'dst') os.mkdir(dstdir2) @@ -2207,16 +2215,23 @@ def test_unpack_archive_zip_badpaths(self): 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('abspath')) + self.assertFalse(os.path.exists('C_')) + self.assertFalse(os.path.exists('server')) + self.assertFalse(os.path.exists('server2')) + if os.name != 'nt': + self.assertTrue(os.path.isfile(os.path.join('C:', 'abspath'))) + self.assertTrue(os.path.isfile('D:\\abspath')) + self.assertTrue(os.path.isfile('E:abspath')) + self.assertTrue(os.path.isfile(os.path.join('F:', 'G:', 'abspath'))) + self.assertTrue(os.path.isfile('\\\\server2\\share\\abspath')) + if os.pardir == '..': + self.assertFalse(os.path.exists(os.path.join('..', 'relpath'))) + self.assertFalse(os.path.exists('relpath')) + else: + self.assertTrue(os.path.isfile(os.path.join('..', 'relpath'))) self.assertFalse(os.path.exists(os.path.join(os.pardir, 'relpath2'))) - self.assertTrue(os.path.exists('relpath2')) + self.assertFalse(os.path.exists('relpath2')) def test_unpack_registry(self): diff --git a/Lib/zipfile/__init__.py b/Lib/zipfile/__init__.py index 51e0ce9fa36d7e..1e0cc5f6234f28 100644 --- a/Lib/zipfile/__init__.py +++ b/Lib/zipfile/__init__.py @@ -1410,6 +1410,7 @@ class ZipFile: fp = None # Set here since __del__ checks it _windows_illegal_name_trans_table = None + _ignore_invalid_names = False def __init__(self, file, mode="r", compression=ZIP_STORED, allowZip64=True, compresslevel=None, *, strict_timestamps=True, metadata_encoding=None): @@ -1890,21 +1891,31 @@ def _extract_member(self, member, targetpath, pwd): # build the destination pathname, replacing # forward slashes to platform specific separators. - arcname = member.filename.replace('/', os.path.sep) - - if os.path.altsep: + arcname = member.filename + if os.path.sep != '/': + arcname = arcname.replace('/', os.path.sep) + if os.path.altsep and os.path.altsep != '/': arcname = arcname.replace(os.path.altsep, os.path.sep) # interpret absolute pathname as relative, remove drive letter or # UNC path, redundant separators, "." and ".." components. - arcname = os.path.splitdrive(arcname)[1] + drive, root, arcname = os.path.splitroot(arcname) + if self._ignore_invalid_names and (drive or root): + return None + if self._ignore_invalid_names and os.path.pardir in arcname.split(os.path.sep): + return None invalid_path_parts = ('', os.path.curdir, os.path.pardir) arcname = os.path.sep.join(x for x in arcname.split(os.path.sep) if x not in invalid_path_parts) if os.path.sep == '\\': # filter illegal characters on Windows - arcname = self._sanitize_windows_name(arcname, os.path.sep) + arcname2 = self._sanitize_windows_name(arcname, os.path.sep) + if self._ignore_invalid_names and arcname2 != arcname: + return None + arcname = arcname2 if not arcname and not member.is_dir(): + if self._ignore_invalid_names: + return None raise ValueError("Empty filename.") targetpath = os.path.join(targetpath, arcname) From 12cf7350f1192e3ffee32add81c666229bf0b92e Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 29 Mar 2026 12:51:37 +0300 Subject: [PATCH 4/4] Add a NEWS entry. --- .../Security/2026-03-29-12-51-33.gh-issue-146581.4vZfB0.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 Misc/NEWS.d/next/Security/2026-03-29-12-51-33.gh-issue-146581.4vZfB0.rst diff --git a/Misc/NEWS.d/next/Security/2026-03-29-12-51-33.gh-issue-146581.4vZfB0.rst b/Misc/NEWS.d/next/Security/2026-03-29-12-51-33.gh-issue-146581.4vZfB0.rst new file mode 100644 index 00000000000000..98e65549d79016 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2026-03-29-12-51-33.gh-issue-146581.4vZfB0.rst @@ -0,0 +1,5 @@ +Fix vulnerability in :func:`shutil.unpack_archive` for ZIP files on Windows +which allowed to write files outside of the destination tree if the patch in +the archive contains a Windows drive prefix. Now such invalid paths will be +skipped. Files containing ".." in the name (like "foo..bar") are no longer +skipped.