Properly quote all needed components of shell commands (#83365)

* Properly quote all needed components of shell commands

* Use self.quote, add new self.join

changelogs/fragments/82535-properly-quote-shell.yml

@@ -0,0 +1,2 @@
+bugfixes:
+- shell plugin - properly quote all needed components of shell commands (https://github.com/ansible/ansible/issues/82535)

lib/ansible/plugins/shell/__init__.py

@@ -85,7 +85,7 @@ class ShellBase(AnsiblePlugin):
         return 'ansible-tmp-%s-%s-%s' % (time.time(), os.getpid(), random.randint(0, 2**48))
 
     def env_prefix(self, **kwargs):
-        return ' '.join(['%s=%s' % (k, shlex.quote(text_type(v))) for k, v in kwargs.items()])
+        return ' '.join(['%s=%s' % (k, self.quote(text_type(v))) for k, v in kwargs.items()])
 
     def join_path(self, *args):
         return os.path.join(*args)
@@ -101,41 +101,33 @@ class ShellBase(AnsiblePlugin):
     def chmod(self, paths, mode):
         cmd = ['chmod', mode]
         cmd.extend(paths)
-        cmd = [shlex.quote(c) for c in cmd]
-
-        return ' '.join(cmd)
+        return self.join(cmd)
 
     def chown(self, paths, user):
         cmd = ['chown', user]
         cmd.extend(paths)
-        cmd = [shlex.quote(c) for c in cmd]
-
-        return ' '.join(cmd)
+        return self.join(cmd)
 
     def chgrp(self, paths, group):
         cmd = ['chgrp', group]
         cmd.extend(paths)
-        cmd = [shlex.quote(c) for c in cmd]
-
-        return ' '.join(cmd)
+        return self.join(cmd)
 
     def set_user_facl(self, paths, user, mode):
         """Only sets acls for users as that's really all we need"""
         cmd = ['setfacl', '-m', 'u:%s:%s' % (user, mode)]
         cmd.extend(paths)
-        cmd = [shlex.quote(c) for c in cmd]
-
-        return ' '.join(cmd)
+        return self.join(cmd)
 
     def remove(self, path, recurse=False):
-        path = shlex.quote(path)
+        path = self.quote(path)
         cmd = 'rm -f '
         if recurse:
             cmd += '-r '
         return cmd + "%s %s" % (path, self._SHELL_REDIRECT_ALLNULL)
 
     def exists(self, path):
-        cmd = ['test', '-e', shlex.quote(path)]
+        cmd = ['test', '-e', self.quote(path)]
         return ' '.join(cmd)
 
     def mkdtemp(self, basefile=None, system=False, mode=0o700, tmpdir=None):
@@ -194,8 +186,7 @@ class ShellBase(AnsiblePlugin):
         # Check that the user_path to expand is safe
         if user_home_path != '~':
             if not _USER_HOME_PATH_RE.match(user_home_path):
-                # shlex.quote will make the shell return the string verbatim
-                user_home_path = shlex.quote(user_home_path)
+                user_home_path = self.quote(user_home_path)
         elif username:
             # if present the user name is appended to resolve "that user's home"
             user_home_path += username
@@ -207,20 +198,20 @@ class ShellBase(AnsiblePlugin):
         return 'echo %spwd%s' % (self._SHELL_SUB_LEFT, self._SHELL_SUB_RIGHT)
 
     def build_module_command(self, env_string, shebang, cmd, arg_path=None):
-        # don't quote the cmd if it's an empty string, because this will break pipelining mode
-        if cmd.strip() != '':
-            cmd = shlex.quote(cmd)
+        env_string = env_string.strip()
+        if env_string:
+            env_string += ' '
 
-        cmd_parts = []
-        if shebang:
-            shebang = shebang.replace("#!", "").strip()
-        else:
-            shebang = ""
-        cmd_parts.extend([env_string.strip(), shebang, cmd])
-        if arg_path is not None:
-            cmd_parts.append(arg_path)
-        new_cmd = " ".join(cmd_parts)
-        return new_cmd
+        if shebang is None:
+            shebang = ''
+
+        cmd_parts = [
+            shebang.removeprefix('#!').strip(),
+            cmd.strip(),
+            arg_path,
+        ]
+
+        return f'{env_string}%s' % self.join(cps for cp in cmd_parts if cp and (cps := cp.strip()))
 
     def append_command(self, cmd, cmd_to_append):
         """Append an additional command if supported by the shell"""
@@ -237,3 +228,7 @@ class ShellBase(AnsiblePlugin):
     def quote(self, cmd):
         """Returns a shell-escaped string that can be safely used as one token in a shell command line"""
         return shlex.quote(cmd)
+
+    def join(self, cmd_parts):
+        """Returns a shell-escaped string from a list that can be safely used in a shell command line"""
+        return shlex.join(cmd_parts)

lib/ansible/plugins/shell/sh.py

@@ -13,8 +13,6 @@ extends_documentation_fragment:
   - shell_common
 '''
 
-import shlex
-
 from ansible.plugins.shell import ShellBase
 
 
@@ -66,7 +64,7 @@ class ShellModule(ShellBase):
         # Quoting gets complex here.  We're writing a python string that's
         # used by a variety of shells on the remote host to invoke a python
         # "one-liner".
-        shell_escaped_path = shlex.quote(path)
+        shell_escaped_path = self.quote(path)
         test = "rc=flag; [ -r %(p)s ] %(shell_or)s rc=2; [ -f %(p)s ] %(shell_or)s rc=1; [ -d %(p)s ] %(shell_and)s rc=3; %(i)s -V 2>/dev/null %(shell_or)s rc=4; [ x\"$rc\" != \"xflag\" ] %(shell_and)s echo \"${rc}  \"%(p)s %(shell_and)s exit 0" % dict(p=shell_escaped_path, i=python_interp, shell_and=self._SHELL_AND, shell_or=self._SHELL_OR)  # NOQA
         csums = [
             u"({0} -c 'import hashlib; BLOCKSIZE = 65536; hasher = hashlib.sha1();{2}afile = open(\"'{1}'\", \"rb\"){2}buf = afile.read(BLOCKSIZE){2}while len(buf) > 0:{2}\thasher.update(buf){2}\tbuf = afile.read(BLOCKSIZE){2}afile.close(){2}print(hasher.hexdigest())' 2>/dev/null)".format(python_interp, shell_escaped_path, self._SHELL_EMBEDDED_PY_EOL),  # NOQA  Python > 2.4 (including python3)

test/integration/targets/json_cleanup/library/bad_json

@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/bash
 
 set -eu
 

test/integration/targets/json_cleanup/module_output_cleaning.yml

@@ -2,10 +2,17 @@
   hosts: localhost
   gather_facts: false
   tasks:
+    - name: where is bash
+      shell: command -v bash
+      register: bash
+      changed_when: false
+
     - name: call module that spews extra stuff
       bad_json:
       register: clean_json
       ignore_errors: true
+      vars:
+        ansible_bash_interpreter: '{{ bash.stdout }}'
 
     - name: all expected is there
       assert:

test/integration/targets/shell/meta/main.yml

@@ -0,0 +1,3 @@
+dependencies:
+  - prepare_tests
+  - setup_remote_tmp_dir

test/integration/targets/shell/tasks/command-building.yml

@@ -0,0 +1,53 @@
+- vars:
+    atd: '{{ remote_tmp_dir }}/shell/t m p'
+    api: '{{ remote_tmp_dir }}/shell/p y t h o n'
+  block:
+    - name: create test dir
+      file:
+        path: '{{ atd|dirname }}'
+        state: directory
+
+    - name: create tempdir with spaces
+      file:
+        path: '{{ atd }}'
+        state: directory
+
+    - name: create symlink for ansible_python_interpreter to file with spaces
+      file:
+        dest: '{{ api }}'
+        src: '{{ ansible_facts.python.executable }}'
+        state: link
+
+    - name: run simple test playbook
+      command: >-
+        ansible-playbook -vvv -i inventory
+        -e 'ansible_python_interpreter="{{ api }}"'
+        -e 'ansible_pipelining=0'
+        "{{ role_path }}/test-command-building-playbook.yml"
+      environment:
+        ANSIBLE_REMOTE_TMP: '{{ atd }}'
+        ANSIBLE_NOCOLOR: "1"
+        ANSIBLE_FORCE_COLOR: "0"
+      register: command_building
+      delegate_to: localhost
+
+    - debug:
+        var: command_building.stdout_lines
+
+    - block:
+        - debug:
+            var: py_cmd
+
+        - debug:
+            var: tmp_dir
+
+        - assert:
+            that:
+              - py_cmd in exec_line
+              - tmp_dir in exec_line
+      vars:
+        exec_line: '{{ command_building.stdout_lines | select("search", "EXEC.*p y t h o n") | first }}'
+        py_cmd: >-
+          '"'"'{{ api }}'"'"'
+        tmp_dir: >-
+          '"'"'{{ atd }}/ansible-tmp-

test/integration/targets/shell/tasks/main.yml

@@ -34,3 +34,5 @@
   with_items:
   - powershell
   - sh
+
+- import_tasks: command-building.yml

test/integration/targets/shell/test-command-building-playbook.yml

@@ -0,0 +1,4 @@
+- hosts: testhost
+  gather_facts: false
+  tasks:
+    - ping: