[issue30400] Race condition in shutil.copyfile(): source file replaced file during copy
Preston Moore added the comment: 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 -- ___ Python tracker <https://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile(): source file replaced file during copy
Preston Moore added the comment: 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 -- ___ Python tracker <https://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile(): source file replaced file during copy
Preston Moore added the comment: 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? -- ___ Python tracker <https://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile()
Preston Moore added the comment: Sure. I'll get everything up to date for 3.8 and create and required test. Thanks! -- ___ Python tracker <https://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile()
Preston Moore added the comment: Pull request is now passing with no conflicts. -- ___ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile()
Preston Moore added the comment: 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? -- ___ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30400] Race condition in shutil.copyfile()
New submission from Preston Moore: 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). -- components: Library (Lib) messages: 293938 nosy: Preston Moore priority: normal severity: normal status: open title: Race condition in shutil.copyfile() type: security versions: Python 2.7, Python 3.7 ___ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue30400> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
