From ca5f8072a4c7be6fdebef494664a27bbd73340ff Mon Sep 17 00:00:00 2001 From: David Mulder Date: Thu, 17 Nov 2022 16:33:24 -0700 Subject: [PATCH] gp: PAM Access should implicitly deny ALL w/ allow If an allow entry is specified, the PAM Access CSE should implicitly deny ALL (everyone other than the explicit allow entries). Signed-off-by: David Mulder Reviewed-by: Jeremy Allison --- python/samba/gp/vgp_access_ext.py | 54 ++++++++++++++++++++++++++----- python/samba/netcmd/gpo.py | 3 +- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/python/samba/gp/vgp_access_ext.py b/python/samba/gp/vgp_access_ext.py index efd91ef93fb..474d5e278ee 100644 --- a/python/samba/gp/vgp_access_ext.py +++ b/python/samba/gp/vgp_access_ext.py @@ -19,6 +19,7 @@ from samba.gp.gpclass import gp_xml_ext from hashlib import blake2b from tempfile import NamedTemporaryFile from samba.common import get_bytes +from samba.gp.util.logging import log intro = ''' ### autogenerated by samba @@ -31,11 +32,34 @@ intro = ''' ''' +# The deny all file is implicit any time an allow entry is used +DENY_BOUND = 9000000000 +DENY_FILE = '_gp_DENY_ALL.conf' + +# Each policy MUST create it's own DENY_ALL file if an allow entry exists, +# otherwise policies will conflict and one could remove a DENY_ALL when another +# one still requires it. +def deny_file(access): + deny_filename = os.path.join(access, + '%d%s' % (select_next_deny(access), DENY_FILE)) + with NamedTemporaryFile(delete=False, dir=access) as f: + with open(f.name, 'w') as w: + w.write(intro) + w.write('-:ALL:ALL') + os.chmod(f.name, 0o644) + os.rename(f.name, deny_filename) + return deny_filename + +def select_next_deny(directory): + configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE in f] + return max([int(m.group(1)) for m in configs if m]+[DENY_BOUND])+1 + # Access files in /etc/security/access.d are read in the order of the system # locale. Here we number the conf files to ensure they are read in the correct -# order. +# order. Ignore the deny file, since allow entries should always come before +# the implicit deny ALL. def select_next_conf(directory): - configs = [re.match(r'(\d+)', f) for f in os.listdir(directory)] + configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE not in f] return max([int(m.group(1)) for m in configs if m]+[0])+1 class vgp_access_ext(gp_xml_ext): @@ -47,9 +71,10 @@ class vgp_access_ext(gp_xml_ext): for guid, settings in deleted_gpo_list: self.gp_db.set_guid(guid) if str(self) in settings: - for attribute, access_file in settings[str(self)].items(): - if os.path.exists(access_file): - os.unlink(access_file) + for attribute, policy_files in settings[str(self)].items(): + for access_file in policy_files.split(':'): + if os.path.exists(access_file): + os.unlink(access_file) self.gp_db.delete(str(self), attribute) self.gp_db.commit() @@ -63,14 +88,20 @@ class vgp_access_ext(gp_xml_ext): path = os.path.join(gpo.file_sys_path, deny) deny_conf = self.parse(path) entries = [] + policy_files = [] if allow_conf: policy = allow_conf.find('policysetting') data = policy.find('data') - for listelement in data.findall('listelement'): + allow_listelements = data.findall('listelement') + for listelement in allow_listelements: adobject = listelement.find('adobject') name = adobject.find('name').text domain = adobject.find('domain').text entries.append('+:%s\\%s:ALL' % (domain, name)) + if len(allow_listelements) > 0: + log.info('Adding an implicit deny ALL because an allow' + ' entry is present') + policy_files.append(deny_file(access)) if deny_conf: policy = deny_conf.find('policysetting') data = policy.find('data') @@ -79,10 +110,14 @@ class vgp_access_ext(gp_xml_ext): name = adobject.find('name').text domain = adobject.find('domain').text entries.append('-:%s\\%s:ALL' % (domain, name)) + if len(allow_listelements) > 0: + log.warn("Deny entry '%s' is meaningless with " + "allow present" % entries[-1]) if len(entries) == 0: continue conf_id = select_next_conf(access) access_file = os.path.join(access, '%010d_gp.conf' % conf_id) + policy_files.append(access_file) access_contents = '\n'.join(entries) attribute = blake2b(get_bytes(access_contents)).hexdigest() old_val = self.gp_db.retrieve(str(self), attribute) @@ -96,7 +131,7 @@ class vgp_access_ext(gp_xml_ext): w.write(access_contents) os.chmod(f.name, 0o644) os.rename(f.name, access_file) - self.gp_db.store(str(self), attribute, access_file) + self.gp_db.store(str(self), attribute, ':'.join(policy_files)) self.gp_db.commit() def rsop(self, gpo): @@ -113,13 +148,16 @@ class vgp_access_ext(gp_xml_ext): if allow_conf: policy = allow_conf.find('policysetting') data = policy.find('data') - for listelement in data.findall('listelement'): + allow_listelements = data.findall('listelement') + for listelement in allow_listelements: adobject = listelement.find('adobject') name = adobject.find('name').text domain = adobject.find('domain').text if str(self) not in output.keys(): output[str(self)] = [] output[str(self)].append('+:%s\\%s:ALL' % (name, domain)) + if len(allow_listelements) > 0: + output[str(self)].append('-:ALL:ALL') if deny_conf: policy = deny_conf.find('policysetting') data = policy.find('data') diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py index 2bcee59bd93..9cd08273aaa 100644 --- a/python/samba/netcmd/gpo.py +++ b/python/samba/netcmd/gpo.py @@ -3815,7 +3815,8 @@ class cmd_add_access(Command): """Adds a VGP Host Access Group Policy to the sysvol This command adds a host access setting to the sysvol for applying to winbind -clients. +clients. Any time an allow entry is detected by the client, an implicit deny +ALL will be assumed. Example: samba-tool gpo manage access add {31B2F340-016D-11D2-945F-00C04FB984F9} allow goodguy example.com