Race condition in shutil.copyfile(): source file replaced file during copy

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

BPO	30400
Nosy	@vstinner, @giampaolo, @zware, @pkmoore
PRs	python/cpython#1659

^{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: ```python assignee = None closed_at = None created_at = labels = ['type-security', '3.8', 'library'] title = 'Race condition in shutil.copyfile(): source file replaced file during copy' updated_at = user = 'https://github.com/pkmoore' ``` bugs.python.org fields: ```python activity = actor = 'Preston Moore' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'Preston Moore' dependencies = [] files = [] hgrepos = [] issue_num = 30400 keywords = [] message_count = 12.0 messages = ['293938', '293940', '296037', '321069', '321201', '321278', '321468', '321640', '321649', '321696', '322235', '323501'] nosy_count = 4.0 nosy_names = ['vstinner', 'giampaolo.rodola', 'zach.ware', 'Preston Moore'] pr_nums = ['1659'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'security' url = 'https://bugs.python.org/issue30400' versions = ['Python 3.8'] ```

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

A race condition exists in shutil.copyfile() that allows the file being copied to be replaced between the time it was initially checked with stat() in this function and when it is actually open()'d and copied. This issue can be triggered from shutil.move() (and likely other places) when attempting to move a file from one location to another where the source and destination are on different devices. This behavior can be replicated by setting a catchpoint in gdb on calls to stat() and, after the initial call to stat in shutil.copyfile(), replacing the source file.

The attached pull request addresses this issue by storing the inode number of the source file when it is initially stat()'d and comparing this value to an inode value taken from a call to fstat() after the file is open. If these two values differ, the file has been replaced. This is the pattern employed by coreutil's mv utility in an effort to address this issue.

This bug was found as part of an ongoing research effort into detecting and addressing bugs caused by differences in the environments in which an application may be run (the environmental issue in this place being the difficulties around correctly copying a file from one disk to another).

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

It looks like the PR is not passing 3 tests. I think this might be a result of the mock file objects not having inode numbers? Any feedback on this front?

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

Pull request is now passing with no conflicts.

[zware]

This can only be accepted into 3.8 at this point due to the new exception type.

The patch looks fairly straightforward; would it be possible to add a test that hits the new raise condition?

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

Sure. I'll get everything up to date for 3.8 and create and required test.

Thanks!

[giampaolo]

All copy* functions and move() are subjects to race conditions (the only exception is rmtree()). You may solve the race condition in copyfile() but then you'd have the same problem in copystat() and copymode() which are used by copy() and copy2(). The definitive solution to this problem would theoretically be to have all these interconnected functions pass fds instead of "paths" but of course that is hardly possible. Personally I don't think this issue is worth being fixed.

[vstinner]

The definitive solution to this problem would theoretically be to have all these interconnected functions pass fds instead of "paths" but of course that is hardly possible.

I agree that it's the way to go.

Python makes little difference between a path and file descriptor. For example, os.stat() accepts both a filename (string) or a file descriptor (integer).

Even if we cannot fix everything, it makes sense to me to try to fix at least shutil.copyfile(). Even we cannot modify existing other public copy*() functions to handle file descriptors, maybe we can add private helper functions accepting file descriptors, and modify existing public functions to reuse them?

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

I like Victor's idea for updating public functions to support file descriptors. I could submit a patch for this instead if desired.

In the meantime, I've updated the pull request for this issue so the patch I originally created that compares inode numbers applies to 3.8. I've included a test case as well.

I think Victor's idea may be superior to the inode comparison. It is in the same spirit as glibc preferring openat() rather than open() (https://lwn.net/Articles/738694/) to counter directories changing during an operation.

Alternatively, coreutils (https://github.com/coreutils/coreutils/blob/439741053256618eb651e6d43919df29625b8714/src/copy.c#L1051) deals with this issue using inode comparison. Perhaps this is because they don't have a clean way of supporting both paths and fds like we do with Python.

This overall issue has extra cause for concern because it can be exploited by an attacker as a security vulnerability (https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files). Additionally, Python projects have encountered similar bugs when re-implementing file operations themselves (https://code.djangoproject.com/ticket/8479)

I would be happy to fix this bug in any other spots identified using whichever strategy is preferred. If it is possible to get functions operating on file descriptors without breaking public functions I think that is the strategy we should prefer. If that is not possible, the above inode comparison strategy provides improvement if not a complete fix.

Should I submit additional bug reports for these other cases or should I just follow on here?

[giampaolo]

For example, os.stat() accepts both a filename (string) or a file descriptor (integer).

Interesting. I never realized that. Other functions involved in file copy are:

os.lchmod os.chmod os.listxattr os.getxattr

I checked and it appears they all support fd args (this is not documented and it should). os.path.* all use os.stat() internally so they can easily be replaced. I wonder whether this can introduce any change in semantic though:

>>> import os
>>> os.stat('foobar')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: 'foobar'
>>> os.stat(333)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 9] Bad file descriptor: 333

Also wonder it this would play nice on Windows.

[vstinner]

Python doesn't use FD of directories by default because it introduces issues of FD limit with deep directory tree, issue of managing FD lifetime, etc. That's why some API have one flavor for path and another for FD.

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

Hey everyone,

I have updated the pull request to include a version of copyfile() that attempts to address the discussed race condition by open()’ing a file descriptor to the relevant files as early as possible and maintaining it throughout processing. In order for this to work I had to use some lower level posix-only features. Specifically, I had to open() the files non-blocking initially and using fcntl() to make them blocking once some initial checks have been performed. The functions behavior under Windows should be unchanged. I chose this course because Windows doesn’t support fcntl() or os.open() with O_NONBLOCK.

I will add a few additional tests around the new checks and get things ready for a merge. I would also be glad to use a strategy of this nature to fix similar bugs in the other areas discussed above.

Thanks, Preston

[f9247ab1-c828-45bc-a688-e41f8c22bc36]

Hello everyone,

I've just updated my pull request to include an additional test so everything should be in shape testing wise.

Once I get confirmation that this strategy is acceptable and/or this PR is merged I will get to work addressing the other identified problem areas in a similar fashion. There are a few more weeks before classes start back up for the fall semester so I should have available bandwidth to make good progress.

Thanks, Preston