From a47a6ebc62b9773eeb73bb62be6fd39322ffc28c Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Mon, 10 Jun 2024 09:19:57 -0500 Subject: [PATCH] [stable-2.17] Properly quote all needed components of shell commands (#83365) (#83396) * [stable-2.17] Properly quote all needed components of shell commands (#83365) * Properly quote all needed components of shell commands * Use self.quote, add new self.join (cherry picked from commit 93b8b86) Co-authored-by: Matt Martz * remove new function --- .../fragments/82535-properly-quote-shell.yml | 2 + lib/ansible/plugins/shell/__init__.py | 51 ++++++++---------- lib/ansible/plugins/shell/sh.py | 4 +- .../targets/json_cleanup/library/bad_json | 2 +- .../json_cleanup/module_output_cleaning.yml | 7 +++ test/integration/targets/shell/meta/main.yml | 3 ++ .../targets/shell/tasks/command-building.yml | 53 +++++++++++++++++++ test/integration/targets/shell/tasks/main.yml | 2 + .../shell/test-command-building-playbook.yml | 4 ++ 9 files changed, 94 insertions(+), 34 deletions(-) create mode 100644 changelogs/fragments/82535-properly-quote-shell.yml create mode 100644 test/integration/targets/shell/meta/main.yml create mode 100644 test/integration/targets/shell/tasks/command-building.yml create mode 100644 test/integration/targets/shell/test-command-building-playbook.yml diff --git a/changelogs/fragments/82535-properly-quote-shell.yml b/changelogs/fragments/82535-properly-quote-shell.yml new file mode 100644 index 00000000000..be93f30c59d --- /dev/null +++ b/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) diff --git a/lib/ansible/plugins/shell/__init__.py b/lib/ansible/plugins/shell/__init__.py index 5aa0a1b89ec..56be533cc4d 100644 --- a/lib/ansible/plugins/shell/__init__.py +++ b/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 shlex.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 shlex.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 shlex.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 shlex.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' % shlex.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""" diff --git a/lib/ansible/plugins/shell/sh.py b/lib/ansible/plugins/shell/sh.py index e0412b762de..ef85596c267 100644 --- a/lib/ansible/plugins/shell/sh.py +++ b/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) diff --git a/test/integration/targets/json_cleanup/library/bad_json b/test/integration/targets/json_cleanup/library/bad_json index 1df8c725f02..e84de04ec18 100644 --- a/test/integration/targets/json_cleanup/library/bad_json +++ b/test/integration/targets/json_cleanup/library/bad_json @@ -1,4 +1,4 @@ -#!/usr/bin/env bash +#!/bin/bash set -eu diff --git a/test/integration/targets/json_cleanup/module_output_cleaning.yml b/test/integration/targets/json_cleanup/module_output_cleaning.yml index 165352aab89..94ae9c1d9ab 100644 --- a/test/integration/targets/json_cleanup/module_output_cleaning.yml +++ b/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: diff --git a/test/integration/targets/shell/meta/main.yml b/test/integration/targets/shell/meta/main.yml new file mode 100644 index 00000000000..cb6005d042c --- /dev/null +++ b/test/integration/targets/shell/meta/main.yml @@ -0,0 +1,3 @@ +dependencies: + - prepare_tests + - setup_remote_tmp_dir diff --git a/test/integration/targets/shell/tasks/command-building.yml b/test/integration/targets/shell/tasks/command-building.yml new file mode 100644 index 00000000000..bd452618b52 --- /dev/null +++ b/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- diff --git a/test/integration/targets/shell/tasks/main.yml b/test/integration/targets/shell/tasks/main.yml index d6f2a2b59a0..7a442f43d86 100644 --- a/test/integration/targets/shell/tasks/main.yml +++ b/test/integration/targets/shell/tasks/main.yml @@ -34,3 +34,5 @@ with_items: - powershell - sh + +- import_tasks: command-building.yml diff --git a/test/integration/targets/shell/test-command-building-playbook.yml b/test/integration/targets/shell/test-command-building-playbook.yml new file mode 100644 index 00000000000..f77cf4ceb8c --- /dev/null +++ b/test/integration/targets/shell/test-command-building-playbook.yml @@ -0,0 +1,4 @@ +- hosts: testhost + gather_facts: false + tasks: + - ping: