diff options
author | James Cammarata <jimi@sngx.net> | 2014-07-20 21:48:31 -0500 |
---|---|---|
committer | James Cammarata <jimi@sngx.net> | 2014-07-20 21:48:31 -0500 |
commit | 62a1295a3e08cb6c3e9f1b2a1e6e5dcaeab32527 (patch) | |
tree | d9056bc3ca0d203c00417b95a3b5658a07e7a5fd | |
parent | 69d85c22c7475ccf8169b6ec9dee3ee28c92a314 (diff) | |
download | ansible-62a1295a3e08cb6c3e9f1b2a1e6e5dcaeab32527.tar.gz |
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)
-rw-r--r-- | lib/ansible/module_utils/basic.py | 2 | ||||
-rw-r--r-- | lib/ansible/runner/__init__.py | 46 | ||||
-rw-r--r-- | lib/ansible/runner/action_plugins/assemble.py | 19 | ||||
-rw-r--r-- | lib/ansible/runner/action_plugins/copy.py | 23 | ||||
-rw-r--r-- | lib/ansible/runner/action_plugins/template.py | 9 | ||||
-rw-r--r-- | lib/ansible/utils/__init__.py | 62 | ||||
-rw-r--r-- | library/commands/command | 61 | ||||
-rw-r--r-- | test/integration/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 423bdfb514..ceb3793f24 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -772,6 +772,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 44764cd2a5..f39feb93fc 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 @@ -394,6 +395,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) + # ***************************************************** @@ -612,6 +642,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) @@ -823,7 +856,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 d99d202e24..defc6f886c 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._low_level_exec_command(conn, "chmod a+r %s" % 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 df5266c4c0..baab7157c3 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 96d8f97a3a..e887f64340 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._low_level_exec_command(conn, "chmod a+r %s" % 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 a6856e5872..f5719ec4f0 100644 --- a/lib/ansible/utils/__init__.py +++ b/lib/ansible/utils/__init__.py @@ -51,6 +51,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: @@ -313,38 +317,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 @@ -379,10 +389,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: @@ -615,7 +646,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): @@ -1062,11 +1093,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 f1a4892212..1b27f40b81 100644 --- a/library/commands/command +++ b/library/commands/command @@ -176,38 +176,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>[\'"])?(.*?)(?(quote)(?<!\\)(?P=quote))((?<!\\)(?=\s)|$)') - for m in r.finditer(args): - v = m.group(4).replace("\\", "") - if m.group(2) == "creates": - params['creates'] = v - elif m.group(2) == "removes": - params['removes'] = v - elif m.group(2) == "chdir": - v = os.path.expanduser(v) - v = os.path.abspath(v) - if not (os.path.exists(v) and os.path.isdir(v)): - self.fail_json(rc=258, msg="cannot change to directory '%s': path does not exist" % v) - params['chdir'] = v - elif m.group(2) == "executable": - v = os.path.expanduser(v) - v = os.path.abspath(v) - if not (os.path.exists(v)): - self.fail_json(rc=258, msg="cannot use executable '%s': file does not exist" % v) - params['executable'] = v - elif m.group(2) == "NO_LOG": - params['NO_LOG'] = self.boolean(v) - args = r.sub("", args) - params['args'] = args + # use shlex to split up the args, while being careful to preserve + # single quotes so they're not removed accidentally + lexer = shlex.shlex(args, posix=True) + lexer.whitespace_split = True + lexer.quotes = '"' + lexer.ignore_quotes = "'" + items = list(lexer) + + command_args = '' + for x in items: + if '=' in x: + # check to see if this is a special parameter for the command + k, v = x.split('=', 1) + if k in ('creates', 'removes', 'chdir', 'executable', 'NO_LOG'): + if k == "chdir": + v = os.path.abspath(os.path.expanduser(v)) + if not (os.path.exists(v) and os.path.isdir(v)): + self.fail_json(rc=258, msg="cannot change to directory '%s': path does not exist" % v) + elif k == "executable": + v = os.path.abspath(os.path.expanduser(v)) + if not (os.path.exists(v)): + self.fail_json(rc=258, msg="cannot use executable '%s': file does not exist" % v) + params[k] = v + else: + # this isn't a valid parameter, so just append it back to the list of arguments + command_args = "%s %s" % (command_args, x) + else: + # not a param, so just append it to the list of arguments + command_args = "%s %s" % (command_args, x) + params['args'] = command_args.strip() return (params, params['args']) main() diff --git a/test/integration/roles/test_iterators/tasks/main.yml b/test/integration/roles/test_iterators/tasks/main.yml index 0352d3c1ae..c95eaff3da 100644 --- a/test/integration/roles/test_iterators/tasks/main.yml +++ b/test/integration/roles/test_iterators/tasks/main.yml @@ -19,7 +19,7 @@ # WITH_ITEMS - name: test with_items - set_fact: "{{ item + '=moo' }}" + set_fact: "{{ item }}=moo" with_items: - 'foo' - 'bar' @@ -36,7 +36,7 @@ # WITH_NESTED - name: test with_nested - set_fact: "{{ item.0 + item.1 + '=x' }}" + set_fact: "{{ item.0 + item.1 }}=x" with_nested: - [ 'a', 'b' ] - [ 'c', 'd' ] @@ -57,7 +57,7 @@ # WITH_SEQUENCE - name: test with_sequence - set_fact: "{{ 'x' + item + '=' + item }}" + set_fact: "{{ 'x' + item }}={{ item }}" with_sequence: start=0 end=3 - name: verify with_sequence @@ -71,7 +71,7 @@ # WITH_RANDOM_CHOICE - name: test with_random_choice - set_fact: "{{ 'random=' + item }}" + set_fact: "random={{ item }}" with_random_choice: - "foo" - "bar" @@ -84,7 +84,7 @@ # WITH_SUBELEMENTS - name: test with_subelements - set_fact: "{{ '_'+ item.0.id + item.1 + '=' + item.1 }}" + set_fact: "{{ '_'+ item.0.id + item.1 }}={{ item.1 }}" with_subelements: - element_data - the_list @@ -101,7 +101,7 @@ - name: test with_together #shell: echo {{ item }} - set_fact: "{{ item.0 + '=' + item.1 }}" + set_fact: "{{ item.0 }}={{ item.1 }}" with_together: - [ 'a', 'b', 'c', 'd' ] - [ '1', '2', '3', '4' ] @@ -124,7 +124,7 @@ - name: test with_first_found #shell: echo {{ item }} - set_fact: "{{ 'first_found=' + item }}" + set_fact: "first_found={{ item }}" with_first_found: - "{{ output_dir + '/does_not_exist' }}" - "{{ output_dir + '/foo1' }}" @@ -146,7 +146,7 @@ - name: test with_lines #shell: echo "{{ item }}" - set_fact: "{{ item + '=set' }}" + set_fact: "{{ item }}=set" with_lines: for i in $(seq 1 5); do echo "l$i" ; done; - name: verify with_lines results @@ -164,7 +164,7 @@ register: list_data - name: create indexed list - set_fact: "{{ item[1] + item[0]|string + '=set' }}" + set_fact: "{{ item[1] + item[0]|string }}=set" with_indexed_items: list_data.stdout_lines - name: verify with_indexed_items result @@ -179,8 +179,7 @@ # WITH_FLATTENED - name: test with_flattened - #shell: echo {{ item + "test" }} - set_fact: "{{ item + '=flattened' }}" + set_fact: "{{ item }}=flattened" with_flattened: - [ 'a__' ] - [ 'b__', ['c__', 'd__'] ] |