2

I'm wracking my brain trying to find a way to exploit this setup. My concept is this: my twisted server receives messages (via LineReceiver). The messages are base64 JSON, which SHOULD contain a single dictionary. The dictionary has a "INSTRUCTION" key which indicates what sort of client action the server should be processing.

At this portion of the program, line is a base64 string received over the network.

def Decode(line):
    return json.loads(base64.b64decode(line))

And the interpretation (inside a twisted Protocol object)

def lineReceived(self, line):
    instruction = Decode(line)  #dict
    if instruction and "INSTRUCTION" in instruction:
        if instruction["INSTRUCTION"] in ("register", "join", "create", 
         "list", "passturn", "impmove", "warpmove","laserattack",
         "torpattack", "mine", "data", "status"):
            cmdstring = instruction["INSTRUCTION"] + "(self)"
            eval(cmdstring)

Why I think it's safe:

  • eval will only run if I get a plain string as the value.
  • JSON can't pack complex objecst, only python basics. An attacker shouldn't have __str__ access to what I receive, right?
  • I'm using eval to provide a less readable but much more compact replacement for a dozen if instruction["INSTRUCTION"] == "functionA": functionA(self) lines. I'm just running eval to choose a function from a specific list.

Is this safe? Is this considered acceptable style or form? Is this block of code robust enough for a multiplayer game, since the client cannot be trusted? (Validation that the instruction follows the game rules is later, here I want to protect my server from destructive tinkering.)

Is there a better way to do what I'm attempting (remote execution of a function, I suppose) that is more standard / safe?

4

1 回答 1

4

You'd be better of using getattr():

if instruction and "INSTRUCTION" in instruction:
    instr_callable = getattr(self, 'do_' + instruction['INSTRUCTION'], None)
    if instr_callable is not None:
        instr_callable()

where do_ is prefixed to the instruction name to ensure that only allowed methods are called through this method.

If your instruction functions live in the global namespace, use globals() instead, and use it as a mapping:

if instruction and "INSTRUCTION" in instruction:
    instr_callable = globals().get('do_' + instruction['INSTRUCTION'], None)
    if instr_callable is not None:
        instr_callable(self)

However, you'd then be better of putting those callables in an explicit mapping:

instr_callables = dict(
    register=register,
    join=join,
    ...
)

if instruction and "INSTRUCTION" in instruction:
    instr_callable = instr_callables.get(instruction['INSTRUCTION'], None)
    if instr_callable is not None:
        instr_callable(self)

eval() is rarely, if ever, needed to look up arbitrary objects in python namespaces.

于 2012-10-28T17:56:03.040 回答