Opened 10 years ago
Last modified 4 years ago
#13579 closed defect
Python sys.path security risk — at Version 45
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: |
Description (last modified by )
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
Change History (47)
comment:1 Changed 10 years ago by
- Description modified (diff)
comment:2 Changed 10 years ago by
- Milestone changed from sage-5.5 to sage-5.4
Changed 10 years ago by
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
- 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
- Description modified (diff)
comment:6 Changed 10 years ago by
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: ↓ 8 Changed 10 years ago by
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
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: ↓ 10 Changed 10 years ago by
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
Replying to vbraun:
I'm against including a function that is unsafe by default, thats just asking for trouble.
test_executable
either has tochdir
to ensure that the user did not forget it, or refuse to run ifcwd
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: ↓ 12 Changed 10 years ago by
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.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 10 years ago by
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
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
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
- 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: ↓ 17 ↓ 18 Changed 10 years ago by
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
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
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: ↓ 20 Changed 10 years ago by
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
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
- 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
I plan to add a patch to the Python spkg fixing the root cause of this issue.
comment:23 Changed 10 years ago by
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
comment:24 Changed 10 years ago by
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: ↓ 27 ↓ 30 Changed 10 years ago by
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: ↓ 28 Changed 10 years ago by
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
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
Replying to vbraun:
Also, if you had a shell script in
/tmp
then you wouldn't putsource 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
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
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: ↓ 33 Changed 10 years ago by
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
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: ↓ 34 Changed 10 years ago by
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
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
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:36 Changed 10 years ago by
- Description modified (diff)
comment:37 Changed 10 years ago by
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).
comment:38 follow-up: ↓ 40 Changed 10 years ago by
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).
comment:39 Changed 10 years ago by
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.
comment:40 in reply to: ↑ 38 Changed 10 years ago by
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.
comment:41 follow-up: ↓ 42 Changed 10 years ago by
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.
comment:42 in reply to: ↑ 41 Changed 10 years ago by
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
comment:43 Changed 10 years ago by
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...
comment:44 Changed 10 years ago by
- Summary changed from test_executable security risk to Python sys.path security risk
comment:45 Changed 10 years ago by
- Description modified (diff)
- Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
Oops, looks like we did it at the same time.