Skip to content

Commit 67be92b

Browse files
hynekhynek
committed
#4489: Add a shutil.rmtree that isn't suspectible to symlink attacks
It is used automatically on platforms supporting the necessary os.openat() and os.unlinkat() functions. Main code by Martin von Löwis.
1 parent 46cb1ef commit 67be92b

File tree

4 files changed

+150
-43
lines changed

4 files changed

+150
-43
lines changed

‎Doc/library/shutil.rst‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -190,14 +190,27 @@ Directory and files operations
190190
handled by calling a handler specified by *onerror* or, if that is omitted,
191191
they raise an exception.
192192

193+
.. warning::
194+
195+
The default :func:`rmtree` function is susceptible to a symlink attack:
196+
given proper timing and circumstances, attackers can use it to delete
197+
files they wouldn't be able to access otherwise. Thus -- on platforms
198+
that support the necessary fd-based functions :func:`os.openat` and
199+
:func:`os.unlinkat` -- a safe version of :func:`rmtree` is used, which
200+
isn't vulnerable.
201+
193202
If *onerror* is provided, it must be a callable that accepts three
194-
parameters: *function*, *path*, and *excinfo*. The first parameter,
195-
*function*, is the function which raised the exception; it will be
196-
:func:`os.path.islink`, :func:`os.listdir`, :func:`os.remove` or
197-
:func:`os.rmdir`. The second parameter, *path*, will be the path name passed
198-
to *function*. The third parameter, *excinfo*, will be the exception
199-
information return by :func:`sys.exc_info`. Exceptions raised by *onerror*
200-
will not be caught.
203+
parameters: *function*, *path*, and *excinfo*.
204+
205+
The first parameter, *function*, is the function which raised the exception;
206+
it depends on the platform and implementation. The second parameter,
207+
*path*, will be the path name passed to *function*. The third parameter,
208+
*excinfo*, will be the exception information returned by
209+
:func:`sys.exc_info`. Exceptions raised by *onerror* will not be caught.
210+
211+
.. versionchanged:: 3.3
212+
Added a safe version that is used automatically if platform supports
213+
the fd-based functions :func:`os.openat` and :func:`os.unlinkat`.
201214

202215

203216
.. function:: move(src, dst)

‎Lib/shutil.py‎

