shutil.rmtree is vulnerable to a symlink attack

[mrts]

BPO	4489
Nosy	@loewis, @birkenfeld, @jcea, @ncoghlan, @pitrou, @larryhastings, @blueyed, @tarekziade, @ezio-melotti, @merwok, @akheron, @hynek
PRs	bpo-4489: tests(tests_shutil): fix comment with check_args_to_onerror #22968
Dependencies	bpo-4761: create Python wrappers for openat() and others bpo-10755: Add posix.fdlistdir bpo-13734: Add a generic directory walker method to avoid symlink attacks bpo-14773: fwalk breaks on dangling symlinks
Files	shutil_patched.py issue4489_first_attempt.diff test_issue4489.sh i4489.patch: Initial patch and test i4489_v2.patch: Updated patch i4489_v3.patch: Updated patch i4489_v4.patch rmtree-with-fwalk-v1.diff rmtree-with-fwalk-docs-v1.diff rmtree-with-fwalk-v2.diff rmtree-with-fwalk-v3.diff direct_rmtree_safe.diff mvl-revisited-plus-docs.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/hynek'
closed_at = <Date 2012-06-28.10:39:44.997>
created_at = <Date 2008-12-02.15:42:01.840>
labels = ['type-security', 'library', 'release-blocker']
title = 'shutil.rmtree is vulnerable to a symlink attack'
updated_at = <Date 2020-10-25.13:33:48.102>
user = 'https://bugs.python.org/mrts'

bugs.python.org fields:

activity = <Date 2020-10-25.13:33:48.102>
actor = 'blueyed'
assignee = 'hynek'
closed = True
closed_date = <Date 2012-06-28.10:39:44.997>
closer = 'hynek'
components = ['Library (Lib)']
creation = <Date 2008-12-02.15:42:01.840>
creator = 'mrts'
dependencies = ['4761', '10755', '13734', '14773']
files = ['12482', '12484', '12485', '20274', '20277', '20279', '23261', '25630', '25631', '25649', '25660', '25935', '26089']
hgrepos = []
issue_num = 4489
keywords = ['patch', 'needs review']
message_count = 83.0
messages = ['76753', '78389', '78391', '78398', '78405', '78406', '78418', '78425', '78440', '78441', '78442', '78443', '78444', '78445', '78446', '78447', '78448', '78451', '103686', '124472', '125425', '125429', '125435', '125436', '125446', '142609', '144621', '145113', '145133', '147058', '147059', '147080', '147217', '147249', '150794', '150810', '150834', '150952', '159467', '159622', '161047', '161048', '161050', '161130', '161207', '161250', '161256', '161266', '162558', '162559', '162596', '162609', '163089', '163092', '163338', '163444', '163636', '163655', '163721', '163722', '163723', '163726', '163729', '163731', '163732', '163733', '163734', '163735', '163736', '163738', '163874', '163877', '163883', '163884', '163941', '164197', '164234', '164235', '164245', '164247', '164248', '164251', '164255']
nosy_count = 19.0
nosy_names = ['loewis', 'georg.brandl', 'jcea', 'ncoghlan', 'pitrou', 'larry', 'blueyed', 'schmir', 'tarek', 'ezio.melotti', 'eric.araujo', 'Arfrever', 'mrts', 'neologix', 'teamnoir', 'rosslagerwall', 'python-dev', 'petri.lehtinen', 'hynek']
pr_nums = ['22968']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue4489'
versions = ['Python 3.3']

[mrts]

Race condition in the rmtree function in the shutils module allows local
users to delete arbitrary files and directories via a symlink attack.

See also http://bugs.debian.org/286922

Attack:

---

# emulate removing /etc
$ sudo cp -a /etc /root/etc/
$ sudo python2.6
 >>> for i in xrange(0, 50000):
...      with open("/root/etc/" + str(i), "w") as f:
...             f.write("0")
...
$ ls /root/etc > orig_list.txt

