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.

[robertwb]

See #2347 which is related.

[jdemeyer]

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

[embray]

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]

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

[vdelecroix]

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.

[vdelecroix]

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]

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.

[embray]

Replying to vbraun:

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.

+1

[embray]

Ticket retargeted after milestone closed

[mkoeppe]

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

[mkoeppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[chapoton]

here is a simple-minded patch. Unless somebody proposes something better, I think it makes sense to merge that now

---

New commits:

​1e89c2a

simple-minded check for string input of CC

[git]

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

​57d20f1

simple-minded check for string input of CC

[git]

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

​e59be13	Merge branch 'u/chapoton/2877' in 9.5.b4
​53f6c2a	fix some CC bad string input

[chapoton]

bot is morally green, so please review

[tscrim]

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

[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

[chapoton]

ok, done (and squashed the commits)

[chapoton]

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

[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

[chapoton]

bot is morally green now

[klee]

Sorry for late comment, but how about this?

src/sage/rings/complex_mpfr.pyx

@@ -504 +504 @@
             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 +516 @@
                 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]

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.

[git]

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

​45039da

some details

[chapoton]

ok, done.

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

[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

[chapoton]

I fixed the failing doctest

[tscrim]

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

[klee]

Replying to tscrim:

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

I fully agree. Thanks.

[slelievre]

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

[chapoton]

voilà, voilà.

[slelievre]

Merci.