CVE-2021-4041: Improper Encoding or Escaping of Output (CWE-116)
value: A flaw was found in ansible-runner. An improper escaping of the shell command, while calling the ansible_runner.interface.run_command, can lead to parameters getting executed as host's shell command. A developer could unintentionally write code that gets executed in the host rather than the virtual environment.
CWE description: The product prepares a structured message for communication with another component, but encoding or escaping of the data is either missing or done incorrectly. As a result, the intended structure of the message is not preserved.
CVSS v3 base score: 7.8
Published: 2022-08-24T16:15Z
Repository: https://github.com/ansible/ansible-runner
Language: Python
Files changed: ansible_runner/config/doc.py, ansible_runner/runner.py, test/integration/test_interface.py, test/unit/config/test_doc.py
Commit date: 2021-10-25 14:08:09 -0400
Commit message: Remove shell use in subprocess

Co-authored-by: Sam Doran <sdoran@redhat.com>

Diff: diff --git a/ansible_runner/config/doc.py b/ansible_runner/config/doc.py
index fe99ec9..6612baf 100644
--- a/ansible_runner/config/doc.py
+++ b/ansible_runner/config/doc.py
@@ -85,11 +85,11 @@ class DocConfig(BaseConfig):
             self.cmdline_args.extend(['--playbook-dir', playbook_dir])
 
         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)
 
     def prepare_plugin_list_command(self, list_files=None, response_format=None, plugin_type=None,
diff --git a/ansible_runner/runner.py b/ansible_runner/runner.py
index 44a4567..1656e72 100644
--- a/ansible_runner/runner.py
+++ b/ansible_runner/runner.py
@@ -202,12 +202,16 @@ class Runner(object):
             import getpass
             import grp
             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
                 logger.error('Unable to create cgroup: {}'.format(stderr))
                 raise RuntimeError('Unable to create cgroup: {}'.format(stderr))
@@ -247,16 +251,15 @@ class Runner(object):
                     'stdin': input_fd,
                     'stdout': output_fd,
                     '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
                 self.rc = proc_out.returncode
             except CalledProcessError as exc:
@@ -389,12 +392,12 @@ class Runner(object):
                     if not res:
                         raise
                 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))
                 raise RuntimeError('Failed to delete cgroup: {}'.format(stderr))
 
@@ -530,12 +533,12 @@ class Runner(object):
         Internal method to terminate a container being used for job isolation
         '''
         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))
             else:
                 logger.info("Killed container {}".format(container_name))
diff --git a/test/integration/test_interface.py b/test/integration/test_interface.py
index 3a0eb23..bd43d5b 100644
--- a/test/integration/test_interface.py
+++ b/test/integration/test_interface.py
@@ -150,10 +150,34 @@ def test_run_command(project_fixtures):
     assert "Hello world!" in out
     assert rc == 0
     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'
     inventory = private_data_dir / 'inventory' / 'inv_1'
     playbook = private_data_dir / 'project' / 'debug.yml'
diff --git a/test/unit/config/test_doc.py b/test/unit/config/test_doc.py
index 2b6edc7..1058dac 100755
--- a/test/unit/config/test_doc.py
+++ b/test/unit/config/test_doc.py
@@ -50,11 +50,11 @@ def test_invalid_plugin_name_value():
 def test_prepare_plugin_docs_command():
     rc = DocConfig()
     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
 
 
@@ -111,11 +111,11 @@ def test_prepare_plugin_docs_command_with_containerization(tmp_path, runtime, mo
         'my_container',
         'ansible-doc',
         '-s',
         '-t', 'module',
         '--playbook-dir', '/tmp/test',
-        'copy '
+        'copy',
         'file',
     ])
 
     assert expected_command_start == rc.command
 

Vulnerable code:         self.cmdline_args.append(" ".join(plugin_names))
            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)
                    'shell': True
                proc_out = run_subprocess(" ".join(command), **kwargs)
            cmd = 'cgdelete -g cpuacct,memory,pids:{}'.format(cgroup_path)
            proc = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
            cmd = '{} kill {}'.format(container_cli, container_name)
            proc = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
    expected_command = [get_executable_path('ansible-doc'), '-s', '-t', 'module', '--playbook-dir', '/tmp/test', 'copy file']
        'copy '
Fixed code:         self.cmdline_args.extend(plugin_names)
            cmd = ['cgcreate',
                   '-a', f'{user}:{group}',
                   '-t', f'{user}:{group}',
                   '-g', f'cpuacct,memory,pids:{cgroup_path}',
                   ]
            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
                proc_out = run_subprocess(command, **kwargs)
            cmd = ['cgdelete', '-g', f'cpuacct,memory,pids:{cgroup_path}']
            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
            cmd = [container_cli, 'kill', container_name]
            proc = Popen(cmd, stdout=PIPE, stderr=PIPE)
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


    expected_command = [get_executable_path('ansible-doc'), '-s', '-t', 'module', '--playbook-dir', '/tmp/test', 'copy', 'file']
        'copy',