From cb4ce13c1af20c28e7540ebfc0e6501cb509fba8 Mon Sep 17 00:00:00 2001 From: Score_Under Date: Wed, 31 Jul 2019 16:11:28 +0100 Subject: [PATCH 1/4] Fix environment in cmd.run runas on MacOS --- salt/modules/cmdmod.py | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index a733e396ce57..723eddc67b04 100644 --- a/salt/modules/cmdmod.py +++ b/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: From 92c1801942c0b6060ddcc45fc81f096c40688ae7 Mon Sep 17 00:00:00 2001 From: Score_Under Date: Wed, 31 Jul 2019 16:53:29 +0100 Subject: [PATCH 2/4] Add integration tests for cwd/runas changes on MacOS Also fix a unit test that was testing for broken behaviour. --- tests/integration/modules/test_cmdmod.py | 50 ++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/tests/integration/modules/test_cmdmod.py b/tests/integration/modules/test_cmdmod.py index ef96f5843064..457cfdce0995 100644 --- a/tests/integration/modules/test_cmdmod.py +++ b/tests/integration/modules/test_cmdmod.py @@ -287,14 +287,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,6 +302,52 @@ def test_quotes_runas(self): runas=runas).strip() self.assertEqual(result, expected_result) + @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()) + 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) + + @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) + + cwd_normal = self.run_function('cmd.run_stdout', [cmd], cwd=tmp_cwd).rstrip('\n') + self.assertEqual(tmp_cwd, cwd_normal) + + cwd_runas = self.run_function('cmd.run_stdout', [cmd], cwd=tmp_cwd, runas=self.runas_usr).rstrip('\n') + self.assertEqual(tmp_cwd, cwd_runas) + + @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. + ''' + 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) + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') @skip_if_not_root @destructiveTest From 3cfaa354049da1b26ac8a49bc63e1446f20e698d Mon Sep 17 00:00:00 2001 From: Score_Under Date: Thu, 1 Aug 2019 11:07:51 +0100 Subject: [PATCH 3/4] Fixes to ITs - ensure runas user is created when used - mark runas tests destructive due to user creation/deletion - fix tmpdir permission issue when cd'ing as runas user --- tests/integration/modules/test_cmdmod.py | 38 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/integration/modules/test_cmdmod.py b/tests/integration/modules/test_cmdmod.py index 457cfdce0995..ef76bd6f0922 100644 --- a/tests/integration/modules/test_cmdmod.py +++ b/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): ''' @@ -302,6 +310,7 @@ 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): @@ -313,12 +322,14 @@ def test_avoid_injecting_shell_code_as_root(self): root_id = self.run_function('cmd.run_stdout', [cmd]) runas_root_id = self.run_function('cmd.run_stdout', [cmd], runas=this_user()) - user_id = self.run_function('cmd.run_stdout', [cmd], runas=self.runas_usr) + 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): @@ -328,13 +339,16 @@ def test_cwd_runas(self): ''' 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) - cwd_runas = self.run_function('cmd.run_stdout', [cmd], cwd=tmp_cwd, runas=self.runas_usr).rstrip('\n') + 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): @@ -342,7 +356,8 @@ def test_runas_env(self): cmd.run should be able to change working directory correctly, whether or not runas is in use. ''' - user_path = self.run_function('cmd.run_stdout', ['printf %s "$PATH"'], runas=self.runas_usr) + 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. @@ -355,11 +370,8 @@ 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') From cbdb8b0cf5ac30a5185bfb8b2998a73eba0d44ab Mon Sep 17 00:00:00 2001 From: Score_Under Date: Thu, 1 Aug 2019 11:25:35 +0100 Subject: [PATCH 4/4] Add integration test justifying strange use of braces after cd in cmd.run --- tests/integration/modules/test_cmdmod.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/integration/modules/test_cmdmod.py b/tests/integration/modules/test_cmdmod.py index ef76bd6f0922..0f3a9f8c0d95 100644 --- a/tests/integration/modules/test_cmdmod.py +++ b/tests/integration/modules/test_cmdmod.py @@ -363,6 +363,30 @@ def test_runas_env(self): # 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