Opened 14 years ago

Closed 4 weeks ago

#2877 closed enhancement (fixed)

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

Reported by: robertwb Owned by: cwitty
Priority: major Milestone: sage-9.5
Component: misc Keywords:
Cc: tscrim, slelievre, gh-kliem, klee Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw, Kwankyu Lee
Report Upstream: N/A Work issues:
Branch: 57e8e9b (Commits, GitHub, GitLab) Commit: 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0
Dependencies: Stopgaps:

Status badges

Description (last modified by chapoton)

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.

Change History (44)

comment:1 Changed 14 years ago by robertwb

See #2347 which is related.

comment:2 Changed 14 years ago by mabshoff

  • Milestone set to sage-3.0

comment:3 Changed 13 years ago by AlexGhitza

  • Type changed from defect to enhancement

comment:4 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Report Upstream set to N/A
  • Reviewers set to Jeroen Demeyer
  • Status changed from new to needs_review

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

comment:9 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:10 Changed 7 years ago by vbraun

  • Resolution set to invalid
  • Status changed from positive_review to closed

comment:11 Changed 2 years ago by embray

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-8.9
  • Resolution invalid deleted
  • Status changed from closed to new

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.

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

Last edited 2 years ago by vbraun (previous) (diff)

comment:13 Changed 2 years ago by 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.

comment:14 Changed 2 years ago by 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".

comment:15 Changed 2 years ago by 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.

comment:16 in reply to: ↑ 12 Changed 2 years ago by 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

comment:17 Changed 2 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:18 Changed 20 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

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.

comment:19 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:20 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

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

comment:21 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:22 Changed 6 weeks ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/2877
  • Commit set to 1e89c2acb8b9fdccfa475dd45d33007cd1cd6923
  • Status changed from new to needs_review

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


New commits:

1e89c2asimple-minded check for string input of CC

comment:23 Changed 6 weeks ago by git

  • Commit changed from 1e89c2acb8b9fdccfa475dd45d33007cd1cd6923 to 57d20f14f55b063e9b4b9456827d5030be7b31aa

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

57d20f1simple-minded check for string input of CC

comment:24 Changed 6 weeks ago by chapoton

  • Reviewers Jeroen Demeyer deleted

comment:25 Changed 6 weeks ago by git

  • Commit changed from 57d20f14f55b063e9b4b9456827d5030be7b31aa to 53f6c2a3e36b4c9e8f9d60a9027f9dfdef1f21c9

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

e59be13Merge branch 'u/chapoton/2877' in 9.5.b4
53f6c2afix some CC bad string input

comment:26 Changed 6 weeks ago by chapoton

  • Cc tscrim slelievre gh-kliem klee added

bot is morally green, so please review

comment:27 Changed 5 weeks ago by 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

comment:28 Changed 5 weeks ago by git

  • Commit changed from 53f6c2a3e36b4c9e8f9d60a9027f9dfdef1f21c9 to 77031e8a39e6470b0eb3981f69f61f27a2d7ef5c

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

77031e8simple-minded check for string input of CC

comment:29 Changed 5 weeks ago by chapoton

ok, done (and squashed the commits)

comment:30 Changed 5 weeks ago by chapoton

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

comment:31 Changed 5 weeks ago by git

  • Commit changed from 77031e8a39e6470b0eb3981f69f61f27a2d7ef5c to feab03f2ae95b6f4452bbc3211e6a84ad56aad68

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

feab03fsimple-minded check for string input of CC

comment:32 follow-up: Changed 5 weeks ago by chapoton

bot is morally green now

comment:33 in reply to: ↑ 32 Changed 5 weeks ago by klee

Sorry for late comment, but how about this?

  • src/sage/rings/complex_mpfr.pyx

    a b class ComplexField_class(sage.rings.abc.ComplexField): 
    504504            sage: CC('hello')
    505505            Traceback (most recent call last):
    506506            ...
    507             ValueError: given string (hello) is not a complex number
     507            ValueError: given string 'hello' is not a complex number
    508508        """
    509509        if not isinstance(x, (RealNumber, tuple)):
    510510            if isinstance(x, ComplexDoubleElement):
    class ComplexField_class(sage.rings.abc.ComplexField): 
    516516                x = x.replace('E', 'e')
    517517                allowed = '+-.*0123456789Ie'
    518518                if not all(letter in allowed for letter in x):
    519                     raise ValueError(f'given string ({x}) is not a complex number')
     519                    raise ValueError(f'given string {x!r} is not a complex number')
    520520                # This should rather use a proper parser to validate input.
    521521                # TODO: this is probably not the best and most
    522522                # efficient way to do this.  -- Martin Albrecht

and does not express a complex number.

comment:34 Changed 5 weeks ago by 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.

comment:35 Changed 5 weeks ago by git

  • Commit changed from feab03f2ae95b6f4452bbc3211e6a84ad56aad68 to 45039da5348873d5363020f0cde9e5996e4b3de7

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

45039dasome details

comment:36 Changed 5 weeks ago by chapoton

ok, done.

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

comment:37 Changed 5 weeks ago by git

  • Commit changed from 45039da5348873d5363020f0cde9e5996e4b3de7 to 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0

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

57e8e9bsimple-minded check for string input of CC

comment:38 Changed 5 weeks ago by chapoton

I fixed the failing doctest

comment:39 follow-up: Changed 5 weeks ago by tscrim

  • Reviewers set to Travis Scrimshaw, Kwankyu Lee
  • Status changed from needs_review to positive_review

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

comment:40 in reply to: ↑ 39 Changed 5 weeks ago by 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.

comment:41 Changed 5 weeks ago by 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?)

comment:42 Changed 5 weeks ago by chapoton

  • Description modified (diff)
  • Summary changed from security risk -- several constructors use eval to parse input to security risk -- restrict the input of eval in CC constructor

voilà, voilà.

comment:43 Changed 5 weeks ago by slelievre

Merci.

comment:44 Changed 4 weeks ago by vbraun

  • Branch changed from u/chapoton/2877 to 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.