Add a generic directory walker method to avoid symlink attacks

[hynek]

BPO	13734
Nosy	@jcea, @ncoghlan, @pitrou, @benjaminp, @tarekziade, @hynek
Files	fdwalk-2.diff fwalk-3.diff fwalk-single_fd.diff fwalk-doc.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 = None
closed_at = <Date 2012-02-05.15:19:30.863>
created_at = <Date 2012-01-08.00:14:02.897>
labels = ['type-security', 'library']
title = 'Add a generic directory walker method to avoid symlink attacks'
updated_at = <Date 2012-05-12.19:52:12.003>
user = 'https://github.com/hynek'

bugs.python.org fields:

activity = <Date 2012-05-12.19:52:12.003>
actor = 'benjamin.peterson'
assignee = 'none'
closed = True
closed_date = <Date 2012-02-05.15:19:30.863>
closer = 'neologix'
components = ['Library (Lib)']
creation = <Date 2012-01-08.00:14:02.897>
creator = 'hynek'
dependencies = []
files = ['24211', '24382', '24383', '25550']
hgrepos = []
issue_num = 13734
keywords = ['patch']
message_count = 29.0
messages = ['150833', '150838', '150845', '150855', '150858', '150864', '150870', '150873', '150916', '150919', '150925', '150991', '151032', '151077', '152376', '152377', '152378', '152385', '152386', '152387', '152408', '152410', '152425', '152426', '152689', '152691', '160376', '160472', '160490']
nosy_count = 9.0
nosy_names = ['jcea', 'ncoghlan', 'pitrou', 'benjamin.peterson', 'tarek', 'neologix', 'rosslagerwall', 'python-dev', 'hynek']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue13734'
versions = ['Python 3.3']

Linked PRs

• gh-57943: disambiguate issue number in os._fwalk comment #137327

[hynek]

This is an offspring of bpo-4489 (which is a security bug). The method is AFAIU intended to be private.

As shown in the discussion of the mentioned bpo-4489, there is a whole family of attacks that exploit the time window between gathering path names and executing a function on them. A general description of this problem can be found in: https://www.securecoding.cert.org/confluence/display/seccode/POS35-C.+Avoid+race+conditions+while+checking+for+the+existence+of+a+symbolic+link

While the consequences in rmtree() are probably most dramatic, other recursive functions could benefit too (chmodtree() and chowntree() were mentioned) so Charles-François suggested to write a "generic walker method that would take as argument the methods to call on a directory and on a file (or link)".

Some (probably) necessary helper functions has been already implemented in bpo-4761 (*at()) and bpo-10755 (fdlistdir()).

Has there already been done any work? Ross mentioned he wanted to take a stab?

[ncoghlan]

I'm working on a library of general directory walking tools that will hopefully make their way back into the stdlib at some point (http://walkdir.readthedocs.org).

They're designed to filter and transform the output of os.walk (and similar iterators) in various ways.

It may provide a good environment for prototyping a general purpose "tree_map" for applying an operation to a filesystem tree without being vulnerable to symlink attacks.

[rosslagerwall]

Has there already been done any work? Ross mentioned he wanted to take a stab?

Unfortunately, I'm rather busy at the moment but when I get some free time and if no one else wants to work on it then I'll take a look.

[ncoghlan]

Since walkdir is currently entirely based on returning filesystem paths as strings (just like os.walk()) and hence shares the pervasive symlink attack vulnerability, I'm particularly interested in the question of whether or not the various *at APIs can be used to avoid symlink attacks if we just have a os.walkfd() API that emits a (dirfd, subdirs, files) triple instead of the os.walk style (dirpath, subdirs, files).

The reason I'd find that interesting is that many of walkdir's filtering steps (notably those for including and excluding directories) don't care about the value of dirpath, so they could still be used with such an API.

Thoughts?

[ncoghlan]

Another, possibly better, alternative would be to produce a tuple-subclass that adds a separate "dirfd" attribute to the (dirpath, subdirs, files) triple.

