Don't create world-readable module and tempfiles without explicit user permission

This commit is contained in:
Toshio Kuratomi 2016-03-21 14:17:53 -07:00
parent 0cabef19ad
commit 52e9209491
14 changed files with 217 additions and 78 deletions

View file

@ -85,25 +85,33 @@ on how it works. Users should be aware of these to avoid surprises.
Becoming an Unprivileged User
=============================
Ansible has a limitation with regards to becoming an
Ansible 2.0.x and below has a limitation with regards to becoming an
unprivileged user that can be a security risk if users are not aware of it.
Ansible modules are executed on the remote machine by first substituting the
parameters into the module file, then copying the file to the remote machine,
and finally executing it there. If the module file is executed without using
become, when the become user is root, or when the connection to the remote
machine is made as root then the module file is created with permissions that
only allow reading by the user and root.
and finally executing it there.
If the become user is an unprivileged user and then Ansible has no choice but
to make the module file world readable as there's no other way for the user
Ansible connects as to save the file so that the user that we're becoming can
read it.
Everything is fine if the module file is executed without using ``become``,
when the ``become_user`` is root, or when the connection to the remote machine
is made as root. In these cases the module file is created with permissions
that only allow reading by the user and root.
The problem occurs when the ``become_user`` is an unprivileged user. Ansible
2.0.x and below make the module file world readable in this case as the module
file is written as the user that Ansible connects as but the file needs to
be reasable by the user Ansible is set to ``become``.
.. note:: In Ansible 2.1, this window is further narrowed: If the connection
is made as a privileged user (root) then Ansible 2.1 and above will use
chown to set the file's owner to the unprivileged user being switched to.
This means both the user making the connection and the user being switched
to via ``become`` must be unprivileged in order to trigger this problem.
If any of the parameters passed to the module are sensitive in nature then
those pieces of data are readable by reading the module file for the duration
of the Ansible module execution. Once the module is done executing Ansible
will delete the temporary file. If you trust the client machines then there's
no problem here. If you do not trust the client machines then this is
those pieces of data are located in a world readable module file for the
duration of the Ansible module execution. Once the module is done executing
Ansible will delete the temporary file. If you trust the client machines then
there's no problem here. If you do not trust the client machines then this is
a potential danger.
Ways to resolve this include:
@ -113,9 +121,32 @@ Ways to resolve this include:
the remote python interpreter's stdin. Pipelining does not work for
non-python modules.
* (Available in Ansible 2.1) Install filesystem acl support on the managed
host. If the temporary directory on the remote host is mounted with
filesystem acls enabled and the :command:`setfacl` tool is in the remote
``PATH`` then Ansible will use filesystem acls to share the module file with
the second unprivileged instead of having to make the file readable by
everyone.
* Don't perform an action on the remote machine by becoming an unprivileged
user. Temporary files are protected by UNIX file permissions when you
become root or do not use become.
``become`` root or do not use ``become``. In Ansible 2.1 and above, UNIX
file permissions are also secure if you make the connection to the managed
machine as root and then use ``become`` to an unprivileged account.
.. versionchanged:: 2.1
In addition to the additional means of doing this securely, Ansible 2.1 also
makes it harder to unknowingly do this insecurely. Whereas in Ansible 2.0.x
and below, Ansible will silently allow the insecure behaviour if it was unable
to find another way to share the files with the unprivileged user, in Ansible
2.1 and above Ansible defaults to issuing an error if it can't do this
securely. If you can't make any of the changes above to resolve the problem
and you decide that the machine you're running on is secure enough for the
modules you want to run there to be world readable you can turn on
``allow_world_readable_tmpfiles`` in the :file:`ansible.cfg` file. Setting
``allow_world_readable_tmpfiles`` will change this from an error into
a warning and allow the task to run as it did prior to 2.1.
Connection Plugin Support
=========================

View file

@ -60,7 +60,7 @@ General defaults
In the [defaults] section of ansible.cfg, the following settings are tunable:
.. _action_plugins:
.. _cfg_action_plugins:
action_plugins
==============

View file

@ -212,6 +212,14 @@
# prevents logging of tasks, but only on the targets, data is still logged on the master/controller
#no_target_syslog = False
# controls whether Ansible will raise an error or warning if a task has no
# choice but to create world readable temporary files to execute a module on
# the remote machine. This option is False by default for security. Users may
# turn this on to have behaviour more like Ansible prior to 2.1.x. See
# https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user
# for more secure ways to fix this than enabling this option.
#allow_world_readable_tmpfiles = False
# controls the compression level of variables sent to
# worker processes. At the default of 0, no compression
# is used. This value must be an integer from 0 to 9.

