From 84759faa0950146a6bae8452580b4a4cede6d871 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Mon, 21 Jul 2014 11:20:49 -0500 Subject: [PATCH] Security fixes: * Strip lookup calls out of inventory variables and clean unsafe data returned from lookup plugins (CVE-2014-4966) * Make sure vars don't insert extra parameters into module args and prevent duplicate params from superseding previous params (CVE-2014-4967) --- lib/ansible/module_utils/basic.py | 2 + lib/ansible/runner/__init__.py | 46 ++++++++++++++ lib/ansible/runner/action_plugins/assemble.py | 19 ++++-- lib/ansible/runner/action_plugins/copy.py | 23 ++++--- lib/ansible/runner/action_plugins/template.py | 9 ++- lib/ansible/utils/__init__.py | 62 ++++++++++++++----- library/commands/command | 61 ++++++++++-------- .../roles/test_iterators/tasks/main.yml | 21 +++---- 8 files changed, 178 insertions(+), 65 deletions(-) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index e08c817fe7..c63a7b492e 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -856,6 +856,8 @@ class AnsibleModule(object): (k, v) = x.split("=",1) except Exception, e: self.fail_json(msg="this module requires key=value arguments (%s)" % (items)) + if k in params: + self.fail_json(msg="duplicate parameter: %s (value=%s)" % (k, v)) params[k] = v params2 = json.loads(MODULE_COMPLEX_ARGS) params2.update(params) diff --git a/lib/ansible/runner/__init__.py b/lib/ansible/runner/__init__.py index 585ea37f76..12b5d3c030 100644 --- a/lib/ansible/runner/__init__.py +++ b/lib/ansible/runner/__init__.py @@ -31,6 +31,7 @@ import sys import pipes import jinja2 import subprocess +import shlex import getpass import ansible.constants as C @@ -388,6 +389,35 @@ class Runner(object): return actual_user + def _count_module_args(self, args): + ''' + Count the number of k=v pairs in the supplied module args. This is + basically a specialized version of parse_kv() from utils with a few + minor changes. + ''' + options = {} + if args is not None: + args = args.encode('utf-8') + try: + lexer = shlex.shlex(args, posix=True) + lexer.whitespace_split = True + lexer.quotes = '"' + lexer.ignore_quotes = "'" + vargs = list(lexer) + except ValueError, ve: + if 'no closing quotation' in str(ve).lower(): + raise errors.AnsibleError("error parsing argument string '%s', try quoting the entire line." % args) + else: + raise + vargs = [x.decode('utf-8') for x in vargs] + for x in vargs: + if "=" in x: + k, v = x.split("=",1) + if k in options: + raise errors.AnsibleError("a duplicate parameter was found in the argument string (%s)" % k) + options[k] = v + return len(options) + # ***************************************************** @@ -604,6 +634,9 @@ class Runner(object): items_terms = self.module_vars.get('items_lookup_terms', '') items_terms = template.template(basedir, items_terms, inject) items = utils.plugins.lookup_loader.get(items_plugin, runner=self, basedir=basedir).run(items_terms, inject=inject) + # strip out any jinja2 template syntax within + # the data returned by the lookup plugin + items = utils._clean_data_struct(items, from_remote=True) if type(items) != list: raise errors.AnsibleError("lookup plugins have to return a list: %r" % items) @@ -826,7 +859,20 @@ class Runner(object): # render module_args and complex_args templates try: + # When templating module_args, we need to be careful to ensure + # that no variables inadvertantly (or maliciously) add params + # to the list of args. We do this by counting the number of k=v + # pairs before and after templating. + num_args_pre = self._count_module_args(module_args) module_args = template.template(self.basedir, module_args, inject, fail_on_undefined=self.error_on_undefined_vars) + num_args_post = self._count_module_args(module_args) + if num_args_pre != num_args_post: + raise errors.AnsibleError("A variable inserted a new parameter into the module args. " + \ + "Be sure to quote variables if they contain equal signs (for example: \"{{var}}\").") + # And we also make sure nothing added in special flags for things + # like the command/shell module (ie. #USE_SHELL) + if '#USE_SHELL' in module_args: + raise errors.AnsibleError("A variable tried to add #USE_SHELL to the module arguments.") complex_args = template.template(self.basedir, complex_args, inject, fail_on_undefined=self.error_on_undefined_vars) except jinja2.exceptions.UndefinedError, e: raise errors.AnsibleUndefinedVariable("One or more undefined variables: %s" % str(e)) diff --git a/lib/ansible/runner/action_plugins/assemble.py b/lib/ansible/runner/action_plugins/assemble.py index 1a980c1df4..58a4905479 100644 --- a/lib/ansible/runner/action_plugins/assemble.py +++ b/lib/ansible/runner/action_plugins/assemble.py @@ -122,14 +122,25 @@ class ActionModule(object): self.runner._remote_chmod(conn, 'a+r', xfered, tmp) # run the copy module - module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(src))) + new_module_args = dict( + src=xfered, + dest=dest, + original_basename=os.path.basename(src), + ) + module_args_tmp = utils.merge_module_args(module_args, new_module_args) if self.runner.noop_on_check(inject): return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), diff=dict(before_header=dest, after_header=src, after=resultant)) else: - res = self.runner._execute_module(conn, tmp, 'copy', module_args, inject=inject) + res = self.runner._execute_module(conn, tmp, 'copy', module_args_tmp, inject=inject) res.diff = dict(after=resultant) return res else: - module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(src))) - return self.runner._execute_module(conn, tmp, 'file', module_args, inject=inject) + new_module_args = dict( + src=xfered, + dest=dest, + original_basename=os.path.basename(src), + ) + module_args_tmp = utils.merge_module_args(module_args, new_module_args) + + return self.runner._execute_module(conn, tmp, 'file', module_args_tmp, inject=inject) diff --git a/lib/ansible/runner/action_plugins/copy.py b/lib/ansible/runner/action_plugins/copy.py index c59042fb2b..3587ad61ec 100644 --- a/lib/ansible/runner/action_plugins/copy.py +++ b/lib/ansible/runner/action_plugins/copy.py @@ -238,11 +238,16 @@ class ActionModule(object): # src and dest here come after original and override them # we pass dest only to make sure it includes trailing slash in case of recursive copy - module_args_tmp = "%s src=%s dest=%s original_basename=%s" % (module_args, - pipes.quote(tmp_src), pipes.quote(dest), pipes.quote(source_rel)) + new_module_args = dict( + src=tmp_src, + dest=dest, + original_basename=source_rel + ) if self.runner.no_log: - module_args_tmp = "%s NO_LOG=True" % module_args_tmp + new_module_args['NO_LOG'] = True + + module_args_tmp = utils.merge_module_args(module_args, new_module_args) module_return = self.runner._execute_module(conn, tmp_path, 'copy', module_args_tmp, inject=inject, complex_args=complex_args, delete_remote_tmp=delete_remote_tmp) module_executed = True @@ -260,12 +265,16 @@ class ActionModule(object): tmp_src = tmp_path + source_rel # Build temporary module_args. - module_args_tmp = "%s src=%s original_basename=%s" % (module_args, - pipes.quote(tmp_src), pipes.quote(source_rel)) + new_module_args = dict( + src=tmp_src, + dest=dest, + ) if self.runner.noop_on_check(inject): - module_args_tmp = "%s CHECKMODE=True" % module_args_tmp + new_module_args['CHECKMODE'] = True if self.runner.no_log: - module_args_tmp = "%s NO_LOG=True" % module_args_tmp + new_module_args['NO_LOG'] = True + + module_args_tmp = utils.merge_module_args(module_args, new_module_args) # Execute the file module. module_return = self.runner._execute_module(conn, tmp_path, 'file', module_args_tmp, inject=inject, complex_args=complex_args, delete_remote_tmp=delete_remote_tmp) diff --git a/lib/ansible/runner/action_plugins/template.py b/lib/ansible/runner/action_plugins/template.py index f7ac3b34e0..b16e5f66e6 100644 --- a/lib/ansible/runner/action_plugins/template.py +++ b/lib/ansible/runner/action_plugins/template.py @@ -117,12 +117,17 @@ class ActionModule(object): self.runner._remote_chmod(conn, 'a+r', xfered, tmp) # run the copy module - module_args = "%s src=%s dest=%s original_basename=%s" % (module_args, pipes.quote(xfered), pipes.quote(dest), pipes.quote(os.path.basename(source))) + new_module_args = dict( + src=xfered, + dest=dest, + original_basename=os.path.basename(source), + ) + module_args_tmp = utils.merge_module_args(module_args, new_module_args) if self.runner.noop_on_check(inject): return ReturnData(conn=conn, comm_ok=True, result=dict(changed=True), diff=dict(before_header=dest, after_header=source, before=dest_contents, after=resultant)) else: - res = self.runner._execute_module(conn, tmp, 'copy', module_args, inject=inject, complex_args=complex_args) + res = self.runner._execute_module(conn, tmp, 'copy', module_args_tmp, inject=inject, complex_args=complex_args) if res.result.get('changed', False): res.diff = dict(before=dest_contents, after=resultant) return res diff --git a/lib/ansible/utils/__init__.py b/lib/ansible/utils/__init__.py index f5f879d2a7..4a92907e6b 100644 --- a/lib/ansible/utils/__init__.py +++ b/lib/ansible/utils/__init__.py @@ -53,6 +53,10 @@ VERBOSITY=0 MAX_FILE_SIZE_FOR_DIFF=1*1024*1024 +# caching the compilation of the regex used +# to check for lookup calls within data +LOOKUP_REGEX=re.compile(r'lookup\s*\(') + try: import json except ImportError: @@ -341,38 +345,44 @@ def json_loads(data): return json.loads(data) -def _clean_data(orig_data): +def _clean_data(orig_data, from_remote=False, from_inventory=False): ''' remove template tags from a string ''' data = orig_data if isinstance(orig_data, basestring): - for pattern,replacement in (('{{','{#'), ('}}','#}'), ('{%','{#'), ('%}','#}')): + sub_list = [('{%','{#'), ('%}','#}')] + if from_remote or (from_inventory and '{{' in data and LOOKUP_REGEX.search(data)): + # if from a remote, we completely disable any jinja2 blocks + sub_list.extend([('{{','{#'), ('}}','#}')]) + for pattern,replacement in sub_list: data = data.replace(pattern, replacement) return data -def _clean_data_struct(orig_data): +def _clean_data_struct(orig_data, from_remote=False, from_inventory=False): ''' walk a complex data structure, and use _clean_data() to remove any template tags that may exist ''' + if not from_remote and not from_inventory: + raise errors.AnsibleErrors("when cleaning data, you must specify either from_remote or from_inventory") if isinstance(orig_data, dict): data = orig_data.copy() for key in data: - new_key = _clean_data_struct(key) - new_val = _clean_data_struct(data[key]) + new_key = _clean_data_struct(key, from_remote, from_inventory) + new_val = _clean_data_struct(data[key], from_remote, from_inventory) if key != new_key: del data[key] data[new_key] = new_val elif isinstance(orig_data, list): data = orig_data[:] for i in range(0, len(data)): - data[i] = _clean_data_struct(data[i]) + data[i] = _clean_data_struct(data[i], from_remote, from_inventory) elif isinstance(orig_data, basestring): - data = _clean_data(orig_data) + data = _clean_data(orig_data, from_remote, from_inventory) else: data = orig_data return data -def parse_json(raw_data, from_remote=False): +def parse_json(raw_data, from_remote=False, from_inventory=False): ''' this version for module return data only ''' orig_data = raw_data @@ -407,10 +417,31 @@ def parse_json(raw_data, from_remote=False): return { "failed" : True, "parsed" : False, "msg" : orig_data } if from_remote: - results = _clean_data_struct(results) + results = _clean_data_struct(results, from_remote, from_inventory) return results +def merge_module_args(current_args, new_args): + ''' + merges either a dictionary or string of k=v pairs with another string of k=v pairs, + and returns a new k=v string without duplicates. + ''' + if not isinstance(current_args, basestring): + raise errors.AnsibleError("expected current_args to be a basestring") + # we use parse_kv to split up the current args into a dictionary + final_args = parse_kv(current_args) + if isinstance(new_args, dict): + final_args.update(new_args) + elif isinstance(new_args, basestring): + new_args_kv = parse_kv(new_args) + final_args.update(new_args_kv) + # then we re-assemble into a string + module_args = "" + for (k,v) in final_args.iteritems(): + if isinstance(v, basestring): + module_args = "%s=%s %s" % (k, pipes.quote(v), module_args) + return module_args.strip() + def smush_braces(data): ''' smush Jinaj2 braces so unresolved templates like {{ foo }} don't get parsed weird by key=value code ''' while '{{ ' in data: @@ -641,7 +672,7 @@ def parse_kv(args): for x in vargs: if "=" in x: k, v = x.split("=",1) - options[k]=v + options[k] = v return options def merge_hash(a, b): @@ -1089,11 +1120,14 @@ def list_intersection(a, b): def safe_eval(expr, locals={}, include_exceptions=False): ''' - this is intended for allowing things like: + This is intended for allowing things like: with_items: a_list_variable - where Jinja2 would return a string - but we do not want to allow it to call functions (outside of Jinja2, where - the env is constrained) + + Where Jinja2 would return a string but we do not want to allow it to + call functions (outside of Jinja2, where the env is constrained). If + the input data to this function came from an untrusted (remote) source, + it should first be run through _clean_data_struct() to ensure the data + is further sanitized prior to evaluation. Based on: http://stackoverflow.com/questions/12523516/using-ast-and-whitelists-to-make-pythons-eval-safe diff --git a/library/commands/command b/library/commands/command index 342e3db493..f61bd1f7dc 100644 --- a/library/commands/command +++ b/library/commands/command @@ -184,38 +184,45 @@ class CommandModule(AnsibleModule): ''' read the input and return a dictionary and the arguments string ''' args = MODULE_ARGS params = {} - params['chdir'] = None - params['creates'] = None - params['removes'] = None - params['shell'] = False + params['chdir'] = None + params['creates'] = None + params['removes'] = None + params['shell'] = False params['executable'] = None if "#USE_SHELL" in args: args = args.replace("#USE_SHELL", "") params['shell'] = True - r = re.compile(r'(^|\s)(creates|removes|chdir|executable|NO_LOG)=(?P[\'"])?(.*?)(?(quote)(?