I'll stop talking about the walkdir implications here. Instead, I've created a corresponding issue on walkdir's own tracker:
https://bitbucket.org/ncoghlan/walkdir/issue/8/add-walkfd

[pitrou]

Since walkdir is currently entirely based on returning filesystem
paths as strings (just like os.walk()) and hence shares the pervasive
symlink attack vulnerability, I'm particularly interested in the
question of whether or not the various *at APIs can be used to avoid
symlink attacks if we just have a os.walkfd() API that emits a (dirfd,
subdirs, files) triple instead of the os.walk style (dirpath, subdirs,
files).

Be aware that you have to manage dirfd's lifetime, which can make things
interesting.
Also be aware that symlinks mean sometimes you won't have a dirfd: if
you have a symlink that points to another directory, you can't open that
directory using openat from the symlink's directory. So if you follow
symlinks (or have an option to do so) you must also take that case into
account.

[neologix]

Here's a possible walkfd() implementation.

Example:
"""
$ cat /home/cf/testwalkfd.py
import os
import sys

topfd = os.open(sys.argv[1], os.O_RDONLY)

for rootfd, dirs, files in os.walkfd(topfd):
    print(rootfd, dirs, files)
$ ./python ~/testwalkfd.py /etc/apt/
3 ['sources.list.d', 'preferences.d', 'trusted.gpg.d', 'apt.conf.d']
['trustdb.gpg', 'trusted.gpg~', 'sources.list', 'trusted.gpg']
4 [] []
4 [] []
4 [] []
4 [] ['70debconf', '01autoremove', '00trustcdrom']
[44194 refs]
"""

AFAICT, a safe rmtree could be implemented simply with
walkfd(topdown=False), but Antoine's remarks make me thing I missed
something.

Be aware that you have to manage dirfd's lifetime, which can make things
interesting.

Basically, this means that doing:
for rootfd, dirs, files in walkfd(topfd):
print(fstat(rootfd), dirs, files))

is valid whereas

print([(fstat(rootfd), dirs, files) for (rootfd, dirs, files) in
walkfd(topfd)]) isn't.

Also be aware that symlinks mean sometimes you won't have a dirfd: if
you have a symlink that points to another directory, you can't open that
directory using openat from the symlink's directory. So if you follow
symlinks (or have an option to do so) you must also take that case into
account.

I'm not sure I understand this. Why "you can't open that directory
using openat from the symlink's directory". Could you elaborate?

[pitrou]

> Also be aware that symlinks mean sometimes you won't have a dirfd: if
> you have a symlink that points to another directory, you can't open that
> directory using openat from the symlink's directory. So if you follow
> symlinks (or have an option to do so) you must also take that case into
> account.

I'm not sure I understand this. Why "you can't open that directory
using openat from the symlink's directory". Could you elaborate?

Hmm, sorry, I must have misremembered. I thought openat didn't follow
symlinks.

As for the patch, I think there's a problem with the API:

+ This behaves exactly like walk(), except that it accepts a file descriptor
+ as top directory, and yields a 3-tuple
+
+ dirfd, dirnames, filenames

It doesn't tell you to which dirname corresponds dirfd, so you don't
know the path of the directory you are handed (which can be useful for
progress report, error report, or anything else where you need the name

• e.g. making a zip archive of the directory). Also giving the dirnames
  without their fds encourages using them by name, not by fd ;-)

Also, walkfd would be easier to use if callable with a str or bytes path
rather than an int fd.

[ncoghlan]

Thanks for that Charles-François - do you mind if I adapt that for walkdir?

The changes I would make are basically those that Antoine pointed out:

• rather than replacing the dirpath entry, instead yield a 4-tuple that appends the dirfd attribute at the end*
• if the dup(fd) fails, fall back to assuming top is a string or bytes path

*I'm still interested in opinions on this aspect. I see 5 main possibilities:

