10

A function f() uses eval() (or something as dangerous) with data which I created and stored in local_file on the machine running my program:

import local_file

def f(str_to_eval):
    # code....
    # .... 
    eval(str_to_eval)    
    # ....
    # ....    
    return None


a = f(local_file.some_str)

f() is safe to run since the strings I provide to it are my own.

However, if I ever decide to use it for something unsafe (e.g. user input) things could go terribly wrong. Also, if the local_file stops being local then it would create a vulnerability since I would need to trust the machine that provides that file as well.

How should I ensure that I never "forget" that this function is unsafe to use (unless specific criteria are met)?

Note: eval() is dangerous and can usually be replaced by something safe.

user
  • 449
  • 1
  • 5
  • 20
  • 1
    "*I would need to trust the machine that provides that file as well*" - you always need to trust the machine you are executing your code at. – Bergi Mar 04 '16 at 05:04
  • Put a comment in the file saying "This string gets eval'd; please do not put malicious code here"? – user253751 Mar 04 '16 at 11:08
  • @immibis A comment wouldn't be sufficient. I will of course have a huge "WARNING: This function uses eval()" in the function's docs, but i would rather rely on something much safer than that. For example (due to limited time) I m not always rereading a function's docs. Neither do I think i should be. There are faster (that is, more efficient) ways to know something is unsafe, like the methods [linked](http://www.joelonsoftware.com/articles/Wrong.html) by Murphy in his answer. – user Mar 04 '16 at 11:15
  • 1
    I usually like to go along with the questions that are asked rather than saying "you should not do this", but I will make an exception in this case. I am quite certain that there is no reason why you should be using eval. These functions are included with languages such as PHP and Python as a sort of proof-of-concept how much can be done with interpreted languages. It is fundamentally inconceivable that you can write code that creates code, but that you cannot code that same logic into a function. Python likely has callbacks and function bindings that will allow you to do what you need with so – user1122069 Mar 04 '16 at 00:22
  • @Fermiparadox when you take eval out of the question it becomes without useful example. Are you talking now about a function which is insecure because of deliberate oversight, such as not escaping SQL parameters. Do you mean test code not safe for a production environment? Anyway, now I read your comment on Amon's answer - basically you were looking for how to secure your function. – user1122069 Mar 04 '16 at 17:17
  • @user1122069 It's not just `eval()` that is dangerous. `exec()` and [`sympy.sympify()`](https://stackoverflow.com/questions/16718644/how-safe-is-sympys-sympifystring-evalf) are nearly as dangerous (`sympify` might change in the future, but it's current state is using `eval()` for speed and convenience). I'm sure there are other examples as well which i'm unaware of. – user Mar 04 '16 at 20:22
  • @Fermiparadox The relevant fact is ***not*** that the function calls `eval`. The relevant fact is that `local_file.some_str` is interpreted as code. Nobody cares *how* you do that - whether it's by calling `eval`, or by writing the code to a file and running another `python` process, or by running PyPy, or whatever. – user253751 Mar 05 '16 at 06:24
  • @user1122069 `eval()` can be perfectly fine when used correctly. It can save enormous amounts of time at the cost of security. But when security isn't an issue (e.g. it is used only on non-user input) then it can be a great solution. – user Mar 13 '16 at 10:56
  • @Fermiparadox how so? You want to run eval("$a = 2 + 2"); instead of "$a = 2 + 2;"? Granted, maybe you want to do something like this eval($function+"("+$arg1+")");, but doesn't Python have a way to call functions by string name? In PHP there is call_user_func_array() which allows calling of functions and class methods, static methods by string and passing any number of arguments. Faster than eval. In JavaScript I will use eval, but it is a minor convenience instead of writing a closure. I can't see what could be faster using eval in Python. – user1122069 Mar 14 '16 at 18:43
  • @user1122069 Assume I need `eval()` to run some mathematical expression I provide which can consist of 100s of different functions (`sin`, `cos`, `exp`, .. ). The effort to write and maintain an alternative to `eval()` can be huge. Of course if the expression was not provided by me but by a user, then I would have no choice but to avoid `eval()`. So, `eval()` is safe and useful in _some_ cases. As for being faster in a case like above, that's what I have heard from a developer of a popular math related python library; he assumes that `eval()` would be faster. – user Mar 15 '16 at 10:55
  • @Fermiparadox I don't understand. Why can you not write those 100s of different functions in code. You need to say something more than that, as whatever passes eval will pass the compiler, ie. that you are doing code generation that is too difficult to code statically. I don't believe it. But really, I'm not granting you to assume a plausible scenario but asking for one. Yes, it is there and all is fine if you have a reason to use it, but in reality it is an excuse for bad coding. Take 5 minutes to think about how it can be done with recursion and/or functions and build a real system. – user1122069 Mar 16 '16 at 00:56

