avoid mkdir -p (#68921) (#68927)

* 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

(cherry picked from commit 8077d8e)

* C

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

lib/ansible/plugins/action/__init__.py

@@ -326,18 +326,18 @@ def _make_tmp_path(self, remote_user=None):
         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)
 
@@ -356,9 +356,9 @@ def _make_tmp_path(self, remote_user=None):
             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']

lib/ansible/plugins/shell/__init__.py

@@ -23,7 +23,7 @@
 import re
 import time
 
-import ansible.constants as C
+from ansible import 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
@@ -82,6 +82,10 @@ def set_options(self, task_keys=None, var_options=None, direct=None):
         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()])
 
@@ -131,7 +135,7 @@ def exists(self, path):
 
     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.
@@ -143,7 +147,8 @@ def mkdtemp(self, basefile=None, system=False, mode=0o700, tmpdir=None):
         # 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
@@ -157,7 +162,9 @@ def mkdtemp(self, basefile=None, system=False, mode=0o700, tmpdir=None):
 
         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

lib/ansible/plugins/shell/powershell.py

@@ -135,6 +135,8 @@ def remove(self, path, recurse=False):
     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')