Opened 14 years ago
Closed 10 months 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: |
Description (last modified by )
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
comment:2 Changed 14 years ago by
- Milestone set to sage-3.0
comment:3 Changed 14 years ago by
- Type changed from defect to enhancement
comment:4 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:5 Changed 9 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:7 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:8 Changed 8 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 8 years ago by
- Status changed from needs_review to positive_review
comment:10 Changed 8 years ago by
- Resolution set to invalid
- Status changed from positive_review to closed
comment:11 Changed 3 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 3 years ago by
A simpler fix would be to use a limited eval, e.g. https://newville.github.io/asteval/
comment:13 Changed 3 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 3 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 3 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 3 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 3 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:18 Changed 2 years 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 2 years ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:20 Changed 18 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 13 months ago by
- Milestone changed from sage-9.4 to sage-9.5
comment:22 Changed 10 months ago by
- 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:
1e89c2a | simple-minded check for string input of CC
|
comment:23 Changed 10 months ago by
- Commit changed from 1e89c2acb8b9fdccfa475dd45d33007cd1cd6923 to 57d20f14f55b063e9b4b9456827d5030be7b31aa
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
|
comment:24 Changed 10 months ago by
- Reviewers Jeroen Demeyer deleted
comment:25 Changed 10 months ago by
- Commit changed from 57d20f14f55b063e9b4b9456827d5030be7b31aa to 53f6c2a3e36b4c9e8f9d60a9027f9dfdef1f21c9
comment:26 Changed 10 months ago by
- Cc tscrim slelievre gh-kliem klee added
bot is morally green, so please review
comment:27 Changed 10 months ago by
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 10 months ago by
- Commit changed from 53f6c2a3e36b4c9e8f9d60a9027f9dfdef1f21c9 to 77031e8a39e6470b0eb3981f69f61f27a2d7ef5c
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
|
comment:29 Changed 10 months ago by
ok, done (and squashed the commits)
comment:30 Changed 10 months ago by
but this will break the doctest in singular.pyx... (sigh)
comment:31 Changed 10 months ago by
- Commit changed from 77031e8a39e6470b0eb3981f69f61f27a2d7ef5c to feab03f2ae95b6f4452bbc3211e6a84ad56aad68
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
|
comment:32 follow-up: ↓ 33 Changed 10 months ago by
bot is morally green now
comment:33 in reply to: ↑ 32 Changed 10 months ago by
Sorry for late comment, but how about this?
-
src/sage/rings/complex_mpfr.pyx
a b class ComplexField_class(sage.rings.abc.ComplexField): 504 504 sage: CC('hello') 505 505 Traceback (most recent call last): 506 506 ... 507 ValueError: given string (hello)is not a complex number507 ValueError: given string 'hello' is not a complex number 508 508 """ 509 509 if not isinstance(x, (RealNumber, tuple)): 510 510 if isinstance(x, ComplexDoubleElement): … … class ComplexField_class(sage.rings.abc.ComplexField): 516 516 x = x.replace('E', 'e') 517 517 allowed = '+-.*0123456789Ie' 518 518 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') 520 520 # This should rather use a proper parser to validate input. 521 521 # TODO: this is probably not the best and most 522 522 # efficient way to do this. -- Martin Albrecht
and does not express a complex number
.
comment:34 Changed 10 months ago by
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 10 months ago by
- Commit changed from feab03f2ae95b6f4452bbc3211e6a84ad56aad68 to 45039da5348873d5363020f0cde9e5996e4b3de7
Branch pushed to git repo; I updated commit sha1. New commits:
45039da | some details
|
comment:36 Changed 10 months ago by
ok, done.
WARNING: note that CC('1.0+2.0*j') works, but not CC('1.0+2.0j').
comment:37 Changed 10 months ago by
- Commit changed from 45039da5348873d5363020f0cde9e5996e4b3de7 to 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0
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
|
comment:38 Changed 10 months ago by
I fixed the failing doctest
comment:39 follow-up: ↓ 40 Changed 10 months ago by
- 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 10 months ago by
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 10 months ago by
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 10 months ago by
- 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 10 months ago by
Merci.
comment:44 Changed 10 months ago by
- Branch changed from u/chapoton/2877 to 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0
- Resolution set to fixed
- Status changed from positive_review to closed
See #2347 which is related.