Python sys.path security risk

[vbraun]

test_executable runs various executables in /tmp. When running a script, Python puts the directory containing that script in sys.path. Therefore, it is trivial for any user to have code executed by the user running the doctests. For example:

[eviluser@hostname ~]$ echo 'print "EVIL!!"' > /tmp/socket.py
...
[vbraun@hostname ~]$ sage -t -force_lib devel/sage/sage/tests/cmdline.py
sage -t -force_lib "devel/sage/sage/tests/cmdline.py"       
**********************************************************************
File "/home/vbraun/opt/sage-5.4.beta1/devel/sage/sage/tests/cmdline.py", line 248:
    sage: print out
Expected:
    1
Got:
    EVIL!!

test_executable should securely create a temp directory and run the executable in there.

• apply 13579_secure_tmp.patch​ to the Sage library.
• apply trac_13579_fix_test_executable.patch​ to the Sage library.
• new spkg: ​http://boxen.math.washington.edu/home/jdemeyer/spkg/python-2.7.3.p1.spkg (patch added: sys_path_security.patch​)

Reported upstream: ​http://bugs.python.org/issue16202

[vbraun]

Oops, looks like we did it at the same time.

[vbraun]

I'm fine with your patch. All doctests pass with both patches applied.

[jdemeyer]

I'd say that the function get_tmpdir() is a bit overkill.

Why not simply put

sage: test_tmpdir = tmp_dir('cmdline_test_')

inside the doctest?

[vbraun]

In the doctests there is also the case where you check that you can read from the current dir, so you need to create a file in the tmp dir and execute in the same dir. Hence its important that the name is cached in sage.tests.cmdline...

[jdemeyer]

Replying to vbraun:

execute in the same dir.

I don't think we should supply an explicit working directory in Popen(). I think the explicit os.chdir() statements in the doctest are clearer.

There are various other things which I'd like to change, so I'll prepare a reviewer patch.

[vbraun]

I'm against including a function that is unsafe by default, thats just asking for trouble. test_executable either has to chdir to ensure that the user did not forget it, or refuse to run if cwd is writeable by anybody but the user.

[jdemeyer]

Replying to vbraun:

I'm against including a function that is unsafe by default, thats just asking for trouble. test_executable either has to chdir to ensure that the user did not forget it, or refuse to run if cwd is writeable by anybody but the user.

The insecurity is not because of test_executable, but because of Python. So such a check should be added in spkg/bin/sage before running sage-run.

[vbraun]

Its pretty difficult to reliably determine if others have write access to the directory. Your system might not use the traditional u/g/o security model but rely on one of the SELinux ACMs, or even a completely different security model.

I'm in favor of removing cwd from sys.path in sage-env if it is not a subdirectory of the users home directory. Thats a pretty specific measure that keeps the convenience of easy importing while keeping use pretty safe. But that should be an additional security measure, and is definitely not the solution to the issue here.

test_executable has to be implemented in a manner that its safety is not dependent on other scripts, or we will see the same issue crop up again at a later time.

[jdemeyer]

Replying to vbraun:

Its pretty difficult to reliably determine if others have write access to the directory. Your system might not use the traditional u/g/o security model but rely on one of the SELinux ACMs, or even a completely different security model.

How about checking that the script is owned by the same user as the directory it is contained in? I think that would work pretty well.

But that should be an additional security measure, and is definitely not the solution to the issue here.

I disagree. I think sage-run is the real issue and that adding stuff to test_executable is simply a work-around.

[jdemeyer]

Replying to jdemeyer:

Replying to vbraun:

Its pretty difficult to reliably determine if others have write access to the directory. Your system might not use the traditional u/g/o security model but rely on one of the SELinux ACMs, or even a completely different security model.

I think all systems on which Sage runs have at least the classic unix security model. Additions like SELinux mostly affect system services, and normally have no impact on normal users. As far as Python or Sage is concerned, I think we can still check the standard Unix permissions.

[vbraun]

Well test_executable doesn't just run Sage, it will run other programs as well. Which may or may not look at files relative to cwd, or may introduce such a misfeature in a future version. Or a user might just import the sage library into their own python interpreter. There is only one way to be absolutely safe, and that is by controlling the path explicitly. Hence the design of my patch.

[jdemeyer]

