zuul
/
zuul
Code Issues Proposed changes

Merge "Fix safe path check for directories containing symlinks"

This commit is contained in:
Zuul 2018-03-14 20:24:34 +00:00 committed by Gerrit Code Review
parent 88cbbf3af9 e54fcde58a
commit fe0274cfca
15 changed files with 78 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
tests/fixtures/bwrap-mounts/dir/file vendored Normal file
View File

@ -0,0 +1 @@
test

4
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/assemble-bad-dir-with-symlink.yaml vendored Normal file
View File

@ -0,0 +1,4 @@
- hosts: all
roles:
- role: assemble-test
src_file: dir-symlink

4
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/copy-bad-dir-with-symlink.yaml vendored Normal file
View File

@ -0,0 +1,4 @@
- hosts: all
roles:
- role: copy-test
src_file: dir-symlink

4
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/includevars-bad-dir-with-double-symlink.yaml vendored Normal file
View File

@ -0,0 +1,4 @@
- hosts: all
roles:
- role: includevarsdir-test
src_file: dir-double-symlink

4
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/includevars-bad-dir-with-symlink.yaml vendored Normal file
View File

@ -0,0 +1,4 @@
- hosts: all
roles:
- role: includevarsdir-test
src_file: dir-symlink

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/assemble-test/files/dir-symlink/one vendored Symbolic link
View File

@ -0,0 +1 @@
/opt/file

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/copy-test/files/dir-symlink/symlink vendored Symbolic link
View File

@ -0,0 +1 @@
/opt/dir

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/dir-double-symlink/double-symlink-sidekick vendored Symbolic link
View File

@ -0,0 +1 @@
../double-symlink-sidekick

3
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/dir-double-symlink/one.yaml vendored Normal file
View File

@ -0,0 +1,3 @@
---
first_var: one

3
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/dir-symlink/one.yaml vendored Normal file
View File

@ -0,0 +1,3 @@
---
first_var: one

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/dir-symlink/two.yaml vendored Symbolic link
View File

@ -0,0 +1 @@
/opt/vars.yaml

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/dir/sub/recursive-link vendored Symbolic link
View File

@ -0,0 +1 @@
..

1
tests/fixtures/config/remote-action-modules/git/org_project/playbooks/roles/includevarsdir-test/vars/double-symlink-sidekick/vars.yaml vendored Symbolic link
View File

@ -0,0 +1 @@
/opt/vars.yaml

8
tests/remote/test_remote_action_modules.py
View File

@ -83,12 +83,16 @@ class TestActionModules(AnsibleZuulTestCase):
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',
ERROR_ACCESS_OUTSIDE)
def test_copy_module(self):
self._run_job('copy-good', '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',
ERROR_ACCESS_OUTSIDE)
def test_includevars_module(self):
self._run_job('includevars-good', 'SUCCESS')
@ -100,6 +104,10 @@ class TestActionModules(AnsibleZuulTestCase):
self._run_job('includevars-bad-dir', 'FAILURE', ERROR_ACCESS_OUTSIDE)
self._run_job('includevars-bad-dir-symlink', 'FAILURE',
ERROR_ACCESS_OUTSIDE)
self._run_job('includevars-bad-dir-with-symlink', 'FAILURE',
ERROR_ACCESS_OUTSIDE)
self._run_job('includevars-bad-dir-with-double-symlink', 'FAILURE',
ERROR_ACCESS_OUTSIDE)
def test_patch_module(self):
self._run_job('patch-good', 'SUCCESS')

47
zuul/ansible/paths.py
View File

@ -38,15 +38,50 @@ def _full_path(path):
def _is_safe_path(path, allow_trusted=False):
full_path = _full_path(path)
home_path = os.path.abspath(os.path.expanduser('~'))
if not full_path.startswith(home_path):
if allow_trusted:
trusted_path = os.path.abspath(
os.path.join(home_path, '../trusted'))
if full_path.startswith(trusted_path):
allowed_paths = [home_path]
if allow_trusted:
allowed_paths.append(
os.path.abspath(os.path.join(home_path, '../trusted')))
def _is_safe(path_to_check):
for allowed_path in allowed_paths:
if path_to_check.startswith(allowed_path):
return True
return False
# We need to really check the whole subtree starting from path. So first
# start with the root and do an os.walk if path resolves to a directory.
full_path = _full_path(path)
if not _is_safe(full_path):
return False
# Walk the whole tree and check dirs and files. In order to mitigate
# chained symlink attacks we also need to follow symlinks.
visited = set()
for root, dirs, files in os.walk(full_path, followlinks=True):
# We recurse with follow links so check root first, then the files.
# The dirs will be checked during recursion.
full_root = _full_path(root)
if not _is_safe(full_root):
return False
# NOTE: os.walk can lead to infinite recursion when following links
# so filter out the dirs for further processing if we already checked
# this one.
if full_root in visited:
del dirs[:]
# we already checked the files here so we can just continue to the
# next iteration
continue
visited.add(full_root)
for entry in files:
full_path = _full_path(os.path.join(root, entry))
if not _is_safe(full_path):
return False
return True