tarfile: Traversal attack vulnerability

@birkenfeld

BPO	21109
Nosy	@birkenfeld, @jcea, @gustaebel, @vstinner, @taleinat, @tiran, @benjaminp, @jwilk, @ned-deily, @vadmium, @serhiy-storchaka, @psyker156, @shanxS, @epicfaace, @websurfer5
PRs	bpo-21109: Add SafeTarFile #15244
Dependencies	bpo-17102: tarfile extract can write files outside the destination path bpo-29788: [Security] tarfile: Add absolute_path option to tarfile, disabled by default
Files	prevent-tar-traversal-attack.diff: patch to prevent safetarfile-1.diff: New SafeTarFile class and documentation safetarfile-2.diff safetarfile-3.diff safetarfile-4.diff

^{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 = 'https://github.com/gustaebel'
closed_at = None
created_at = <Date 2014-03-31.08:14:19.090>
labels = ['type-security', 'library', '3.9']
title = 'tarfile: Traversal attack vulnerability'
updated_at = <Date 2021-02-27.08:56:06.564>
user = 'https://bugs.python.org/DanielGarcia'

bugs.python.org fields:

activity = <Date 2021-02-27.08:56:06.564>
actor = 'vstinner'
assignee = 'lars.gustaebel'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2014-03-31.08:14:19.090>
creator = 'Daniel.Garcia'
dependencies = ['17102', '29788']
files = ['34676', '35127', '47800', '47803', '47826']
hgrepos = []
issue_num = 21109
keywords = ['patch', 'security_issue']
message_count = 35.0
messages = ['215222', '215223', '215224', '215225', '215226', '215237', '215239', '215242', '215656', '215658', '216675', '217188', '217189', '217690', '277339', '289438', '324193', '324198', '324262', '324908', '325229', '325329', '325491', '325607', '325635', '326423', '326437', '327451', '327458', '334921', '335078', '335292', '349517', '349583', '387772']
nosy_count = 19.0
nosy_names = ['georg.brandl', 'jcea', 'lars.gustaebel', 'vstinner', 'taleinat', 'christian.heimes', 'benjamin.peterson', 'jwilk', 'ned.deily', 'Arfrever', 'martin.panter', 'serhiy.storchaka', 'edulix', 'Daniel.Garcia', 'Philippe.Godbout', 'shanxS', 'epicfaace', 'uhei3nn9', 'Jeffrey.Kintscher']
pr_nums = ['15244']
priority = 'high'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue21109'
versions = ['Python 3.9']

The application does not validate the filenames inside the tar archive, allowing to extract files in arbitrary path. An attacker can craft a tar file to override files.

I've view this vulnerability in libtar:
http://lwn.net/Vulnerabilities/587141/
I've checked that python tarfile doesn't validate the filenames so python tarfile is vulnerable to this attack.

The solution in the patch is based on the gnutar solution to this, removing the prefix when extracting and adding.

Setting as release blocker pending evaluation.

It's a known and well-documented behavior of the tar module:

https://docs.python.org/2.7/library/tarfile.html#tarfile.TarFile.extractall

It's a known and well-documented behavior of the tar module

Would it possible to disable this behaviour by default, and only enable ti explicitly? The tar command line program has for example the -P / --absolute-paths option.

Yes, this behavior is documented, but still it is desirable to fix it. The tar utility has a lot of switches which controls extracting and by default it prevents three ways of attack (absolute names, '..' and symlinks), but there are other possible ways of attack. This is complex issue and I'm working on it. See also bpo-19974.

In any case we should be very careful because every protection against attack changes a behavior (which can be safe if you know what you do), so perhaps we should add parameters which controls behavior. This is possible only in new Python version.

Note that any issues here should also be considered for zipfile and shutil. (Well, shutil can just use the other two once the security is available.) See bpo-20907.

Don't forget about SUID and SGID, too.

In the past, our answer to these kinds of bug reports has always been that you must not extract an archive from an untrusted source without making sure that it has no malicious contents. And that tarfile conforms to the posix specifications with respect to extraction of files and pathname resolution. That's why we put this prominent warning in the documentation, and I think its advice still holds.

I don't think that this issue should be marked as a release blocker, because the way tarfile currently works was a conscious decision, not an accident. tarfile does what it is designed to do: it processes a sequence of instructions to store a number of files in the filesystem. So the attack that is described by Daniel Garcia exploits neither a bug in tarfile nor a loophole in the tar archive format. A necessary condition for this attack to work is that the attacker has to trick the user into extracting the malicious archive first. After that, tarfile interprets the contained instructions word-for-word but still only within the boundaries defined by the user's privileges.

I think it is obvious that it is potentially dangerous to extract tar archives we didn't create ourselves, because we actually give another person direct access to our filesystem. tarfile could mitigate some of the adverse effects, but this will not change the fact that it remains unsafe to use tarfile to a certain degree unless you use it with your own data or take reasonable precautions.

Anyway, if we come to the conclusion that we want to eliminate this kind of attack, we must be aware that there is a lot more to do than that. tarfile as it is today is vulnerable to all known attacks against tar programs, and maybe even a few more that rely on its specific implementation.

Path traversal:

The archive contains files names e.g. /etc/passwd or ../etc/passwd.
Symlink file attack:

foo links to /etc/passwd.
Another member named foo follows, its data overwrites the target file's data.
Symlink directory attack:

foo links to /etc.
The following member foo/passwd overwrites /etc/passwd.
Hardlink attack:

Hardlink member foo links to /etc/passwd.
tarfile creates the hardlink to /etc/passwd because it cannot find it inside the archive and falls back to the one in the filesystem.
Another file named foo follows, its data overwrites /etc/passwd's data.
Permission manipulation:

The archive contains an executable that is placed somewhere in PATH with its setuid flag set, so that an unprivileged user is able to gain root privileges.
Device file attacks:

The archive contains a device node foo with the same major and minor numbers as an attached device.
Another member named foo follows, its data is written to the device.
Huge zero file attacks:

Bzip2 and lzma allow it to store huge blobs of repetetive data in tiny archives. When unpacked this data may fill up an entire filesystem.
Excessive memory usage:

tarfile saves one TarInfo object per member it finds in an archive. If the archive contains several millions of members, this may fill up the memory.
Saving a huge sparse file:

tarfile is unable to detect holes in sparse files and thus cannot store them efficiently. Archiving a huge sparse file can take very long and may lead to a very big archive that fills up the filesystem.

Additionally, there are more issues mentioned in the GNU tar manual:

https://www.gnu.org/software/tar/manual/html_node/Security.html

In conclusion, I like to emphasize that tarfile is a library, it is no replacement for GNU tar. And as a library it has a different focus, it is merely a building block for an application, and has to be used with a little bit of responsibility. And even if we start to implement all possible checks, I'm afraid we never can do without a warning in the documentation that reminds everyone to keep an eye on what they're doing.

Thank you Lars for your thorough reply.

While I agree that this isn't a release blocker, as it was clearly designed to behave this way... it seems to me that it wouldn't take much to make the tarfile module a lot safer. Specifically:

Don't allow creating files whose absolute path is not under the
destination.
Don't allow creating links (hard or soft) which link to a path
outside of the destination.
Don't create device nodes.

This would fix your listed attacks 1-6. The remaining attacks you cite are denial-of-service attacks; while they're undesirable, they shouldn't compromise the security of the machine. (I suppose we could even address those, adding "reasonable" quotas for disk space and number of files.)

I doubt that would make tarfile secure. But maybe "practicality beats purity"?

Seems like shutil._unpack_tarfile() is affected. I guess it could at least do with one of those warnings in the documentation for make_archive().

The patch for this bug looks a bit over enthusiastic, for example skip_prefixes("blaua../stuff") would incorrectly strip the first bit and just return "stuff".

It seems there might already be plenty of existing code to check for bad paths. Examples that come to mind:

http.server.SimpleHTTPRequestHandler.translate_path()
zipfile.ZipFile._extract_member()
shutil._unpack_zipfile()

This code either ignores the bad path elements, or ignores the whole path. Perhaps some of it could be recycled into a common function somewhere, rather than implementing it all over again for tar files.

I have written my own function joinpath() to do this sort of checking, which you are welcome to use:

https://bitbucket.org/vadmium/pyrescene/src/34264f6/rescene/utility.py#cl-217

You would call it with something like joinpath(tarpath.split("/"), osdir).

Do we have any final decision on what's the best approach to solve this? I see some possibilities:

a) leave the issue to the library user. I think that's a not good solution security-wise as many will be unaware of the problem and this promotes code duplication for the fix. On the other hand, this does not change default behavior.