• (dirfd, subdirs, files) triple (problematic for the reasons Antoine pointed out)
• (dirpath, subdirs, files, dirfd) 4-tuple
• ((dirpath, dirfd), subdirs, files) nested tuple
• (dirpath, subdirs, files) tuple subclass with separate dirfd attribute
• (dirpath, subdirs, files) triple with dirpath as a str subclass with a separate fd attribute

I'm currently leaning towards the simple 4-tuple approach - it's simple, explicit and the walkdir pipeline operations can easily accept either underlying iterable by using indexing operations rather than tuple unpacking (a change I already planned to make so that the pipeline passed along the objects from the underlying iterable, anyway)

(I'll also need to create ctypes-based variants of all the relevant *at functions, since the stdlib wrappers won't be available in existing versions of Python)

[rosslagerwall]

I'm currently leaning towards the simple 4-tuple approach

I would also take that approach. It seems simplest to me.

[neologix]

Hmm, sorry, I must have misremembered. I thought openat didn't follow
symlinks.

OK, I was afraid I had missed something.

> As for the patch, I think there's a problem with the API

Yes, it was really a proof-of-concept, the directory names are missing.

Also, walkfd would be easier to use if callable with a str or bytes path
rather than an int fd.

Agreed.

Also giving the dirnames without their fds encourages using them
by name, not by fd ;-)

Well, that's not easy:

• right now, the code uses O(depth of directory hierarchy) FDs
• returning FDs for sub-directories would require O(number of nodes in
  the hierarchy), or at least O(max number of child nodes): I fear we'll
  run out of FDs quite easily
  Also, I don't think it's really a problem, since you have to use the
  names anyway. The *at() family accepts a FD as a pointer to the
  containing directory, but the target entries are accessed by name.
  For example, to perform a safe rmtree, you would do something like:

for dirfd, dirs, files in os.walkfd(topfd, topdown=False):
    for file in files:
        os.unlinkat(dirfd, file)
    for dir in dirs:
        os.unlinkat(dirfd, dir, os.AT_REMOVEDIR)

Thanks for that Charles-François - do you mind if I adapt that for walkdir?

Of course not, go ahead. I'll update walkfd() accordingly, and write
doc and test for it.
By the way, do you plan to get walkdir merged in 3.3?
I've been doing a lot of sys-admin scripts lately, and this would be
really helpful.

I'm currently leaning towards the simple 4-tuple approach

Sounds good to me.

[ncoghlan]

OK, os.walkfd is sounding good:

• accepts a file descriptor, byte sequence or string for "top"
• produces 4-tuples, with the dirfd added at the end
• documents clearly that the dirfd is normally only valid until the next iteration step, so you need to call os.dup() if you want to hang onto it

As far as walkdir integration goes, I currently plan to add it as a "Directory Walking" subsection in shutil before the first alpha. However, it needs a few more updates in PyPI first (e.g. preserving the tuples produced by the underlying iterators, making sure it behaves itself when handed binary paths). I'll post to python-dev about it before I actually commit anything.

[neologix]

Here's a patch with tests and documentation.
I noticed something surprising:
walk() with followlinks=False returns links to directories as
directories (in dirnames).
I find this surprising, since if you don't follow symlinks, those are
just files (and you don't recurse into it). Also, it's a pain when you
want to remove dirnames, since you have to distinguish between a link
and a directory (unlink()/rmdir() or unlinkat() without/with
AT_REMOVEDIR)
To be consistent with this behavior, I had to change fdwalk() (I
renamed it to be consistent with fdlistdir()) to perform a call to
fstatat() without AT_SYMLINK_NOFOLLOW, since otherwise it would report
such links as files.
So the bottom line is that because of this, you can have up to 3
stat() calls per entry:

• fstatat(rootfd, name)
• fstatat(rootfd, name, AT_SYMLINK_NOFOLLOW) right before opening the directory
• fstat(dirfd) right after open to check that we're dealing with the same file
  (walk() currently uses two stat() per entry, so it's not too bad).