#2347 closed defect (fixed)
[with bundle, positive review] better parsing for symbolics
Reported by: | robertwb | Owned by: | robertwb |
---|---|---|---|
Priority: | blocker | Milestone: | sage-3.0 |
Component: | calculus | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This is a security risk, and limits the potential uses of Sage. For example, if I wanted to put a text box on my website where people could type in a function and it would return the derivative (computed using Sage) someone could "ask" for the derivative of 2*os.system('rm -rf /')
. Symbolic expressions should be able to be parsed in such a way that one can safely reject expressions using unknown (or non-whitelisted) functions.
Attachments (12)
Change History (30)
comment:1 Changed 14 years ago by
- Owner changed from was to robertwb
- Status changed from new to assigned
Changed 14 years ago by
comment:2 Changed 14 years ago by
- Summary changed from Symbolic parsing uses eval() to [with bundle, needs review] better parsing for symbolics
comment:3 Changed 14 years ago by
- Priority changed from major to blocker
Changed 14 years ago by
comment:4 Changed 14 years ago by
I've attached the bundle as a patch which I will review once 3.0.alpha4 comes out. There were some problems applying the bundle against 3.0.alpha3.
comment:5 Changed 14 years ago by
I will rebase the bundle as I don't want to loose the history.
comment:6 Changed 14 years ago by
Sounds good although patches are much easier to deal with.
comment:7 Changed 14 years ago by
Just FYI, you could use queue repositories to be able to get patches that have history. See the the help for qcommit, etc.
Changed 14 years ago by
comment:8 Changed 14 years ago by
The new bundle (against alpha3) works fine. There was only one minor conflict. Do you anticipate any major changes with alpha4? (If it's up before I go to bed I'll make sure it works against that.)
Jason: Using mercurial queues won't help here, the issue is that half a dozen commits were compressed into a single patch. When there are more than four or five separate changesets attached to a given ticket, I find bundles a lot easier to deal with (rather than attaching all the patches separately).
Changed 14 years ago by
Changed 14 years ago by
Changed 14 years ago by
Changed 14 years ago by
Changed 14 years ago by
Changed 14 years ago by
Changed 14 years ago by
comment:9 Changed 14 years ago by
For those of you who prefer patches, I've attached them. Patches 1-7 are exactly the same contents as the bundles, except the rebased bundle resolves a (trivial to fix) conflict in patch 2 against alpha3.
Changed 14 years ago by
comment:10 Changed 14 years ago by
- Summary changed from [with bundle, needs review] better parsing for symbolics to [with bundle, positive review] better parsing for symbolics
I've looked at the changes and tested things out, and things look good to me. This is a definite improvement than what was there before. I included a change to combinat/root_system/dynkin_diagram.py. 2347.hg is the bundle that should be merged.
comment:11 Changed 14 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged in Sage 3.0.alpha5
comment:12 Changed 14 years ago by
Robert,
I am seeing one doctest failure:
sage -t -long devel/sage/sage/rings/number_field/number_field.py ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/tmp/number_field.py", line 4878: sage: L.lift_to_base(b^4) Exception raised: Traceback (most recent call last): File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/doctest.py", line 1212, in __run compileflags, 1) in test.globs File "<doctest __main__.example_145[6]>", line 1, in <module> L.lift_to_base(b**Integer(4))###line 4878: sage: L.lift_to_base(b^4) File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/site-packages/sage/rings/number_field/number_field.py", line 4892, in lift_to_base f = QQ['y'](str_poly) File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/site-packages/sage/rings/polynomial/polynomial_ring.py", line 225, in __call__ raise TypeError,"Unable to coerce string" TypeError: Unable to coerce string ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/tmp/number_field.py", line 211: sage: L.lift_to_base(b^3 + b) Exception raised: Traceback (most recent call last): File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/doctest.py", line 1212, in __run compileflags, 1) in test.globs File "<doctest __main__.example_3[12]>", line 1, in <module> L.lift_to_base(b**Integer(3) + b)###line 211: sage: L.lift_to_base(b^3 + b) File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/site-packages/sage/rings/number_field/number_field.py", line 4892, in lift_to_base f = QQ['y'](str_poly) File "/scratch/mabshoff/release-cycle/sage-3.0.alpha5/local/lib/python2.5/site-packages/sage/rings/polynomial/polynomial_ring.py", line 225, in __call__ raise TypeError,"Unable to coerce string" TypeError: Unable to coerce string **********************************************************************
Cheers,
Michael
comment:13 Changed 14 years ago by
Hmm... I ran a -testall before rebasing the bundle, but I'll see if I can get a patch for this. Should be pretty simple. (Really, it's ugly that it's going via strings at all.)
BTW, do you have a sage-3.0.alpha4-sage.math-only-x86_64-Linux.tar.gz
I could grab?
comment:14 Changed 14 years ago by
An sage-3.0.alpha4-sage.math-only-x86_64-Linux.tar.gz should be up in the usual place in five minutes. Mike Hansen is also poking around in the general area.
Cheers,
Michael
Changed 14 years ago by
comment:15 Changed 14 years ago by
I've added 2347-doctest.patch which fixes the issue.
comment:16 Changed 14 years ago by
Merged 2347-doctest.patch in Sage 3.0.alpha5
comment:17 Changed 14 years ago by
Thanks, good catch.
comment:18 Changed 14 years ago by
For the record: Mike's patch fixes the issue.
Cheers,
Michael
Of course, we don't want to re-write the Python parser or try to certify generic code to be safe, but in this constrained situation we should be able to treat an expression as data without worrying about it being treated as code.
There is an added benefit that unknown tokens gan be treated as symbolic variables. I wrote up a parser in Cython that is actually 10 times faster than eval(...) and handles symbolic expressions that I think is ready to plug in, I just need to provide it with a good list of "whitelist" functions that may be called.