Opened 9 years ago

Last modified 3 years ago

#13579 closed defect

test_executable security risk — at Version 21

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

Change History (22)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.5 to sage-5.4

Changed 9 years ago by jdemeyer

comment:3 Changed 9 years ago by vbraun

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

comment:4 Changed 9 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 9 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 years ago by vbraun (previous) (diff)

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

Note: See TracTickets for help on using tickets.