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)

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__'] ]