Add missing localhost delegation checks to some modules

Currently we don't check some modules for delegation to
localhost. This would make it possible to overwrite any data which is
writable within the bwrap context. Further the script module allows
arbitrary code execution when delegated to localhost.

The following modules are affected:
* assemble: add safe path check
* copy: add safe path check
* patch: add safe path check
* script: block completely
* template: add safe path check
* unarchive: add tests, fixed by safe path check of copy

Change-Id: I2360219f50e6a28bb134468ead08ec72148ad192
Story: 2001681
This commit is contained in:
Tobias Henkel 2018-03-13 13:36:59 +01:00
parent e56801f2e8
commit 5763b8e4d7
No known key found for this signature in database
GPG Key ID: 03750DEC158E5FA2
53 changed files with 516 additions and 7 deletions

View File

@ -0,0 +1 @@
This is a readme

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- assemble-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- assemble-test-localhost
- hosts: 127.0.0.1
roles:
- assemble-test-localhost
- hosts: "::1"
roles:
- assemble-test-localhost

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- copy-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- copy-test-localhost
- hosts: 127.0.0.1
roles:
- copy-test-localhost
- hosts: "::1"
roles:
- copy-test-localhost

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- patch-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- patch-test-localhost
- hosts: 127.0.0.1
roles:
- patch-test-localhost
- hosts: "::1"
roles:
- patch-test-localhost

View File

@ -0,0 +1,14 @@
- name: Assemble
assemble:
src: dir
dest: /opt/assemble-dest
remote_src: no
delegate_to: "{{ item }}"
ignore_errors: true
register: result
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Assemble must fail due to accessing files outside the working dir

View File

@ -0,0 +1,22 @@
- include: assemble-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost
- name: Define target dir
set_fact:
targetdir: "{{ zuul.executor.work_root }}/assemble-target"
- name: Create target dir
file:
state: directory
path: "{{ targetdir }}"
delegate_to: localhost
- name: Assemble to safe local path
assemble:
src: dir
dest: "{{ targetdir }}/assemble-dest.conf"
remote_src: no
delegate_to: localhost

View File

@ -0,0 +1,13 @@
- name: Assemble
assemble:
src: dir
dest: /opt/assemble-dest
remote_src: no
ignore_errors: true
register: result
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Assemble must fail due to accessing files outside the working dir

View File

@ -0,0 +1,13 @@
- name: Copy
copy:
src: file
dest: /opt/copy-dest
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Copy must fail due to accessing files outside the working dir

View File

@ -0,0 +1,27 @@
- include: copy-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost
- name: Define target dir
set_fact:
targetdir: "{{ zuul.executor.work_root }}/copy-target"
- name: Create target dir
file:
state: directory
path: "{{ targetdir }}"
delegate_to: localhost
- name: Copy file into safe path
copy:
src: file
dest: "{{ targetdir }}/dest-file"
delegate_to: localhost
- name: Copy file into safe directory
copy:
src: file
dest: "{{ targetdir }}/dest-dir/"
delegate_to: localhost

View File

@ -0,0 +1,12 @@
- name: Copy
copy:
src: file
dest: /opt/copy-dest
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Copy must fail due to accessing files outside the working dir

View File

@ -0,0 +1,7 @@
diff --git a/readme.txt b/readme.txt
index 24308cb..b07f0ed 100644
--- a/readme.txt
+++ b/readme.txt
@@ -1 +1 @@
-This is a readme
+This is a README

View File

@ -0,0 +1,41 @@
- include: patch-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost
- name: Define target dir
set_fact:
targetdir: "{{ zuul.executor.work_root }}/patch-target"
- name: Create target dir
file:
state: directory
path: "{{ targetdir }}"
delegate_to: localhost
- name: Copy readme
copy:
src: readme.txt
dest: "{{ targetdir }}/readme.txt"
delegate_to: localhost
- name: Patch in safe path using basedir
patch:
src: "patch"
basedir: "{{ targetdir }}"
strip: 1
delegate_to: localhost
- name: Copy readme again
copy:
src: readme.txt
dest: "{{ targetdir }}/readme.txt"
delegate_to: localhost
- name: Patch in safe path using dest
patch:
src: "patch"
dest: "{{ targetdir }}/readme.txt"
strip: 1
delegate_to: localhost

View File

@ -0,0 +1,29 @@
- name: Patch with basedir
patch:
src: patch
basedir: "/opt/patch-dest"
strip: 1
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Patch must fail due to accessing files outside the working dir
- name: Patch with dest
patch:
src: patch
dest: "/opt/patch-dest/readme"
strip: 1
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Patch must fail due to accessing files outside the working dir

View File

