Yet another attempt at a safe eval() call

G

Grant Edwards

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?

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.
 
T

Tim Chase

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.

For what definition of "safe"? Are CPython segfaults a problem?
Blowing the stack? Do you aim to prevent exploitable things like
system calls or network/file access?

-tkc
 
G

Grant Edwards

For what definition of "safe"? Are CPython segfaults a problem?

Not by themselves, no.
Blowing the stack?

Not a problem either. I don't care if the program crashes. It's a
pretty dumb assembler, and it gives up and exits after the first error
anyway.
Do you aim to prevent exploitable things like system calls or
network/file access?

Yes, that's mainly what I was wondering wondering about.
 
S

Steven D'Aprano

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.
 
C

Chris Rebert

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?

Given http://nedbatchelder.com/blog/201206/eval_really_is_dangerous.html
and similar, I suspect the answer is "a fair bit".
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.

Using the aformentioned article as a basis, I was able to get this
doozy working, albeit under Python 3:

$ python3
Python 3.3.0 (default, Nov 4 2012, 17:47:16)
[GCC 4.2.1 Compatible Apple Clang 4.0 ((tags/Apple/clang-421.0.57))] on darwin
Type "help", "copyright", "credits" or "license" for more information.
expr = "[klass for klass in ().__class__.__bases__[0].__subclasses__() if klass.__name__ == 'Codec'][0].encode.__globals__['__builtins__']['__im'+'port__']('os').remove"
eval(expr, {'__builtins__': None}, {})

Since the original attack was itself devised against Python 2.x, it's
highly likely that similar convoluted attacks against 2.x remain
possible, unless perhaps you were use a modified interpreter.

Cheers,
Chris
 
T

Terry Reedy

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.

Will ast.literal_eval do what you want?
 
S

Steven D'Aprano

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.

Will ast.literal_eval do what you want?

No. Grant needs to support variables, not just literal constants, hence
the symbol table.
 
G

Grant Edwards

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.

I suspected that was the case.
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.

I can disallow underscores in names.
* 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.

OK, quotes and dots are out as well.
* 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.

That's obviously the "right" thing to do. I suppose I should figure
out how to use the ast module.
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.

Thanks! It's definitely an improvement.
 
G

Grant Edwards

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.

Will ast.literal_eval do what you want?

No. Grant needs to support variables, not just literal constants, hence
the symbol table.

Right. ast.literal_eval() doesn't even support arithmetic expressions
involving only literal constats such as "3+1" (that's the bare minimum
requirement). It would also be very highly desirable to allow
expressions involving symblic constants such as "PC+1".

Google has found me exapmles of ast-based code that does pretty much
what I want, but I haven't tried it yet because of that solution's
size and complexity.
 
M

Michael Torrie

That's obviously the "right" thing to do. I suppose I should figure
out how to use the ast module.

Or PyParsing.

As for your program being "secure" I don't see that there's much to
exploit. You're not running as a service, and you're not running your
assembler as root, called from a normal user. The user has your code
and can "exploit" it anytime he wants.
 
G

Grant Edwards

Or PyParsing.

As for your program being "secure" I don't see that there's much to
exploit.

There isn't.
You're not running as a service, and you're not running your
assembler as root, called from a normal user. The user has your code
and can "exploit" it anytime he wants.

I'm just trying to prevent surprises for people who are running the
assembler. We have to assume that they trust the assembler code to
not cause damage intentionally. But, one would not expect them to
have to worry that assembly language input fed to the assembler code
might cause some sort of collateral damage.

Sure, I can change the source code for gcc so that it wreaks havok
when I invoke it. But, using the stock gcc compiler there shouldn't
be any source file I can feed it that will cause it to mail my bank
account info to somebody in Eastern Europe, install a keylogger, and
then remove all my files.
 
G

Grant Edwards

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.

[...]

[ my attaempt at a safer eval() ]
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.

I've added equals, backslash, commas, square/curly brackets, colons and semicolons to the
prohibited character list. I also reduced the maximum length to 60
characters. It's unfortunate that parentheses are overloaded for both
expression grouping and for function calling...

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

Exploits anyone?
 
C

Chris Angelico

I've added equals, backslash, commas, square/curly brackets, colons and semicolons to the
prohibited character list. I also reduced the maximum length to 60
characters. It's unfortunate that parentheses are overloaded for both
expression grouping and for function calling...

I have to say that an expression evaluator that can't handle parens
for grouping is badly flawed. Can you demand that open parenthesis be
preceded by an operator (or beginning of line)? For instance:

(1+2)*3+4 # Valid
1+2*(3+4) # Valid
1+2(3+4) # Invalid, this will attempt to call 2

You could explain it as a protection against mistaken use of algebraic
notation (in which the last two expressions have the same meaning and
evaluate to 15). Or, alternatively, you could simply insert the
asterisk yourself, though that could potentially be VERY confusing.

Without parentheses, your users will be forced to store intermediate
results in variables, which gets tiresome fast.

discriminant = b*b-4*a*c
denominator = 2*a
# Okay, this expression demands a square rooting, but let's pretend that's done.
sol1 = -b+discriminant
sol2 = -b-discrminant
sol1 = sol1/denominator
sol2 /= denominator # if they know about augmented assignment

You can probably recognize the formula I'm working with there, but
it's far less obvious and involves six separate statements rather than
two. And this is a fairly simple formula. It'll get a lot worse in
production.

ChrisA
 
G

Grant Edwards

I have to say that an expression evaluator that can't handle parens
for grouping is badly flawed.