$ mkdir /tmp/attack
$ cp -a /root/etc/* /tmp/attack

$ sudo python2.6
 >>> from shutil import rmtree
 >>> rmtree('/tmp/attack')
 >>> # press ctrl-z to suspend execution
^Z
[1]+  Stopped                 sudo python2.6

$ mv /tmp/attack /tmp/dummy; ln -s /root/etc /tmp/attack
$ fg
sudo python2.6
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.6/shutil.py", line 225, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/local/lib/python2.6/shutil.py", line 223, in rmtree
    os.rmdir(path)
OSError: [Errno 20] Not a directory: '/tmp/attack'

$ ls /root/etc > new_list.txt
$ diff -q orig_list.txt new_list.txt
Files orig_list.txt and new_list.txt differ

---

If the attack wasn't successful, /root/etc would not be modified and
orig_list.txt and new_list.txt would be identical.

[pitrou]

What course of action do you suggest? First chmod 0700 on the directory?

[pitrou]

Mmmh, very recent Linux kernels (>= 2.6.16) seem to have specific
functions to deal with this (see man pages for openat, unlinkat, etc.).

[mrts]

A shameless copy of the Perl fix for the bug
http://bugs.debian.org/286922 looks like the evident solution.

Somebody has to examine the fix though, I'm afraid I'm not currently
able to do it.

[pitrou]

The Perl patch is here:
http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=36;filename=etch_03_fix_file_path;att=1;bug=286922

It is a recursive implementation of rmtree. What it does is 1) get the
inode of the path 2) unlink it altogether if not a dir 3) otherwise,
chdir to it 4) check that '.' still has the same inode, otherwise bail out.

[pitrou]

Mmmh, the problem with Perl's approach is that it changes the current
working directory (calls to chdir()), which is process-specific and not
thread-specific. Currently, no function in shutil changes the current
working directory, which is a nice behaviour and should IMO be preserved.

[mrts]

Mmmh, the problem with Perl's approach is that it changes the current
working directory (calls to chdir()), which is process-specific and not
thread-specific. Currently, no function in shutil changes the current
working directory, which is a nice behaviour and should IMO be preserved.

Using chdir() makes sense and it doesn't look like a too big problem to me:

def rmtree(...):
    ...
    curdir = os.getcwd()
    try:
        call chdir() as required
    finally:
        try:
            os.chdir(curdir)
        except:
            warnings.warn("Unable to chdir to previous current dir")
    ...

[pitrou]

Using chdir() makes sense and it doesn't look like a too big problem to me:

It's a problem if another thread in the process is making file
operations using relative paths at the same time.

Since shutil functions have until now been safe against this
possibility, I don't think it's ok to make them unsafe in future
versions.

[mrts]

Ah, right you are. Attaching an initial alpha-quality patched shutil.py
and a script to test the attack.

Run the script by sourcing it with . test_issue4489.sh, not by executing
(job control won't work in this case).

[mrts]

And here's the diff so you can review what I was up to.

Note that this does not yet fix the problem (although the logic looks
about right), I have to examine the problem more thoroughly.

[mrts]

Aha, got it -- while removing /a/b/c/d, there's no easy way to detect
that b or c has become a symlink.

I.e.

given directory tree

a
-- b |-- c -- d

1. os.rmdir('/a/b/c') succeeds
2. execution is suspended
3. '/a/b' is made a symlink to a path that contains 'd'
4. '/a/b/d' is neither a symlink, nor has it's inode been recorded, so
   os.rmdir('/a/b/d') succeeds

I'm afraid the solution for the Perl bug is susceptible to the same problem.

[mrts]

A blunt, ineffective solution would be to walk the tree before removing
it and recording path : inode pairs in a dict on first pass and then
checking that the inodes have not changed during removal on second pass.

If no clever bulletproof fix emerges, perhaps this should be added as
shutil.rmtree_safe (duh, API bloat...)?

[pitrou]

A blunt, ineffective solution would be to walk the tree before removing
it and recording path : inode pairs in a dict on first pass and then
checking that the inodes have not changed during removal on second pass.

There's no way to do the "check inode then remove" sequence atomically.

[mrts]

Fixed a minor bug in test script and added Perl test as well.

Perl with File-Path-2.07 passes the test.

[mrts]

Antoine, what if we add another function, rmtree_safe() that uses
chdir() and document that it is protected from the race condition but
may have the side effect of changing the current dir in threaded
environment?

[mrts]

Replying to previous comment:

There's no way to do the "check inode then remove" sequence atomically.

Right, although the attack window would be tiny, this is not a real
solution.

[pitrou]

Antoine, what if we add another function, rmtree_safe() that uses
chdir() and document that it is protected from the race condition but
may have the side effect of changing the current dir in threaded
environment?

I don't have any strong opinion on it, maybe it should be brought on the
mailing-list (or just wait for some other devs to show up here).

[pitrou]

FWIW, I've opened a separate bug entry for the creation of the openat(),
etc. wrappers: bpo-4761.
Those functions seem to exist on recent Linux distros (even Debian stable).

[tarekziade]

Mart,

I took over the maintenance of shutil, if you are interested in contributing a bullet-proof version of rmtree, please drop a line at python-ideas, and let's see what we can do in a new function maybe.

I'll be happy to review and apply patches if we get a consensus

[teamnoir]

How does "rm -rf" address this issue? Or does it?

shutils.rmtree should probably do the same thing.

[rosslagerwall]

Here is a draft patch.

It uses the *at functions and fdlistdir consequently it only makes it safe if those functions are available. It works using a recursive implementation and an open file descriptor pointing to a directory, instead of maintaining state by changing the current directory. If the *at functions are unavailable, it falls back to the unsafe implementation.

It requires the patches from bpo-4761 and bpo-10755 to work.

[pitrou]

Thanks for the patch.

There seems to be a race remaining here:

+ try:
+ if os.path.islink(path):
+ # symlinks to directories are forbidden, see bug bpo-1669
+ raise OSError("Cannot call rmtree on a symbolic link")
+ except OSError:
+ onerror(os.path.islink, path, sys.exc_info())
+ # can't continue even if onerror hook returns
+ return
+ fd = os.open(path, os.O_RDONLY)

Someone could change path to be a symlink between the calls to islink() and open(). You probably need to stat the fd instead.

Some other things:

• if close() is meant to be a private helper, it should be named _close()
• instead of a bare "except" in close(), use "except EnvironmentError" or "except OSError"

I haven't looked at the tests yet.

[rosslagerwall]

Updated patch removes the race condition. Since an open follows symlinks, you can't just fstat the fd to see if it is a link. I followed the following to overcome this:
https://www.securecoding.cert.org/confluence/display/seccode/POS35-C.+Avoid+race+conditions+while+checking+for+the+existence+of+a+symbolic+link

[pitrou]

Le mercredi 05 janvier 2011 à 16:58 +0000, Ross Lagerwall a écrit :

Ross Lagerwall <rosslagerwall@gmail.com> added the comment:

Updated patch removes the race condition. Since an open follows symlinks, you can't just fstat the fd to see if it is a link. I followed the following to overcome this:
https://www.securecoding.cert.org/confluence/display/seccode/POS35-C.+Avoid+race+conditions+while+checking+for+the+existence+of+a+symbolic+link

Nice. I am unsure about the following piece of code:

+ if stat.S_ISDIR(mode):
+ if stat.S_ISLNK(mode):
+ try:
+ raise OSError("Cannot call rmtree on a symbolic
link")
+ except OSError:
+ onerror(os.fstatat, (dirfd, name), sys.exc_info())

If rmtree() encounters a symlink *inside* the tree, I would expect it to
simply remove the symlink, rather than choke and abort (it's also what
the unsafe implementation does).

[rosslagerwall]

I think I misread the original implementation. Here is an updated version with that code just taken out.

[merwok]

I made two comments on rietveld but the email was rejected.

[rosslagerwall]

Updated patch based on Eric's comments:
Store _supports_safe_rmdir at the module level.
Move imports up to module level
Skip test on non-threading build

[hynek]

Excellent Nick! I discussed the very same thing with David yesterday but somehow we didn't come up with a good name. So we kept it on "safe" (which predates the secure discussion).

[larryhastings]

Bikeshedding:

(os.unlink in os.supports_dir_fd and os.open in os.supports_dir_fd)

could be rewritten as

{ os.open, os.unlink } <= os.supports_dir_fd

As you were!

[merwok]

The code using set operations seems slightly too cryptic for me, even though I’m comfortable with sets and functions as first-class objects. A matter of taste I guess.

BTW it sounds a bit strange to me to have the verb in the singular in supports_dir_fd when its meaning is “the functions in this set support dir_fd”. I guess it’s too late to propose “os.open.supports_dir_fd and os.unlink.supports_dir_fd” (and I don’t know if that is feasible with C functions) :)

[python-dev]

New changeset c2be81151994 by Nick Coghlan in branch 'default':
Issue bpo-4489: Rename the feature marker for the symlink resistant rmtree and store it as a function attribute
http://hg.python.org/cpython/rev/c2be81151994

[larryhastings]

I guess it’s too late to propose “os.open.supports_dir_fd and
os.unlink.supports_dir_fd” (and I don’t know if that is feasible
with C functions) :)

Where were you when "is_implemented" was being savagely torn apart last week? ;-)

[merwok]

I was at work, and moving out of my apartment, and desperating at the subthreads spawned by my mail about packaging, and agreeing with the people being -1 on is_implemented on Signature objects :-)

[ncoghlan]

Éric - there's almost certainly going to be a PEP for 3.4 about doing this kind of feature advertisement in a cleaner and more consistent way.

[merwok]

Yep, that is promising. At worst we’ll have a new cool API and three obsolete sets in the os module, not a big deal.

[hynek]

Okay everyone, let's call it day – after 3,5 years. :) Further enhancement requests please in separate tickets. Thanks to everybody who took part!

[Arfrever]

The fix for this issue broke support for bytes in shutil.rmtree:

$ mkdir -p /tmp/a/b
$ python3.2 -c 'import shutil; shutil.rmtree(b"/tmp/a")'
$ mkdir -p /tmp/a/b
$ python3.3 -c 'import shutil; shutil.rmtree(b"/tmp/a")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.3/shutil.py", line 444, in rmtree
    _rmtree_safe_fd(fd, path, onerror)
  File "/usr/lib64/python3.3/shutil.py", line 381, in _rmtree_safe_fd
    fullname = os.path.join(path, name)
  File "/usr/lib64/python3.3/posixpath.py", line 78, in join
    if b.startswith(sep):
TypeError: startswith first arg must be str or a tuple of str, not bytes
$

[larryhastings]

The fix for this issue broke support for bytes in shutil.rmtree:

What platform? Windows, or non-Windows? It'll probably be obvious regardless, but that might help.

[ncoghlan]

Tinkering with os.path.join, that traceback means that "name" is a str instance, while "path" is a bytes instance.

The culprit actually appears to be the fact that the type returned by os.listdir (et al) when handed a file descriptor is always a string (this is not explicitly documented, a problem in itself, but that's the behaviour I see here on linux).

One way to handle this would be to use the filesystem encoding with surrogateescape to decode any bytes path passed in to shutil.rmtree (at least in the _rmtree_safe_fd case) and handle the actual removal with Unicode throughout.

So long as the original encoding of the supplied bytes path is compatible with that of the underlying filesystem, surrogateescape should ensure that everything round trips correctly.

[larryhastings]

Your deduction is correct. listdir can't tell what the original argument type was based on the output--path_converter abstracts away those details. So it separately tests the type of the first argument. Staring at it again it's about as clear as mud, but the goal was, the output is always strings unless the user specified "path" as bytes.

I'll make a separate issue regarding making the code easier to read and adding a clarification to the documentation. We should spare future programmers from having to guess at this behavior :)

[python-dev]

New changeset 2e2329aeb5c1 by Hynek Schlawack in branch 'default':
bpo-4489 Make fd based rmtree work on bytes
http://hg.python.org/cpython/rev/2e2329aeb5c1

[Arfrever]

The fix (c910af2e3c98 + 53fc7f59c7bb) for this issue broke deletion of directories, which contain symlinks to directories.
(Directories with symlinks to regular files or symlinks to nonexistent files are unaffected.)

$ mkdir -p /tmp/a/b
$ ln -s b /tmp/a/c
$ python3.2 -c 'import shutil; shutil.rmtree("/tmp/a")'
$ mkdir -p /tmp/a/b
$ ln -s b /tmp/a/c
$ python3.3 -c 'import shutil; shutil.rmtree("/tmp/a")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib64/python3.3/shutil.py", line 447, in rmtree
    _rmtree_safe_fd(fd, path, onerror)
  File "/usr/lib64/python3.3/shutil.py", line 395, in _rmtree_safe_fd
    _rmtree_safe_fd(dirfd, fullname, onerror)
  File "/usr/lib64/python3.3/shutil.py", line 406, in _rmtree_safe_fd
    onerror(os.rmdir, path, sys.exc_info())
  File "/usr/lib64/python3.3/shutil.py", line 404, in _rmtree_safe_fd
    os.rmdir(path)
NotADirectoryError: [Errno 20] Not a directory: '/tmp/a/c'
$

[python-dev]

New changeset f9f798f1421e by Hynek Schlawack in branch 'default':
bpo-4489: Don't follow ever symlinks in rmtree
http://hg.python.org/cpython/rev/f9f798f1421e

[hynek]

Thanks you for catching that! It seems we did something really stupid: followed symlinks.

I’ve fixed that and added regression tests. This one is a facepalm gentlepeople.

[larryhastings]

I'm not a security guy, but: shouldn't the os.unlink call when it isn't a directory specify follow_symlinks=False? And wouldn't it be safer if the os.rmdir() call also used dir_fd=?

Additionally, I think you missed some stuff for shutil._use_fd_functions. Assuming I'm right on both of the above, you should also check:

• os.listdir in os.supports_dir_fd
• os.rmdir in os.supports_dir_fd
• os.stat in os.supports_dir_fd
• os.stat in os.supports_follow_symlinks
• os.unlink in os.supports_follow_symlinks

I'd spell that
_use_fd_functions = ({os.listdir, os.open, os.rmdir, os.stat, os.unlink} <
os.supports_dir_fd and
{os.stat, os.unlink} <= os.supports_follow_symlinks)

Finally, up to you, but I'd be tempted to change the "lstat" "and "fstat" calls to "stat" calls using the relevant parameters.

[hynek]

I'm not a security guy, but: shouldn't the os.unlink call when it isn't a directory specify follow_symlinks=False?

os.unlink has no follow_symlinks argument. Imagine what would happen if
you‘d do a os.unlink() on a link and it would just remove the link
destination. :)

And wouldn't it be safer if the os.rmdir() call also used dir_fd=?

Unfortunately, os.rmdir('.', dir_fd=topfd) doesn’t work. As in the worst
case it could delete only an empty directory, I think it’s fine.

Additionally, I think you missed some stuff for shutil._use_fd_functions. Assuming I'm right on both of the above, you should also check:

• os.listdir in os.supports_dir_fd
• os.rmdir in os.supports_dir_fd
• os.stat in os.supports_dir_fd
• os.stat in os.supports_follow_symlinks
• os.unlink in os.supports_follow_symlinks

Interestingly, os.listdir is not in os.supports_dir_fd although it works:

False

Will you fix it right away or shall I open a ticket?

I'd spell that
_use_fd_functions = ({os.listdir, os.open, os.rmdir, os.stat, os.unlink} <
os.supports_dir_fd and
{os.stat, os.unlink} <= os.supports_follow_symlinks)

It would be:

_use_fd_functions = ({os.listdir, os.open, os.stat, os.unlink} <=
                     os.supports_dir_fd and
                     os.stat in os.supports_follow_symlinks)

But currently can’t do.

Finally, up to you, but I'd be tempted to change the "lstat" "and "fstat" calls to "stat" calls using the relevant parameters.

That's not 3.3 fodder IMHO, feel free to open an enhancement ticket.

[larryhastings]

I'm pretty busy right now, please open a ticket for listdir.

_rmtree_safe_fd could remove the directory just after the recursive step using the parent's dirfd. Of course you'd also have to add a rmdir for the very-tippy-top after the original call in shutil.rmtree too. But this would prevent the malicious user from even removing empty directories.

[hynek]

I'm pretty busy right now, please open a ticket for listdir.

done

_rmtree_safe_fd could remove the directory just after the recursive step using the parent's dirfd. Of course you'd also have to add a rmdir for the very-tippy-top after the original call in shutil.rmtree too. But this would prevent the malicious user from even removing empty directories.

Okay I looked into it and it seems okay. IIRC, when Martin wrote the
code (and I the fwalk version before), there was no known fd-based rmdir
function.

[python-dev]

New changeset 9134bb4d0578 by Hynek Schlawack in branch 'default':
bpo-4489: Use dir_fd in rmdir in _rmtree_safe_fd()
http://hg.python.org/cpython/rev/9134bb4d0578