Opened 13 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.5 |
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 (21)
comment:1 Changed 13 years ago by
comment:2 Changed 13 years ago by
- Milestone set to sage-3.0
comment:3 Changed 13 years ago by
- Type changed from defect to enhancement
comment:4 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:5 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:7 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:8 Changed 7 years ago by
- 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
- Status changed from needs_review to positive_review
comment:10 Changed 7 years ago by
- Resolution set to invalid
- Status changed from positive_review to closed
comment:11 Changed 2 years ago by
- 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 NameError
s, but not SyntaxError
s; 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: ↓ 16 Changed 2 years ago by
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.
comment:13 Changed 2 years ago by
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
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
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
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'sliteral_eval
capabilities.
+1
comment:17 Changed 21 months ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:18 Changed 18 months ago by
- 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 13 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:20 Changed 7 months ago by
- 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 2 months ago by
- Milestone changed from sage-9.4 to sage-9.5
See #2347 which is related.