classification
Title: sys.path[0] security issues
Type: security Stage: needs patch
Components: Distutils, Interpreter Core Versions: Python 3.4, Python 3.3, Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: Alan.WiIliams, Arfrever, benjamin.peterson, christian.heimes, eric.araujo, eric.snow, georg.brandl, hasufell, haypo, hynek, iankko, jdemeyer, jwilk, ncoghlan, robertwb, schmir, tarek, vbraun
Priority: normal Keywords: patch

Created on 2012-10-11 20:10 by jdemeyer, last changed 2014-01-30 13:54 by jwilk.

Files
File name Uploaded Description Edit
fix_distutils.patch robertwb, 2012-10-12 16:03 review
fix_distutils.patch robertwb, 2012-10-12 18:36 review
sys_path_security.patch jdemeyer, 2012-11-08 13:39
Messages (20)
msg172686 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-10-11 20:10
There is a serious security problem with Python's default sys.path.  If I execute

$ python /tmp/somescript.py

then Python will add /tmp as sys.path[0], such that an "import foobar" will cause Python to read /tmp/foobar (or variations).  This vulnerability exists in particular in distutils.util.byte_compile() with direct=False.  Since the Python test suite calls this function, users running the Python test suite are vulnerable.

I think the root of this issue should be fixed: Python should not simply add stuff to sys.path without checking.  In prepared a patch for CPython-2.7 which only adds sys.path[0] if it seems secure to do so, by looking at file/directory permissions and ownership.  In particular, it would never add /tmp to sys.path, but it would still keep the current behaviour when running a script in a directory owned by the current user with 0755 permissions.  See the patch for details.

I realize this goes against documented Python behaviour, but I think that a broken spec needs to be fixed.  I also think that in most use cases, nothing is going to change because normally one doesn't need to import from /tmp.  In any case, users can still manipulate sys.path directly.

Feel free to fix this issue in a different way than my patch, but I hope you at least implement the spirit of my patch.  The patch has only been tested on Linux systems.

Credit goes to Volker Braun for first discovering this issue in Sage, see http://trac.sagemath.org/sage_trac/ticket/13579
msg172709 - (view) Author: Robert Bradshaw (robertwb) Date: 2012-10-11 23:06
Alternatively, one could fix distutils.util.byte_compile() to execute the script in safe, empty temp directory.  Running scripts in /tmp remains, as it has always been, a bad idea.

Trying to determine if an import is "safe" can be arbitrarily complicated (e.g. what if the group-write bit is set, but you're the only member of that group, or there are special allow or deny ACLs for other users that aren't detected here).  What notion of safeness belongs in the spec?
msg172722 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-10-12 07:32
Robert: I don't think that running scripts in /tmp is inherently unsafe.  It is Python's sys.path handling which makes it unsafe.  That being said, I am not against distutils being "fixed" but I do think the root issue should be fixed.

And of course you're right about complicated permission checking and ACLs and what not.  But I think my patch does the Right Thing in 99% of the cases, in particular for /tmp.  I tried to err on the safe side.
msg172736 - (view) Author: Volker Braun (vbraun) Date: 2012-10-12 10:02
The fact that Python's own testsuite tripped over this proves that this is subtle enough to merit some special handling.

1) It is not, and has never been, a good idea to run/compile anything off /tmp. This isn't specific to Python, it is just common sense that you don't hand over control of directory contents to others.

2) Removing /tmp from sys.path upon startup is not enough to guarantee safety. Many Python modules will happily add it back. Just as a random example, see profile.py: "sys.path.insert(0, os.path.dirname(progname))". The aim of the patch should be to warn the user of the dangers of running code in /tmp, not trying to make it safe (and, therefore, implicitly encouraging it).

3) The patch is too restrictive in my opinion, it rules out some plausible and perfectly safe use cases. For example, root owns directory and wheel owns Python script. Or sharing a group with a trusted user. Just disallowing o+w would be enough to save the unwary from executing in /tmp.
msg172750 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-12 15:08
Robert Bradshaw's idea is the only feasible option for Python 2.7 or any other version except for 3.4dev. Your suggested modification to sys.path is out of option as it would create a backwards incompatibility with existing software.

I'm adding 2.6 to 3.4 as all versions of Python are affected.
msg172755 - (view) Author: Robert Bradshaw (robertwb) Date: 2012-10-12 16:03
Here's a fix to distutils.  I think at least a warning is in order for running scripts from insecure directories, and ideally some happy medium can be found.
msg172756 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-12 16:13
I'm all in favor for the proposal to add a warning when the script is in a world-writable directory. But any modification can't be added to stable version as it's a new feature.