@ -0,0 +1,7 @@
diff --git a/readme.txt b/readme.txt
index 24308cb..b07f0ed 100644
--- a/readme.txt
+++ b/readme.txt
@@ -1 +1 @@
-This is a readme
+This is a README

View File

@ -0,0 +1,27 @@
- name: Patch with basedir
patch:
src: patch
basedir: "/opt/patch-dest"
strip: 1
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Patch must fail due to accessing files outside the working dir
- name: Patch with dest
patch:
src: patch
dest: "/opt/patch-dest/readme"
strip: 1
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Patch must fail due to accessing files outside the working dir

View File

@ -0,0 +1,5 @@
- include: script-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost

View File

@ -0,0 +1,11 @@
- name: Script
script: script.sh
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Executing local code is prohibited' in result.msg"
msg: Script must fail due to local code execution restriction

View File

@ -0,0 +1,10 @@
- name: Script
script: script.sh
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Executing local code is prohibited' in result.msg"
msg: Script must fail due to local code execution restriction

View File

@ -0,0 +1,21 @@
- include: template-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost
- name: Define target dir
set_fact:
targetdir: "{{ zuul.executor.work_root }}/template-target"
- name: Create target dir
file:
state: directory
path: "{{ targetdir }}"
delegate_to: localhost
- name: Template into safe path
template:
src: template
dest: "{{ targetdir }}/dest-file"
delegate_to: localhost

View File

@ -0,0 +1,13 @@
- name: Template
copy:
src: template
dest: /opt/copy-dest
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Template must fail due to accessing files outside the working dir

View File

@ -0,0 +1,12 @@
- name: Template
copy:
src: template
dest: /opt/copy-dest
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Template must fail due to accessing files outside the working dir

View File

@ -0,0 +1,21 @@
- include: unarchive-delegate.yaml
with_items:
- ::1
- 127.0.0.1
- localhost
- name: Define target dir
set_fact:
targetdir: "{{ zuul.executor.work_root }}/unarchive-target"
- name: Create target dir
file:
state: directory
path: "{{ targetdir }}"
delegate_to: localhost
- name: Unarchive
copy:
src: archive.tar
dest: "{{ targetdir }}"
delegate_to: localhost

View File

@ -0,0 +1,13 @@
- name: Unarchive
copy:
src: archive.tar
dest: /opt/unarchive-dest
delegate_to: "{{ item }}"
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Unarchive must fail due to accessing files outside the working dir

View File

@ -0,0 +1,12 @@
- name: Unarchive
copy:
src: archive.tar
dest: /opt/unarchive-dest
register: result
ignore_errors: true
- assert:
that:
- "result.failed == true"
- "'Accessing files from outside the working dir' in result.msg"
msg: Unarchive must fail due to accessing files outside the working dir

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- script-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- script-test-localhost
- hosts: 127.0.0.1
roles:
- script-test-localhost
- hosts: "::1"
roles:
- script-test-localhost

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- template-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- template-test-localhost
- hosts: 127.0.0.1
roles:
- template-test-localhost
- hosts: "::1"
roles:
- template-test-localhost

View File

@ -0,0 +1,3 @@
- hosts: all
roles:
- unarchive-test-delegate

View File

@ -0,0 +1,11 @@
- hosts: localhost
roles:
- unarchive-test-localhost
- hosts: 127.0.0.1
roles:
- unarchive-test-localhost
- hosts: "::1"
roles:
- unarchive-test-localhost

View File

