Yet another attempt at a safe eval() call

[Steven D'Aprano]

On Thu, 03 Jan 2013 23:25:51 +0000, Grant Edwards wrote:

> I've written a small assembler in Python 2.[67], and it needs to
> evaluate integer-valued arithmetic expressions in the context of a
> symbol table that defines integer values for a set of names.  The
> "right" thing is probably an expression parser/evaluator using ast, but
> it looked like that would take more code that the rest of the assembler
> combined, and I've got other higher-priority tasks to get back to.
> 
> How badly am I deluding myself with the code below?

Pretty badly, sorry. See trivial *cough* exploit below.


> def lessDangerousEval(expr):
>     global symbolTable
>     if 'import' in expr:
>         raise ParseError("operand expressions are not allowed to contain
>         the string 'import'")
>     globals = {'__builtins__': None}
>     locals  = symbolTable
>     return eval(expr, globals, locals)
> 
> I can guarantee that symbolTable is a dict that maps a set of string
> symbol names to integer values.


Here's one exploit. I make no promises that it is the simplest such one.

# get access to __import__
s = ("[x for x in (1).__class__.__base__.__subclasses__() "
     "if x.__name__ == 'catch_warnings'][0]()._module"
     ".__builtins__['__imp' + 'ort__']")
# use it to get access to any module we like
t = s + "('os')"
# and then do bad things
urscrewed = t + ".system('echo u r pwned!')"

lessDangerousEval(urscrewed)


At a minimum, I would recommend:

* Do not allow any underscores in the expression being evaluated. Unless 
you absolutely need to support them for names, they can only lead to 
trouble.

* If you must allow underscores, don't allow double underscores. Every 
restriction you apply makes it harder to exploit.

* Since you're evaluating mathematical expressions, there's probably no 
need to allow quotation marks either. They too can only lead to trouble.

* Likewise for dots, since this is *integer* maths.

* Set as short as possible limit on the length of the string as you can 
bare; the shorter the limit, the shorter any exploit must be, and it is 
harder to write a short exploit than a long exploit.

* But frankly, you should avoid eval, and write your own mini-integer 
arithmetic evaluator which avoids even the most remote possibility of 
exploit.

So, here's my probably-not-safe-either "safe eval":


def probably_not_safe_eval(expr):
    if 'import' in expr.lower():
        raise ParseError("'import' prohibited")
    for c in '_"\'.':
        if c in expr:
            raise ParseError('prohibited char %r' % c)
    if len(expr) > 120:
        raise ParseError('expression too long')
    globals = {'__builtins__': None}
    locals  = symbolTable
    return eval(expr, globals, locals)  # fingers crossed!

I can't think of any way to break out of these restrictions, but that may 
just mean I'm not smart enough.


-- 
Steven