diff --git a/ansible_runner/config/doc.py b/ansible_runner/config/doc.py index fe99ec9ac..6612baf3c 100644 --- a/ansible_runner/config/doc.py +++ b/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) diff --git a/ansible_runner/runner.py b/ansible_runner/runner.py index 44a45670a..1656e72fc 100644 --- a/ansible_runner/runner.py +++ b/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)) diff --git a/test/integration/test_interface.py b/test/integration/test_interface.py index 3a0eb236c..bd43d5b6a 100644 --- a/test/integration/test_interface.py +++ b/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' diff --git a/test/unit/config/test_doc.py b/test/unit/config/test_doc.py index 2b6edc7c3..1058dac19 100755 --- a/test/unit/config/test_doc.py +++ b/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', ])