@ -81,6 +81,12 @@ class TestActionModules(AnsibleZuulTestCase):
def test_assemble_module(self):
self._run_job('assemble-good', 'SUCCESS')
# assemble-delegate does multiple tests with various delegates and
# safe and non-safe paths. It asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('assemble-delegate', 'SUCCESS')
self._run_job('assemble-localhost', 'SUCCESS')
self._run_job('assemble-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('assemble-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('assemble-bad-dir-with-symlink', 'FAILURE',
@ -89,6 +95,12 @@ class TestActionModules(AnsibleZuulTestCase):
def test_copy_module(self):
self._run_job('copy-good', 'SUCCESS')
# copy-delegate does multiple tests with various delegates and
# safe and non-safe paths. It asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('copy-delegate', 'SUCCESS')
self._run_job('copy-localhost', 'SUCCESS')
self._run_job('copy-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('copy-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('copy-bad-dir-with-symlink', 'FAILURE',
@ -112,23 +124,47 @@ class TestActionModules(AnsibleZuulTestCase):
def test_patch_module(self):
self._run_job('patch-good', 'SUCCESS')
# patch-delegate does multiple tests with various delegates and
# safe and non-safe paths. It asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('patch-delegate', 'SUCCESS')
self._run_job('patch-localhost', 'SUCCESS')
self._run_job('patch-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('patch-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)
def test_script_module(self):
self._run_job('script-good', 'SUCCESS')
# script-delegate does multiple tests with various delegates. It
# asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('script-delegate', 'SUCCESS')
self._run_job('script-localhost', 'SUCCESS')
self._run_job('script-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('script-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)
def test_template_module(self):
self._run_job('template-good', 'SUCCESS')
# template-delegate does multiple tests with various delegates and
# safe and non-safe paths. It asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('template-delegate', 'SUCCESS')
self._run_job('template-localhost', 'SUCCESS')
self._run_job('template-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('template-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)
def test_unarchive_module(self):
self._run_job('unarchive-good', 'SUCCESS')
# template-delegate does multiple tests with various delegates and
# safe and non-safe paths. It asserts by itself within ansible so we
# expect SUCCESS here.
self._run_job('unarchive-delegate', 'SUCCESS')
self._run_job('unarchive-localhost', 'SUCCESS')
self._run_job('unarchive-bad', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('unarchive-bad-symlink', 'FAILURE', ERROR_ACCESS_OUTSIDE)

View File

@ -29,4 +29,7 @@ class ActionModule(assemble.ActionModule):
if not paths._is_official_module(self):
return paths._fail_module_dict(self._task.action)
if paths._is_localhost_task(self):
paths._fail_if_unsafe(self._task.args['dest'])
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -29,4 +29,7 @@ class ActionModule(copy.ActionModule):
if not paths._is_official_module(self):
return paths._fail_module_dict(self._task.action)
if paths._is_localhost_task(self):
paths._fail_if_unsafe(self._task.args['dest'])
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -35,12 +35,7 @@ class ActionModule(normal.ActionModule):
def run(self, tmp=None, task_vars=None):
'''Overridden primary method from the base class.'''
if (self._play_context.connection == 'local'
or self._play_context.remote_addr == 'localhost'
or self._play_context.remote_addr.startswith('127.')
or self._task.delegate_to == 'localhost'
or (self._task.delegate_to
and self._task.delegate_to.startswith('127.'))):
if paths._is_localhost_task(self):
if not self.dispatch_handler():
raise AnsibleError("Executing local code is prohibited")
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -13,7 +13,6 @@
# You should have received a copy of the GNU General Public License
# along with this software. If not, see <http://www.gnu.org/licenses/>.
from zuul.ansible import paths
patch = paths._import_ansible_action_plugin("patch")
@ -28,4 +27,17 @@ class ActionModule(patch.ActionModule):
if not paths._is_official_module(self):
return paths._fail_module_dict(self._task.action)
if paths._is_localhost_task(self):
# The patch module has two possibilities of describing where to
# operate, basedir and dest. We need to perform the safe path check
# for both.
dirs_to_check = [
self._task.args.get('basedir'),
self._task.args.get('dest'),
]
for directory in dirs_to_check:
if directory is not None:
paths._fail_if_unsafe(directory)
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -14,6 +14,7 @@
# along with this software. If not, see <http://www.gnu.org/licenses/>.
from ansible.errors import AnsibleError
from zuul.ansible import paths
script = paths._import_ansible_action_plugin("script")
@ -29,4 +30,7 @@ class ActionModule(script.ActionModule):
if not paths._is_official_module(self):
return paths._fail_module_dict(self._task.action)
if paths._is_localhost_task(self):
raise AnsibleError("Executing local code is prohibited")
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -28,4 +28,9 @@ class ActionModule(unarchive.ActionModule):
if not paths._is_official_module(self):
return paths._fail_module_dict(self._task.action)
# Note: The unarchive module reuses the copy module to copy the archive
# to the remote. Thus we don't need to check the dest here if we run
# against localhost. We also have tests that would break if this
# changes in the future.
return super(ActionModule, self).run(tmp, task_vars)

View File

@ -151,3 +151,29 @@ def _fail_if_local_module(module):
if not _is_official_module(module):
msg_dict = _fail_module_dict(module._task.action)
raise AnsibleError(msg_dict['msg'])
def _is_localhost_task(task):
# remote_addr is what's in the value of ansible_host and/or the opposite
# side of a mapping. So if you had an inventory with:
#
# all:
# hosts:
# ubuntu-xenial:
# ansible_connection: ssh
# ansible_host: 23.253.109.74
# remote_addr would be 23.253.109.74.
#
# localhost is special, since it's not in the inventory but instead is
# added directly by ansible.
#
# The only way a user could supply a remote_addr with arbitrary ipv6
# values is if they used add_host - which we don't let unprivileged code
# do.
if (task._play_context.connection == 'local'
or task._play_context.remote_addr == 'localhost'
or task._task.delegate_to == 'localhost'):
return True
return False