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.

11
  • 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 Mar 4, 2016 at 0:22
  • 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 4, 2016 at 5:04
  • Put a comment in the file saying "This string gets eval'd; please do not put malicious code here"?
    – user253751
    Mar 4, 2016 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 by Murphy in his answer.
    – user
    Mar 4, 2016 at 11:15
  • @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. Mar 4, 2016 at 17:17

3 Answers 3

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!

6
  • 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 3, 2016 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 4, 2016 at 6:32
  • 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 4, 2016 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. Mar 4, 2016 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 4, 2016 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 :-)

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.