From d1362ea83d4d7eae9dd786109c02224928b2bf01 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Sat, 3 Oct 2020 20:16:02 +0000 Subject: [PATCH] Replace show result with debug evaluate command The show result subcommand was quite unsafe. Even though it limited the globals for strings passed to the env() builtin, it was still possible for admins to do things like call the exec() builtin and then import other modules, or use open() to overwrite files writeable by the user under which the engine was running. Introduce a new evaluate command as a substitute and remove the show result subcommand. Use the debugging framework to limit access to evaluate so that it's only available if debug mode is enabled in the configuration at the time the daemon is started. Further limit evaluate to not have most of the normal builtins, and explicitly reject any strings containing a double-underscore (__) so that base attributes of other modules such as __builtins__ can't be called into easily, or "lambda" so that lambda functions can't be used to work around protections. Also add some selftests to make sure evaluate can still use the expressions we previously tested with show result, and that only administrators can use it, and that it's only available to them when debug mode is enabled. The evaluate command is still to be considered quite unsafe, and debug mode should only be engaged when all administrators with access to the service are trusted with the same permissions the system account running the service also possesses. --- doc/source/admin.rst | 19 +++++++++++++------ doc/source/coder.rst | 4 ++-- mudpy/command.py | 42 ++++++++++++++++++++++++++++-------------- mudpy/tests/selftest.py | 43 +++++++++++++++++++++++++++++++------------ share/command.yaml | 17 +++++++++++++++-- 5 files changed, 89 insertions(+), 36 deletions(-) diff --git a/doc/source/admin.rst b/doc/source/admin.rst index b25d6ae..3ece3f7 100644 --- a/doc/source/admin.rst +++ b/doc/source/admin.rst @@ -60,7 +60,7 @@ troubleshooting The administrative :command:`show` command provides a number of useful inspection tools. Here's an example testing with the -:command:`show result` subcommand from an active session with a +:command:`evaluate` debug command from an active session with a couple of avatars awake, comparing with the output from related :command:`show group` and :command:`show element` invocations:: @@ -71,7 +71,7 @@ couple of avatars awake, comparing with the output from related actor.avatar_admin_0 actor.avatar_luser0_0 - > show result actor.universe.groups['actor'].keys() + > evaluate actor.universe.groups['actor'].keys() dict_keys(['avatar_admin_0', 'avatar_luser0_0']) @@ -85,10 +85,17 @@ couple of avatars awake, comparing with the output from related location: area.0,0,0 name: Keyo - > show result actor.universe.contents['actor.avatar_luser0_0'].get('name') + > evaluate actor.universe.contents['actor.avatar_luser0_0'].get('name') 'Keyo' -Note that for safety the :command:`show result` executes within the context of -a command handler with only Python's :code:`__builtins__`, the :code:`mudpy` -library package, and the active :code:`universe` available. +Note that for safety the :command:`evaluate` executes within the context of +a command handler with limited Python :code:`__builtins__`, the +:code:`mudpy` library package, and the active :code:`universe` available, +and also blocks evaluation of any statement containing double-underscores +(:code:`__`) as well as :code:`lambda` functions. For admins to gain access +to unsafe debugging commands, the ``.mudpy.limit.debug`` option must be +enabled in configuration first and the service completely restarted. It +should still be considered unsafe, since the engine's file handling +functions could easily alter accessible files or expressions like +``9**9**9`` could be used to hang the service for indeterminate periods. diff --git a/doc/source/coder.rst b/doc/source/coder.rst index b3ed6da..e91c464 100644 --- a/doc/source/coder.rst +++ b/doc/source/coder.rst @@ -208,7 +208,7 @@ how I'd go about implementing it: skill, spell, item, or however finding that information would best fit into your setting). This is stored in the element's :code:`.key` attribute, and you can play around with it on the - command line with :command:`show result` like this:: + command line with :command:`evaluate` like this:: > look Center Sample Location @@ -218,7 +218,7 @@ how I'd go about implementing it: A sample prop sits here. Utso is here. - > show result [ + > evaluate [ a.key.split('_', 1)[1] for a in actor.universe.contents[ actor.get('location') diff --git a/mudpy/command.py b/mudpy/command.py index 279da78..5ee7ce8 100644 --- a/mudpy/command.py +++ b/mudpy/command.py @@ -134,6 +134,34 @@ def error(actor, input_data): return True +def evaluate(actor, parameters): + """Evaluate a Python expression.""" + + if not parameters: + message = "You need to supply a Python expression." + elif "__" in parameters: + message = "Double-underscores (__) are not allowed in expressions." + elif "lambda" in parameters: + message = "Lambda functions are not allowed in expressions." + else: + # Strictly limit the allowed builtins and modules + eval_globals = {"__builtins__": dict()} + for allowed in ("dir", "globals", "len", "locals"): + eval_globals["__builtins__"][allowed] = __builtins__[allowed] + eval_globals["mudpy"] = mudpy + eval_globals["universe"] = actor.universe + try: + # there is no other option than to use eval() for this, since + # its purpose is to evaluate arbitrary expressions, so do what + # we can to secure it and allow it for bandit analysis + message = repr(eval(parameters, eval_globals)) # nosec + except Exception as e: + message = ("$(red)Your expression raised an exception...$(eol)" + "$(eol)$(bld)%s$(nrm)" % e) + actor.send(message) + return True + + def halt(actor, parameters): """Halt the world.""" if actor.owner: @@ -548,20 +576,6 @@ def show(actor, parameters): (facet, str(facets[facet]))) else: message = 'Element "' + arguments[1] + '" does not exist.' - elif arguments[0] == "result": - if len(arguments) < 2: - message = "You need to specify an expression." - else: - try: - # there is no other option than to use eval() for this, since - # its purpose is to evaluate arbitrary expressions, so do what - # we can to secure it and allow it for bandit analysis - message = repr(eval( # nosec - " ".join(arguments[1:]), - {"mudpy": mudpy, "universe": actor.universe})) - except Exception as e: - message = ("$(red)Your expression raised an exception...$(eol)" - "$(eol)$(bld)%s$(nrm)" % e) elif arguments[0] == "log": if len(arguments) == 4: if re.match(r"^\d+$", arguments[3]) and int(arguments[3]) >= 0: diff --git a/mudpy/tests/selftest.py b/mudpy/tests/selftest.py index 0bd2dd9..c6f3398 100644 --- a/mudpy/tests/selftest.py +++ b/mudpy/tests/selftest.py @@ -340,17 +340,32 @@ test_show_element = ( r' \x1b\[32mgender: \x1b\[31mfemale.*> ', ""), ) -test_show_result = ( - (2, "> ", "show result 12345*67890"), - (2, r"\r\n838102050\r\n.*> ", "show result 1/0"), - (2, r"Your expression raised an exception.*division by zero.*> ", - "show result mudpy"), - (2, r" ", "show result re"), - (2, r"Your expression raised an exception.*name 're' is not defined.*> ", - "show result universe"), - (2, r" ", "show result actor"), - (2, r"Your expression raised an exception.*name 'actor' is not " - r"defined.*> ", ""), +test_evaluate = ( + (2, "> ", "evaluate 12345*67890"), + (2, r"\r\n838102050\r\n.*> ", "evaluate 1/0"), + (2, "Your expression raised an exception.*division by zero.*> ", + "evaluate mudpy"), + (2, " ", "evaluate re"), + (2, "Your expression raised an exception.*name 're' is not defined.*> ", + "evaluate universe"), + (2, r" ", "evaluate actor"), + (2, "Your expression raised an exception.*name 'actor' is not defined.*> ", + "evaluate dir(mudpy)"), + (2, "__builtins__.*> ", "evaluate mudpy.__builtins__.open"), + (2, "not allowed.*> ", "evaluate (lambda x: x + 1)(2)"), + (2, "not allowed.*> ", ""), +) + +test_debug_restricted = ( + (0, "> ", "help evaluate"), + (0, r"That is not an available command\.", "evaluate"), + (0, '(not sure what "evaluate" means|Arglebargle, glop-glyf)', ""), +) + +test_debug_disabled = ( + (2, "> ", "help evaluate"), + (2, r"That is not an available command\.", "evaluate"), + (2, '(not sure what "evaluate" means|Arglebargle, glop-glyf)', ""), ) test_show_log = ( @@ -426,7 +441,9 @@ dialogue = odict(( (test_show_groups, "show groups"), (test_show_group, "show group"), (test_show_element, "show element"), - (test_show_result, "show result of a python expression"), + (test_evaluate, "show results of python expressions"), + (test_debug_restricted, "only admins can run debug commands"), + (test_debug_disabled, "debugging commands only in debug mode"), (test_show_log, "show log"), (test_custom_loglevel, "custom loglevel"), (test_invalid_loglevel, "invalid loglevel"), @@ -435,9 +452,11 @@ dialogue = odict(( )) debug_tests = ( + test_evaluate, ) nondebug_tests = ( + test_debug_disabled, ) diff --git a/share/command.yaml b/share/command.yaml index 0f4df40..87da9a3 100644 --- a/share/command.yaml +++ b/share/command.yaml @@ -20,6 +20,20 @@ command.create.description: Create a new element in the universe. command.create.help: Ways to create an element:$(eol)$(eol) create actor.avatar_fred_1$(eol) create other.garply foo/bar/baz +command.evaluate.debugging: true +command.evaluate.description: Evaluate a Python expression. +command.evaluate.help: For debugging purposes, you can use this to run certain + Python language expressions within the running engine's context, though for + safety reasons only a limited set of builtins are allowed, as well as + objects in the mudpy package namespace and the active universe object. + Expressions containing "__" or "lambda" are also prohobited for additional + safety. Everything following the word "evaluate" is assumed to be a Python + expression, and is passed to the eval() built-in, outputting a string + representation of whatever it returns. Any exceptions are caught in an + attempt to avoid accidentally crashing the engine. This command is mostly + useful for inspecting the contents of in-memory objects, for + example:$(eol)$(eol) evaluate universe.groups['actor'].keys() + command.delete.administrative: true command.delete.description: Delete an existing facet from an element. command.delete.help: You can delete any facet of an element as @@ -95,6 +109,5 @@ command.show.help: Here are the possible incantations ( is required, data files)$(eol) show group (list the elements in a group)$(eol) show groups (list all element group names)$(eol) show log [level [start [stop]]] (list logs above level from start to - stop)$(eol) show result (evaluate a python - expression)$(eol) show time (return several current timer + stop)$(eol) show time (return several current timer values)$(eol) show version (display running version and dependencies) -- 2.11.0