From 8077d8e40148fe77e2393caa5f2b2ea855149d63 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 13 Apr 2020 17:16:29 -0400 Subject: [PATCH] avoid mkdir -p (#68921) * also consolidated temp dir name generation, added pid for more 'uniqness' * generalize error message * added notes about remote expansion CVE-2020-1733 fixes #67791 --- changelogs/fragments/remote_mkdir_fix.yml | 2 ++ lib/ansible/plugins/action/__init__.py | 16 ++++++++-------- lib/ansible/plugins/shell/__init__.py | 14 ++++++++++---- lib/ansible/plugins/shell/powershell.py | 2 ++ 4 files changed, 22 insertions(+), 12 deletions(-) create mode 100644 changelogs/fragments/remote_mkdir_fix.yml diff --git a/changelogs/fragments/remote_mkdir_fix.yml b/changelogs/fragments/remote_mkdir_fix.yml new file mode 100644 index 00000000000..0efdbb6660c --- /dev/null +++ b/changelogs/fragments/remote_mkdir_fix.yml @@ -0,0 +1,2 @@ +bugfixes: + - Ensure we get an error when creating a remote tmp if it already exists. CVE-2020-1733 diff --git a/lib/ansible/plugins/action/__init__.py b/lib/ansible/plugins/action/__init__.py index 40fdd670fcf..967c5b027cb 100644 --- a/lib/ansible/plugins/action/__init__.py +++ b/lib/ansible/plugins/action/__init__.py @@ -339,18 +339,18 @@ class ActionBase(with_metaclass(ABCMeta, object)): Create and return a temporary path on a remote box. ''' - become_unprivileged = self._is_become_unprivileged() - remote_tmp = self.get_shell_option('remote_tmp', default='~/.ansible/tmp') - - # deal with tmpdir creation - basefile = 'ansible-tmp-%s-%s' % (time.time(), random.randint(0, 2**48)) # Network connection plugins (network_cli, netconf, etc.) execute on the controller, rather than the remote host. # As such, we want to avoid using remote_user for paths as remote_user may not line up with the local user # This is a hack and should be solved by more intelligent handling of remote_tmp in 2.7 if getattr(self._connection, '_remote_is_local', False): tmpdir = C.DEFAULT_LOCAL_TMP else: - tmpdir = self._remote_expand_user(remote_tmp, sudoable=False) + # NOTE: shell plugins should populate this setting anyways, but they dont do remote expansion, which + # we need for 'non posix' systems like cloud-init and solaris + tmpdir = self._remote_expand_user(self.get_shell_option('remote_tmp', default='~/.ansible/tmp'), sudoable=False) + + become_unprivileged = self._is_become_unprivileged() + basefile = self._connection._shell._generate_temp_dir_name() cmd = self._connection._shell.mkdtemp(basefile=basefile, system=become_unprivileged, tmpdir=tmpdir) result = self._low_level_execute_command(cmd, sudoable=False) @@ -369,9 +369,9 @@ class ActionBase(with_metaclass(ABCMeta, object)): elif u'No space left on device' in result['stderr']: output = result['stderr'] else: - output = ('Authentication or permission failure. ' + output = ('Failed to create temporary directory.' 'In some cases, you may have been able to authenticate and did not have permissions on the target directory. ' - 'Consider changing the remote tmp path in ansible.cfg to a path rooted in "/tmp". ' + 'Consider changing the remote tmp path in ansible.cfg to a path rooted in "/tmp", for more error information use -vvv. ' 'Failed command was: %s, exited with result %d' % (cmd, result['rc'])) if 'stdout' in result and result['stdout'] != u'': output = output + u", stdout output: %s" % result['stdout'] diff --git a/lib/ansible/plugins/shell/__init__.py b/lib/ansible/plugins/shell/__init__.py index a81c3301282..4050d8227b5 100644 --- a/lib/ansible/plugins/shell/__init__.py +++ b/lib/ansible/plugins/shell/__init__.py @@ -23,7 +23,6 @@ import random import re import time -import ansible.constants as C from ansible.errors import AnsibleError from ansible.module_utils.six import text_type from ansible.module_utils.six.moves import shlex_quote @@ -76,6 +75,10 @@ class ShellBase(AnsiblePlugin): except KeyError: pass + @staticmethod + def _generate_temp_dir_name(): + 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()]) @@ -125,7 +128,7 @@ class ShellBase(AnsiblePlugin): def mkdtemp(self, basefile=None, system=False, mode=0o700, tmpdir=None): if not basefile: - basefile = 'ansible-tmp-%s-%s' % (time.time(), random.randint(0, 2**48)) + basefile = self.__class__._generate_temp_dir_name() # When system is specified we have to create this in a directory where # other users can read and access the tmp directory. @@ -137,7 +140,8 @@ class ShellBase(AnsiblePlugin): # passed in tmpdir if it is valid or the first one from the setting if not. if system: - tmpdir = tmpdir.rstrip('/') + if tmpdir: + tmpdir = tmpdir.rstrip('/') if tmpdir in self.get_option('system_tmpdirs'): basetmpdir = tmpdir @@ -151,7 +155,9 @@ class ShellBase(AnsiblePlugin): basetmp = self.join_path(basetmpdir, basefile) - cmd = 'mkdir -p %s echo %s %s' % (self._SHELL_SUB_LEFT, basetmp, self._SHELL_SUB_RIGHT) + # use mkdir -p to ensure parents exist, but mkdir fullpath to ensure last one is created by us + cmd = 'mkdir -p %s echo %s %s' % (self._SHELL_SUB_LEFT, basetmpdir, self._SHELL_SUB_RIGHT) + cmd += '%s mkdir %s' % (self._SHELL_AND, basetmp) cmd += ' %s echo %s=%s echo %s %s' % (self._SHELL_AND, basefile, self._SHELL_SUB_LEFT, basetmp, self._SHELL_SUB_RIGHT) # change the umask in a subshell to achieve the desired mode diff --git a/lib/ansible/plugins/shell/powershell.py b/lib/ansible/plugins/shell/powershell.py index ca2d5ebf5ba..d387bdab2fe 100644 --- a/lib/ansible/plugins/shell/powershell.py +++ b/lib/ansible/plugins/shell/powershell.py @@ -135,6 +135,8 @@ class ShellModule(ShellBase): def mkdtemp(self, basefile=None, system=False, mode=None, tmpdir=None): # Windows does not have an equivalent for the system temp files, so # the param is ignored + if not basefile: + basefile = self.__class__._generate_temp_dir_name() basefile = self._escape(self._unquote(basefile)) basetmpdir = tmpdir if tmpdir else self.get_option('remote_tmp')