Opened 12 years ago

Last modified 2 months ago

#2877 new enhancement

security risk -- several constructors use eval to parse input

Reported by: robertwb Owned by: cwitty
Priority: major Milestone: sage-9.1
Component: misc Keywords:
Cc: Merged in:
Authors: Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Change History (17)

comment:1 Changed 12 years ago by robertwb

See #2347 which is related.

comment:2 Changed 12 years ago by mabshoff

  • Milestone set to sage-3.0

comment:3 Changed 11 years ago by AlexGhitza

  • Type changed from defect to enhancement

comment:4 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 5 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 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:10 Changed 5 years ago by vbraun

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

comment:11 Changed 8 months 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 8 months 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 8 months ago by vbraun (previous) (diff)

comment:13 Changed 8 months 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 8 months 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 8 months 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 8 months 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 months ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.