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

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

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

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)

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)

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

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