sys.path[0] security issues

[jdemeyer]

BPO	16202
Nosy	@birkenfeld, @jaraco, @ncoghlan, @vstinner, @tiran, @benjaminp, @tarekziade, @jwilk, @merwok, @ericsnowcurrently, @hynek, @jdemeyer, @zooba, @aw1231
Files	fix_distutils.patch fix_distutils.patch sys_path_security.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/merwok'
closed_at = None
created_at = <Date 2012-10-11.20:10:23.538>
labels = ['type-security', 'interpreter-core', '3.8', '3.9', '3.10', '3.7']
title = 'sys.path[0] security issues'
updated_at = <Date 2021-02-03.18:21:55.365>
user = 'https://github.com/jdemeyer'

bugs.python.org fields:

activity = <Date 2021-02-03.18:21:55.365>
actor = 'christian.heimes'
assignee = 'eric.araujo'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2012-10-11.20:10:23.538>
creator = 'jdemeyer'
dependencies = []
files = ['27542', '27543', '27923']
hgrepos = []
issue_num = 16202
keywords = ['patch']
message_count = 24.0
messages = ['172686', '172709', '172722', '172736', '172750', '172755', '172756', '172762', '172764', '172766', '172768', '172798', '172799', '172803', '172812', '172981', '172983', '172998', '173000', '175155', '275215', '275598', '311782', '386330']
nosy_count = 21.0
nosy_names = ['georg.brandl', 'jaraco', 'ncoghlan', 'vstinner', 'ThomasAH', 'christian.heimes', 'schmir', 'robertwb', 'benjamin.peterson', 'tarek', 'jwilk', 'eric.araujo', 'Arfrever', 'iankko', 'eric.snow', 'hynek', 'jdemeyer', 'steve.dower', 'Alan.Williams', 'vbraun', 'hasufell']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue16202'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8', 'Python 3.9', 'Python 3.10']

[jdemeyer]

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

[robertwb]

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?

[jdemeyer]

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.

[vbraun]

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.

[tiran]

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.

[robertwb]

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.

[tiran]

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.

[jdemeyer]

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

[tiran]

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.

[benjaminp]

disutils should definitely be fixed.

[robertwb]

Good point about cleanup, patch updated.

[ncoghlan]

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

[ncoghlan]

Also, what's up with that weird fallback code in distutils? When is tempfile.mkdtemp ever missing?

[vbraun]

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

[ericsnowcurrently]

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.

[iankko]

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

[ncoghlan]

It's actually the same as bpo-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).

[jdemeyer]

It's sort of the same as bpo-946373, except that bug report deals with other bad consequences of sys.path[0], unrelated to security.

bpo-5753 is specifically about the C API, not about running "plain" Python.

[jdemeyer]

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.

[jdemeyer]

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.

[tiran]

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

[ncoghlan]

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

[ThomasAH]

I just stumbled across this problem when starting "idle3" in a directory containing a copy of textwrap.py which was not compatible with python3.

In bpo-13506 idle3 was changed to behave like the regular python shell, i.e. as described here in this issue, and since my ~/.pythonrc.py imports readline, I could reproduce this problem by creating a bogus readline.py file and starting a python interpreter.

So besides being a security issue when starting python or idle in untrusted directories, it may cause technical issues when working in directories containing .py files with certain names.

[zooba]

Distutils is now deprecated (see PEP-632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools

[hauntsaninja]

I think this can be closed since #57684 and #31542