Opened 10 years ago

Last modified 4 years ago

#13579 closed defect

test_executable security risk — at Version 36

Reported by: vbraun Owned by: mvngu
Priority: blocker Milestone: sage-5.4
Component: doctest coverage Keywords:
Cc: jdemeyer Merged in:
Authors: Jeroen Demeyer, Volker Braun Reviewers: Volker Braun, Jeroen Demeyer
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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.

Change History (38)

comment:1 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-5.5 to sage-5.4

Changed 10 years ago by jdemeyer

comment:3 Changed 10 years ago by vbraun

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

comment:4 Changed 10 years ago by vbraun

  • Authors set to Jeroen Demeyer, Volker Braun
  • Description modified (diff)
  • Reviewers set to Volker Braun, Jeroen Demeyer
  • Status changed from new to needs_review

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

comment:5 Changed 10 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 10 years ago by 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?

comment:7 follow-up: Changed 10 years ago by 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...

comment:8 in reply to: ↑ 7 Changed 10 years ago by 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.

comment:9 follow-up: Changed 10 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 10 years ago by 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.

comment:11 follow-up: Changed 10 years ago by 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.

Last edited 10 years ago by vbraun (previous) (diff)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 10 years ago by 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.

comment:13 in reply to: ↑ 12 Changed 10 years ago by 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.

comment:14 Changed 10 years ago by 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.

comment:15 Changed 10 years ago by jdemeyer

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

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

comment:16 follow-ups: Changed 10 years ago by 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.

comment:17 in reply to: ↑ 16 Changed 10 years ago by 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.

comment:18 in reply to: ↑ 16 Changed 10 years ago by 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.

comment:19 follow-up: Changed 10 years ago by 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.

comment:20 in reply to: ↑ 19 Changed 10 years ago by 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.

comment:21 Changed 10 years ago by vbraun

  • Description modified (diff)

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.

comment:22 Changed 10 years ago by jdemeyer

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

comment:23 Changed 10 years ago by 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

Changed 10 years ago by vbraun

Updated patch

comment:24 Changed 10 years ago by 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.

comment:25 follow-ups: Changed 10 years ago by 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.

comment:26 follow-up: Changed 10 years ago by 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?

comment:27 in reply to: ↑ 25 Changed 10 years ago by 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.

comment:28 in reply to: ↑ 26 Changed 10 years ago by 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.

comment:29 Changed 10 years ago by jdemeyer

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

comment:30 in reply to: ↑ 25 Changed 10 years ago by 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.

comment:31 follow-up: Changed 10 years ago by 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...

comment:32 Changed 10 years ago by 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.

comment:33 in reply to: ↑ 31 ; follow-up: Changed 10 years ago by 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]?

comment:34 in reply to: ↑ 33 Changed 10 years ago by 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).

comment:35 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:36 Changed 10 years ago by jdemeyer

  • Description modified (diff)
Note: See TracTickets for help on using tickets.