[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Roundup Robot devn...@psf.upfronthosting.co.za added the comment: New changeset 5f7e71cfbcd6 by Charles-François Natali in branch '3.2': Issue #12464: tempfile.TemporaryDirectory.cleanup() should not follow symlinks: http://hg.python.org/cpython/rev/5f7e71cfbcd6 New changeset c0bae008df81 by Charles-François Natali in branch 'default': Issue #12464: tempfile.TemporaryDirectory.cleanup() should not follow symlinks: http://hg.python.org/cpython/rev/c0bae008df81 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Charles-François Natali neolo...@free.fr added the comment: Committed. Petri, thanks for the patch. -- resolution: - fixed stage: patch review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Petri Lehtinen pe...@digip.org added the comment: Attached an updated patch: - the test now uses support.skip_unless_symlink decorator - added an explicit assertion ensuring that the contents of the linked directory aren't deleted - removed issue reference from the code -- Added file: http://bugs.python.org/file22769/issue12464_v2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Charles-François Natali neolo...@free.fr added the comment: The patch looks good to me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Charles-François Natali neolo...@free.fr added the comment: I agree with Antoine - it's a simple bug Alright, in that case I agree (I thought this was considered as a security issue). Two comments on the patch: Lib/tempfile.py: # Don't recurse to symlinked directories (issue #12464) Is it really necessary to indicate the issue reference here (there's already version control, no need to tag the code itself)? It does make sense in the test, though. Lib/test/test_tempfile.py: def test_cleanup_with_symlink_to_a_directory(self): # cleanup() should not follow symlinks to directories (issue #12464) [...] # Symlink d1/foo - d2 os.symlink(d2.name, os.path.join(d1.name, foo)) Does Windows have symlinks? Looking at Modules/posixmodule.c, I'm not sure, and there seems to be a version of os.symlink() that takes an argument indicating whether the target is a directory or not. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Petri Lehtinen pe...@digip.org added the comment: Charles-François Natali wrote: I agree with Antoine - it's a simple bug Alright, in that case I agree (I thought this was considered as a security issue). Yes. The problem is that cleanup() does not delete the temporary directory but deletes files in the linked directory, even if it's outside the temporary directory. Two comments on the patch: Lib/tempfile.py: # Don't recurse to symlinked directories (issue #12464) Is it really necessary to indicate the issue reference here (there's already version control, no need to tag the code itself)? It does make sense in the test, though. True. Lib/test/test_tempfile.py: def test_cleanup_with_symlink_to_a_directory(self): # cleanup() should not follow symlinks to directories (issue #12464) [...] # Symlink d1/foo - d2 os.symlink(d2.name, os.path.join(d1.name, foo)) Does Windows have symlinks? Looking at Modules/posixmodule.c, I'm not sure, and there seems to be a version of os.symlink() that takes an argument indicating whether the target is a directory or not. According to the documentation of os.symlink(), symlinks are available on Windows 6.0 (Vista) and later, so the test should check for this. I'm not sure how this can be done, though, as there's no requires_windows_version() decorator in test.support. The documentation also says that the extra parameter of os.symlink() only has an effect it the symlink target does not exist when the link is created. In the test it does, so I think os.link() and os.symlink() should both work correctly, also on Windows. If someone suggests how to test for the Windows version, I'll update the patch, also to remove the issue reference from the code. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Charles-François Natali neolo...@free.fr added the comment: If someone suggests how to test for the Windows version, I'll update the patch, also to remove the issue reference from the code. Well, I don't know Windows, but there's platform.win32_ver() (http://docs.python.org/library/platform.html#platform.win32_ver) that could probably be useful. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Antoine Pitrou pit...@free.fr added the comment: You can simply use support.skip_unless_symlink(). Charles-François Natali neolo...@free.fr added the comment: If someone suggests how to test for the Windows version, I'll update the patch, also to remove the issue reference from the code. Well, I don't know Windows, but there's platform.win32_ver() (http://docs.python.org/library/platform.html#platform.win32_ver) that could probably be useful. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Petri Lehtinen pe...@digip.org added the comment: Attached a patch that fixes the issue and adds a test case for it. -- keywords: +needs review, patch stage: - patch review Added file: http://bugs.python.org/file22754/issue12464.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Petri Lehtinen pe...@digip.org added the comment: Adding potential reviewers to nosy list. -- nosy: +georg.brandl, ncoghlan ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Charles-François Natali neolo...@free.fr added the comment: I'm not sure I see what the problem is: - if the idea behind this is the risk of symlink attack (like issue #4489), it's not the case here, because the directory is created with 0600 permission - furthermore, the attached patch has a TOCTTOU race, between the the call to os.path.islink() and the call to rmtree() So I'd like to know the problem we're trying to solve here. -- nosy: +neologix ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Antoine Pitrou pit...@free.fr added the comment: Without even mentioning the possibility attacks, I think it's wrong for the cleanup routine to follow symbolic links. It should instead simply remove the links, and not mess with anything outside of the temporary directory. Note that shutil.rmtree() does the right thing by calling lstat(). TemporaryDirectory interestingly uses a stripped down version of rmtree which doesn't retain that subtlety. -- nosy: +pitrou ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Nick Coghlan ncogh...@gmail.com added the comment: I agree with Antoine - it's a simple bug introduced by the attempt to allow temporary directories to be correctly cleaned up during interpreter shutdown. The race condition due to the usage of LBYL is shared with the full shutil.rmtree implementation, so the patch looks reasonable to me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
Changes by Petri Lehtinen pe...@digip.org: -- nosy: +petri.lehtinen ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue12464] tempfile.TemporaryDirectory.cleanup follows symbolic links
New submission from Evgeny Kapun abacabadabac...@gmail.com: TemporaryDirectory.cleanup follows symbolic links to directories and tries to clean them as well. Try this (on Linux): import os, tempfile with tempfile.TemporaryDirectory() as d: os.symlink(/proc, d + /test) -- components: Library (Lib) messages: 139571 nosy: abacabadabacaba priority: normal severity: normal status: open title: tempfile.TemporaryDirectory.cleanup follows symbolic links type: behavior versions: Python 3.2, Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue12464 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