3 Answers3

26

Define a type to decorate “safe” inputs. In your function f(), assert that the argument has this type or throw an error. Be smart enough to never define a shortcut def g(x): return f(SafeInput(x)).

Example:

class SafeInput(object):
  def __init__(self, value):
    self._value = value

  def value(self):
    return self._value

def f(safe_string_to_eval):
  assert type(safe_string_to_eval) is SafeInput, "safe_string_to_eval must have type SafeInput, but was {}".format(type(safe_string_to_eval))
  ...
  eval(safe_string_to_eval.value())
  ...

a = f(SafeInput(local_file.some_str))

While this can be easily circumvented, it makes it harder to do so by accident. People might criticize such a draconian type check, but they would miss the point. Since the SafeInput type is just a datatype and not an object-oriented class that could be subclassed, checking for type identity with type(x) is SafeInput is safer than checking for type compatibility with isinstance(x, SafeInput). Since we do want to enforce a specific type, ignoring the explicit type and just doing implicit duck typing is also not satisfactory here. Python has a type system, so let's use it to catch possible mistakes!

Uyghur Lives Matter
  • 126
  • 1
  • 1
  • 10
amon
  • 132,749
  • 27
  • 279
  • 375
  • 5
    A small change might be needed though. `assert` is removed automatically since I ll be using optimized python code. Something like `if ... : raise NotSafeInputError` would be needed. – user Mar 03 '16 at 20:57
  • 4
    If you're going down this road anyway, I would *strongly* recommend moving the `eval()` into a method on `SafeInput`, so that you don't need an explicit type check. Give the method some name that isn't used in any of your other classes. That way, you can still mock the whole thing out for testing purposes. – Kevin Mar 04 '16 at 06:32
  • @Kevin: Seeing as `eval` has access to local variables, it might be that the code depends on how it mutates the variables. By moving the eval into another method, it will no longer have the ability to mutate local variables. – Sjoerd Job Postmus Mar 04 '16 at 08:00
  • 1
    A further step might be to have `SafeInput` constructor take a name/handle of the local file and do the read itself, ensuring that the data does indeed come from a local file. – Owen Mar 04 '16 at 12:00
  • 1
    I don't have much experience with python, but isn't it better to throw an exception? In most languages asserts can be turned off and are (as I understand them) more for debugging than for safety. Exceptions and exiting to console guarantee that the next code will not be run. – user1122069 Mar 04 '16 at 17:21
  • 1
    @user1122069 In Python, `assert test, msg` is equivalent to `if __debug__ and not test: raise AssertionError(msg)`, where `__debug__` is true by default unless optimization is requested (`python -O …`). But whether assertion syntax is used or an explicit error is thrown is not relevant for the idea of using a type system to mark safe/unsafe input. FermiParadox is probably right that an explicit error would be more fitting for their use case. – amon Mar 04 '16 at 17:46
11

As Joel once pointed out, make wrong code look wrong (scroll down to section "The Real Solution", or better yet read it completely; it's worth it, even if it addresses variable instead of function names). So I humbly suggest to rename your function to something like f_unsafe_for_external_use() - you'll most probably come up with some abbreviation scheme yourself.

Edit: I think amon's suggestion is a very good, OO based variation on the same theme :-)

Murphy
  • 821
  • 6
  • 15
0

Complementing the suggestion by @amon, you can also think about combining the strategy pattern here, as well as the method object pattern.

class AbstractFoobarizer(object):
    def handle_cond(self, foo, bar):
        """
        Handle the event or something.
        """
        raise NotImplementedError

    def run(self):
        # do some magic
        pass

And then a 'normal' implementation

class PrintingFoobarizer(AbstractFoobarizer):
    def handle_cond(self, foo, bar):
        print("Something went very well regarding {} and {}".format(foo, bar))

And an admin-input-eval implementation

class AdminControllableFoobarizer(AbstractFoobarizer):
    def handle_cond(self, foo, bar):
        # Look up code in database/config file/...
        code = get_code_from_settings()
        eval(code)

(Note: with a real-world-ish example, I might be able to give a better example).

Sjoerd Job Postmus
  • 1,814
  • 1
  • 10
  • 12