From fc8c7722a1a55f6d34d611f03cb6d59680ef5366 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Wed, 11 Oct 2023 22:53:28 +0200 Subject: [PATCH 1/8] tools.py: call apparmor.read_profiles() in __init__() ... instead of calling it in every cmd_* function. --- utils/apparmor/tools.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index d4cd0b3b3..f6dbff545 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -1,6 +1,6 @@ # ---------------------------------------------------------------------- # Copyright (C) 2013 Kshitij Gupta -# Copyright (C) 2015-2022 Christian Boltz +# Copyright (C) 2015-2023 Christian Boltz # # This program is free software; you can redistribute it and/or # modify it under the terms of version 2 of the GNU General Public @@ -27,6 +27,7 @@ _ = init_translation() class aa_tools: def __init__(self, tool_name, args): apparmor.init_aa(profiledir=args.dir, confdir=args.configdir) + apparmor.read_profiles() if not user_perm(apparmor.profile_dir): raise AppArmorException("Cannot write to profile directory: %s" % (apparmor.profile_dir)) @@ -92,9 +93,6 @@ class aa_tools: yield (program, profile) def cleanprof_act(self): - # used by aa-cleanprof - apparmor.read_profiles() - for (program, profile) in self.get_next_to_profile(): if program is None: program = profile @@ -118,8 +116,6 @@ class aa_tools: sys.exit(1) def cmd_disable(self): - apparmor.read_profiles() - for (program, profile) in self.get_next_to_profile(): output_name = profile if program is None else program @@ -134,8 +130,6 @@ class aa_tools: self.unload_profile(profile) def cmd_enforce(self): - apparmor.read_profiles() - for (program, profile) in self.get_next_to_profile(): output_name = profile if program is None else program @@ -149,8 +143,6 @@ class aa_tools: self.reload_profile(profile) def cmd_complain(self): - apparmor.read_profiles() - for (program, profile) in self.get_next_to_profile(): output_name = profile if program is None else program @@ -164,8 +156,6 @@ class aa_tools: self.reload_profile(profile) def cmd_audit(self): - apparmor.read_profiles() - for (program, profile) in self.get_next_to_profile(): output_name = profile if program is None else program @@ -189,7 +179,6 @@ class aa_tools: self.reload_profile(profile) def cmd_autodep(self): - apparmor.read_profiles() apparmor.loadincludes() for (program, profile) in self.get_next_to_profile(): From 918a15e2442b26c6cea024489cb4181470c35871 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Wed, 11 Oct 2023 23:36:11 +0200 Subject: [PATCH 2/8] Merge common parts of mode changes into get_next_for_modechange() --- utils/apparmor/tools.py | 47 ++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index f6dbff545..687577f8e 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -92,6 +92,18 @@ class aa_tools: yield (program, profile) + def get_next_for_modechange(self): + """common code for mode/flags changes""" + + for (program, prof_filename) in self.get_next_to_profile(): + output_name = prof_filename if program is None else program + + if not os.path.isfile(prof_filename) or is_skippable_file(prof_filename): + aaui.UI_Info(_('Profile for %s not found, skipping') % output_name) + continue + + yield (program, prof_filename, output_name) + def cleanprof_act(self): for (program, profile) in self.get_next_to_profile(): if program is None: @@ -116,53 +128,26 @@ class aa_tools: sys.exit(1) def cmd_disable(self): - for (program, profile) in self.get_next_to_profile(): - - output_name = profile if program is None else program - - if not os.path.isfile(profile) or is_skippable_file(profile): - aaui.UI_Info(_('Profile for %s not found, skipping') % output_name) - continue - + for (program, profile, output_name) in self.get_next_for_modechange(): aaui.UI_Info(_('Disabling %s.') % output_name) self.disable_profile(profile) self.unload_profile(profile) def cmd_enforce(self): - for (program, profile) in self.get_next_to_profile(): - - output_name = profile if program is None else program - - if not os.path.isfile(profile) or is_skippable_file(profile): - aaui.UI_Info(_('Profile for %s not found, skipping') % output_name) - continue - + for (program, profile, output_name) in self.get_next_for_modechange(): apparmor.set_enforce(profile, program) self.reload_profile(profile) def cmd_complain(self): - for (program, profile) in self.get_next_to_profile(): - - output_name = profile if program is None else program - - if not os.path.isfile(profile) or is_skippable_file(profile): - aaui.UI_Info(_('Profile for %s not found, skipping') % output_name) - continue - + for (program, profile, output_name) in self.get_next_for_modechange(): apparmor.set_complain(profile, program) self.reload_profile(profile) def cmd_audit(self): - for (program, profile) in self.get_next_to_profile(): - - output_name = profile if program is None else program - - if not os.path.isfile(profile) or is_skippable_file(profile): - aaui.UI_Info(_('Profile for %s not found, skipping') % output_name) - continue + for (program, profile, output_name) in self.get_next_for_modechange(): # keep this to allow toggling 'audit' flags if not self.remove: From 4d1c17b42642757c58bf55c562f80a0e25b1f0e9 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 00:03:12 +0200 Subject: [PATCH 3/8] Drop enable_profile() and disable_profile() enable_profile() was unused. disable_profile() was only used once, inline it into cmd_disable() --- utils/apparmor/tools.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index 687577f8e..c6ece1951 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -130,7 +130,8 @@ class aa_tools: def cmd_disable(self): for (program, profile, output_name) in self.get_next_for_modechange(): aaui.UI_Info(_('Disabling %s.') % output_name) - self.disable_profile(profile) + + apparmor.create_symlink('disable', profile) self.unload_profile(profile) @@ -215,12 +216,6 @@ class aa_tools: else: raise AppArmorException(_('The profile for %s does not exists. Nothing to clean.') % program) - def enable_profile(self, filename): - apparmor.delete_symlink('disable', filename) - - def disable_profile(self, filename): - apparmor.create_symlink('disable', filename) - def unload_profile(self, profile): if not self.do_reload: return From 4f51c93f9dc2516a32bfccc79b4dcf4985e61f47 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 00:31:56 +0200 Subject: [PATCH 4/8] get_next_to_profile(): return profile and prof_filename Before, the 'profile' return value was either a profile name or a profile filename, depending on the active module (cleanprof vs. everything else). Separate the return values so that it's clear what we get. Notes: - This commit doesn't change functionality, only the number of return values and some variable names. - There's no guarantee that all return values are set. They can also be None. (This might change in the future.) Also adjust the callers of get_next_to_profile(), and rename 'profile' to 'prof_filename' in calling functions that actually use the profile filename. --- utils/apparmor/tools.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index c6ece1951..ce120ad6a 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -54,17 +54,20 @@ class aa_tools: program = None profile = None + prof_filename = None + if os.path.exists(p) or p.startswith('/'): fq_path = apparmor.get_full_path(p).strip() if os.path.commonprefix([apparmor.profile_dir, fq_path]) == apparmor.profile_dir: program = None - profile = fq_path + prof_filename = fq_path + profile = None else: program = fq_path if self.name == 'cleanprof': profile = apparmor.active_profiles.profile_from_attachment(fq_path) else: - profile = apparmor.get_profile_filename_from_attachment(fq_path, True) + prof_filename = apparmor.get_profile_filename_from_attachment(fq_path, True) else: which_ = which(p) if self.name == 'cleanprof' and p in apparmor.aa: @@ -75,13 +78,13 @@ class aa_tools: if self.name == 'cleanprof': profile = program else: - profile = apparmor.get_profile_filename_from_attachment(program, True) + prof_filename = apparmor.get_profile_filename_from_attachment(program, True) elif os.path.exists(os.path.join(apparmor.profile_dir, p)): program = None if self.name == 'cleanprof': profile = p else: - profile = apparmor.get_full_path(os.path.join(apparmor.profile_dir, p)).strip() + prof_filename = apparmor.get_full_path(os.path.join(apparmor.profile_dir, p)).strip() else: if '/' not in p: aaui.UI_Info(_("Can't find %(program)s in the system path list. If the name of the application\nis correct, please run 'which %(program)s' as a user with correct PATH\nenvironment set up in order to find the fully-qualified path and\nuse the full path as parameter.") @@ -90,12 +93,12 @@ class aa_tools: aaui.UI_Info(_("%s does not exist, please double-check the path.") % p) continue - yield (program, profile) + yield (program, profile, prof_filename) def get_next_for_modechange(self): """common code for mode/flags changes""" - for (program, prof_filename) in self.get_next_to_profile(): + for (program, _, prof_filename) in self.get_next_to_profile(): output_name = prof_filename if program is None else program if not os.path.isfile(prof_filename) or is_skippable_file(prof_filename): @@ -105,7 +108,7 @@ class aa_tools: yield (program, prof_filename, output_name) def cleanprof_act(self): - for (program, profile) in self.get_next_to_profile(): + for (program, profile, _) in self.get_next_to_profile(): if program is None: program = profile @@ -167,9 +170,9 @@ class aa_tools: def cmd_autodep(self): apparmor.loadincludes() - for (program, profile) in self.get_next_to_profile(): + for (program, _, prof_filename) in self.get_next_to_profile(): if not program: - aaui.UI_Info(_('Please pass an application to generate a profile for, not a profile itself - skipping %s.') % profile) + aaui.UI_Info(_('Please pass an application to generate a profile for, not a profile itself - skipping %s.') % prof_filename) continue apparmor.check_qualifiers(program) From 1d5f90efcd6aa9d58330d45e492e9d64b1242d0b Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 13:04:29 +0200 Subject: [PATCH 5/8] Rename profile variable to prof_filename ... if it contains the profile filename. This avoids confusion with the "real" 'profile' variable that contains a profile name. --- utils/apparmor/tools.py | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index ce120ad6a..b23e76076 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -131,41 +131,41 @@ class aa_tools: sys.exit(1) def cmd_disable(self): - for (program, profile, output_name) in self.get_next_for_modechange(): + for (program, prof_filename, output_name) in self.get_next_for_modechange(): aaui.UI_Info(_('Disabling %s.') % output_name) - apparmor.create_symlink('disable', profile) + apparmor.create_symlink('disable', prof_filename) - self.unload_profile(profile) + self.unload_profile(prof_filename) def cmd_enforce(self): - for (program, profile, output_name) in self.get_next_for_modechange(): - apparmor.set_enforce(profile, program) + for (program, prof_filename, output_name) in self.get_next_for_modechange(): + apparmor.set_enforce(prof_filename, program) - self.reload_profile(profile) + self.reload_profile(prof_filename) def cmd_complain(self): - for (program, profile, output_name) in self.get_next_for_modechange(): - apparmor.set_complain(profile, program) + for (program, prof_filename, output_name) in self.get_next_for_modechange(): + apparmor.set_complain(prof_filename, program) - self.reload_profile(profile) + self.reload_profile(prof_filename) def cmd_audit(self): - for (program, profile, output_name) in self.get_next_for_modechange(): + for (program, prof_filename, output_name) in self.get_next_for_modechange(): # keep this to allow toggling 'audit' flags if not self.remove: aaui.UI_Info(_('Setting %s to audit mode.') % output_name) else: aaui.UI_Info(_('Removing audit mode from %s.') % output_name) - apparmor.change_profile_flags(profile, program, 'audit', not self.remove) + apparmor.change_profile_flags(prof_filename, program, 'audit', not self.remove) - disable_link = '%s/disable/%s' % (apparmor.profile_dir, os.path.basename(profile)) + disable_link = '%s/disable/%s' % (apparmor.profile_dir, os.path.basename(prof_filename)) if os.path.exists(disable_link): - aaui.UI_Info(_('\nWarning: the profile %s is disabled. Use aa-enforce or aa-complain to enable it.') % os.path.basename(profile)) + aaui.UI_Info(_('\nWarning: the profile %s is disabled. Use aa-enforce or aa-complain to enable it.') % os.path.basename(prof_filename)) - self.reload_profile(profile) + self.reload_profile(prof_filename) def cmd_autodep(self): apparmor.loadincludes() @@ -219,18 +219,18 @@ class aa_tools: else: raise AppArmorException(_('The profile for %s does not exists. Nothing to clean.') % program) - def unload_profile(self, profile): + def unload_profile(self, prof_filename): if not self.do_reload: return # FIXME: should ensure profile is loaded before unloading - cmd_info = cmd([apparmor.parser, '-I%s' % apparmor.profile_dir, '--base', apparmor.profile_dir, '-R', profile]) + cmd_info = cmd([apparmor.parser, '-I%s' % apparmor.profile_dir, '--base', apparmor.profile_dir, '-R', prof_filename]) if cmd_info[0] != 0: raise AppArmorException(cmd_info[1]) - def reload_profile(self, profile): + def reload_profile(self, prof_filename): if not self.do_reload: return - apparmor.reload_profile(profile, raise_exc=True) + apparmor.reload_profile(prof_filename, raise_exc=True) From 0c595ac8017a7e1c721306e55e2a6fe8ad3c803e Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 13:08:13 +0200 Subject: [PATCH 6/8] clean_profile(): rename filename to prof_filename ... for consistency with the variable name in all the other functions. --- utils/apparmor/tools.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index b23e76076..3ad181147 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -185,19 +185,20 @@ class aa_tools: apparmor.reload(program) def clean_profile(self, program, profile): - filename = apparmor.get_profile_filename_from_profile_name(profile) import apparmor.cleanprofile as cleanprofile - prof = cleanprofile.Prof(filename) + + prof_filename = apparmor.get_profile_filename_from_profile_name(profile) + prof = cleanprofile.Prof(prof_filename) cleanprof = cleanprofile.CleanProf(True, prof, prof) deleted = cleanprof.remove_duplicate_rules(profile) aaui.UI_Info(_("\nDeleted %s rules.") % deleted) apparmor.changed[profile] = True - if filename: + if prof_filename: if not self.silent: q = aaui.PromptQuestion() q.title = 'Changed Local Profiles' - q.explanation = _('The local profile for %(program)s in file %(file)s was changed. Would you like to save it?') % {'program': program, 'file': filename} + q.explanation = _('The local profile for %(program)s in file %(file)s was changed. Would you like to save it?') % {'program': program, 'file': prof_filename} q.functions = ['CMD_SAVE_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ABORT'] q.default = 'CMD_VIEW_CHANGES' q.options = [] @@ -208,14 +209,14 @@ class aa_tools: ans, arg = q.promptUser() if ans == 'CMD_SAVE_CHANGES': apparmor.write_profile_ui_feedback(profile) - self.reload_profile(filename) + self.reload_profile(prof_filename) elif ans == 'CMD_VIEW_CHANGES': # oldprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.original_aa), profile, {}) newprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.aa), profile, {}) # , {'is_attachment': True}) - aaui.UI_Changes(filename, newprofile, comments=True) + aaui.UI_Changes(prof_filename, newprofile, comments=True) else: apparmor.write_profile_ui_feedback(profile, True) - self.reload_profile(filename) + self.reload_profile(prof_filename) else: raise AppArmorException(_('The profile for %s does not exists. Nothing to clean.') % program) From 5d8347bc2684e28600ad09ef277534881ffa280f Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 13:11:30 +0200 Subject: [PATCH 7/8] clean_profile(): re-order code Error out early (avoids a tab level), and handle the short branch first in the if condition. --- utils/apparmor/tools.py | 48 ++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index 3ad181147..ec5e8e195 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -194,32 +194,32 @@ class aa_tools: aaui.UI_Info(_("\nDeleted %s rules.") % deleted) apparmor.changed[profile] = True - if prof_filename: - if not self.silent: - q = aaui.PromptQuestion() - q.title = 'Changed Local Profiles' - q.explanation = _('The local profile for %(program)s in file %(file)s was changed. Would you like to save it?') % {'program': program, 'file': prof_filename} - q.functions = ['CMD_SAVE_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ABORT'] - q.default = 'CMD_VIEW_CHANGES' - q.options = [] - q.selected = 0 - ans = '' - arg = None - while ans != 'CMD_SAVE_CHANGES': - ans, arg = q.promptUser() - if ans == 'CMD_SAVE_CHANGES': - apparmor.write_profile_ui_feedback(profile) - self.reload_profile(prof_filename) - elif ans == 'CMD_VIEW_CHANGES': - # oldprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.original_aa), profile, {}) - newprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.aa), profile, {}) # , {'is_attachment': True}) - aaui.UI_Changes(prof_filename, newprofile, comments=True) - else: - apparmor.write_profile_ui_feedback(profile, True) - self.reload_profile(prof_filename) - else: + if not prof_filename: raise AppArmorException(_('The profile for %s does not exists. Nothing to clean.') % program) + if self.silent: + apparmor.write_profile_ui_feedback(profile, True) + self.reload_profile(prof_filename) + else: + q = aaui.PromptQuestion() + q.title = 'Changed Local Profiles' + q.explanation = _('The local profile for %(program)s in file %(file)s was changed. Would you like to save it?') % {'program': program, 'file': prof_filename} + q.functions = ['CMD_SAVE_CHANGES', 'CMD_VIEW_CHANGES', 'CMD_ABORT'] + q.default = 'CMD_VIEW_CHANGES' + q.options = [] + q.selected = 0 + ans = '' + arg = None + while ans != 'CMD_SAVE_CHANGES': + ans, arg = q.promptUser() + if ans == 'CMD_SAVE_CHANGES': + apparmor.write_profile_ui_feedback(profile) + self.reload_profile(prof_filename) + elif ans == 'CMD_VIEW_CHANGES': + # oldprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.original_aa), profile, {}) + newprofile = apparmor.serialize_profile(apparmor.split_to_merged(apparmor.aa), profile, {}) # , {'is_attachment': True}) + aaui.UI_Changes(prof_filename, newprofile, comments=True) + def unload_profile(self, prof_filename): if not self.do_reload: return From f2f24884c361c18d354d663565f230dae5c7daaa Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 12 Oct 2023 13:37:18 +0200 Subject: [PATCH 8/8] get_next_to_profile(): ensure all branches set all variables This also means we can get rid of most cleanprof-specific conditions without changing the behaviour (because the other functions don't use 'profile' yet). Also hand over prof_filename to clean_profile() so that it doesn't need to find it out itsself. --- utils/apparmor/tools.py | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/utils/apparmor/tools.py b/utils/apparmor/tools.py index ec5e8e195..e8a99bbe6 100644 --- a/utils/apparmor/tools.py +++ b/utils/apparmor/tools.py @@ -52,10 +52,6 @@ class aa_tools: if not p: continue - program = None - profile = None - prof_filename = None - if os.path.exists(p) or p.startswith('/'): fq_path = apparmor.get_full_path(p).strip() if os.path.commonprefix([apparmor.profile_dir, fq_path]) == apparmor.profile_dir: @@ -64,33 +60,29 @@ class aa_tools: profile = None else: program = fq_path - if self.name == 'cleanprof': - profile = apparmor.active_profiles.profile_from_attachment(fq_path) - else: - prof_filename = apparmor.get_profile_filename_from_attachment(fq_path, True) + profile = apparmor.active_profiles.profile_from_attachment(fq_path) + prof_filename = apparmor.get_profile_filename_from_attachment(fq_path, True) else: which_ = which(p) if self.name == 'cleanprof' and p in apparmor.aa: program = p # not really correct, but works profile = p + prof_filename = apparmor.get_profile_filename_from_profile_name(profile) elif which_ is not None: program = apparmor.get_full_path(which_) - if self.name == 'cleanprof': - profile = program - else: - prof_filename = apparmor.get_profile_filename_from_attachment(program, True) + profile = program + prof_filename = apparmor.get_profile_filename_from_attachment(program, True) elif os.path.exists(os.path.join(apparmor.profile_dir, p)): program = None - if self.name == 'cleanprof': - profile = p - else: - prof_filename = apparmor.get_full_path(os.path.join(apparmor.profile_dir, p)).strip() + profile = p + prof_filename = apparmor.get_full_path(os.path.join(apparmor.profile_dir, p)).strip() else: if '/' not in p: aaui.UI_Info(_("Can't find %(program)s in the system path list. If the name of the application\nis correct, please run 'which %(program)s' as a user with correct PATH\nenvironment set up in order to find the fully-qualified path and\nuse the full path as parameter.") % {'program': p}) else: aaui.UI_Info(_("%s does not exist, please double-check the path.") % p) + continue yield (program, profile, prof_filename) @@ -108,7 +100,7 @@ class aa_tools: yield (program, prof_filename, output_name) def cleanprof_act(self): - for (program, profile, _) in self.get_next_to_profile(): + for (program, profile, prof_filename) in self.get_next_to_profile(): if program is None: program = profile @@ -120,7 +112,7 @@ class aa_tools: sys.exit(1) if program and profile in apparmor.aa: - self.clean_profile(program, profile) + self.clean_profile(program, profile, prof_filename) else: if '/' not in program: @@ -184,10 +176,9 @@ class aa_tools: if self.aa_mountpoint: apparmor.reload(program) - def clean_profile(self, program, profile): + def clean_profile(self, program, profile, prof_filename): import apparmor.cleanprofile as cleanprofile - prof_filename = apparmor.get_profile_filename_from_profile_name(profile) prof = cleanprofile.Prof(prof_filename) cleanprof = cleanprofile.CleanProf(True, prof, prof) deleted = cleanprof.remove_duplicate_rules(profile)