Fix safe path check for directories containing symlinks
Currently it is possible to bypass safe path checks by utilising modules that can operate on directories instead of files like assemble. This can be done by putting symlinks into a directory the module is allowed to access. This can be fixed by walking the whole sub tree and checking the paths instead of just checking the path itself. Change-Id: Iaa4efcf0737e47429339e9afd66eecf4e38fd8ea
This commit is contained in:
parent
fe8e71be09
commit
e54fcde58a
|
@ -0,0 +1 @@
|
|||
test
|
|
@ -0,0 +1,4 @@
|
|||
- hosts: all
|
||||
roles:
|
||||
- role: assemble-test
|
||||
src_file: dir-symlink
|
|
@ -0,0 +1,4 @@
|
|||
- hosts: all
|
||||
roles:
|
||||
- role: copy-test
|
||||
src_file: dir-symlink
|
|
@ -0,0 +1,4 @@
|
|||
- hosts: all
|
||||
roles:
|
||||
- role: includevarsdir-test
|
||||
src_file: dir-double-symlink
|
|
@ -0,0 +1,4 @@
|
|||
- hosts: all
|
||||
roles:
|
||||
- role: includevarsdir-test
|
||||
src_file: dir-symlink
|
|
@ -0,0 +1 @@
|
|||
/opt/file
|
|
@ -0,0 +1 @@
|
|||
/opt/dir
|
|
@ -0,0 +1 @@
|
|||
../double-symlink-sidekick
|
|
@ -0,0 +1,3 @@
|
|||
---
|
||||
|
||||
first_var: one
|
|
@ -0,0 +1,3 @@
|
|||
---
|
||||
|
||||
first_var: one
|
|
@ -0,0 +1 @@
|
|||
/opt/vars.yaml
|
|
@ -0,0 +1 @@
|
|||
..
|
|
@ -0,0 +1 @@
|
|||
/opt/vars.yaml
|
|
@ -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')
|
||||
|
|
|
@ -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
|
||||
|
||||
|
||||
|
|
Loading…
Reference in New Issue