GeezerVison is showing github.com — [Security] CVE-2007-4559: tarfile: Add absolute_path option to tarfile, disabled by default · Issue #73974 · python/cpython
[Security] CVE-2007-4559: tarfile: Add absolute_path option to tarfile, disabled by default #73974Description
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 = None
closed_at = None
created_at = <Date 2017-03-10.16:13:44.639>
labels = ['type-security', 'library', '3.11']
title = '[Security] tarfile: Add absolute_path option to tarfile, disabled by default'
updated_at = <Date 2021-05-21.23:25:06.720>
user = 'https://github.com/vstinner' bugs.python.org fields:
activity = <Date 2021-05-21.23:25:06.720>
actor = 'ned.deily'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2017-03-10.16:13:44.639>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 29788
keywords = []
message_count = 2.0
messages = ['289388', '289437']
nosy_count = 5.0
nosy_names = ['lars.gustaebel', 'vstinner', 'jwilk', 'berker.peksag', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue29788'
versions = ['Python 3.11'] Reacted by Maciej "RooTer" Urbański, chai3, Jhong-Ken Chen and davus Activityvstinner commented on Mar 10, 2017 I noticed that "python3 -m tarfile -x archive.tar" uses absolute paths by default, whereas the UNIX tar command doesn't by default. The UNIX tar command requires to add explicitly --absolute-paths (-P) option.
I suggest to add a boolean absolute_path option to tarfile, disabled by default.
Example to create such archive. See that tar also removes "/" by default and requires to pass explicitly -P:
$ cd $HOME
# /home/haypo
$ echo TEST > test
$ tar -cf test.tar /home/haypo/test
tar: Removing leading `/' from member names
$ rm -f test.tar
$ tar -P -cf test.tar /home/haypo/test
$ rm -f test Extracting such archive using tar is safe *by default*:
$ mkdir z
$ cd z
$ tar -xf ~/test.tar
tar: Removing leading `/' from member names
$ find
.
./home
./home/haypo
./home/haypo/test Extracting such archive using Python is unsafe:
$ python3 -m tarfile -e ~/test.tar
$ cat ~/test
TEST
$ pwd
/home/haypo/z Python creates files outside the current directory which is unsafe, wheras tar doesn't. Reacted by Christian Buhtz changed the title [-]Add absolute_path option to tarfile, disabled by default[/-] [+]tarfile: Add absolute_path option to tarfile, disabled by default[/+] on Mar 10, 2017vadmium commented on Mar 11, 2017 The CLI was added in bpo-13477. I didn’t see any discussion of traversal attacks there, so maybe it was overlooked. Perhaps there should also be a warning, like with the Tarfile.extract and “extractall” methods.
However I did see one of the goals was to keep the CLI simple, which I agree with. I would suggest that the CLI get this proposed behaviour by default (matching the default behaviour of modern “tar” commands), with no option to restore the current less-robust behaviour.
To implement it, I suggest to fix the module internals first: bpo-21109 and/or bpo-17102.
FWIW BSD calls the option “--absolute-paths” (plural paths) <https://www.freebsd.org/cgi/man.cgi?tar%281%29#OPTIONS\>, while Gnu calls it “--absolute-names” <https://www.gnu.org/software/tar/manual/html_chapter/tar_6.html#SEC121\>. Both these options disable other checks, such as for parent directories (..) and external symbolic link targets, so I think the term “absolute” is too specific. But please use at least replace the underscore with a dash or hyphen: “--absolute-path”, not “--absolute_path”. Reacted by Bruno Heremans changed the title [-]tarfile: Add absolute_path option to tarfile, disabled by default[/-] [+][Security] tarfile: Add absolute_path option to tarfile, disabled by default[/+] on May 29, 2018changed the title [-][Security] tarfile: Add absolute_path option to tarfile, disabled by default[/-] [+][Security] CVE-2007-4559: tarfile: Add absolute_path option to tarfile, disabled by default[/+] on Sep 23, 2022jowagner commented on Sep 23, 2022 Questions:
- Can we use https://bugs.python.org/file8339/insecure_pathnames.diff (found in issue tarfile insecure pathname extraction #45385) as a basis for a PR? How do we give credit if @gustaebel is not the contributor but just another user with the same surname?
- Can we use the abspath + commonprefix approach from https://gist.github.com/Kasimir123/9bdc38f2eac9a19f83ef6cc52c7ce82e?
- Changing behaviour can break existing software, e.g. restoring a backup that uses absolute paths. How much notice should be given in this case? On the one hand, the issue should be fixed soon as it impacts security. On the other hand, what are a few years more after 15 years? What is the process for ensuring the defaults are changed for a specific target version or year?
absolute-path is not a good name if we also test for .. path components.- In the safe mode, should we (a) bail out as soon as an absolute path is encountered or (b) only if the absolute path brings us outside the hierarchy of the target directory (the current working directory by default), or (c) only if the effective target directory is not '/', or (d) only if '/' is not explicitly specified as the target directory (parameter
path of the extract functions)? Consider '/etc/passwd' in a tar file. Should this file allowed to be written in safe mode when inside '/etc'? Should it be ok if running from the top-level '/' root folder? Should it only work if path='/' is set when calling an extract function of the tarfile module? The latter is something we could ask developers of restore utilities to do to continue being able to restore backups produced with absolute paths (and this would also be compatible with future backups stripping the leading /).
- The patch in issue tarfile insecure pathname extraction #45385 suggests a parameter
check_path. While this naming does not make explicit what is being checked, it can cover all future security issues arising from tarfile member names. Or would it be better to have a separate parameter for each check?
- Should the idea to allow the user to provide a function with
check_path that returns True/False for whether to skip the entry (if an exception is desired the function can raise it - no need to define a return value for that) or even allows to change the path (returning a string) be implemented in the same PR or should this be a separate PR?
- Could a specially crafted tar file cause the tarfile module to create a symlink to a directory outside the target tree, e.g.
example --> / and then contain a member example/etc/passwd that overwrites /etc/passwd? To protect against this, one could call realpath() but this would also evaluate pre-existing symlinks. Users may expect the old behaviour e.g. when receiving updates via tar files for multiple folders and one of the folders has been moved to a different filesystem on the receiving system. To satisfy the expectation of these users and being safe, we need a version of realpath() that does not consult the actual filesystem but only the folders and symlinks found in the tar file so far. [Edit: Yes, see https://github.com/tarfile: Traversal attack vulnerability #65308#issuecomment-1093650963]
zhuofeng6 commented on Sep 24, 2022 Which versions are affected by this CVE? nanonyme commented on Sep 24, 2022 To be honest, whenever path is non-empty, I would expect it to be regarded "safe" to the extent that any final path must be contained inside the path user gives. So essentially passing relative paths would be fine as long as it does not escape the directory given by the user of the API. So eg I give path "/", then tarfile is allowed to unpack anywhere. I give path "", then there is no checking whatsoever (for backwards compat reasons). When I give path "something-else", then trying to unpack outside "something-else" raises an exception. mandolaerik commented on Nov 28, 2022 Note that the symlink trick can be applied also to file symlinks; the tar file created by:
import tarfile
from pathlib import Path
with tarfile.open('foo.tar', 'w') as tf:
ti = tarfile.TarInfo('foo')
ti.type = tarfile.SYMTYPE
ti.linkname = '../../../etc/passwd'
tf.addfile(ti)
Path('foo').write_text('garbage')
tf.add('foo')
will try to write passwd when extracted by extractall. It seems that tar x doesn't have this problem. The following naive fix plugs this hole (only tested manually):
index 169c88d63f..aa4513e510 100755
--- a/Lib/tarfile.py
+++ b/Lib/tarfile.py
@@ -2211,6 +2211,8 @@ def makefile(self, tarinfo, targetpath):
source = self.fileobj
source.seek(tarinfo.offset_data)
bufsize = self.copybufsize
+ if os.path.lexists(targetpath) and not os.path.isdir(targetpath):
+ os.unlink(targetpath)
with bltn_open(targetpath, "wb") as target:
if tarinfo.sparse is not None:
for offset, size in tarinfo.sparse:
(needs some more work, like a test, plus I suppose makefifo/makedev also need a corresponding fix) pombredanne commented on Jan 13, 2023 FWIW, there is a spam bot PR campaign by @TrellixVulnTeam and @Kasimir123 (they should be banned) that has been going on since September and spams a large number of python projects with junk PRs with incorrect patches.
Examples include:
JLLeitschuh commented on Jan 23, 2023 Hi All,
My name is Jonathan Leitschuh, I'm a Senior Software Security Researcher for Project Alpha Omega at the Open Source Security Foundation under the Linux Foundation.
I recently attended a talk by @Kasimir123 at ShmooCon about this vulnerability and this work. They recently opened 61,895 pull requests across the OSS ecosystem to fix this vulnerability as the Python maintainers have taken no action to fix this severe vulnerability after 6 years.
To be clear, vulnerabilities like TAR-slip (aka Zip-slip) can lead to remote code execution by allowing an attacker to write code into executable file locations. In the right contexts, this vulnerability has a CVSSv3.1 score of 10/10 (AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H).
While I believe that fixing the simlink problem is important. I believe that this should be broken down into two issues:
- Destination escape by path traversal
- Destination escape by simlink
Given the first is far more likely to be exploited in the real world, I propose fixing it first, then focusing on the second problem.
This will ensure that end-users are protected against the most likely risk sooner, while the less likely risk discussion continues.
To provide an incentive: I would also like to note that any developer who resolves this security issue in the python standard library would most likely be eligible for a multi-thousand dollar financial reward under the Secure Open Source Rewards (https://sos.dev) project which is also operated out of the Open Source Security Foundation. encukou commented on Jan 23, 2023 FWIW, I'm reading up on this issue, and I plan to post a proposal on Discourse this week to unblock things. jowagner commented on Jan 24, 2023 @encukou We have multiple proposals in the comments here and the linked issues. The problem is that no proposal has received positive feedback from the core cpython team to motivate one of us go to the next step of implementing it and making a PR. Since a previous PR fixing the issue has been rejected, rewards from a new PR (financial or not) are uncertain. If you make yet another proposal please link to it from here. encukou commented on Jan 25, 2023 Yeah, it would be unfair if I ended up getting money for this, outside my normal salary.
Any solution will necessarily be based on lots of prior work and discussion, by people who seem to have burned out trying to fix the issue.
I won't apply for any bounty, and if I get one anyway I'll donate it to the PSF. encukou commented on Jan 25, 2023 encukou commented on May 3, 2023 PEP-706, which adds filter rather than absolute_path, has been implemented in #102950. See the added docs.
Python 3.12, and security updates to some earlier releases, will allow users to avoid CVE-2007-4559 by changing their code/settings.
Python 3.12 will emit a warning urging people to do that.
Python 3.14 will fix CVE-2007-4559 by making safer behaviour the default. Reacted by Éric and Jonathan Leitschuh nanonyme commented on May 3, 2023 @encukou have I missed something or do these changes now break shutil.unpack_archive abstraction layer for zipfile vs tarfile? encukou commented on May 3, 2023 Yes. Sadly, in Python 3.12 and 3.13 you'll get a warning about changing behaviour if you use shutil.unpack_archive with a tarball.
You can ignore the warning, or inspect the archive and pass filter='data' if it's a tarball. Or if you can change settings of the whole Python interpreter, set tarfile.TarFile.extraction_filter = tarfile.data_filter.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|