b) fix the problem as proposed in the patch sent by Daniel. This makes the tarfile secure against this kind of attacks. It does change the behavior and doesn't allow to extract in arbitrary paths, though.

c) fix the problem so that by default extracting in arbitrary paths is not allowed, but allow somehow to do that optionally. This way we change the default behavior but provide an easy fix for those that depend on that functionality.

d) do not change the default, but provide a well documented and easy way to activate the safety checks that fix this kind of attacks. The advantage is that it doesn't change the default behavior, the disadvantage is that many people will have to modify their code to be secure, and that the default is not very secure.

For what is worth, I believe either b or c should be chosen to fix this issue.

Is there any update on this? Will this be fixed in the next release?

There was progress made as described on this issue, but there is yet work to be done, and no-one seems to be taking this upon themselves at the moment.

I agree that it would be great to have this in 3.8.

I've added a PR in which I'm working on adding in the tests. Wanted to make sure this is the approach you had in mind? It wasn't as simple as how tests are handled in, say, test_binascii.py, because over there there was only one class that handled the main test suite, while in this file, there are multiple.

SafeTarFile does not pass the existing tests, mainly because the existing file Lib/test/tarfiletestdata/testtar.tar seems to be "unsafe", producing errors like these:

tarfile.SecurityError: <TarInfo 'ustar/blktype' at 0x7fb9119b3bb0>: block device

