Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix cmd.run on MacOS -- wrong environment variables #54079

Closed
wants to merge 4 commits into from
Closed

Fix cmd.run on MacOS -- wrong environment variables #54079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb4ce13
Fix environment in cmd.run runas on MacOS
ScoreUnder Jul 31, 2019
92c1801
Add integration tests for cwd/runas changes on MacOS
ScoreUnder Jul 31, 2019
3cfaa35
Fixes to ITs
ScoreUnder Aug 1, 2019
cbdb8b0
Add integration test justifying strange use of braces after cd in cmd…
ScoreUnder Aug 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions salt/modules/cmdmod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, great work!

I have a question related with this line. What does happen if my default shell is zsh for example and all my settings are provided in the ~/.zshrc file instead of the ~/.bash_profile one?

Will this change load my PATH or any other exports?

This is an important point to take into account since starting with macOS Catalina, the default shell for new users will be zsh. More info at: https://support.apple.com/en-us/HT208050

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example, when the following command is executed:

sudo salt-call --local cmd.run cmd='env' runas='Carlos'

this is the result with line cmd = '/bin/bash -l -c {cmd}'.format(cmd=_cmd_quote(cmd)) uncommented

local:
    /usr/local/share/bash-completion/bash_completion: line 1893: declare: -A: invalid option
    declare: usage: declare [-afFirtx] [-p] [name[=value] ...]
    /usr/local/share/bash-completion/bash_completion: line 2057: complete: -D: invalid option
    complete: usage: complete [-abcdefgjksuv] [-pr] [-o option] [-A action] [-G globpat] [-W wordlist] [-P prefix] [-S suffix] [-X filterpat] [-F function] [-C command] [name ...]
    ls: /Users/Carlos/.bashrc.d/*: No such file or directory
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    LESS_TERMCAP_md=
    TERM=xterm-256color                                                                                                                                                                                     SHELL=/usr/local/bin/zsh
    HISTSIZE=32768
    DISABLE_FZF_KEY_BINDINGS=false
    PROMPT_DIRTRIM=3
    LC_ALL=en_US.UTF-8
    HISTFILESIZE=32768
    USER=Carlos
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    PWD=/tmp
    EDITOR=/usr/local/bin/mvim --remote-wait-silent
    LANG=en_US.UTF-8
    HISTCONTROL=ignoreboth
    GPG_TTY=/dev/ttys002
    SHLVL=1
    HOME=/Users/Carlos
    LOGNAME=Carlos
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    OLDPWD=/Users/Carlos
    _=/usr/bin/env

and this is the result with the same line commented:

local:
    USER=Carlos
    HOME=/Users/Carlos
    SHELL=/usr/local/bin/zsh
    PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/Carlos/bin:/usr/local/sbin
    TERM=xterm-256color
    LOGNAME=Carlos
    SHLVL=0
    PWD=/tmp
    OLDPWD=/Users/Carlos
    LANG=en_US.UTF-8
    LC_ALL=en_US.UTF-8
    MANPATH=/usr/share/man:/usr/local/share/man:/usr/local/man
    GPG_TTY=/dev/ttys002
    FZF_DEFAULT_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    FZF_DEFAULT_OPTS=--ansi --height 100% --layout=reverse --border --inline-info
        --preview="bat --style=numbers --color=always {} 2> /dev/null"
        --bind ctrl-j:preview-page-down --bind ctrl-k:preview-page-up
        --bind ctrl-p:toggle-preview
    FZF_CTRL_T_COMMAND=fd --type f --hidden --follow --color always --exclude .git
    DISABLE_FZF_AUTO_COMPLETION=false
    DISABLE_FZF_KEY_BINDINGS=false
    _=/usr/bin/env

As you can see, with that line commented zsh stills getting the right environment, so a more specific solution is required for bash shells instead of making it the default solution.


# 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd = [ 'su', '-l', runas, '-c', 'cd '{0}' && {1}'.format(_cmd_quote(cwd), cmd) ]
When past as a list a shell is not invoke to before running the su (no security issue). When past as a string a shell is invoke to call the su and its arguments which means $abc is expanded before calling su and at the high privileged.(security issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command is coerced to a string before executing, so either case is the same. I have covered this specific exploit in the integration test test_avoid_injecting_shell_code_as_root (part of this PR) which passes on the machines I have tested on. Do you know of an input which could bypass the current quoting mechanisms?


# 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:
Expand Down Expand Up @@ -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:
Expand Down
106 changes: 93 additions & 13 deletions tests/integration/modules/test_cmdmod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
'''
Expand Down Expand Up @@ -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"'

Expand All @@ -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')
Expand Down