Fix cmd.run on MacOS (rebased)

salt/modules/cmdmod.py

@@ -407,16 +407,27 @@ def _get_stripped(cmd):
         return win_runas(cmd, runas, password, cwd)
 
     if runas and salt.utils.platform.is_darwin():
-        # we need to insert the user simulation into the command itself and not
-        # just run it from the environment on macOS as that
-        # method doesn't work properly when run as root for certain commands.
+        # We need to insert the user simulation into the command itself and not
+        # just run it from the environment on macOS as that method doesn't work
+        # properly when run as root for certain commands.
         if isinstance(cmd, (list, tuple)):
             cmd = ' '.join(map(_cmd_quote, cmd))
 
-        cmd = 'su -l {0} -c "{1}"'.format(runas, cmd)
-        # set runas to None, because if you try to run `su -l` as well as
-        # simulate the environment macOS will prompt for the password of the
-        # user and will cause salt to hang.
+        # Ensure directory is correct before running command
+        cmd = 'cd -- {dir} && {{ {cmd}\n }}'.format(dir=_cmd_quote(cwd), cmd=cmd)
+
+        # Ensure environment is correct for a newly logged-in user by running
+        # the command under bash as a login shell
+        cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd))
+
+        # Ensure the login is simulated correctly (note: su runs sh, not bash,
+        # which causes the environment to be initialised incorrectly, which is
+        # fixed by the previous line of code)
+        cmd = 'su -l {0} -c {1}'.format(_cmd_quote(runas), _cmd_quote(cmd))
+
+        # Set runas to None, because if you try to run `su -l` after changing
+        # user, su will prompt for the password of the user and cause salt to
+        # hang.
         runas = None
 
     if runas:
@@ -459,7 +470,7 @@ def _get_stripped(cmd):
                 'sys.stdout.write(\"' + marker + '\");'
             )
 
-            if use_sudo or __grains__['os'] in ['MacOS', 'Darwin']:
+            if use_sudo:
                 env_cmd = ['sudo']
                 # runas is optional if use_sudo is set.
                 if runas:

tests/integration/modules/test_cmdmod.py

@@ -2,6 +2,7 @@
 
 # Import python libs
 from __future__ import absolute_import, print_function, unicode_literals
+from contextlib import contextmanager
 import os
 import sys
 import tempfile
@@ -43,11 +44,18 @@ def setUp(self):
         if salt.utils.platform.is_darwin():
             self.runas_usr = 'macsalttest'
 
