Issue 4489: shutil.rmtree is vulnerable to a symlink attack (original) (raw)
Created on 2008-12-02 15:42 by mrts, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (83)
Author: Mart Sõmermaa (mrts)
Date: 2008-12-02 15:41
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 "", line 1, in 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.
Author: Antoine Pitrou (pitrou) * 
Date: 2008-12-27 23:23
What course of action do you suggest? First chmod 0700 on the directory?
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-27 23:31
Mmmh, very recent Linux kernels (>= 2.6.16) seem to have specific functions to deal with this (see man pages for openat, unlinkat, etc.).
Author: Mart Sõmermaa (mrts)
Date: 2008-12-28 11:56
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.
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-28 14:38
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.
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-28 14:49
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.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-28 16:38
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") ...
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-28 19:07
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.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 08:22
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).
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 08:30
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.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 08:41
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
- os.rmdir('/a/b/c') succeeds
- execution is suspended
- '/a/b' is made a symlink to a path that contains 'd'
- '/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.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 08:46
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...)?
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-29 10:05
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.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 11:15
Fixed a minor bug in test script and added Perl test as well.
Perl with File-Path-2.07 passes the test.
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 11:19
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?
Author: Mart Sõmermaa (mrts)
Date: 2008-12-29 11:26
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.
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-29 11:32
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).
Author: Antoine Pitrou (pitrou) *
Date: 2008-12-29 12:27
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).
Author: Tarek Ziadé (tarek) *
Date: 2010-04-20 09:26
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
Author: K Richard Pixley (teamnoir)
Date: 2010-12-22 01:13
How does "rm -rf" address this issue? Or does it?
shutils.rmtree should probably do the same thing.
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-01-05 14:27
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 and to work.
Author: Antoine Pitrou (pitrou) *
Date: 2011-01-05 15:21
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](issue1669 "[closed] shutil.rmtree fails on symlink, after deleting contents")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 returnsreturnfd = 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.
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-01-05 16:58
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
Author: Antoine Pitrou (pitrou) *
Date: 2011-01-05 17:06
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).
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-01-05 18:19
I think I misread the original implementation. Here is an updated version with that code just taken out.
Author: Éric Araujo (eric.araujo) *
Date: 2011-08-21 10:57
I made two comments on rietveld but the email was rejected.
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-09-29 19:42
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
Author: Éric Araujo (eric.araujo) *
Date: 2011-10-07 17:35
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?
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-10-07 19:34
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".
Author: Charles-François Natali (neologix) *
Date: 2011-11-04 23:58
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 "", line 1, in 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?
Author: Antoine Pitrou (pitrou) *
Date: 2011-11-05 00:11
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.
Author: Charles-François Natali (neologix) *
Date: 2011-11-05 11:56
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).
Author: Antoine Pitrou (pitrou) *
Date: 2011-11-07 10:54
I think the best thing would be to let rmtree fail (provided it closes all the FDs it opened)
Agreed.
Author: Ross Lagerwall (rosslagerwall)
Date: 2011-11-07 19:11
Thanks Charles, I'll take your comments into account and take a look at making a general walker method.
Author: Hynek Schlawack (hynek) *
Date: 2012-01-07 13:13
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?
Author: Antoine Pitrou (pitrou) *
Date: 2012-01-07 17:56
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.
Author: Hynek Schlawack (hynek) *
Date: 2012-01-08 00:15
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
Author: Éric Araujo (eric.araujo) *
Date: 2012-01-09 16:40
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.
Author: Hynek Schlawack (hynek) *
Date: 2012-04-27 15:20
Anybody working on this one? I’d give it a shot otherwise.
Author: Charles-François Natali (neologix) *
Date: 2012-04-29 16:11
Anybody working on this one? I’d give it a shot otherwise.
Go ahead. You could - should? - probably use the new os.fwalk() to walk directories in a safe maner.
Author: Hynek Schlawack (hynek) *
Date: 2012-05-18 11:30
I've implemented a _safe_rmtree which gets used if os.fwalk() and os.unlinkat() are available.
Test suite still passes in regression mode both on Mac (= no effect) and Linux.
Let me know if I missed something.
Author: Petri Lehtinen (petri.lehtinen) *
Date: 2012-05-18 11:42
Same a rmtree but uses safe functions to avoid race conditions ^ typo
onerror(os.unlinkat, os.path.join(path, name), sys.exc_info())onerror(os.fwalk, path, sys.exc_info())
The documentation currently states that the first argument of onerror will be one of os.path.islink, os.listdir, os.remove, os.rmdir. You shuld probably add os.unlinkat and os.fwalk to that list now.
Otherwise, looks good to me.
Author: Hynek Schlawack (hynek) *
Date: 2012-05-18 12:51
Thanks Petri. I've added the missing s in my repo and attach a proposed separate doc patch, I'd like reviewed by someone whose English is better than mine. :)
Author: Hynek Schlawack (hynek) *
Date: 2012-05-19 15:28
I'm taking Charles-François' review comments here.
- since fwalk() uses O(depth directory tree) file descriptors, we might run out of FD on really deep directory hierarchies. It shouldn't be a problem in practise
Should I mention it in the docs? The old one uses recursion and we don't warn about the stack too...
- there is a slight API change, since the API exposes the function that triggered the failure. I don't think there's a lot a of code that depends on this, but it's definitely a change
I was pondering whether I should "fake" the method names as they pretty much map: listdir instead of fwalk and unlink instead of unlink at… what do you all think about that?
Author: Hynek Schlawack (hynek) *
Date: 2012-05-20 14:05
I’ve incorporated all the feedback (I hope).
I had to refactor the tests slightly, but we have 100% code coverage for both versions of rmtree. Speaking of: I’ve renamed the default version of rmtree to _default_rmtree and _safe_rmtree to _safe_fwalk_rmtree as we’ll need more _safe versions for OS X and Windows. Also, that way the default version isn’t lost.
(this time both docs and code are in one patch, sorry for the experiment)
This might not be the final patch though as I’m waiting on feedback for the backward compatibility question (<http://mail.python.org/pipermail/python-dev/2012-May/119543.html>). That might simplify the tests again a bit if we decide to "fake".
Have a nice Sunday everyone. :)
Author: Hynek Schlawack (hynek) *
Date: 2012-05-21 07:47
A small docs update: I don't list the possible functions for onerror at all anymore, as that list is likely to grow and is a PITA to maintain. People shouldn’t rely on a certain set of functions here.
Martin proposed <http://mail.python.org/pipermail/python-dev/2012-May/119555.html> to re-implement os.fwalk inside of rmdir which I’m -1 <http://mail.python.org/pipermail/python-dev/2012-May/119556.html> on.
If error reporting is the only issue, I’d rather expand fwalk to supply the function that failed to the caller. Be it using a custom field in exceptions or be it using expanding its onerror.
Author: Antoine Pitrou (pitrou) *
Date: 2012-05-21 09:12
Martin proposed <http://mail.python.org/pipermail/python-dev/2012-May/119555.html> to re-implement os.fwalk inside of rmdir which I’m -1 <http://mail.python.org/pipermail/python-dev/2012-May/119556.html> on.
I agree that it isn't worth the hassle.
Author: Martin v. Löwis (loewis) *
Date: 2012-05-21 11:54
I don't think that doing direct flistdir/fstatat calls would be more complex. We already have directory walking implemented (in default_rmtree), and I think the fd-based calls could nicely integrate with that. I'll provide a patch.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-09 11:13
Martin, are you still committed to this? I still think code duplication is bad (especially for security related code) but I’d be willing to write a fwalk-less version so it doesn’t look like I’m just defending my code here.
Author: Martin v. Löwis (loewis) *
Date: 2012-06-09 12:26
Martin, are you still committed to this?
Yes, I'll provide a patch RSN. I doubt that there will be that much code duplication.
Author: Martin v. Löwis (loewis) *
Date: 2012-06-10 20:40
Here is a patch with just the shutil changes. Compared to rmtree-with-fwalk-v3.diff, this changes 90 lines of rmtree, whereas the fwalk version changes only 70 lines. On the plus side, it's much more obvious in this version that the *at variant has the same algorithm as the non-*at version.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-11 11:47
Fair enough, I'm not going to question your obviously superior judgement here. :)
However, your patch currently breaks the test suite on any platform that uses the fallback rmtree: You forgot the ignore_errors=False in the _rmtree_unsafe signature (and obviously also the argument when calling it as a fallback).
You also didn't seem to have touched the tests?
Author: Hynek Schlawack (hynek) *
Date: 2012-06-18 07:36
Martin, what exactly is the intended proceeding now? Are you going to fix your patch and tests as soon as you have time or was that just a PoC and expect me/us to bring it into shape? (<- troll-free question, I have no idea what to do next here and would like to see a safe rmtree in 3.3)
Author: Martin v. Löwis (loewis) *
Date: 2012-06-18 07:53
Martin, what exactly is the intended proceeding now? Are you going to fix your patch and tests as soon as you have time or was that just a PoC and expect me/us to bring it into shape? (<- troll-free question, I have no idea what to do next here and would like to see a safe rmtree in 3.3)
I still plan to work on this, but I'm also really really short on time. I still favor my own approach (obviously), so if you want to bring it into shape - that would be appreciated.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-21 14:59
I'll try to get it in before beta1 then.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-22 17:28
Here is my revised patch with Martin's code integrated.
Differences:
- fixed docs
- added some tests, test_on_errors could still use some refactorings though
- renamed _rmtree_safe to _rmtree_safe_fd so we can implement more of them
- _rmtree_safe_fd & _rmtree_unsafe don't need default arguments and the unsafe version also doesn't need argument normalization as both get onerror filled out by rmtree.
If nobody objects, I'd commit it soonish (definitely before Sunday).
Author: Roundup Robot (python-dev) 
Date: 2012-06-23 16:18
New changeset c910af2e3c98 by Hynek Schlawack in branch 'default': #4489: Add a shutil.rmtree that isn't suspectible to symlink attacks http://hg.python.org/cpython/rev/c910af2e3c98
Author: Roundup Robot (python-dev)
Date: 2012-06-23 18:30
New changeset 53fc7f59c7bb by Hynek Schlawack in branch 'default': #4489: Fix usage of fd-based functions to new api introduced earlier today http://hg.python.org/cpython/rev/53fc7f59c7bb
Author: Éric Araujo (eric.araujo) *
Date: 2012-06-24 06:20
Can I suggest setting a “safe” attribute on the rmtree function object instead of adding another name to the module?
Author: Hynek Schlawack (hynek) *
Date: 2012-06-24 06:25
I thought about that too but couldn't find a precedent (I didn't look very long though :)) where we've done that before so I went for an attribute. I'd change it immediately if others agree.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2012-06-24 06:27
I'm in the process of updating the LBYL support to use a "rmtree.avoids_symlink_attacks" function attribute rather than the "rmtree_is_safe" module level attribute.
As I said in the hmac.secure_compare function discussion, the words "safe" and "secure" are too vague to ever make for good API design. Much better to tell people exactly what they're safe against (rmtree_is_safe -> rmtree.avoids_symlink_attacks), or designed to be appropriate for (hmac.secure_compare -> hmac.compare_digest).
Author: Hynek Schlawack (hynek) *
Date: 2012-06-24 06:30
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).
Author: Larry Hastings (larry) *
Date: 2012-06-24 06:36
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!
Author: Éric Araujo (eric.araujo) *
Date: 2012-06-24 06:42
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) :)
Author: Roundup Robot (python-dev)
Date: 2012-06-24 06:43
New changeset c2be81151994 by Nick Coghlan in branch 'default': Issue #4489: Rename the feature marker for the symlink resistant rmtree and store it as a function attribute http://hg.python.org/cpython/rev/c2be81151994
Author: Larry Hastings (larry) *
Date: 2012-06-24 06:44
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? ;-)
Author: Éric Araujo (eric.araujo) *
Date: 2012-06-24 06:46
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 :-)
Author: Alyssa Coghlan (ncoghlan) *
Date: 2012-06-24 06:46
É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.
Author: Éric Araujo (eric.araujo) *
Date: 2012-06-24 06:48
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.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-24 07:08
Okay everyone, let's call it day – after 3,5 years. :) Further enhancement requests please in separate tickets. Thanks to everybody who took part!
Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) *
Date: 2012-06-24 23:27
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 "", line 1, in 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 $
Author: Larry Hastings (larry) *
Date: 2012-06-24 23:52
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.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2012-06-25 03:18
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.
Author: Larry Hastings (larry) *
Date: 2012-06-25 03:32
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 :)
Author: Roundup Robot (python-dev)
Date: 2012-06-25 11:32
New changeset 2e2329aeb5c1 by Hynek Schlawack in branch 'default': #4489 Make fd based rmtree work on bytes http://hg.python.org/cpython/rev/2e2329aeb5c1
Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) *
Date: 2012-06-27 20:32
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 "", line 1, in 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' $
Author: Roundup Robot (python-dev)
Date: 2012-06-28 10:08
New changeset f9f798f1421e by Hynek Schlawack in branch 'default': #4489: Don't follow ever symlinks in rmtree http://hg.python.org/cpython/rev/f9f798f1421e
Author: Hynek Schlawack (hynek) *
Date: 2012-06-28 10:08
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.
Author: Larry Hastings (larry) *
Date: 2012-06-28 11:22
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.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-28 11:53
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.
Author: Larry Hastings (larry) *
Date: 2012-06-28 12:01
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.
Author: Hynek Schlawack (hynek) *
Date: 2012-06-28 12:33
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.
Author: Roundup Robot (python-dev)
Date: 2012-06-28 13:32
New changeset 9134bb4d0578 by Hynek Schlawack in branch 'default': #4489: Use dir_fd in rmdir in _rmtree_safe_fd() http://hg.python.org/cpython/rev/9134bb4d0578
History
Date
User
Action
Args
2022-04-11 14:56:42
admin
set
github: 48739
2020-10-25 13:33:48
blueyed
set
nosy: + blueyed
pull_requests: + <pull%5Frequest21884>
2012-06-28 13:32:20
python-dev
set
messages: +
2012-06-28 12:33:03
hynek
set
messages: +
2012-06-28 12:01:43
larry
set
messages: +
2012-06-28 11:53:28
hynek
set
messages: +
2012-06-28 11:22:29
larry
set
messages: +
2012-06-28 10:39:44
hynek
set
status: open -> closed
resolution: fixed
stage: resolved
2012-06-28 10:08:32
hynek
set
messages: +
2012-06-28 10:08:09
python-dev
set
messages: +
2012-06-27 20:32:16
Arfrever
set
status: closed -> open
resolution: fixed -> (no value)
messages: +
stage: resolved -> (no value)
2012-06-26 07:21:51
Arfrever
set
status: open -> closed
resolution: fixed
stage: resolved
2012-06-25 11:32:48
python-dev
set
messages: +
2012-06-25 03:32:05
larry
set
messages: +
2012-06-25 03🔞55
ncoghlan
set
messages: +
2012-06-24 23:52:05
larry
set
messages: +
2012-06-24 23:27:41
Arfrever
set
status: closed -> open
priority: normal -> release blocker
messages: +
resolution: fixed -> (no value)
stage: resolved -> (no value)
2012-06-24 07:08:22
hynek
set
status: open -> closed
resolution: fixed
messages: +
stage: patch review -> resolved
2012-06-24 06:48:20
eric.araujo
set
messages: +
2012-06-24 06:46:40
ncoghlan
set
messages: +
2012-06-24 06:46:26
eric.araujo
set
messages: +
2012-06-24 06:44:06
larry
set
messages: +
2012-06-24 06:43:21
python-dev
set
messages: +
2012-06-24 06:42:05
eric.araujo
set
messages: +
2012-06-24 06:36:54
larry
set
messages: +
2012-06-24 06:30:36
hynek
set
messages: +
2012-06-24 06:27:24
ncoghlan
set
nosy: + ncoghlan
messages: +
2012-06-24 06:25:51
hynek
set
messages: +
2012-06-24 06:20:34
eric.araujo
set
nosy: + larry
messages: +
2012-06-23 18:30:11
python-dev
set
messages: +
2012-06-23 16🔞12
python-dev
set
nosy: + python-dev
messages: +
2012-06-22 17:28:52
hynek
set
files: + mvl-revisited-plus-docs.diff
messages: +
2012-06-21 14:59:04
hynek
set
messages: +
2012-06-18 07:53:54
loewis
set
messages: +
2012-06-18 07:36:10
hynek
set
messages: +
2012-06-11 11:47:24
hynek
set
messages: +
2012-06-10 20:40:04
loewis
set
files: + direct_rmtree_safe.diff
messages: +
2012-06-09 12:26:14
loewis
set
messages: +
2012-06-09 11:13:41
hynek
set
messages: +
2012-05-21 11:54:06
loewis
set
messages: +
2012-05-21 09:12:22
pitrou
set
messages: +
2012-05-21 07:47:38
hynek
set
files: + rmtree-with-fwalk-v3.diff
nosy: + loewis
messages: +
2012-05-20 14:05:47
hynek
set
files: + rmtree-with-fwalk-v2.diff
messages: +
2012-05-19 15:28:05
hynek
set
messages: +
2012-05-18 12:51:11
hynek
set
files: + rmtree-with-fwalk-docs-v1.diff
nosy: + georg.brandl, ezio.melotti
messages: +
2012-05-18 11:42:49
petri.lehtinen
set
messages: +
2012-05-18 11:30:39
hynek
set
files: + rmtree-with-fwalk-v1.diff
messages: +
assignee: hynek
keywords: + patch, needs review
stage: needs patch -> patch review
2012-05-10 19:31:59
hynek
set
keywords: - patch
assignee: tarek -> (no value)
dependencies: + fwalk breaks on dangling symlinks
stage: patch review -> needs patch
2012-04-30 13:14:47
Arfrever
set
nosy: + Arfrever
2012-04-29 16:11:14
neologix
set
messages: +
2012-04-27 15:20:41
hynek
set
messages: +
2012-01-09 16:40:01
eric.araujo
set
messages: +
2012-01-09 14:29:21
jcea
set
nosy: + jcea
2012-01-08 06:20:24
rosslagerwall
set
dependencies: + Add a generic directory walker method to avoid symlink attacks
2012-01-08 00:15:51
hynek
set
messages: +
2012-01-07 17:56:23
pitrou
set
messages: +
2012-01-07 13:13:06
hynek
set
messages: +
2011-11-07 19:11:47
rosslagerwall
set
messages: +
2011-11-07 10:54:48
pitrou
set
messages: +
2011-11-05 11:56:49
neologix
set
messages: +
2011-11-05 00:11:26
pitrou
set
messages: +
2011-11-04 23:58:29
neologix
set
nosy: + neologix
messages: +
2011-10-07 19:34:36
rosslagerwall
set
messages: +
2011-10-07 17:35:39
eric.araujo
set
messages: +
2011-09-29 19:42:55
rosslagerwall
set
files: + i4489_v4.patch
messages: +
2011-08-21 11:04:08
petri.lehtinen
set
nosy: + petri.lehtinen
2011-08-21 11:01:53
hynek
set
nosy: + hynek
2011-08-21 10:57:24
eric.araujo
set
messages: +
2011-01-05 18:48:07
pitrou
set
nosy:pitrou, schmir, tarek, eric.araujo, mrts, teamnoir, rosslagerwall
dependencies: + create Python wrappers for openat() and others, Add posix.fdlistdir
2011-01-05 18:19:22
rosslagerwall
set
files: + i4489_v3.patch
nosy:pitrou, schmir, tarek, eric.araujo, mrts, teamnoir, rosslagerwall
messages: +
2011-01-05 17:06:01
pitrou
set
nosy:pitrou, schmir, tarek, eric.araujo, mrts, teamnoir, rosslagerwall
messages: +
2011-01-05 16:58:16
rosslagerwall
set
files: + i4489_v2.patch
nosy:pitrou, schmir, tarek, eric.araujo, mrts, teamnoir, rosslagerwall
messages: +
2011-01-05 15:36:17
schmir
set
nosy: + schmir
2011-01-05 15:21:16
pitrou
set
nosy:pitrou, tarek, eric.araujo, mrts, teamnoir, rosslagerwall
versions: - Python 2.6, Python 2.5, Python 3.1, Python 2.7, Python 3.2
messages: +
stage: needs patch -> patch review
2011-01-05 14:27:03
rosslagerwall
set
files: + i4489.patch
nosy: + rosslagerwall
messages: +
2010-12-22 09:21:02
eric.araujo
set
nosy: + eric.araujo
stage: needs patch
versions: + Python 2.5
2010-12-22 01:13:39
teamnoir
set
nosy: + teamnoir
messages: +
2010-04-20 09:26:07
tarek
set
versions: + Python 3.1, Python 2.7, Python 3.2, Python 3.3, - Python 2.5, Python 2.4, Python 2.3, Python 3.0
nosy: + tarek
messages: +
assignee: tarek
2008-12-29 12:27:27
pitrou
set
messages: +
2008-12-29 11:32:50
pitrou
set
messages: +
2008-12-29 11:26:41
mrts
set
messages: +
2008-12-29 11:19:18
mrts
set
messages: +
2008-12-29 11:15:02
mrts
set
files: + test_issue4489.sh
messages: +
2008-12-29 11:13:35
mrts
set
files: - test_issue4489.sh
2008-12-29 10:05:04
pitrou
set
messages: +
2008-12-29 08:46:05
mrts
set
messages: +
2008-12-29 08:41:50
mrts
set
messages: +
2008-12-29 08:30:40
mrts
set
files: + issue4489_first_attempt.diff
keywords: + patch
messages: +
2008-12-29 08:23:14
mrts
set
files: + test_issue4489.sh
2008-12-29 08:22:55
mrts
set
files: + shutil_patched.py
messages: +
2008-12-28 19:07:15
pitrou
set
messages: +
2008-12-28 16:38:35
mrts
set
messages: +
2008-12-28 14:49:48
pitrou
set
messages: +
2008-12-28 14:38:39
pitrou
set
messages: +
2008-12-28 11:56:31
mrts
set
messages: +
2008-12-27 23:31:55
pitrou
set
messages: +
2008-12-27 23:23:11
pitrou
set
nosy: + pitrou
messages: +
2008-12-02 15:42:01
mrts
create