Robert, you have to cleanup and remove the directory manually at the end of the block. mkdtemp() creates the directory but doesn't do house keeping.
msg172762 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-10-12 16:54
If you don't plan any further Python-2 releases, it would be pity that this cannot be fixed.  If you do plan a further Python-2 release, I find backwards compatibility a poor excuse.  I'm not saying that backwards compatibility should be totally ignored, but it certainly should not trump everything either, especially for security issues.  I carefully designed my patch to have no impact for most existing secure setups (but as I said, I'm open for improvements).
msg172764 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-10-12 17:15
Ultimately it's Benjamin's and Georg's decision. They are the release managers of 2.7 to 3.3 and need to come to an agreement. You have to convince them that the proposed security restriction is worth the risk of breaking 3rd party software. 

It would help if you describe the circumstances under which your patch doesn't add the module's path to sys.path.
msg172766 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-10-12 18:08
disutils should definitely be fixed.
msg172768 - (view) Author: Robert Bradshaw (robertwb) Date: 2012-10-12 18:36
Good point about cleanup, patch updated.
msg172798 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-10-13 06:57
Definite +1 on distutils needing to be fixed in the upcoming maintenance releases for 2.7, 3.2 and 3.3.

-1 on doing the strict path security checks on a normal invocation, -0 on doing them when -S or -E have been passed in, +0 if it is *just* a warning to users that they're doing something risky, but proceeds with normal (backwards compatible) sys.path initialisation.

For 3.4, I plan to have a look at the organically-grown-over-time mess that is CPython's current interpreter initialisation system and see if I can figure out something a bit more sane and easier to configure/control (especially when embedding Python in a larger application) :P
msg172799 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-10-13 07:00
Also, what's up with that weird fallback code in distutils? When is tempfile.mkdtemp ever missing?
msg172803 - (view) Author: Volker Braun (vbraun) Date: 2012-10-13 10:20
> When is tempfile.mkdtemp ever missing

It was added in Python 2.3, in the dark ages before that there was only tempfile.mktemp. Though I guess we can remove the fallback now...
msg172812 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-10-13 17:05
> For 3.4, I plan to have a look at the organically-grown-over-time mess
> that is CPython's current interpreter initialisation system and see if I
> can figure out something a bit more sane and easier to configure/control
> (especially when embedding Python in a larger application) :P

I'm totally on board with any effort in this regard.  Sign me up if you need a hand.
msg172981 - (view) Author: Jan Lieskovsky (iankko) Date: 2012-10-15 15:06
Jeroen,

  just out of curiosity. Is the current issue different from
CVE-2008-5983 (at first quick glance it looks the be the same issue):?
[1] http://bugs.python.org/issue5753

Thank you, Jan.
--
Jan iankko Lieskovsky
msg172983 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-10-15 15:38
It's actually the same as #946373 - it's not about adding the current directory to sys.path, it's adding the directory of a script that's in a world-writable directory (such as /tmp).

The difference is that the proposed solution this time recognises that simply not adding that directory would break the world, so it aims for a more nuanced approach (plus distutils itself writing a script to /tmp and then running it is just plain wrong).
msg172998 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-10-15 19:56
It's sort of the same as #946373, except that bug report deals with other bad consequences of sys.path[0], unrelated to security.

#5753 is specifically about the C API, not about running "plain" Python.
msg173000 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-10-15 20:24
I should point out that there is also dangerous code in Lib/test/test_subprocess.py in the test_cwd() function.  There, the following is executed from /tmp:

  python -c 'import sys,os; sys.stdout.write(os.getcwd())'

As Python luckily knows where to import sys and os from, this doesn't seem exploitable, but it should be fixed.
msg175155 - (view) Author: Jeroen Demeyer (jdemeyer) Date: 2012-11-08 13:42
I updated sys_path_security.patch by a newer version. This version will be merged in the Python package of Sage (http://www.sagemath.org/).

I realise that it looks unlikely that it will be merged in CPython, but at least it's here for reference.
History
Date User Action Args
2014-01-30 13:54:41jwilksetnosy: + jwilk
2012-11-08 13:42:41jdemeyersetmessages: + msg175155
2012-11-08 13:39:39jdemeyersetfiles: - sys_path_security.patch
2012-11-08 13:39:26jdemeyersetfiles: + sys_path_security.patch
2012-10-15 20:24:59jdemeyersetmessages: + msg173000
2012-10-15 19:56:09jdemeyersetmessages: + msg172998
2012-10-15 15:38:06ncoghlansetmessages: + msg172983
2012-10-15 15:06:39iankkosetnosy: + iankko
messages: + msg172981
2012-10-14 13:32:48hasufellsetnosy: + hasufell
2012-10-13 17:05:08eric.snowsetnosy: + eric.snow
messages: + msg172812
2012-10-13 10:20:57vbraunsetmessages: + msg172803
2012-10-13 07:00:14ncoghlansetmessages: + msg172799
2012-10-13 06:57:01ncoghlansetmessages: + msg172798
2012-10-13 04:20:49eric.araujosetnosy: + ncoghlan, eric.araujo, tarek

components: + Distutils
assignee: eric.araujo
2012-10-12 21:35:59hayposetnosy: + haypo
2012-10-12 18:36:44robertwbsetfiles: + fix_distutils.patch

messages: + msg172768
2012-10-12 18:08:40benjamin.petersonsetmessages: + msg172766
2012-10-12 17:15:13christian.heimessetnosy: + georg.brandl, benjamin.peterson
messages: + msg172764
2012-10-12 16:54:45jdemeyersetmessages: + msg172762
2012-10-12 16:13:56christian.heimessetmessages: + msg172756
2012-10-12 16:03:56robertwbsetfiles: + fix_distutils.patch

messages: + msg172755
2012-10-12 15:08:31christian.heimessetstage: needs patch
messages: + msg172750
versions: + Python 2.6, Python 3.1, Python 3.2, Python 3.3, Python 3.4
2012-10-12 10:38:44hyneksetnosy: + hynek
2012-10-12 10:02:26vbraunsetnosy: + vbraun
messages: + msg172736
2012-10-12 07:32:54jdemeyersetmessages: + msg172722
2012-10-11 23:40:01Arfreversetnosy: + Arfrever
2012-10-11 23:06:32robertwbsetnosy: + robertwb
messages: + msg172709
2012-10-11 21:41:48christian.heimessetnosy: + christian.heimes
2012-10-11 20:49:50schmirsetnosy: + schmir
2012-10-11 20:14:35Alan.WiIliamssetnosy: + Alan.WiIliams
2012-10-11 20:10:23jdemeyercreate