Lines changed: 81 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -337,23 +337,8 @@ def copytree(src, dst, symlinks=False, ignore=None, copy_function=copy2,
337337
raise Error(errors)
338338
return dst
339339

340-
def rmtree(path, ignore_errors=False, onerror=None):
341-
"""Recursively delete a directory tree.
342-
343-
If ignore_errors is set, errors are ignored; otherwise, if onerror
344-
is set, it is called to handle the error with arguments (func,
345-
path, exc_info) where func is os.listdir, os.remove, or os.rmdir;
346-
path is the argument to that function that caused it to fail; and
347-
exc_info is a tuple returned by sys.exc_info(). If ignore_errors
348-
is false and onerror is None, an exception is raised.
349-
350-
"""
351-
if ignore_errors:
352-
def onerror(*args):
353-
pass
354-
elif onerror is None:
355-
def onerror(*args):
356-
raise
340+
# version vulnerable to race conditions
341+
def _rmtree_unsafe(path, onerror):
357342
try:
358343
if os.path.islink(path):
359344
# symlinks to directories are forbidden, see bug #1669
@@ -374,7 +359,7 @@ def onerror(*args):
374359
except os.error:
375360
mode = 0
376361
if stat.S_ISDIR(mode):
377-
rmtree(fullname, ignore_errors, onerror)
362+
_rmtree_unsafe(fullname, onerror)
378363
else:
379364
try:
380365
os.remove(fullname)
@@ -385,6 +370,84 @@ def onerror(*args):
385370
except os.error:
386371
onerror(os.rmdir, path, sys.exc_info())
387372

373+
# Version using fd-based APIs to protect against races
374+
def _rmtree_safe_fd(topfd, path, onerror):
375+
names = []
376+
try:
377+
names = os.flistdir(topfd)
378+
except os.error:
379+
onerror(os.flistdir, path, sys.exc_info())
380+
for name in names:
381+
fullname = os.path.join(path, name)
382+
try:
383+
orig_st = os.fstatat(topfd, name)
384+
mode = orig_st.st_mode
385+
except os.error:
386+
mode = 0
387+
if stat.S_ISDIR(mode):
388+
try:
389+
dirfd = os.openat(topfd, name, os.O_RDONLY)
390+
except os.error:
391+
onerror(os.openat, fullname, sys.exc_info())
392+
else:
393+
try:
394+
if os.path.samestat(orig_st, os.fstat(dirfd)):
395+
_rmtree_safe_fd(dirfd, fullname, onerror)
396+
finally:
397+
os.close(dirfd)
398+
else:
399+
try:
400+
os.unlinkat(topfd, name)
401+
except os.error:
402+
onerror(os.unlinkat, fullname, sys.exc_info())
403+
try:
404+
os.rmdir(path)
405+
except os.error:
406+
onerror(os.rmdir, path, sys.exc_info())
407+
408+
_use_fd_functions = hasattr(os, 'openat') and hasattr(os, 'unlinkat')
409+
def rmtree(path, ignore_errors=False, onerror=None):
410+
"""Recursively delete a directory tree.
411+
412+
If ignore_errors is set, errors are ignored; otherwise, if onerror
413+
is set, it is called to handle the error with arguments (func,
414+
path, exc_info) where func is os.listdir, os.remove, or os.rmdir;
415+
path is the argument to that function that caused it to fail; and
416+
exc_info is a tuple returned by sys.exc_info(). If ignore_errors
417+
is false and onerror is None, an exception is raised.
418+
419+
"""
420+
if ignore_errors:
421+
def onerror(*args):
422+
pass
423+
elif onerror is None:
424+
def onerror(*args):
425+
raise
426+
if _use_fd_functions:
427+
# Note: To guard against symlink races, we use the standard
428+
# lstat()/open()/fstat() trick.
429+
try:
430+
orig_st = os.lstat(path)
431+
except Exception:
432+
onerror(os.lstat, path, sys.exc_info())
433+
return
434+
try:
435+
fd = os.open(path, os.O_RDONLY)
436+
except Exception:
437+
onerror(os.lstat, path, sys.exc_info())
438+
return
439+
try:
440+
if (stat.S_ISDIR(orig_st.st_mode) and
441+
os.path.samestat(orig_st, os.fstat(fd))):
442+
_rmtree_safe_fd(fd, path, onerror)
443+
elif (stat.S_ISREG(orig_st.st_mode)):
444+
raise NotADirectoryError(20,
445+
"Not a directory: '{}'".format(path))
446+
finally:
447+
os.close(fd)
448+
else:
449+
return _rmtree_unsafe(path, onerror)
450+
388451

389452
def _basename(path):
390453
# A basename() variant which first strips the trailing slash, if present.

‎Lib/test/test_shutil.py‎

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -120,29 +120,36 @@ def test_rmtree_errors(self):
120120
def test_on_error(self):
121121
self.errorState = 0
122122
os.mkdir(TESTFN)
123-
self.childpath = os.path.join(TESTFN, 'a')
124-
support.create_empty_file(self.childpath)
123+
self.child_file_path = os.path.join(TESTFN, 'a')
124+
self.child_dir_path = os.path.join(TESTFN, 'b')
125+
support.create_empty_file(self.child_file_path)
126+
os.mkdir(self.child_dir_path)
125127
old_dir_mode = os.stat(TESTFN).st_mode
126-
old_child_mode = os.stat(self.childpath).st_mode
128+
old_child_file_mode = os.stat(self.child_file_path).st_mode
129+
old_child_dir_mode = os.stat(self.child_dir_path).st_mode
127130
# Make unwritable.
128-
os.chmod(self.childpath, stat.S_IREAD)
129-
os.chmod(TESTFN, stat.S_IREAD)
131+
new_mode = stat.S_IREAD|stat.S_IEXEC
132+
os.chmod(self.child_file_path, new_mode)
133+
os.chmod(self.child_dir_path, new_mode)
134+
os.chmod(TESTFN, new_mode)
130135

131136
shutil.rmtree(TESTFN, onerror=self.check_args_to_onerror)
132137
# Test whether onerror has actually been called.
133-
self.assertEqual(self.errorState, 2,
134-
"Expected call to onerror function did not happen.")
138+
self.assertEqual(self.errorState, 3,
139+
"Expected call to onerror function did not "
140+
"happen.")
135141

136142
# Make writable again.
137143
os.chmod(TESTFN, old_dir_mode)
138-
os.chmod(self.childpath, old_child_mode)
144+
os.chmod(self.child_file_path, old_child_file_mode)
145+
os.chmod(self.child_dir_path, old_child_dir_mode)
139146

140147
# Clean up.
141148
shutil.rmtree(TESTFN)
142149

143150
def check_args_to_onerror(self, func, arg, exc):
144151
# test_rmtree_errors deliberately runs rmtree
145-
# on a directory that is chmod 400, which will fail.
152+
# on a directory that is chmod 500, which will fail.
146153
# This function is run when shutil.rmtree fails.
147154
# 99.9% of the time it initially fails to remove
148155
# a file in the directory, so the first time through
@@ -151,20 +158,39 @@ def check_args_to_onerror(self, func, arg, exc):
151158
# FUSE experienced a failure earlier in the process
152159
# at os.listdir. The first failure may legally
153160
# be either.
154-
if self.errorState == 0:
155-
if func is os.remove:
156-
self.assertEqual(arg, self.childpath)
161+
if 0 <= self.errorState < 2:
162+
if (func is os.remove or
163+
hasattr(os, 'unlinkat') and func is os.unlinkat):
164+
self.assertIn(arg, [self.child_file_path, self.child_dir_path])
157165
else:
158-
self.assertIs(func, os.listdir,
159-
"func must be either os.remove or os.listdir")
160-
self.assertEqual(arg, TESTFN)
166+
if self.errorState == 1:
167+
self.assertEqual(func, os.rmdir)
168+
else:
169+
self.assertIs(func, os.listdir, "func must be os.listdir")
170+
self.assertIn(arg, [TESTFN, self.child_dir_path])
161171
self.assertTrue(issubclass(exc[0], OSError))
162-
self.errorState = 1
172+
self.errorState += 1
163173
else:
164174
self.assertEqual(func, os.rmdir)
165175
self.assertEqual(arg, TESTFN)
166176
self.assertTrue(issubclass(exc[0], OSError))
167-
self.errorState = 2
177+
self.errorState = 3
178+
179+
def test_rmtree_does_not_choke_on_failing_lstat(self):
180+
try:
181+
orig_lstat = os.lstat
182+
def raiser(fn):
183+
if fn != TESTFN:
184+
raise OSError()
185+
else:
186+
return orig_lstat(fn)
187+
os.lstat = raiser
188+
189+
os.mkdir(TESTFN)
190+
write_file((TESTFN, 'foo'), 'foo')
191+
shutil.rmtree(TESTFN)
192+
finally:
193+
os.lstat = orig_lstat
168194

169195
@unittest.skipUnless(hasattr(os, 'chmod'), 'requires os.chmod')
170196
@support.skip_unless_symlink
@@ -464,7 +490,7 @@ def test_rmtree_dont_delete_file(self):
464490
# When called on a file instead of a directory, don't delete it.
465491
handle, path = tempfile.mkstemp()
466492
os.close(handle)
467-
self.assertRaises(OSError, shutil.rmtree, path)
493+
self.assertRaises(NotADirectoryError, shutil.rmtree, path)
468494
os.remove(path)
469495

470496
def test_copytree_simple(self):
@@ -629,6 +655,7 @@ def test_rmtree_on_symlink(self):
629655
os.mkdir(src)
630656
os.symlink(src, dst)
631657
self.assertRaises(OSError, shutil.rmtree, dst)
658+
shutil.rmtree(dst, ignore_errors=True)
632659
finally:
633660
shutil.rmtree(TESTFN, ignore_errors=True)
634661

‎Misc/NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ Core and Builtins
4343
Library
4444
-------
4545

46+
- Issue #4489: Add a shutil.rmtree that isn't suspectible to symlink attacks.
47+
It is used automatically on platforms supporting the necessary os.openat()
48+
and os.unlinkat() functions. Main code by Martin von Löwis.
49+
4650
- Issue #15114: the strict mode of HTMLParser and the HTMLParseError exception
4751
are deprecated now that the parser is able to parse invalid markup.
4852

0 commit comments

Comments
 (0)