tarfile.SecurityError: <TarInfo 'ustar/regtype' at 0x7fb9119b3910>: duplicate name

It seems like the solution here is to remove block devices and duplicate names from testtar.tar. However, is this desirable -- do we need to keep these in for the tests for TarFile?

What is the status of this issue?

fwiw this is likely https://nvd.nist.gov/vuln/detail/CVE-2007-4559.

There is now a bot (TrellixVulnTeam) which automatically opens pull requests [0,1] to fix this problem.
It seams like the bot took code from here [2], but the code itself is still vulnerable if the archive contains symlinks to outside of the target directory.
This just demonstrates how difficult it is for a project to check for malicious tar files.

Is anyone working on this right now?

[0] https://github.com/brandsimon/verify-squash-root/pull/10
[1] Project-Awaken/android_external_boringssl#1
[2] https://gist.github.com/Kasimir123/9bdc38f2eac9a19f83ef6cc52c7ce82e?

PEP-706 (Filter for tarfile.extractall) 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.

@encukou

@encukou encukou closed this as completed yesterday

Yahoo! Thanks for your tenacity to go to the root of these issues, design an API for it and a smooth transition, and implementing it!

Navigation Menu

Repository navigation

tarfile: Traversal attack vulnerability #65308

23 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

tarfile: Traversal attack vulnerability #65308

Description

Activity

DanielGarcia commented on Mar 31, 2014

DanielGarcia commented on Mar 31, 2014

ned-deily commented on Mar 31, 2014

tiran commented on Mar 31, 2014

vstinner commented on Mar 31, 2014

serhiy-storchaka commented on Mar 31, 2014

bitdancer commented on Mar 31, 2014

tiran commented on Mar 31, 2014

gustaebel commented on Apr 6, 2014

larryhastings commented on Apr 6, 2014

vadmium commented on Apr 17, 2014

edulix commented on Apr 26, 2014

23 remaining items

taleinat commented on Feb 12, 2019

epicfaace commented on Aug 13, 2019

epicfaace commented on Aug 13, 2019

vstinner commented on Feb 27, 2021

gpshead commented on Sep 21, 2022

brandsimon commented on Nov 8, 2022

encukou commented on May 3, 2023

vstinner commented on May 4, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions