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:  sage9.5 
Component:  misc  Keywords:  
Cc:  tscrim, slelievre, ghkliem, 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 sage3.0
comment:3 Changed 13 years ago by
 Type changed from defect to enhancement
comment:4 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:5 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:6 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:7 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:8 Changed 7 years ago by
 Milestone changed from sage6.4 to sageduplicate/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 sageduplicate/invalid/wontfix to sage8.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/possiblebuginccneedsconfirmation )
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 followup: ↓ 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*I2' 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 constantvalued 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 2 years ago by
 Milestone changed from sage8.9 to sage9.1
Ticket retargeted after milestone closed
comment:18 Changed 20 months ago by
 Milestone changed from sage9.1 to sage9.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
 Milestone changed from sage9.2 to sage9.3
comment:20 Changed 10 months ago by
 Milestone changed from sage9.3 to sage9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:21 Changed 4 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:22 Changed 6 weeks ago by
 Branch set to u/chapoton/2877
 Commit set to 1e89c2acb8b9fdccfa475dd45d33007cd1cd6923
 Status changed from new to needs_review
here is a simpleminded patch. Unless somebody proposes something better, I think it makes sense to merge that now
New commits:
1e89c2a  simpleminded check for string input of CC

comment:23 Changed 6 weeks 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  simpleminded check for string input of CC

comment:24 Changed 6 weeks ago by
 Reviewers Jeroen Demeyer deleted
comment:25 Changed 6 weeks ago by
 Commit changed from 57d20f14f55b063e9b4b9456827d5030be7b31aa to 53f6c2a3e36b4c9e8f9d60a9027f9dfdef1f21c9
comment:26 Changed 6 weeks ago by
 Cc tscrim slelievre ghkliem klee added
bot is morally green, so please review
comment:27 Changed 5 weeks 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) <ipythoninput2a2113f9c148b> in <module> > 1 complex('1+2i') ValueError: complex() arg is a malformed string
comment:28 Changed 5 weeks 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  simpleminded check for string input of CC

comment:29 Changed 5 weeks ago by
ok, done (and squashed the commits)
comment:30 Changed 5 weeks ago by
but this will break the doctest in singular.pyx... (sigh)
comment:31 Changed 5 weeks 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  simpleminded check for string input of CC

comment:32 followup: ↓ 33 Changed 5 weeks ago by
bot is morally green now
comment:33 in reply to: ↑ 32 Changed 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks ago by
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
 Commit changed from 45039da5348873d5363020f0cde9e5996e4b3de7 to 57e8e9bc41c8e41eca5cad8879e67ba49089a4f0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
57e8e9b  simpleminded check for string input of CC

comment:38 Changed 5 weeks ago by
I fixed the failing doctest
comment:39 followup: ↓ 40 Changed 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks ago by
Merci.
comment:44 Changed 4 weeks 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.