Created on 2012-10-11 20:10 by jdemeyer, last changed 2018-02-07 10:34 by ThomasAH.

Messages (23)

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) * 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) * 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) * 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) * 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) * 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) * 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) * 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) * 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.

msg275215 - (view) Author: Christian Heimes (christian.heimes) * Date: 2016-09-08 23:46

What is the status of this issue? Is isolated mode (-I) a sufficient solution for you?

msg275598 - (view) Author: Nick Coghlan (ncoghlan) * Date: 2016-09-10 08:15

Reviewing the issue, I think there's still an open question regarding the way distutils handles generated script execution that may impact setuptools as, so adding Jason to the nosy list. For the "don't set sys.path[0] by default" aspect, we would need a different executable that uses more paranoid defaults, which would be contingent on the PEP 432 startup refactoring landing for 3.7 (as too much behaviour is currently embedded inside Py_Main for alternate defaults to be reasonably maintained).

msg311782 - (view) Author: Thomas Arendsen Hein (ThomasAH) Date: 2018-02-07 10:34