I think this should be reported upstream to Python. Volker, do you intend to do this or shall I?

[vbraun]

CPython handles this already, cwd is not in the path if it is run non-interactively:

[vbraun@laptop ~]$ cat /tmp/test.py
#!/usr/bin/env  python
import sys
print sys.path

[vbraun@laptop ~]$ /tmp/test.py 
['/tmp', '/usr/lib64/python27.zip', '/usr/lib64/python2.7', ...]

[vbraun@laptop ~]$ python /tmp/test.py 
['/tmp', '/usr/lib64/python27.zip', '/usr/lib64/python2.7', ...]

[vbraun@laptop tmp]$ python
Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
[GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path
['', '/usr/lib64/python27.zip', '/usr/lib64/python2.7', ...]

CPython doesn't check for permissions of the script directory, though arguably you want the script directory in the path if you put the script there in the first place.

[jdemeyer]

Replying to vbraun:

CPython handles this already, cwd is not in the path if it is run non-interactively:

The script directory (/tmp in this case) is in sys.path. That's the problem.

[jdemeyer]

Replying to vbraun:

arguably you want the script directory in the path if you put the script there in the first place.

I think it is very unexpected (to me at least) that CPython suddenly reads other files in the directory where the script lives. If I create a shell script in /tmp, it's not going to magically add /tmp to the $PATH. That's fail-safe and that's how it should be.

[vbraun]

Python programs often consist of multiple files that need to be imported, whereas shell scripts don't. So you can't really compare the two cases.

I'm not saying that I'm entirely happy with how Python handles this, but it shows that upstream is aware of the issue and thats how they decided to deal with it.

[jdemeyer]

Replying to vbraun:

I'm not saying that I'm entirely happy with how Python handles this, but it shows that upstream is aware of the issue and thats how they decided to deal with it.

I'm not sure whether upstream is really aware of the security consequences of their sys.path handling.

My proposal would not disable the "script directory" situation completely: I would add the script directory to sys.path only if it is safe to do so. In normal situations (a script with 0755 permissions in a directory with 0755 permissions owned by the same user), I would keep the current sys.path handling.

[vbraun]

I've added a patch to sage-run that matches the plain Python behavior, and will print a warning if the user is not the owner of the script directory.

[jdemeyer]

I plan to add a patch to the Python spkg fixing the root cause of this issue.

[vbraun]

The Python behaviour is explicitly specified here ​http://docs.python.org/library/sys.html#sys.path

I also found the following Gentoo bug report that discusses exactly this issue: ​https://bugs.gentoo.org/show_bug.cgi?id=224925

[vbraun]

Updated patch

[jason]

That new import function in the gentoo bug report seems reasonable. In fact, we can also check all of the sys.path entries and make sure they start with SAGE_ROOT before running the test script.

[vbraun]

The patch in the gentoo bug report was rejected because it breaks the Python specified behavior. In fact, its possible that somebody's code relies on the Python specified behavior and will become a security threat if the script directory is not in sys.path[0]. I'd strongly advise against breaking the Python specs just because you don't like them.

[vbraun]

Also, if you had a shell script in /tmp then you wouldn't put source foobar into it. But if its a Python script then somehow the Python fairy is supposed to save you from import foobar?

[jdemeyer]

Replying to vbraun:

The patch in the gentoo bug report was rejected because it breaks the Python specified behavior.

As I said, that's not what I what do. I would check permissions/owners of the script and its directory and act on that.

[jdemeyer]

Replying to vbraun:

Also, if you had a shell script in /tmp then you wouldn't put source foobar into it. But if its a Python script then somehow the Python fairy is supposed to save you from import foobar?

Sure, but doing something like

import sys

is very common in Python-land. I don't think you can blame the user for doing that.

[jdemeyer]

Even putting the script directory last instead of first in sys.path would help a lot.

[jdemeyer]

Replying to vbraun:

I'd strongly advise against breaking the Python specs just because you don't like them.

That's a very silly advise when the specs are inherently unsafe. If the specs are bad, I don't mind breaking them. Especially if my proposal would not change anything for most use cases.

[vbraun]

What is your proposal? IMHO its ok to refuse to run or to print a warning (as in my patch to sage-run). If you want to silently drop sys.path[0] then you are just teaching unsafe habits, and tears will be shed later when your student uses an unpatched Python...

[jason]

I was suggesting that for running tests, we could modify import.

A separate suggestion is to modify import always, and insist that the user use relative imports when they really want to import something in the current directory. That definitely breaks python behavior, but it is more explicit and secure.

[jdemeyer]

Replying to vbraun:

What is your proposal? IMHO its ok to refuse to run or to print a warning (as in my patch to sage-run).

OK, so how about printing a warning and dropping sys.path[0]?

[vbraun]

Replying to jdemeyer:

OK, so how about printing a warning and dropping sys.path[0]?

Well if you desperately want it then knock yourself out, but I don't see any benefit over just printing a warning.

The reason for why Python acts the way it does is such that the admin can globally install Python programs. Which will generally be in a root-owned directory (just like /tmp but without o+w).

[nbruin]

Do the write permissions on the directory actually say anything about security? Imagine the following:

$ su A
$ mkdir /tmp/test
$ chmod a+wrx /tmp/test
$ mkdir /tmp/test/secure
$ chmod go-w /tmp/test/secure
$ cp python_test.py /tmp/test/secure
$ /tmp/test/secure/python_test.py
[...]

Now (from a different terminal):

$ su B
$ mkdir /tmp/test/new
$ cp -R /tmp/test/secure /tmp/test/new
$ cp evil_sys.py /tmp/test/new/sys.py
$ mv /tmp/test/secure /tmp/test/secure_bak; mv /tmp/test/new /tmp/test/secure

Any open files for the running python_test.py will remain, but any files that are newly looked up by (absolute) path name will be found in what used to be /tmp/test/new. In particular a late import sys.

Of course, this is why there's a t flag on /tmp.

In any case, apparently python comes with a caution: Don't run scripts in directories writeable by people you don't trust, not even t flagged ones.

We're changing that caution to: Don't run scripts in directories that have components in their path that are writeable by people you don't trust, although t flagged is fine lower down. We'll warn you if the top level is writeable, since that is particularly easy to exploit.

The proposed change fixes the particular issue for Sage, but with the formulation above, I don't think there's any chance of this getting accepted upstream. It doesn't look like a real solution. It's just kicking the ball a little further (far enough for us).

[vbraun]

I think there is no way you can expect any degree of safety if you manually make a directory world-writeable and then execute things in there. You should be allowed to shoot your own foot if you desperately want to. In particular, even if Python would check every directory permission there is still a race before chmod go-w /tmp/test/secure.

It would still be good to get rid of some of the sharp edges in Python wrt. execution in /tmp; But Jeroen's current patch would e.g. prevent you from giving file ownership of a script directory to nobody. IMHO it would be enough to check that parent dir is not o+w, anything else might actually be intentional (e.g. you might have set up a special group so you can collaborate on a Python program).

[jdemeyer]

I agree with Volker. Nils's issue isn't a Python issue. It's an obvious consequence of creating subdirectories of a world-writable directory and Unix's permission system. When you do stuff in (subdirectories of) a world-writable directory, you get what you deserve. Similarly, Python cannot (and should not) protect a user from exectuting a script with -rwxrwxrwx permissions.

[nbruin]

Replying to vbraun:

IMHO it would be enough to check that parent dir is not o+w, anything else might actually be intentional (e.g. you might have set up a special group so you can collaborate on a Python program).

Even that may be intentional. Many linux distributions by default make a separate group for each individual. On a machine where you trust every account, o+w would not necessarily indicate a security hole.

That being said, I think the proposed patch provides an acceptable workaround and I don't have any better ideas.

[robertwb]

I agree with Volker, we shouldn't be modifying the default behavior for Python imports to break their spec. A Python program implicitly includes all sibling .py files. If you place a file in a world- (or group-) writable location and then execute it, that's your fault.

A case can be made for tests, but it seems it would be better to simple place test executables in a better place.

[jason]

Replying to robertwb:

A case can be made for tests, but it seems it would be better to simple place test executables in a better place.

+1

[jdemeyer]

Replying to robertwb:

I agree with Volker, we shouldn't be modifying the default behavior for Python imports to break their spec. A Python program implicitly includes all sibling .py files. If you place a file in a world- (or group-) writable location and then execute it, that's your fault.

As I already argued, I totally disagree with this. If the spec is bad then the spec must be fixed.

I'll report this upstream and see what happens...