Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#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:

Status badges

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)

2347-parsing.hg (13.9 KB) - added by robertwb 14 years ago.
2347.patch (49.5 KB) - added by mhansen 14 years ago.
2347-parsing-rebased.hg (31.3 KB) - added by robertwb 14 years ago.
2347-1.patch (27.7 KB) - added by robertwb 14 years ago.
2347-2.patch (9.3 KB) - added by robertwb 14 years ago.
2347-3.patch (6.2 KB) - added by robertwb 14 years ago.
2347-4.patch (64.4 KB) - added by robertwb 14 years ago.
2347-5.patch (8.3 KB) - added by robertwb 14 years ago.
2347-6.patch (5.2 KB) - added by robertwb 14 years ago.
2347-7.patch (1.7 KB) - added by robertwb 14 years ago.
2347.hg (33.8 KB) - added by mhansen 14 years ago.
2347-doctest.patch (1.7 KB) - added by mhansen 14 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 14 years ago by robertwb

  • Owner changed from was to robertwb
  • Status changed from new to assigned

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.

Changed 14 years ago by robertwb

comment:2 Changed 14 years ago by robertwb

  • Summary changed from Symbolic parsing uses eval() to [with bundle, needs review] better parsing for symbolics

comment:3 Changed 14 years ago by mabshoff

  • Priority changed from major to blocker

Changed 14 years ago by mhansen

comment:4 Changed 14 years ago by mhansen

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 robertwb

I will rebase the bundle as I don't want to loose the history.

comment:6 Changed 14 years ago by mhansen

Sounds good although patches are much easier to deal with.

comment:7 Changed 14 years ago by jason

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 robertwb

comment:8 Changed 14 years ago by robertwb

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 robertwb

Changed 14 years ago by robertwb

Changed 14 years ago by robertwb

Changed 14 years ago by robertwb

Changed 14 years ago by robertwb

Changed 14 years ago by robertwb

Changed 14 years ago by robertwb

comment:9 Changed 14 years ago by robertwb

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 mhansen

comment:10 Changed 14 years ago by mhansen

  • 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 mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 3.0.alpha5

comment:12 Changed 14 years ago by mabshoff

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 robertwb

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 mabshoff

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 mhansen

comment:15 Changed 14 years ago by mhansen

I've added 2347-doctest.patch which fixes the issue.

comment:16 Changed 14 years ago by mabshoff

Merged 2347-doctest.patch in Sage 3.0.alpha5

comment:17 Changed 14 years ago by robertwb

Thanks, good catch.

comment:18 Changed 14 years ago by mabshoff

For the record: Mike's patch fixes the issue.

Cheers,

Michael

Note: See TracTickets for help on using tickets.