-    def tearDown(self):
-        if self._testMethodName == 'test_runas':
-            if salt.utils.platform.is_darwin():
-                if self.runas_usr in self.run_function('user.info', [self.runas_usr]).values():
-                    self.run_function('user.delete', [self.runas_usr], remove=True)
+    @contextmanager
+    def _ensure_user_exists(self, name):
+        if name in self.run_function('user.info', [name]).values():
+            # User already exists; don't touch
+            yield
+        else:
+            # Need to create user for test
+            self.run_function('user.add', [name])
+            try:
+                yield
+            finally:
+                self.run_function('user.delete', [name], remove=True)
 
     def test_run(self):
         '''
@@ -287,14 +295,12 @@ def test_quotes(self):
         self.assertEqual(result, expected_result)
 
     @skip_if_not_root
-    @skipIf(salt.utils.platform.is_windows, 'skip windows, requires password')
+    @skipIf(salt.utils.platform.is_windows(), 'skip windows, requires password')
     def test_quotes_runas(self):
         '''
         cmd.run with quoted command
         '''
         cmd = '''echo 'SELECT * FROM foo WHERE bar="baz"' '''
-        if salt.utils.platform.is_darwin():
-            cmd = '''echo 'SELECT * FROM foo WHERE bar=\\"baz\\"' '''
 
         expected_result = 'SELECT * FROM foo WHERE bar="baz"'
 
@@ -304,18 +310,92 @@ def test_quotes_runas(self):
                                    runas=runas).strip()
         self.assertEqual(result, expected_result)
 
+    @destructiveTest
+    @skip_if_not_root
+    @skipIf(salt.utils.platform.is_windows(), 'skip windows, uses unix commands')
+    def test_avoid_injecting_shell_code_as_root(self):
+        '''
+        cmd.run should execute the whole command as the "runas" user, not
+        running substitutions as root.
+        '''
+        cmd = 'echo $(id -u)'
+
+        root_id = self.run_function('cmd.run_stdout', [cmd])
+        runas_root_id = self.run_function('cmd.run_stdout', [cmd], runas=this_user())
+        with self._ensure_user_exists(self.runas_usr):
+            user_id = self.run_function('cmd.run_stdout', [cmd], runas=self.runas_usr)
+
+        self.assertNotEqual(user_id, root_id)
+        self.assertNotEqual(user_id, runas_root_id)
+        self.assertEqual(root_id, runas_root_id)
+
+    @destructiveTest
+    @skip_if_not_root
+    @skipIf(salt.utils.platform.is_windows(), 'skip windows, uses unix commands')
+    def test_cwd_runas(self):
+        '''
+        cmd.run should be able to change working directory correctly, whether
+        or not runas is in use.
+        '''
+        cmd = 'pwd'
+        tmp_cwd = tempfile.mkdtemp(dir=TMP)
+        os.chmod(tmp_cwd, 0o711)
+
+        cwd_normal = self.run_function('cmd.run_stdout', [cmd], cwd=tmp_cwd).rstrip('\n')
+        self.assertEqual(tmp_cwd, cwd_normal)
+
+        with self._ensure_user_exists(self.runas_usr):
+            cwd_runas = self.run_function('cmd.run_stdout', [cmd], cwd=tmp_cwd, runas=self.runas_usr).rstrip('\n')
+        self.assertEqual(tmp_cwd, cwd_runas)
+
+    @destructiveTest
+    @skip_if_not_root
+    @skipIf(not salt.utils.platform.is_darwin(), 'applicable to MacOS only')
+    def test_runas_env(self):
+        '''
+        cmd.run should be able to change working directory correctly, whether
+        or not runas is in use.
+        '''
+        with self._ensure_user_exists(self.runas_usr):
+            user_path = self.run_function('cmd.run_stdout', ['printf %s "$PATH"'], runas=self.runas_usr)
+        # XXX: Not sure of a better way. Environment starts out with
+        # /bin:/usr/bin and should be populated by path helper and the bash
+        # profile.
+        self.assertNotEqual("/bin:/usr/bin", user_path)
+
+    @destructiveTest
+    @skip_if_not_root
+    @skipIf(not salt.utils.platform.is_darwin(), 'applicable to MacOS only')
+    def test_runas_complex_command_bad_cwd(self):
+        '''
+        cmd.run should not accidentally run parts of a complex command when
+        given a cwd which cannot be used by the user the command is run as.
+
+        Due to the need to use `su -l` to login to another user on MacOS, we
+        cannot cd into directories that the target user themselves does not
+        have execute permission for. To an extent, this test is testing that
+        buggy behaviour, but its purpose is to ensure that the greater bug of
+        running commands after failing to cd does not occur.
+        '''
+        tmp_cwd = tempfile.mkdtemp(dir=TMP)
+        os.chmod(tmp_cwd, 0o700)
+
+        with self._ensure_user_exists(self.runas_usr):
+            cmd_result = self.run_function('cmd.run_all', ['pwd; pwd; : $(echo "You have failed the test" >&2)'], cwd=tmp_cwd, runas=self.runas_usr)
+
+        self.assertEqual("", cmd_result['stdout'])
+        self.assertNotIn("You have failed the test", cmd_result['stderr'])
+        self.assertNotEqual(0, cmd_result['retcode'])
+
     @skipIf(salt.utils.platform.is_windows(), 'minion is windows')
     @skip_if_not_root
     @destructiveTest
     def test_runas(self):
         '''
         Ensure that the env is the runas user's
         '''
-        if salt.utils.platform.is_darwin():
-            if self.runas_usr not in self.run_function('user.info', [self.runas_usr]).values():
-                self.run_function('user.add', [self.runas_usr])
-
-        out = self.run_function('cmd.run', ['env'], runas=self.runas_usr).splitlines()
+        with self._ensure_user_exists(self.runas_usr):
+            out = self.run_function('cmd.run', ['env'], runas=self.runas_usr).splitlines()
         self.assertIn('USER={0}'.format(self.runas_usr), out)
 
     @skipIf(not salt.utils.path.which_bin('sleep'), 'sleep cmd not installed')