Merge pull request #886 from Shrews/backport/872/release_2.1

[release_2.1] Remove shell use in subprocess

Backport of PR #872
(cherry picked from commit 3533f26)

Reviewed-by: Sam Doran <sdoran@redhat.com>
Reviewed-by: None <None>

ansible_runner/config/doc.py

@@ -87,7 +87,7 @@ def prepare_plugin_docs_command(self, plugin_names, plugin_type=None, response_f
         if module_path:
             self.cmdline_args.extend(['-M', module_path])
 
-        self.cmdline_args.append(" ".join(plugin_names))
+        self.cmdline_args.extend(plugin_names)
 
         self.command = [self._ansible_doc_exec_path] + self.cmdline_args
         self._handle_command_wrap(self.execution_mode, self.cmdline_args)

ansible_runner/runner.py

@@ -204,8 +204,12 @@ def run(self):
             user = getpass.getuser()
             group = grp.getgrgid(os.getgid()).gr_name
 
-            cmd = 'cgcreate -a {user}:{group} -t {user}:{group} -g cpuacct,memory,pids:{}'.format(cgroup_path, user=user, group=group)
-            proc = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
+            cmd = ['cgcreate',
+                   '-a', f'{user}:{group}',
+                   '-t', f'{user}:{group}',
+                   '-g', f'cpuacct,memory,pids:{cgroup_path}',
+                   ]
+            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
             _, stderr = proc.communicate()
             if proc.returncode:
                 # Unable to create cgroup
@@ -249,12 +253,11 @@ def run(self):
                     'stderr': error_fd,
                     'check': True,
                     'universal_newlines': True,
-                    'shell': True
                 }
                 if subprocess_timeout is not None:
                     kwargs.update({'timeout': subprocess_timeout})
 
-                proc_out = run_subprocess(" ".join(command), **kwargs)
+                proc_out = run_subprocess(command, **kwargs)
 
                 stdout_response = proc_out.stdout
                 stderr_response = proc_out.stderr
@@ -391,8 +394,8 @@ def _delete(retries=15):
                 return True
             _delete()
         if self.resource_profiling:
-            cmd = 'cgdelete -g cpuacct,memory,pids:{}'.format(cgroup_path)
-            proc = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
+            cmd = ['cgdelete', '-g', f'cpuacct,memory,pids:{cgroup_path}']
+            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
             _, stderr = proc.communicate()
             if proc.returncode:
                 logger.error('Failed to delete cgroup: {}'.format(stderr))
@@ -532,8 +535,8 @@ def kill_container(self):
         container_name = self.config.container_name
         if container_name:
             container_cli = self.config.process_isolation_executable
-            cmd = '{} kill {}'.format(container_cli, container_name)
-            proc = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
+            cmd = [container_cli, 'kill', container_name]
+            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
             _, stderr = proc.communicate()
             if proc.returncode:
                 logger.info('Error from {} kill {} command:\n{}'.format(container_cli, container_name, stderr))

test/integration/test_interface.py

@@ -152,6 +152,30 @@ def test_run_command(project_fixtures):
     assert err == ''
 
 
+def test_run_command_injection_error():
+    out, err, rc = run_command(
+        executable_cmd='whoami',
+        cmdline_args=[';hostname'],
+        runner_mode='subprocess',
+    )
+    assert rc == 1
+    assert "usage: whoami" in err or "whoami: extra operand ‘;hostname’" in err
+
+
+@pytest.mark.test_all_runtimes
+def test_run_command_injection_error_within_container(runtime):
+    out, err, rc = run_command(
+        executable_cmd='whoami',
+        cmdline_args=[';hostname'],
+        runner_mode='subprocess',
+        process_isolation_executable=runtime,
+        process_isolation=True,
+        container_image=defaults.default_container_image,
+    )
+    assert rc == 1
+    assert "whoami: extra operand ';hostname'" in err
+
+
 @pytest.mark.test_all_runtimes
 def test_run_ansible_command_within_container(project_fixtures, runtime):
     private_data_dir = project_fixtures / 'debug'

test/unit/config/test_doc.py

@@ -52,7 +52,7 @@ def test_prepare_plugin_docs_command():
     plugin_names = ['copy', 'file']
     plugin_type = 'module'
     rc.prepare_plugin_docs_command(plugin_names, plugin_type=plugin_type, snippet=True, playbook_dir='/tmp/test')
-    expected_command = [get_executable_path('ansible-doc'), '-s', '-t', 'module', '--playbook-dir', '/tmp/test', 'copy file']
+    expected_command = [get_executable_path('ansible-doc'), '-s', '-t', 'module', '--playbook-dir', '/tmp/test', 'copy', 'file']
     assert rc.command == expected_command
     assert rc.runner_mode == 'subprocess'
     assert rc.execution_mode == BaseExecutionMode.ANSIBLE_COMMANDS
@@ -113,7 +113,7 @@ def test_prepare_plugin_docs_command_with_containerization(tmp_path, runtime, mo
         '-s',
         '-t', 'module',
         '--playbook-dir', '/tmp/test',
-        'copy '
+        'copy',
         'file',
     ])