View file

@ -165,6 +165,7 @@ DEFAULT_VAR_COMPRESSION_LEVEL = get_config(p, DEFAULTS, 'var_compression_level',
# disclosure
DEFAULT_NO_LOG = get_config(p, DEFAULTS, 'no_log', 'ANSIBLE_NO_LOG', False, boolean=True)
DEFAULT_NO_TARGET_SYSLOG = get_config(p, DEFAULTS, 'no_target_syslog', 'ANSIBLE_NO_TARGET_SYSLOG', False, boolean=True)
ALLOW_WORLD_READABLE_TMPFILES = get_config(p, DEFAULTS, 'allow_world_readable_tmpfiles', None, False, boolean=True)
# selinux
DEFAULT_SELINUX_SPECIAL_FS = get_config(p, 'selinux', 'special_context_filesystems', None, 'fuse, nfs, vboxsf, ramfs', islist=True)

View file

@ -192,7 +192,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
return True
return False
def _make_tmp_path(self):
def _make_tmp_path(self, remote_user):
'''
Create and return a temporary path on a remote box.
'''
@ -200,12 +200,10 @@ class ActionBase(with_metaclass(ABCMeta, object)):
basefile = 'ansible-tmp-%s-%s' % (time.time(), random.randint(0, 2**48))
use_system_tmp = False
if self._play_context.become and self._play_context.become_user != 'root':
if self._play_context.become and self._play_context.become_user not in ('root', remote_user):
use_system_tmp = True
tmp_mode = None
if self._play_context.remote_user != 'root' or self._play_context.become and self._play_context.become_user != 'root':
tmp_mode = 0o755
tmp_mode = 0o700
cmd = self._connection._shell.mkdtemp(basefile, use_system_tmp, tmp_mode)
result = self._low_level_execute_command(cmd, sudoable=False)
@ -255,6 +253,10 @@ class ActionBase(with_metaclass(ABCMeta, object)):
# If ssh breaks we could leave tmp directories out on the remote system.
self._low_level_execute_command(cmd, sudoable=False)
def _transfer_file(self, local_path, remote_path):
self._connection.put_file(local_path, remote_path)
return remote_path
def _transfer_data(self, remote_path, data):
'''
Copies the module data out to the temporary module path.
@ -269,25 +271,85 @@ class ActionBase(with_metaclass(ABCMeta, object)):
data = to_bytes(data, errors='strict')
afo.write(data)
except Exception as e:
#raise AnsibleError("failure encoding into utf-8: %s" % str(e))
raise AnsibleError("failure writing module data to temporary file for transfer: %s" % str(e))
afo.flush()
afo.close()
try:
self._connection.put_file(afile, remote_path)
self._transfer_file(afile, remote_path)
finally:
os.unlink(afile)
return remote_path
def _remote_chmod(self, mode, path, sudoable=False):
def _fixup_perms(self, remote_path, remote_user, execute=False, recursive=True):
"""
If the become_user is unprivileged and different from the
remote_user then we need to make the files we've uploaded readable by them.
"""
if remote_path is None:
# Sometimes code calls us naively -- it has a var which could
# contain a path to a tmp dir but doesn't know if it needs to
# exist or not. If there's no path, then there's no need for us
# to do work
self._display.debug('_fixup_perms called with remote_path==None. Sure this is correct?')
return remote_path
if self._play_context.become and self._play_context.become_user not in ('root', remote_user):
# Unprivileged user that's different than the ssh user. Let's get
# to work!
if remote_user == 'root':
# SSh'ing as root, therefore we can chown
self._remote_chown(remote_path, self._play_context.become_user, recursive=recursive)
if execute:
# root can read things that don't have read bit but can't
# execute them.
self._remote_chmod('u+x', remote_path, recursive=recursive)
else:
if execute:
mode = 'rx'
else:
mode = 'rX'
# Try to use fs acls to solve this problem
res = self._remote_set_user_facl(remote_path, self._play_context.become_user, mode, recursive=recursive, sudoable=False)
if res['rc'] != 0:
if C.ALLOW_WORLD_READABLE_TMPFILES:
# fs acls failed -- do things this insecure way only
# if the user opted in in the config file
self._display.warning('Using world-readable permissions for temporary files Ansible needs to create when becoming an unprivileged user which may be insecure. For information on securing this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user')
self._remote_chmod('a+%s' % mode, remote_path, recursive=recursive)
else:
raise AnsibleError('Failed to set permissions on the temporary files Ansible needs to create when becoming an unprivileged user. For information on working around this, see https://docs.ansible.com/ansible/become.html#becoming-an-unprivileged-user')
elif execute:
# Can't depend on the file being transferred with execute
# permissions. Only need user perms because no become was
# used here
self._remote_chmod('u+x', remote_path, recursive=recursive)
return remote_path
def _remote_chmod(self, mode, path, recursive=True, sudoable=False):
'''
Issue a remote chmod command
'''
cmd = self._connection._shell.chmod(mode, path, recursive=recursive)
res = self._low_level_execute_command(cmd, sudoable=sudoable)
return res
cmd = self._connection._shell.chmod(mode, path)
def _remote_chown(self, path, user, group=None, recursive=True, sudoable=False):
'''
Issue a remote chown command
'''
cmd = self._connection._shell.chown(path, user, group, recursive=recursive)
res = self._low_level_execute_command(cmd, sudoable=sudoable)
return res
def _remote_set_user_facl(self, path, user, mode, recursive=True, sudoable=False):
'''
Issue a remote call to setfacl
'''
cmd = self._connection._shell.set_user_facl(path, user, mode, recursive=recursive)
res = self._low_level_execute_command(cmd, sudoable=sudoable)
return res
@ -417,6 +479,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
else:
module_args['_ansible_check_mode'] = False
# Get the connection user for permission checks
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
# set no log in the module arguments, if required
module_args['_ansible_no_log'] = self._play_context.no_log or C.DEFAULT_NO_TARGET_SYSLOG
@ -437,7 +502,7 @@ class ActionBase(with_metaclass(ABCMeta, object)):
remote_module_path = None
args_file_path = None
if not tmp and self._late_needs_tmp_path(tmp, module_style):
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
if tmp:
remote_module_filename = self._connection._shell.get_remote_filename(module_name)
@ -462,11 +527,9 @@ class ActionBase(with_metaclass(ABCMeta, object)):
environment_string = self._compute_environment_string()
if tmp and "tmp" in tmp and self._play_context.become and self._play_context.become_user != 'root':
# deal with possible umask issues once sudo'ed to other user
self._remote_chmod('a+r', remote_module_path)
if args_file_path is not None:
self._remote_chmod('a+r', args_file_path)
# Fix permissions of the tmp path and tmp files. This should be
# called after all files have been transferred.
self._fixup_perms(tmp, remote_user, recursive=True)
cmd = ""
in_data = None

View file

@ -98,8 +98,9 @@ class ActionModule(ActionBase):
return result
cleanup_remote_tmp = False
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
cleanup_remote_tmp = True
if boolean(remote_src):
@ -146,16 +147,15 @@ class ActionModule(ActionBase):
)
if path_checksum != dest_stat['checksum']:
resultant = file(path).read()
if self._play_context.diff:
diff = self._get_diff_data(dest, path, task_vars)
xfered = self._transfer_data('src', resultant)
remote_path = self._connection._shell.join_path(tmp, 'src')
xfered = self._transfer_file(path, remote_path)
# fix file permissions when the copy is done as a different user
if self._play_context.become and self._play_context.become_user != 'root':
self._remote_chmod('a+r', xfered)
self._fixup_perms(tmp, remote_user, recursive=True)
new_module_args.update( dict( src=xfered,))

View file

@ -38,8 +38,9 @@ class ActionModule(ActionBase):
result['msg'] = 'check mode not supported for this module'
return result
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
module_name = self._task.action
async_module_path = self._connection._shell.join_path(tmp, 'async_wrapper')
@ -54,15 +55,25 @@ class ActionModule(ActionBase):
# configure, upload, and chmod the target module
(module_style, shebang, module_data) = self._configure_module(module_name=module_name, module_args=module_args, task_vars=task_vars)
self._transfer_data(remote_module_path, module_data)
self._remote_chmod('a+rx', remote_module_path)
# configure, upload, and chmod the async_wrapper module
(async_module_style, shebang, async_module_data) = self._configure_module(module_name='async_wrapper', module_args=dict(), task_vars=task_vars)
self._transfer_data(async_module_path, async_module_data)
self._remote_chmod('a+rx', async_module_path)
argsfile = self._transfer_data(self._connection._shell.join_path(tmp, 'arguments'), json.dumps(module_args))
self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
# Only the following two files need to be executable but we'd have to
# make three remote calls if we wanted to just set them executable.
# There's not really a problem with marking too many of the temp files
# executable so we go ahead and mark them all as executable in the
# line above (the line above is needed in any case [although
# execute=False is okay if we uncomment the lines below] so that all
# the files are readable in case the remote_user and become_user are
# different and both unprivileged)
#self._fixup_perms(remote_module_path, remote_user, execute=True, recursive=False)
#self._fixup_perms(async_module_path, remote_user, execute=True, recursive=False)
async_limit = self._task.async
async_jid = str(random.randint(0, 999999999999))

View file

@ -141,9 +141,10 @@ class ActionModule(ActionBase):
delete_remote_tmp = (len(source_files) == 1)
# If this is a recursive action create a tmp path that we can share as the _exec_module create is too late.
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not delete_remote_tmp:
if tmp is None or "-tmp-" not in tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
# expand any user home dir specifier
dest = self._remote_expand_user(dest)
@ -196,7 +197,7 @@ class ActionModule(ActionBase):
# If this is recursive we already have a tmp path.
if delete_remote_tmp:
if tmp is None or "-tmp-" not in tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
if self._play_context.diff and not raw:
diffs.append(self._get_diff_data(dest_file, source_full, task_vars))
@ -211,16 +212,15 @@ class ActionModule(ActionBase):
tmp_src = self._connection._shell.join_path(tmp, 'source')
if not raw:
self._connection.put_file(source_full, tmp_src)
self._transfer_file(source_full, tmp_src)
else:
self._connection.put_file(source_full, dest_file)
self._transfer_file(source_full, dest_file)
# We have copied the file remotely and no longer require our content_tempfile
self._remove_tempfile_if_content_defined(content, content_tempfile)
# fix file permissions when the copy is done as a different user
if self._play_context.become and self._play_context.become_user != 'root':
self._remote_chmod('a+r', tmp_src)
self._fixup_perms(tmp, remote_user, recursive=True)
if raw:
# Continue to next iteration if raw is defined.

View file

@ -34,6 +34,7 @@ class ActionModule(ActionBase):
src = self._task.args.get('src', None)
remote_src = boolean(self._task.args.get('remote_src', 'no'))
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if src is None:
result['failed'] = True
@ -52,14 +53,12 @@ class ActionModule(ActionBase):
# create the remote tmp dir if needed, and put the source file there
if tmp is None or "-tmp-" not in tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
tmp_src = self._connection._shell.join_path(tmp, os.path.basename(src))
self._connection.put_file(src, tmp_src)
self._transfer_file(src, tmp_src)
if self._play_context.become and self._play_context.become_user != 'root':
if not self._play_context.check_mode:
self._remote_chmod('a+r', tmp_src)
self._fixup_perms(tmp, remote_user, recursive=True)
new_module_args = self._task.args.copy()
new_module_args.update(

View file

@ -38,8 +38,9 @@ class ActionModule(ActionBase):
result['msg'] = 'check mode not supported for this module'
return result
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
creates = self._task.args.get('creates')
if creates:
@ -76,16 +77,11 @@ class ActionModule(ActionBase):
# transfer the file to a remote tmp location
tmp_src = self._connection._shell.join_path(tmp, os.path.basename(source))
self._connection.put_file(source, tmp_src)
self._transfer_file(source, tmp_src)
sudoable = True
# set file permissions, more permissive when the copy is done as a different user
if self._play_context.become and self._play_context.become_user != 'root':
chmod_mode = 'a+rx'
sudoable = False
else:
chmod_mode = '+rx'
self._remote_chmod(chmod_mode, tmp_src, sudoable=sudoable)
self._fixup_perms(tmp, remote_user, execute=True, recursive=True)
# add preparation steps to one ssh roundtrip executing the script
env_string = self._compute_environment_string()

View file

@ -138,8 +138,9 @@ class ActionModule(ActionBase):
return result
cleanup_remote_tmp = False
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
cleanup_remote_tmp = True
local_checksum = checksum_s(resultant)
@ -163,8 +164,7 @@ class ActionModule(ActionBase):
xfered = self._transfer_data(self._connection._shell.join_path(tmp, 'source'), resultant)
# fix file permissions when the copy is done as a different user
if self._play_context.become and self._play_context.become_user != 'root':
self._remote_chmod('a+r', xfered)
self._fixup_perms(tmp, remote_user, recursive=True)
# run the copy module
new_module_args.update(

View file

@ -45,8 +45,9 @@ class ActionModule(ActionBase):
result['msg'] = "src (or content) and dest are required"
return result
remote_user = task_vars.get('ansible_ssh_user') or self._play_context.remote_user
if not tmp:
tmp = self._make_tmp_path()
tmp = self._make_tmp_path(remote_user)
if creates:
# do not run the command if the line contains creates=filename
@ -80,17 +81,15 @@ class ActionModule(ActionBase):
if copy:
# transfer the file to a remote tmp location
tmp_src = tmp + 'source'
self._connection.put_file(source, tmp_src)
tmp_src = self._connection._shell.join_path(tmp, 'source')
self._transfer_file(source, tmp_src)
# handle diff mode client side
# handle check mode client side
# fix file permissions when the copy is done as a different user
if copy:
if self._play_context.become and self._play_context.become_user != 'root':
if not self._play_context.check_mode:
self._remote_chmod('a+r', tmp_src)
if copy:
# fix file permissions when the copy is done as a different user
self._fixup_perms(tmp, remote_user, recursive=True)
# Build temporary module_args.
new_module_args = self._task.args.copy()
new_module_args.update(

View file

@ -52,9 +52,40 @@ class ShellBase(object):
def path_has_trailing_slash(self, path):
return path.endswith('/')
def chmod(self, mode, path):
def chmod(self, mode, path, recursive=True):
path = pipes.quote(path)
return 'chmod %s %s' % (mode, path)
cmd = ['chmod', mode, path]
if recursive:
cmd.append('-R')
return ' '.join(cmd)
def chown(self, path, user, group=None, recursive=True):
path = pipes.quote(path)
user = pipes.quote(user)
if group is None:
cmd = ['chown', user, path]
else:
group = pipes.quote(group)
cmd = ['chown', '%s:%s' % (user, group), path]
if recursive:
cmd.append('-R')
return ' '.join(cmd)
def set_user_facl(self, path, user, mode, recursive=True):
"""Only sets acls for users as that's really all we need"""
path = pipes.quote(path)
mode = pipes.quote(mode)
user = pipes.quote(user)
cmd = ['setfacl']
if recursive:
cmd.append('-R')
cmd.extend(('-m', 'u:%s:%s %s' % (user, mode, path)))
return ' '.join(cmd)
def remove(self, path, recurse=False):
path = pipes.quote(path)

View file

@ -392,27 +392,27 @@ class TestActionBase(unittest.TestCase):
action_base._low_level_execute_command = MagicMock()
action_base._low_level_execute_command.return_value = dict(rc=0, stdout='/some/path')
self.assertEqual(action_base._make_tmp_path(), '/some/path/')
self.assertEqual(action_base._make_tmp_path('root'), '/some/path/')
# empty path fails
action_base._low_level_execute_command.return_value = dict(rc=0, stdout='')
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
# authentication failure
action_base._low_level_execute_command.return_value = dict(rc=5, stdout='')
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
# ssh error
action_base._low_level_execute_command.return_value = dict(rc=255, stdout='', stderr='')
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
play_context.verbosity = 5
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
# general error
action_base._low_level_execute_command.return_value = dict(rc=1, stdout='some stuff here', stderr='')
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
action_base._low_level_execute_command.return_value = dict(rc=1, stdout='some stuff here', stderr='No space left on device')
self.assertRaises(AnsibleError, action_base._make_tmp_path)
self.assertRaises(AnsibleError, action_base._make_tmp_path, 'root')
def test_action_base__remove_tmp_path(self):
# create our fake task
@ -567,8 +567,8 @@ class TestActionBase(unittest.TestCase):
action_base._make_tmp_path = MagicMock()
action_base._transfer_data = MagicMock()
action_base._compute_environment_string = MagicMock()
action_base._remote_chmod = MagicMock()
action_base._low_level_execute_command = MagicMock()
action_base._fixup_perms = MagicMock()
action_base._configure_module.return_value = ('new', '#!/usr/bin/python', 'this is the module data')
action_base._late_needs_tmp_path.return_value = False