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:

1. attachment: 13579_sagelib.patch to the Sage library.
2. attachment: 13579_scripts.patch to Sage scripts (local/bin).
3. new spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/python-2.7.3.p1.spkg (patch added: attachment: sys_path_security.patch)

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

See also: ipython/ipython#7044

Upstream: Reported upstream. No feedback yet.

CC: @jdemeyer

Component: doctest coverage

Author: Jeroen Demeyer, Volker Braun

Reviewer: Volker Braun, Jeroen Demeyer, David Roe

Merged: sage-5.4.rc2

Issue created by migration from https://trac.sagemath.org/ticket/13579

[vbraun]

comment:3

Attachment: 13579_secure_tmp.patch.gz

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

[vbraun]

comment:4

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

[vbraun]

Reviewer: Volker Braun, Jeroen Demeyer

[vbraun]

Author: Jeroen Demeyer, Volker Braun

[jdemeyer]

comment:6

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]

comment:7

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]

comment:8

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]

comment:9

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]

comment:10

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]

comment:11

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]

comment:12

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]

comment:13

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]

comment:14

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]

comment:15

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

[jdemeyer]

Upstream: Not yet reported upstream; Will do shortly.

[vbraun]

comment:16

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]

comment:17

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]

comment:18

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]

comment:19

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.

[roed314]

comment:71

I thought that file had existed before. Anyway, it looks fine, so I'll give a positive review.

[jdemeyer]

comment:72

Volker, I hope you can be happy with these patches. I know test_executable() isn't as secure as you wanted, but I think that problem is solved by doing the tests before allowing doctesting at all.

[jdemeyer]

Attachment: 13579_sagelib.patch.gz

All Sage library patches folded

[jdemeyer]

Merged: sage-5.4.rc2

[dimpase]

comment:75

Replying to @jdemeyer:

I think it's quite unlikely that somebody would doctest the Sage library from /tmp.

I did this many times. (But of course I grew up with an idea that the worst thing that can happen to your program is that the stack of punchcards it is contained in is dropped on the floor and messed up :-))

[vbraun]

comment:76

The patch breaks my patchbot, even though it is running in a safe directory. Details at #13631

[kcrisman]

comment:77

Yo, dudes, you missed a place for temp files.

from sage.misc.temporary_file import tmp_filename, tmp_dir

in animate.py wasn't enough - see this ask.sagemath.org question. I've opened #13807 for this.

[nexttime]

comment:78

Copy-pasted from sage-release:

$ env SAGE_TESTDIR=/tmp ./sage -t devel/sage/sage/tests/cmdline.py
sage -t  "devel/sage/sage/tests/cmdline.py"
sys:1: RuntimeWarning: not adding directory '/private/tmp' to sys.path since everybody can write to it.
Untrusted users could put files in this directory which might then be imported by your Python code. As a general precaution from similar exploits, you should not execute Python code from this directory
**********************************************************************
File "/Users/leif/Sage/sage-5.6.beta3-vanilla/devel/sage/sage/tests/cmdline.py", line 312:
    sage: ret
Expected:
    0
Got:
    128
**********************************************************************
File "/Users/leif/Sage/sage-5.6.beta3-vanilla/devel/sage/sage/tests/cmdline.py", line 314:
    sage: out.find("All tests passed!") >= 0
Expected:
    True
Got:
    False
**********************************************************************
File "/Users/leif/Sage/sage-5.6.beta3-vanilla/devel/sage/sage/tests/cmdline.py", line 317:
    sage: ret
Expected:
    0
Got:
    128
**********************************************************************
File "/Users/leif/Sage/sage-5.6.beta3-vanilla/devel/sage/sage/tests/cmdline.py", line 319:
    sage: out.find("All tests passed!") >= 0
Expected:
    True
Got:
    False
**********************************************************************
1 items had failures:
   4 of 203 in __main__.example_1
***Test Failed*** 4 failures.
For whitespace errors, see the file /tmp/cmdline_77447.py
     [84.3 s]

----------------------------------------------------------------------
The following tests failed:


    sage -t  "devel/sage/sage/tests/cmdline.py"
Total time for all tests: 84.4 seconds


Same with 'make testlong'; './sage -tp 1 ...' and 'make ptestlong' in contrast work.

[jdemeyer]

comment:79

As Volker explained, that's a feature, not a bug.

[nexttime]

comment:80

Replying to @jdemeyer:

As Volker explained, that's a feature, not a bug.

Nope, the doctest shouldn't put files there (which isn't safe for simultaneous testing anyway).

[nexttime]

comment:81

Replying to @nexttime:

Replying to @jdemeyer:

As Volker explained, that's a feature, not a bug.

Nope, the doctest shouldn't put files there (which isn't safe for simultaneous testing anyway).

...
    Testing ``sage --preparse FILE`` and ``sage -t FILE``.  First create
    a file and preparse it::

        sage: s = '\"\"\"\nThis is a test file.\n\"\"\"\ndef my_add(a,b):\n    \"\"\"\n    Add a to b.\n\n        EXAMPLES::\n\n            sage: my_add(2,2)\n            4\n        \"\"\"\n    return a+b\n'
        sage: script = os.path.join(tmp_dir(), 'my_script.sage')
        sage: script_py = script[:-5] + '.py'
        sage: F = open(script, 'w')
        sage: F.write(s)
        sage: F.close()
        sage: (out, err, ret) = test_executable(["sage", "--preparse", script])
        sage: ret
        0
        sage: os.path.isfile(script_py)
        True

    Now test my_script.sage and the preparsed version my_script.py::

        sage: (out, err, ret) = test_executable(["sage", "-t", script])
        sage: ret
        0
        sage: out.find("All tests passed!") >= 0
        True
        sage: (out, err, ret) = test_executable(["sage", "-t", script_py])
        sage: ret
        0
        sage: out.find("All tests passed!") >= 0
        True
...

The latter four tests failed.

[vbraun]

comment:82

You'll always be able to make doctests fail if you point SAGE_TESTDIR at an ill-suited directory. Try SAGE_TESTDIR=/proc.

[nexttime]

comment:83

Replying to @vbraun:

You'll always be able to make doctests fail if you point SAGE_TESTDIR at an ill-suited directory. Try SAGE_TESTDIR=/proc.

That's unrelated. It's IMHO pretty ok to have SAGE_TESTDIR=/tmp (or whatever world-writable scratch directory; same for SAGE_TMP).

The test just shouldn't write the script into that directory, but instead into a "safe" subdir, just where all other doctest scripts end up.

Orthogonal to the safety issue, the test could just fail because different test instances (in this case, on different machines, but sharing the test directory) use the same file at the same time.

[saraedum]

comment:85

Can we get rid of this patch during the Python 3 upgrade by using Python's isolated mode (-I)? There's an unanswered question about this at https://bugs.python.org/issue16202 btw.

[saraedum]

comment:86

There is a followup now at #26457.