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.

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

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

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.

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.

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.

> 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")
    ...

> 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.

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).

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.

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.

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...)?

> 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.

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

Perl with File-Path-2.07 passes the test.

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?

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.

> 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).

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

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

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

shutils.rmtree should probably do the same thing.

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 issue4761 and issue10755 to work.

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 #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.

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

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).

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

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

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

I made another review but my mail was rejected.

http://bugs.python.org/review/4489/diff/3383/10563#newcode319
Lib/test/test_shutil.py:319: @unittest.skipIf(threading == None,
'requires threading')
You can just say skipUnless(threading, 'msg')

http://bugs.python.org/review/4489/diff/3383/10563#newcode344
Lib/test/test_shutil.py:344: raise
Shouldn’t this use self.fail?

http://bugs.python.org/review/4489/diff/3383/10563#newcode319
Lib/test/test_shutil.py:319: @unittest.skipIf(threading == None, 'requires
threading')
On 2011/10/07 19:29:47, eric.araujo wrote:
> You can just say skipUnless(threading, 'msg')

Right.

http://bugs.python.org/review/4489/diff/3383/10563#newcode344
Lib/test/test_shutil.py:344: raise
On 2011/10/07 19:29:47, eric.araujo wrote:
> Shouldn’t this use self.fail?

I would have thought it's better if the original exception is passed through and
displayed rather than some sort of failure message that just says "OSError
occurred".

There's a race:
"""
--- Lib/shutil.py       2011-11-05 00:11:05.745221315 +0100
+++ Lib/shutil.py.new   2011-11-05 00:11:01.445220324 +0100
@@ -307,6 +307,7 @@
        try:
            mode = os.fstatat(dirfd, name, os.AT_SYMLINK_NOFOLLOW).st_mode
         except os.error:
             mode = 0
         if stat.S_ISDIR(mode):
+            input("press enter")
             newfd = os.openat(dirfd, name, os.O_RDONLY)
             _rmtree_safe(newfd, ignore_errors, onerror)
             try:
"""

$ rm -rf /tmp/target
$ mkdir -p /tmp/target/etc
$ ./python -c "import shutil; shutil.rmtree('/tmp/target')"
press enter^Z
[1]+  Stopped                 ./python -c "import shutil; shutil.rmtree('/tmp/target')"
$ rm -r /tmp/target/etc; ln -s /etc /tmp/target/
$ fg
./python -c "import shutil; shutil.rmtree('/tmp/target')"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/cf/python/cpython/Lib/shutil.py", line 290, in rmtree
    _rmtree_safe(fd, ignore_errors, onerror)
  File "/home/cf/python/cpython/Lib/shutil.py", line 314, in _rmtree_safe
    _rmtree_safe(newfd, ignore_errors, onerror)
  File "/home/cf/python/cpython/Lib/shutil.py", line 323, in _rmtree_safe
    onerror(os.unlinkat, (dirfd, name), sys.exc_info())
  File "/home/cf/python/cpython/Lib/shutil.py", line 321, in _rmtree_safe
    os.unlinkat(dirfd, name)
PermissionError: [Errno 13] Permission denied
[52334 refs]

"""
openat(3, "etc", O_RDONLY|O_LARGEFILE)  = 4
dup(4)                                  = 5
fstat64(5, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
fcntl64(5, F_GETFL)                     = 0x8000 (flags O_RDONLY|O_LARGEFILE)
fcntl64(5, F_SETFD, FD_CLOEXEC)         = 0
getdents64(5, /* 162 entries */, 32768) = 5176
getdents64(5, /* 0 entries */, 32768)   = 0
close(5)                                = 0
fstatat64(4, "passwd", {st_mode=S_IFREG|0644, st_size=980, ...}, AT_SYMLINK_NOFOLLOW) = 0
unlinkat(4, "passwd", 0)                = -1 EACCES (Permission denied)
"""

You should use the lstat/open/fstat idiom.
Also, here:
"""
            mode1 = os.lstat(path).st_mode
            if stat.S_ISLNK(mode1):
                raise OSError("Cannot call rmtree on a symbolic link")
        except OSError:
            onerror(os.lstat, path, sys.exc_info())
            # can't continue even if onerror hook returns
            return
        fd = os.open(path, os.O_RDONLY)
        try:
            mode2 = os.fstat(fd).st_mode
            if mode1 != mode2:
                raise OSError("Target changed")
"""

You check that path is not a symlink, then you open it, perform fstat on it, and check that the mode is the same.
But if someone replaces path by a symlink to a directory with the same mode, then rmtree won't catch this. You should also compare st_dev and st_ino to make sure we're dealing with the same file.

One more thing :-)
"""
        fd = os.open(path, os.O_RDONLY)
        try:
            mode2 = os.fstat(fd).st_mode
            if mode1 != mode2:
                raise OSError("Target changed")
        except OSError:
            onerror(os.fstat, fd, sys.exc_info())
            # can't continue if target has changed
            return
"""

