-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
sys.path[0] security issues #60406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
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 |
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? |
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. |
The fact that Python's own testsuite tripped over this proves that this is subtle enough to merit some special handling.
|
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. |
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. |
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. |
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). |
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. |
disutils should definitely be fixed. |
Good point about cleanup, patch updated. |
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 |
Also, what's up with that weird fallback code in distutils? 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... |
I'm totally on board with any effort in this regard. Sign me up if you need a hand. |
Jeroen, just out of curiosity. Is the current issue different from Thank you, Jan.Jan iankko Lieskovsky |
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). |
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. |
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. |
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. |
What is the status of this issue? Is isolated mode (-I) a sufficient solution for you? |
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). |
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. |
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 |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: