Opened 4 years ago

Closed 4 years ago

Last modified 2 years ago

#13579 closed defect (fixed)

Python sys.path security risk

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

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.

Apply:

  1. 13579_sagelib.patch to the Sage library.
  2. 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: sys_path_security.patch)

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

See also: https://github.com/ipython/ipython/issues/7044

Attachments (7)

13579_secure_tmp.patch (3.6 KB) - added by jdemeyer 4 years ago.
trac_13579_fix_test_executable.patch (40.3 KB) - added by vbraun 4 years ago.
Updated patch
13579_review.patch (17.1 KB) - added by jdemeyer 4 years ago.
sys_path_security.patch (11.1 KB) - added by jdemeyer 4 years ago.
Patch added to the Python sources
13579_scripts.patch (2.2 KB) - added by jdemeyer 4 years ago.
13579_test_permissions.patch (3.7 KB) - added by jdemeyer 4 years ago.
13579_sagelib.patch (46.7 KB) - added by jdemeyer 4 years ago.
All Sage library patches folded

Download all attachments as: .zip

Change History (91)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.5 to sage-5.4

Changed 4 years ago by jdemeyer

comment:3 Changed 4 years ago by vbraun

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

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

  • Description modified (diff)

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

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

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

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

Updated patch

comment:24 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 years ago by jdemeyer

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

comment:36 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:37 Changed 4 years ago by 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).

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

comment:39 Changed 4 years ago by 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.

comment:40 in reply to: ↑ 38 Changed 4 years ago by 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.

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

comment:42 in reply to: ↑ 41 Changed 4 years ago by 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

comment:43 Changed 4 years ago by 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...

comment:44 Changed 4 years ago by jdemeyer

  • Summary changed from test_executable security risk to Python sys.path security risk

comment:45 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

comment:46 Changed 4 years ago by jdemeyer

I consider the Python spkg to be ready for review.

comment:47 follow-up: Changed 4 years ago by jdemeyer

Concerning the Sage library patch: why did you change doc/common/builder.py?

comment:48 in reply to: ↑ 47 Changed 4 years ago by vbraun

Replying to jdemeyer:

Concerning the Sage library patch: why did you change doc/common/builder.py?

The new tmp_filename and tmp_dir create a file/directory (the only safe way to deal with temporaries), but the doctest wanted a non-existing directory name. Needless to say, a function that returns a not-yet-existing filename is just a race condition in the making.

comment:49 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:50 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:51 follow-ups: Changed 4 years ago by nbruin

Case (B): (script dir group writeable) - It's perfectly safe to make directories group writeable, provided the group is sufficiently restricted. In fact, it's a very conceivable setup if you have multiple administrators that you want to give a lot of latitude, short of root (e.g., because your filesystem is NFS with root-squash)

Case (C): (script dir only user writable but by different UID): That's how I install packages like sage! Because they need frequent updates, they're not owned by root but by a dedicated maintenance account.

In fact, if the dir is go-w and owned by a different UID, the script is very likely also owned by a different UID. The security risk really isn't in the imports in that case (if there is any at all)

None of these affect correct functioning of sage with the patch, since the required paths are explicitly added to sys.path, but it does illustrate that your detection heuristics are not very accurate.

As pointed out before, even o+w isn't necessarily a security risk, but since /tmp likely is, I'd find that an acceptable compromise. The other cases are harder to work around if they bite you. Really, the import behaviour only causes additional security risks on directories writeable by untrusted people and the sticky bit set. Otherwise, you can't trust the script you're running in the first place! Would it be possible to only do these checks in the presence of a sticky bit?

Last edited 4 years ago by nbruin (previous) (diff)

comment:52 in reply to: ↑ 51 Changed 4 years ago by jdemeyer

Replying to nbruin:

Case (B): (current dir group writeable) - It's perfectly safe to make directories group writeable, provided the group is sufficiently restricted. In fact, it's a very conceivable setup if you have multiple administrators that you want to give a lot of latitude, short of root (e.g., because your filesystem is NFS with root-squash)

Then it's likely that both the Python script and the directory containing it are group-writable by the same group, which is allowed by my patch.

Case (C): (current dir only user writable but by different UID): That's how I install packages like sage! Because they need frequent updates, they're not owned by root but by a dedicated maintenance account.

My patch allows a directory owned by the same user as the Python executable. In fact, I precisely had this scenario in mind, that's why I added this check.

comment:53 in reply to: ↑ 51 Changed 4 years ago by jdemeyer

Replying to nbruin:

Would it be possible to only do these checks in the presence of a sticky bit?

I have no idea about the portability of "sticky bit" behaviour. If you know for sure that every Unix system supports the sticky bit in the same way as Linux, that's fine for me.

Changed 4 years ago by jdemeyer

comment:54 Changed 4 years ago by jdemeyer

The "sticky bit" S_ISVTX is documented by POSIX, see http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_02.

Based on this, I will change my patch.

comment:55 Changed 4 years ago by jdemeyer

New version, needs review.

comment:56 follow-up: Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

First of all, the discussion on the Python bugtracker shows that this version of the patch is not going to be upstream. I would prefer a minimal patch that just adds a warning while keeping the old behavior that could be incorporated upstream. Having said that, I don't mind if we try out this patch in Sage until the next upstream release.

  • Error messages don't clearly state that this is a security risk. The "not adding directory to sys.path since the write permissions are too loose" implies that its OK to run scripts off /tmp and therefore implicitly encourages insecure behavior.
  • Since many packages (including some in the stdlib) re-add cwd if it is missing, the patch doesn't actually guarantee safe execution in /tmp.

