gh-146581: Fix vulnerability in shutil.unpack_archive() for ZIP files on Windows - #146591
Conversation
Sorry, something went wrong.
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.
…ve-extractall' into shutil-unpack_archive-extractall
|
Wasn't os.path.splitroot() introduced in 3.12? if it is backported wouldn't it crash with AttributeError? |
Sorry, something went wrong.
|
Also, the new os.path.pardir check misses backslash based traversals on Linux/macOS. |
Sorry, something went wrong.
Backports will be fixed to use equivalent code.
Backslash is not a separator on Posix. It is a legal character which has no special meaning. |
Sorry, something went wrong.
|
I believe it’s actually a regression. Currently, shutil uses a substring check which correctly catches and skips these backslash traversals on Linux. In this PR it switches to a component based check, which removes that existing protection. So I believe this actually reduces the existing security strictness for non windows users |
Sorry, something went wrong.
|
There is no backslash traversal on Linux. |
Sorry, something went wrong.
|
|
||
| fp = None # Set here since __del__ checks it | ||
| _windows_illegal_name_trans_table = None | ||
| _ignore_invalid_names = False |
There was a problem hiding this comment.
Is this intended to be set by users needing back-compat as well? Should we document it (carefully)?
Sorry, something went wrong.
There was a problem hiding this comment.
It is intended for internal use in shutil.unpack_archive() only.
We could add it as optional parameter, but this does not look like a good long term solution. Even if we make the parameter name underscored, it will be visible in the help, and will confuse users. Users will start to use it, so it will be difficult to get rid of it.
I think that in future versions we can add a hook which would be called for all entry names. It would allow to translate them to valid paths, allow to skip them (with possible logging) or raise an exception. But this is a new feature, it cannot be backported. And it needs a separate discussion.
Sorry, something went wrong.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11, 3.12, 3.13, 3.14. |
Sorry, something went wrong.
|
GH-149064 is a backport of this pull request to the 3.14 branch. |
Sorry, something went wrong.
|
GH-149065 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict. cherry_picker fc829e88753858c8ac669594bf0093f44948c0f4 3.11 |
Sorry, something went wrong.
|
GH-149066 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict. cherry_picker fc829e88753858c8ac669594bf0093f44948c0f4 3.10 |
Sorry, something went wrong.
|
GH-149071 is a backport of this pull request to the 3.11 branch. |
Sorry, something went wrong.
…P files on Windows (GH-146591) (GH-149065) Use ZipFile.extractall() to sanitize file names and extract files. Files with invalid names (e.g. absolute paths) are now skipped. Files containing ".." in the name are no longer skipped. (cherry picked from commit fc829e8) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…P files on Windows (GH-146591) (GH-149064) Use ZipFile.extractall() to sanitize file names and extract files. Files with invalid names (e.g. absolute paths) are now skipped. Files containing ".." in the name are no longer skipped. (cherry picked from commit fc829e8) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Introduced in python#8295 alongside shutil.unpack_archive() as a helper that created the parent directory of each extracted member. Orphaned by pythongh-146581 (pythonGH-146591), the ZIP path-traversal fix, which reworked _unpack_zipfile() to create directories inline and deleted the last _ensure_directory(targetpath) call site in the same change. Since then the private helper has had no caller: a word-boundary search across Lib, Modules, Python, Objects and Include finds zero references outside its own definition, and a GitHub code search finds no downstream importers.
Introduced in python#8295 alongside shutil.unpack_archive() as a helper that created the parent directory of each extracted member. Orphaned by pythongh-146581 (pythonGH-146591), the ZIP path-traversal fix, which reworked _unpack_zipfile() to create directories inline and deleted the last _ensure_directory(targetpath) call site in the same change. Since then the private helper has had no caller: a word-boundary search across Lib, Modules, Python, Objects and Include finds zero references outside its own definition, and a GitHub code search finds no downstream importers.
) No longer used after pythongh-146581 (pythonGH-146591).
…P files on Windows (GH-146591) (#149066) gh-146581: Fix vulnerability in shutil.unpack_archive() for ZIP files on Windows (GH-146591) Use ZipFile.extractall() to sanitize file names and extract files. Files with invalid names (e.g. absolute paths) are now skipped. Files containing ".." in the name are no longer skipped. (cherry picked from commit fc829e8) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…P files on Windows (GH-146591) (#149071) gh-146581: Fix vulnerability in shutil.unpack_archive() for ZIP files on Windows (GH-146591) Use ZipFile.extractall() to sanitize file names and extract files. Files with invalid names (e.g. absolute paths) are now skipped. Files containing ".." in the name are no longer skipped. (cherry picked from commit fc829e8)
…P files on Windows (GH-146591) (GH-149071) (#155440) [3.11] gh-146581: Fix vulnerability in shutil.unpack_archive() for ZIP files on Windows (GH-146591) (GH-149071) gh-146581: Fix vulnerability in shutil.unpack_archive() for ZIP files on Windows (GH-146591) Use ZipFile.extractall() to sanitize file names and extract files. Files with invalid names (e.g. absolute paths) are now skipped. Files containing ".." in the name are no longer skipped. (cherry picked from commit 7ef7dd0) (cherry picked from commit fc829e8) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Use ZipFile.extractall() to sanitize file names and extract files.
Files with invalid names (e.g. absolute paths) are now skipped.
Files containing ".." in the name are no longer skipped.