Here `fd` is not closed (there might be other places leaking FD).

Finally, since writting a such code is tricky, what do you - all - think of making this a generic walker method that would take as argument the methods to call on a directory and on a file (or link), so that we could reuse it to write chmodtree(), chowntree() and friends?

> Finally, since writting a such code is tricky, what do you - all -
> think of making this a generic walker method that would take as
> argument the methods to call on a directory and on a file (or link),
> so that we could reuse it to write chmodtree(), chowntree() and
> friends?

Sounds good.
FYI, I have a pathlib experiment in
http://hg.python.org/features/pathlib/, with an optional openat-based
accessor.

> FYI, I have a pathlib experiment in
> http://hg.python.org/features/pathlib/, with an optional openat-based
> accessor.

Interesting: I used to think that the current API for dealing with paths was a little too basic and terse.

Concerning this issue, one (last) thing: rmtree performs a depth-first traversal of the directory tree, keeping an open FD at each directory level: in case of deeply-nested directory hierarchy, or if there are many open FDs, there's the risk of running out of FDs.
I think the best thing would be to let rmtree fail (provided it closes all the FDs it opened): falling back to the "unsafe" version would be stupid (an attacker would just have to create a deeply-nested hierarchy, and then use the same old symlink race).

> I think the best thing would be to let rmtree fail (provided it closes 
> all the FDs it opened)

Agreed.

Thanks Charles, I'll take your comments into account and take a look at making a general walker method.

What's the current state here? Anyone working on a solution or are we waiting how http://hg.python.org/features/pathlib/ will work out?

If the consensus is to add a generic walker method, wouldn't be appropriate to open a new bug and add it as dependency? Or is there one I've missed?

> What's the current state here? Anyone working on a solution or are we
> waiting how http://hg.python.org/features/pathlib/ will work out?

Well, I am not working on that one, so waiting for it to work out might
be optimistic :)
I don't know what to do with it (the pathlib): is such a feature
desireable enough?

> If the consensus is to add a generic walker method, wouldn't be
> appropriate to open a new bug and add it as dependency?

Agreed.

> > What's the current state here? Anyone working on a solution or are we
> > waiting how http://hg.python.org/features/pathlib/ will work out?
>  
> Well, I am not working on that one, so waiting for it to work out might
> be optimistic :)
> I don't know what to do with it (the pathlib): is such a feature
> desireable enough?

Independently from this bug, I'd say it would be a good thing.

Proof: http://twistedmatrix.com/documents/current/api/twisted.python.filepath.html – Twisted already implemented something similar for themselves.

> > If the consensus is to add a generic walker method, wouldn't be
> > appropriate to open a new bug and add it as dependency?
>  
> Agreed.

See #13734

In case you want opinions on pathlib: I, for one, disliked Jason Orendorff’s path module, because it did not distinguish between pure string operations and I/O-inducing operations, and also because it duplicated os/os.path functions.  Your API doesn’t have these issues, so for my taste it’s conceptually better.  I should clone your repo and play with the module a bit to see if I like it.