security risk -- restrict the input of eval in CC constructor

[robertwb]

There are valid uses for eval() and sage_eval(), it makes it much easier to parse output from interfaces for example.

It is difficult (if not impossible) to completely sanitize arbitrary input, but one should be able to (say) write a backend that takes specific data, calls on Sage to process it, and then returns the result. For example, I might want a webpage that uses Sage to compute Julia sets, and takes as input a complex number. That the following work is scary

sage: CC("os.getpid()")
10324.0000000000
sage: CC("os.mkdir('a')")
NaN - NaN*I
sage: CC("os.rmdir('a')")
NaN - NaN*I
sage: CC("os.exec(...)")

In this ticket, one introduces restrictions on the text input to CC that prevent most of these terrible examples.

CC: @tscrim @slel @kliem @kwankyu

Component: misc

Author: Frédéric Chapoton

Branch/Commit: 57e8e9b

Reviewer: Travis Scrimshaw, Kwankyu Lee

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

[robertwb]

comment:1

See #2347 which is related.

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:8

Sage is not a secure program and nobody ever claimed it was. Sanitise your input before sending it to Sage!

[embray]

comment:11

A strange response coming from Jeroen. The use of sage_eval to convert arbitrary string inputs to elements of different fields is, I think, obviously bad on so many levels, and badly mishandled in several cases (e.g. some of them will catch NameErrors, but not SyntaxErrors; some will just work, but in weird ways, if you pass some arbitrary string; some are just broken as in https://ask.sagemath.org/question/47068/possible-bug-in-cc-needs-confirmation )

If you want to convert string representations of some elements of a field to actual elements of that field there should be proper parsing, not just arbitrary evals.

[vbraun]

comment:12

A simpler fix would be to use a limited eval, e.g. https://newville.github.io/asteval/

The reason for the eval in CC is of course that you want to allow expressions like 2+3*I that exceed python's literal_eval capabilities.

[videlec]

comment:13

I fully agree with Erik.

The following does not work (as expected)

sage: ZZ('2**3 + 3*g - 2')
Traceback (most recent call last):
...
TypeError: unable to convert '2**3 + 3*g - 2' to an integer
sage: RR('2**2 + 3*5 - 2')
Traceback (most recent call last):
...
TypeError: unable to convert '2**3+5*I-2' to a real number

Supporting the following with CC is a nonsense

sage: CC('2**2 + 3*5 - 2')
17.0000000000000
sage: CC('erf(2)')
0.995322265018953

We don't want the element constructor to evaluate a string in hope that it gives a complex number. There should be a clear definition of what is the input format. And the constructor should just stick to specifications. The element constructor of CC is trying to do much more than what it is supposed to.

[videlec]

comment:14

And to my mind the main problem is not the security risk (I agree with Jeroen on that point). I would advice to open another ticket "fix element constructor of CC".

[embray]

comment:15

It's not just CC. It's all of them. It's really flaky to allow a general eval to construct instances of some particular type. Instead, it should really just parse constant-valued expressions for whatever that type is, at the most. That can either involve custom parsing code, or as Volker suggested a custom AST parser.

[fchapoton]

comment:26

bot is morally green, so please review

[tscrim]

comment:27

Do we also want to allow j to match Python's convention for complex numbers:

sage: complex('1+2j')
(1+2j)
sage: complex('1+2i')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-a2113f9c148b> in <module>
----> 1 complex('1+2i')

ValueError: complex() arg is a malformed string

[sagetrac-git]

Changed commit from 53f6c2a to 77031e8

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

77031e8

simple-minded check for string input of CC

[fchapoton]

comment:29

ok, done (and squashed the commits)

[fchapoton]

comment:30

but this will break the doctest in singular.pyx... (sigh)

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

feab03f

simple-minded check for string input of CC

[sagetrac-git]

Changed commit from 77031e8 to feab03f

[fchapoton]

comment:32

bot is morally green now

[kwankyu]

comment:33

Sorry for late comment, but how about this?

--- a/src/sage/rings/complex_mpfr.pyx
+++ b/src/sage/rings/complex_mpfr.pyx
@@ -504,7 +504,7 @@ class ComplexField_class(sage.rings.abc.ComplexField):
             sage: CC('hello')
             Traceback (most recent call last):
             ...
-            ValueError: given string (hello) is not a complex number
+            ValueError: given string 'hello' is not a complex number
         """
         if not isinstance(x, (RealNumber, tuple)):
             if isinstance(x, ComplexDoubleElement):
@@ -516,7 +516,7 @@ class ComplexField_class(sage.rings.abc.ComplexField):
                 x = x.replace('E', 'e')
                 allowed = '+-.*0123456789Ie'
                 if not all(letter in allowed for letter in x):
-                    raise ValueError(f'given string ({x}) is not a complex number')
+                    raise ValueError(f'given string {x!r} is not a complex number')
                 # This should rather use a proper parser to validate input.
                 # TODO: this is probably not the best and most
                 # efficient way to do this.  -- Martin Albrecht

and does not express a complex number.

[tscrim]

comment:34

Thank you. I am also wondering a bit if we want to document that CC also supports j as a valid string input. Although I don't hold a strong opinion on this.

[sagetrac-git]

Changed commit from feab03f to 45039da

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

45039da

some details

[fchapoton]

comment:36

ok, done.

WARNING: note that CC('1.0+2.0*j') works, but not CC('1.0+2.0j').

[sagetrac-git]

Changed commit from 45039da to 57e8e9b

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

57e8e9b

simple-minded check for string input of CC

[fchapoton]

comment:38

I fixed the failing doctest

[tscrim]

Reviewer: Travis Scrimshaw, Kwankyu Lee

[tscrim]

comment:39

Great, thank you. LGTM. Kwankyu, if you do not agree, feel free to revert the positive review.

[kwankyu]

comment:40

Replying to @tscrim:

Great, thank you. LGTM. Kwankyu, if you do not agree, feel free to revert the positive review.

I fully agree. Thanks.

[slel]

comment:41

Update ticket summary and description
to better describe the changes made here?

(Or just contract "should be able to be able to"
if the rest is still fine?)

[fchapoton]

comment:42

voilà, voilà.

[slel]

comment:43

Merci.

[vbraun]

Changed branch from u/chapoton/2877 to 57e8e9b