classification
Title: tempfile.TemporaryDirectory.cleanup follows symbolic links
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: abacabadabacaba, georg.brandl, ncoghlan, neologix, petri.lehtinen, pitrou, python-dev
Priority: normal Keywords: needs review, patch

Created on 2011-07-01 12:30 by abacabadabacaba, last changed 2011-07-29 18:24 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
issue12464.patch petri.lehtinen, 2011-07-25 18:54 review
issue12464_v2.patch petri.lehtinen, 2011-07-27 10:16 review
Messages (14)
msg139571 - (view) Author: Evgeny Kapun (abacabadabacaba) Date: 2011-07-01 12:30
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")
msg141112 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-25 18:54
Attached a patch that fixes the issue and adds a test case for it.
msg141114 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-25 18:58
Adding potential reviewers to nosy list.
msg141122 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-25 20:30
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.
msg141129 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-25 23:17
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.
msg141134 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2011-07-26 01:56
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.
msg141138 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-26 07:45
> 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.
msg141142 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-26 10:31
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.
msg141180 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-26 20:57
> 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.
msg141182 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-26 21:03
You can simply use support.skip_unless_symlink().

> Charles-François Natali <neologix@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.
msg141215 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-27 10:16
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
msg141240 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-27 16:43
The patch looks good to me.
msg141400 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-29 17:00
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
msg141405 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-29 18:24
Committed.
Petri, thanks for the patch.
History
Date User Action Args
2011-07-29 18:24:53neologixsetstatus: open -> closed
resolution: fixed
messages: + msg141405

stage: patch review -> resolved
2011-07-29 17:00:22python-devsetnosy: + python-dev
messages: + msg141400
2011-07-27 16:43:50neologixsetmessages: + msg141240
2011-07-27 10:16:07petri.lehtinensetfiles: + issue12464_v2.patch

messages: + msg141215
2011-07-26 21:03:55pitrousetmessages: + msg141182
2011-07-26 20:57:48neologixsetmessages: + msg141180
2011-07-26 10:31:59petri.lehtinensetmessages: + msg141142
2011-07-26 07:45:10neologixsetmessages: + msg141138
2011-07-26 01:56:08ncoghlansetmessages: + msg141134
2011-07-25 23:17:40pitrousetnosy: + pitrou
messages: + msg141129
2011-07-25 20:30:10neologixsetnosy: + neologix
messages: + msg141122
2011-07-25 18:58:54petri.lehtinensetnosy: + georg.brandl, ncoghlan
messages: + msg141114
2011-07-25 18:54:20petri.lehtinensetkeywords: + patch, needs review
files: + issue12464.patch
messages: + msg141112

stage: patch review
2011-07-03 20:09:28petri.lehtinensetnosy: + petri.lehtinen
2011-07-01 12:30:45abacabadabacabacreate