Indeed. That's why I didn't disallow parens.

What I was implying was that since you have to allow parens for
grouping, there's no simple way to disallow function calls.
Can you demand that open parenthesis be preceded by an operator (or
beginning of line)?

Yes, but once you've parsed the expression to the point where you can
enforce rules like that, you're probably most of the way to doing the
"right" thing and evaluating the expression using ast or pyparsing or
similar.
You can probably recognize the formula I'm working with there, but
it's far less obvious and involves six separate statements rather than
two. And this is a fairly simple formula. It'll get a lot worse in
production.

In the general case, yes. For this assembler I could _probably_ get
by with expressions of the form <symbol> <op> <literal> where op is
'+' or '-'. But, whenever I try to come up with a minimal solution
like that, it tends to get "enhanced" over the years until it's a
complete mess, doesn't work quite right, and took more total man-hours
than a general and "permanent" solution would have.

Some might argue that repeated tweaking of and adding limitiations to
a "safe eval" is just heading down that same road in a different car.
They'd probably be right: in the end, it will probably have been less
work to just do it with ast. But it's still interesting to try. :)
 
C

Chris Angelico

Indeed. That's why I didn't disallow parens.

What I was implying was that since you have to allow parens for
grouping, there's no simple way to disallow function calls.

Yeah, and a safe evaluator that allows function calls is highly vulnerable.
Yes, but once you've parsed the expression to the point where you can
enforce rules like that, you're probably most of the way to doing the
"right" thing and evaluating the expression using ast or pyparsing or
similar.

Some might argue that repeated tweaking of and adding limitiations to
a "safe eval" is just heading down that same road in a different car.
They'd probably be right: in the end, it will probably have been less
work to just do it with ast. But it's still interesting to try. :)

Yep, have fun with it. As mentioned earlier, though, security isn't
all that critical; so in this case, chances are you can just leave
parens permitted and let function calls potentially happen.

ChrisA
 
G

Grant Edwards

Yeah, and a safe evaluator that allows function calls is highly vulnerable.


Yep, have fun with it. As mentioned earlier, though, security isn't
all that critical; so in this case, chances are you can just leave
parens permitted and let function calls potentially happen.

An ast-based evaluator wasn't as complicated as I first thought: the
examples I'd been looking at implemented far more features than I
needed. This morning I found a simpler example at

http://stackoverflow.com/questions/2371436/evaluating-a-mathematical-expression-in-a-string

The error messages are still pretty cryptic, so improving
that will add a few more lines. One nice thing about the ast code is
that it's simple to add code to allow C-like character constants such
that ('A' === 0x41). Here's the first pass at ast-based code:

import ast,operator

operators = \
{
ast.Add: operator.iadd,
ast.Sub: operator.isub,
ast.Mult: operator.imul,
ast.Div: operator.idiv,
ast.BitXor: operator.ixor,
ast.BitAnd: operator.iand,
ast.BitOr: operator.ior,
ast.LShift: operator.lshift,
ast.RShift: operator.rshift,
ast.Invert: operator.invert,
ast.USub: operator.neg,
ast.UAdd: operator.pos,
}

def _eval_expr(node):
global symbolTable
if isinstance(node, ast.Name):
if node.id not in symbolTable:
raise ParseError("name '%s' undefined" % node.id)
return symbolTable[node.id]
elif isinstance(node, ast.Num):
return node.n
elif isinstance(node, ast.operator) or isinstance(node, ast.unaryop):
return operators[type(node)]
elif isinstance(node, ast.BinOp):
return _eval_expr(node.op)(_eval_expr(node.left), _eval_expr(node.right))
elif isinstance(node, ast.UnaryOp):
return _eval_expr(node.op)(_eval_expr(node.operand))
else:
raise ParseError("error parsing expression at node %s" % node)

def eval_expr(expr):
return _eval_expr(ast.parse(expr).body[0].value)
 
C

Chris Angelico

The error messages are still pretty cryptic, so improving
that will add a few more lines. One nice thing about the ast code is
that it's simple to add code to allow C-like character constants such
that ('A' === 0x41). Here's the first pass at ast-based code:

Looks cool, and fairly neat! Now I wonder, is it possible to use that
to create new operators, such as the letter d? Binary operator, takes
two integers...

ChrisA
 
G

Grant Edwards

Looks cool, and fairly neat! Now I wonder, is it possible to use that
to create new operators, such as the letter d? Binary operator, takes
two integers...

I don't think you can define new operators. AFAICT, the
lexing/parsing is done using the built-in Python grammar. You can
control the behavior of the predefined operators and reject operators
you don't like, but you can't add new ones or change precedence/syntax
or anything like that.

If you want to tweak the grammar itself, then I think you need to use
something like pyparsing.
 
C

Chris Angelico

I don't think you can define new operators. AFAICT, the
lexing/parsing is done using the built-in Python grammar. You can
control the behavior of the predefined operators and reject operators
you don't like, but you can't add new ones or change precedence/syntax
or anything like that.

If you want to tweak the grammar itself, then I think you need to use
something like pyparsing.

Oh well, hehe. I've not seen any simple parsers that let you
incorporate D&D-style dice notation ("2d6" means "roll two 6-sided
dice and sum the rolls" - "d6" implies "1d6").

ChrisA
 
O

Oscar Benjamin

That's obviously the "right" thing to do. I suppose I should figure
out how to use the ast module.

Someone has already created a module that does this called numexpr. Is
there some reason why you don't want to use that?
array(22L)


Oscar
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,482
Members
44,901
Latest member
Noble71S45

Latest Threads

Top