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

* 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 8077d8e40148fe77e2393caa5f2b2ea855149d63)

* C

* Update lib/ansible/plugins/shell/__init__.py

Co-Authored-By: Abhijeet Kasurde <akasurde@redhat.com>

* adjusted for missing api

Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>

4 files changed, 26 insertions, 11 deletions

diff --git a/changelogs/fragments/remote_mkdir_fix.yml b/changelogs/fragments/remote_mkdir_fix.yml
new file mode 100644
index 0000000000..0efdbb6660
--- /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 6684542468..5c74ca9015 100644
--- a/lib/ansible/plugins/action/__init__.py
+++ b/lib/ansible/plugins/action/__init__.py
@@ -276,10 +276,6 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         '''
 
         become_unprivileged = self._is_become_unprivileged()
-        try:
-            remote_tmp = self._connection._shell.get_option('remote_tmp')
-        except AnsibleError:
-            remote_tmp = '~/.ansible/tmp'
 
         # deal with tmpdir creation
         basefile = 'ansible-tmp-%s-%s' % (time.time(), random.randint(0, 2**48))
@@ -289,7 +285,15 @@ class ActionBase(with_metaclass(ABCMeta, object)):
         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
+            try:
+                tmpdir = self._connection._shell.get_option('remote_tmp')
+            except AnsibleError:
+                tmpdir = '~/.ansible/tmp'
+            tmpdir = self._remote_expand_user(tmpdir, sudoable=False)
+
+        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)
 
@@ -308,9 +312,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 0c035b1c54..a0f4053578 100644
--- a/lib/ansible/plugins/shell/__init__.py
+++ b/lib/ansible/plugins/shell/__init__.py
@@ -23,7 +23,7 @@ import random
 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
@@ -76,6 +76,10 @@ class ShellBase(AnsiblePlugin):
         except AnsibleError:
             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 +129,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 +141,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 +156,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 7bbf5148ac..af86b74013 100644
--- a/lib/ansible/plugins/shell/powershell.py
+++ b/lib/ansible/plugins/shell/powershell.py
@@ -1574,6 +1574,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')