A short & sweet Warning: It is not safe to run scripts in a world-writeable directory (including /tmp) would be better ihmo.

comment:57 in reply to: ↑ 56 Changed 4 years ago by jdemeyer

I don't mind changing the warning message to something more severe. Something like

RuntimeWarning: not adding directory '%s' to sys.path since the write permissions are too loose. 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.

Changed 4 years ago by jdemeyer

Patch added to the Python sources

comment:58 Changed 4 years ago by jdemeyer

I updated the spkg with the new longer warning message.

Volker, just to be clear: can you confirm that your "positive_review" also covers 13579_review.patch?

comment:59 follow-up: Changed 4 years ago by vbraun

As I've explained before, its a terrible idea to have test_executable rely on the hidden premise that somebody chdir'ed to a safe directory before execution. Why are you so desperate to teach bad habits and booby-trap it for future users? When I changed it in my patch, it was precisely to be free of this hidden assumption. And you undid that for no good reason.

I agree that its safe the way the testsuite currently uses test_executable, so if you really want to ship it its at least better than before.

comment:60 in reply to: ↑ 59 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vbraun:

As I've explained before, its a terrible idea to have test_executable rely on the hidden premise that somebody chdir'ed to a safe directory before execution.

Serious question: why do you consider test_executable() as much more dangerous than other doctesting? I.e. you seem to say that it's okay to run doctests in /tmp except for test_executable().

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

The problem I have with your approach is that it unexpectedly changes directory. In some cases, you might want to run a test from a given directory, but that's impossible with your patch.

Would you accept a patch which checks in test_executable() (or maybe sage-doctest?) that the current directory is not world-writable and raises an exception if it is?

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:61 follow-up: Changed 4 years ago by vbraun

Given what we learned in this ticket, I think the only safe way to use Popen is by supplying a cwd= argument with a directory that you set up (even if its just SAGE_TMP). Perhaps with the exception of posix standard binaries like ls or binaries that you wrote yourself. Its a simple pattern, easy to communicate and enforce, and will absolutely prevent this kind of problem. Basically, it forces the programmer to think about the choice of working directory.

Sure, you can make this whole issue more difficult to exploit by checking that the directory is not world-writable. But that does't plug the underlying issue.

Also, the Sage testsuite never ran a test in a specific directory. Its hard to imagine how that would be portable, to start with. But in the hypothetical case that one would need such a functionality, it could be added as an optional keyword parameter to test_executable.

I don't think that test_executable is the only dangerous function. In fact I'm pretty sure that there are other nuggets hidden if you manage to trick somebody into executing Sage in /tmp.

comment:62 in reply to: ↑ 61 Changed 4 years ago by jdemeyer

Replying to vbraun:

I don't think that test_executable is the only dangerous function. In fact I'm pretty sure that there are other nuggets hidden if you manage to trick somebody into executing Sage in /tmp.

I agree with this, so I think we should stop focusing on test_executable(). I'm preparing a patch, stay tuned...

Changed 4 years ago by jdemeyer

Changed 4 years ago by jdemeyer

comment:63 Changed 4 years ago by jdemeyer

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

comment:64 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:65 Changed 4 years ago by jdemeyer

Volker, I added a patch for sage-test and sage-ptest (why are these 2 different files anyway?) such that they will refuse to run any doctests from an unsafe directory. I think this makes much more sense than fixing "only" test_executable().

Needs review

comment:66 Changed 4 years ago by roed

They're getting deleted by #12415. Now I need to update 12415_script.patch again....

I will make sure this behavior is incorporated into the #12415 framework.

comment:67 follow-up: Changed 4 years ago by roed

What did you do to 13579_test_permissions.patch? I'm happy to sign off on the patch to the scripts repo.

comment:68 in reply to: ↑ 67 Changed 4 years ago by jdemeyer

Replying to roed:

What did you do to 13579_test_permissions.patch?

What do you mean, I don't understand your question.

comment:69 Changed 4 years ago by jdemeyer

  • Reviewers changed from Volker Braun, Jeroen Demeyer to Volker Braun, Jeroen Demeyer, David Roe

comment:70 Changed 4 years ago by roed

The timestamp said that 13579_test_permissions.patch changed after you marked it as needs work the most recent time. That's all.

comment:71 Changed 4 years ago by roed

  • Status changed from needs_review to positive_review

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

comment:72 Changed 4 years ago by jdemeyer

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.

comment:73 Changed 4 years ago by jdemeyer

  • Description modified (diff)

Changed 4 years ago by jdemeyer

All Sage library patches folded

comment:74 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.4.rc2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:75 in reply to: ↑ 60 Changed 4 years ago by dimpase

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 :-))

comment:76 Changed 4 years ago by vbraun

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

comment:77 Changed 4 years ago by kcrisman

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.

comment:78 Changed 4 years ago by leif

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.

comment:79 follow-up: Changed 4 years ago by jdemeyer

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

comment:80 in reply to: ↑ 79 ; follow-up: Changed 4 years ago by leif

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

comment:81 in reply to: ↑ 80 Changed 4 years ago by leif

Replying to leif:

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.

comment:82 follow-up: Changed 4 years ago by vbraun

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

comment:83 in reply to: ↑ 82 Changed 4 years ago by leif

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.

comment:84 Changed 2 years ago by jdemeyer

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