Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Note: I do know eval() is dangerous and can usually be replaced by something safe. My goal is to learn rather than find a solution to an existing problem (Besides, I will be replacing completely the eval in my program soon.)

I have a function f() which 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)?

share|improve this question
    
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 4 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 at 5:04
    
Put a comment in the file saying "This string gets eval'd; please do not put malicious code here"? – immibis Mar 4 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. – Fermi paradox SO Mar 4 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. – user1122069 Mar 4 at 17:17
up vote 23 down vote accepted

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!

share|improve this answer
    
Awesome! My solution was to simply name it as unsafe_blabla so that I visually detect it, but what you suggest is even better. – Fermi paradox SO Mar 3 at 20:53
4  
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. – Fermi paradox SO Mar 3 at 20:57
2  
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 at 6: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 4 at 8: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 4 at 17:21

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

share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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