Skip to content

Commit

Permalink
Allow overriding execute check in setacl in some cases
Browse files Browse the repository at this point in the history
When TrueNAS is joined to active directory it's possible that the
AD administrator has created nested security groups in such a way
that it becomes non-trivial to validate whether a user can gain
access to a path by virtue of being a member of a particular group.

This is because the nested security groups are flattened only when
resolving the groups for a particular user via getgrouplist(3).

We still default to checking access (previous behavior) because
using nested security groups in this way is a security anti-pattern
as it renders the effective permissions on filesystem paths
very difficult or impossible to easily audit.
  • Loading branch information
anodos325 committed Aug 6, 2024
1 parent 988b829 commit 42fdd10
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 26 deletions.
54 changes: 28 additions & 26 deletions src/middlewared/middlewared/plugins/filesystem_/acl.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def chown(self, job, data):
verrors.add("filesystem.chown.uid",
"Please specify either user or group to change.")

loc = self._common_perm_path_validate("filesystem.chown", data, verrors)
self._common_perm_path_validate("filesystem.chown", data, verrors)
verrors.check()

if not options['recursive']:
Expand Down Expand Up @@ -249,7 +249,7 @@ def setperm(self, job, data):
uid = -1 if data['uid'] is None else data.get('uid', -1)
gid = -1 if data['gid'] is None else data.get('gid', -1)

loc = self._common_perm_path_validate("filesystem.setperm", data, verrors)
self._common_perm_path_validate("filesystem.setperm", data, verrors)

current_acl = self.middleware.call_sync('filesystem.getacl', data['path'])
acl_is_trivial = current_acl['trivial']
Expand Down Expand Up @@ -590,13 +590,14 @@ def setacl_nfs4(self, job, data):
self._strip_acl_nfs4(path)

else:
uid_to_check = current_acl['uid'] if uid == -1 else uid
gid_to_check = current_acl['gid'] if gid == -1 else gid
if not options['skip_execute_check']:
uid_to_check = current_acl['uid'] if uid == -1 else uid
gid_to_check = current_acl['gid'] if gid == -1 else gid

self.middleware.call_sync(
'filesystem.check_acl_execute',
path, data['dacl'], uid_to_check, gid_to_check, True
)
self.middleware.call_sync(
'filesystem.check_acl_execute',
path, data['dacl'], uid_to_check, gid_to_check, True
)

self.setacl_nfs4_internal(path, data['dacl'], do_canon, verrors)

Expand Down Expand Up @@ -745,23 +746,24 @@ def setacl_posix1e(self, job, data):
)

if not do_strip:
try:
# check execute on parent paths
uid_to_check = current_acl['uid'] if uid == -1 else uid
gid_to_check = current_acl['gid'] if gid == -1 else gid

self.middleware.call_sync(
'filesystem.check_acl_execute',
path, dacl, uid_to_check, gid_to_check, True
)
except CallError as e:
if e.errno != errno.EPERM:
raise
if not options['skip_execute_check']:
try:
# check execute on parent paths
uid_to_check = current_acl['uid'] if uid == -1 else uid
gid_to_check = current_acl['gid'] if gid == -1 else gid

self.middleware.call_sync(
'filesystem.check_acl_execute',
path, dacl, uid_to_check, gid_to_check, True
)
except CallError as e:
if e.errno != errno.EPERM:
raise

verrors.add(
'filesystem_acl.path',
e.errmsg
)
verrors.add(
'filesystem_acl.path',
e.errmsg
)

aclstring = self.gen_aclstring_posix1e(dacl, recursive, verrors)

Expand Down Expand Up @@ -866,7 +868,8 @@ def setacl_posix1e(self, job, data):
Bool('stripacl', default=False),
Bool('recursive', default=False),
Bool('traverse', default=False),
Bool('canonicalize', default=True)
Bool('canonicalize', default=True),
Bool('skip_execute_check', default=False)
)
), roles=['FILESYSTEM_ATTRS_WRITE'], audit='Filesystem set ACL', audit_extended=lambda data: data['path']
)
Expand Down Expand Up @@ -1123,7 +1126,6 @@ def get_inherited_acl(self, data):
Supports `directory` `option` that allows specifying whether the generated
ACL is for a file or a directory.
"""
init_path = data['path']
verrors = ValidationErrors()
self._common_perm_path_validate('filesystem.get_inherited_acl', data, verrors, True)
verrors.check()
Expand Down
1 change: 1 addition & 0 deletions src/middlewared/middlewared/plugins/pool_/dataset.py
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,7 @@ async def do_create(self, app, data):
acl_job = await self.middleware.call('filesystem.setacl', {
'path': mountpoint,
'dacl': acl_to_set,
'options': {'skip_execute_check': True}
})
await acl_job.wait(raise_error=True)

Expand Down

0 comments on commit 42fdd10

Please sign in to comment.