From 23af115fa54a9c8cbebbfc695d04824fab3feef3 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Wed, 13 May 2020 21:45:00 +0200 Subject: [PATCH 01/19] store include rules (also) in 'inc_ie' (IncludeRueset) ... and write them (only) from 'inc_ie' (IncludeRuleset), which can handle both "include" and "include if exists" rules. This duplicates storage of include rules because 'include' is still used and needed at various places that work on/with the include rules. With this, we get removal of duplicate include lines insinde a profile in aa-cleanprof "for free" - extend cleanprof_test.in to confirm this. --- utils/apparmor/aa.py | 18 ++++++++++++++++-- utils/apparmor/profile_storage.py | 16 +--------------- utils/test/cleanprof_test.in | 2 ++ utils/test/cleanprof_test.out | 6 ++---- utils/test/test-aa.py | 19 +++++++++++++++++++ 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 46520cd6b..669b76c9c 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -50,7 +50,7 @@ from apparmor.regex import (RE_PROFILE_START, RE_PROFILE_END, from apparmor.profile_list import ProfileList from apparmor.profile_storage import (ProfileStorage, add_or_remove_flag, ruletypes, - write_includes, write_list_vars ) + write_list_vars ) import apparmor.rules as aarules @@ -443,6 +443,8 @@ def create_new_profile(localfile, is_stub=False): local_profile[localfile] = ProfileStorage('NEW', localfile, 'create_new_profile()') local_profile[localfile]['flags'] = 'complain' local_profile[localfile]['include']['abstractions/base'] = 1 + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + local_profile[localfile]['inc_ie'].add(IncludeRule('abstractions/base', False, True)) if os.path.exists(localfile) and os.path.isfile(localfile): interpreter_path, abstraction = get_interpreter_and_abstraction(localfile) @@ -453,6 +455,8 @@ def create_new_profile(localfile, is_stub=False): if abstraction: local_profile[localfile]['include'][abstraction] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + local_profile[localfile]['inc_ie'].add(IncludeRule(abstraction, False, True)) handle_binfmt(local_profile[localfile], interpreter_path) else: @@ -571,6 +575,8 @@ def autodep(bin_name, pname=''): if not filelist.get(file, False): filelist[file] = hasher() filelist[file]['include']['tunables/global'] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + active_profiles.add_inc_ie(file, IncludeRule('tunables/global', False, True)) write_profile_ui_feedback(pname) def get_profile_flags(filename, program): @@ -946,6 +952,8 @@ def ask_exec(hashlog): if abstraction: aa[profile][hat]['include'][abstraction] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + aa[profile][hat]['inc_ie'].add(IncludeRule(abstraction, False, True)) handle_binfmt(aa[profile][hat], interpreter_path) @@ -1223,6 +1231,8 @@ def ask_rule_questions(prof_events, profile_name, the_profile, r_types): deleted = delete_all_duplicates(the_profile, inc, r_types) the_profile['include'][inc] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + the_profile['inc_ie'].add(IncludeRule.parse(selection)) aaui.UI_Info(_('Adding %s to profile.') % selection) if deleted: @@ -1955,10 +1965,15 @@ def parse_profile_data(data, file, do_include): if profile: profile_data[profile][hat]['include'][include_name] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + profile_data[profile][hat]['inc_ie'].add(IncludeRule.parse(line)) else: if not filelist.get(file): filelist[file] = hasher() filelist[file]['include'][include_name] = True + # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + active_profiles.add_inc_ie(file, IncludeRule.parse(line)) + # If include is a directory if os.path.isdir(get_include_path(include_name)): for file_name in include_dir_filelist(profile_dir, include_name): @@ -2298,7 +2313,6 @@ def serialize_profile(profile_data, name, options): if filelist.get(prof_filename, False): data += active_profiles.get_clean_first(prof_filename, 0) data += write_list_vars(filelist[prof_filename], 0) - data += write_includes(filelist[prof_filename], 0) data += active_profiles.get_clean(prof_filename, 0) diff --git a/utils/apparmor/profile_storage.py b/utils/apparmor/profile_storage.py index 94051bccb..dd8fced15 100644 --- a/utils/apparmor/profile_storage.py +++ b/utils/apparmor/profile_storage.py @@ -219,21 +219,7 @@ def write_list_vars(ref, depth): return data def write_includes(prof_data, depth): - pre = ' ' * depth - data = [] - - for key in sorted(prof_data['include'].keys()): - if key.startswith('/'): - qkey = '"%s"' % key - else: - qkey = '<%s>' % quote_if_needed(key) - - data.append('%s#include %s' % (pre, qkey)) - - if data: - data.append('') - - return data + return [] # now handled via 'inc_ie' / IncludeRulset def var_transform(ref): data = [] diff --git a/utils/test/cleanprof_test.in b/utils/test/cleanprof_test.in index 028e8a0c3..4ea34a0e5 100644 --- a/utils/test/cleanprof_test.in +++ b/utils/test/cleanprof_test.in @@ -17,6 +17,8 @@ #include #include if exists + #include if exists + include capability sys_admin, audit capability, diff --git a/utils/test/cleanprof_test.out b/utils/test/cleanprof_test.out index b990c355f..394496191 100644 --- a/utils/test/cleanprof_test.out +++ b/utils/test/cleanprof_test.out @@ -5,8 +5,7 @@ alias /foo -> /bar, @{asdf} = "" foo @{xy} = x y -#include - +include include if exists # A simple test comment which will persist @@ -15,8 +14,7 @@ include if exists /usr/bin/a/simple/cleanprof/test/profile { abi "abi/4.20", - #include - + include include if exists set rlimit nofile <= 256, diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index ee3302c6c..d412e3977 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -25,6 +25,7 @@ from apparmor.aa import (check_for_apparmor, get_output, get_reqs, get_interpret from apparmor.aare import AARE from apparmor.common import AppArmorException, AppArmorBug from apparmor.rule.file import FileRule +from apparmor.rule.include import IncludeRule class AaTestWithTempdir(AATest): def AASetup(self): @@ -150,8 +151,12 @@ class AaTest_create_new_profile(AATest): if exp_abstraction: self.assertEqual(set(profile[program][program]['include'].keys()), {exp_abstraction, 'abstractions/base'}) + # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + self.assertEqual(profile[program][program]['inc_ie'].get_clean(), ['include ', 'include <%s>' % exp_abstraction, '']) else: self.assertEqual(set(profile[program][program]['include'].keys()), {'abstractions/base'}) + # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + self.assertEqual(profile[program][program]['inc_ie'].get_clean(), ['include ', '']) class AaTest_get_interpreter_and_abstraction(AATest): tests = [ @@ -832,6 +837,10 @@ class AaTest_get_file_perms_2(AATest): profile['include']['abstractions/base'] = True profile['include']['abstractions/bash'] = True profile['include']['abstractions/enchant'] = True # includes abstractions/aspell + # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['inc_ie'].add(IncludeRule.parse('include ')) profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/** w,')) profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/what/ever a,')) # covered by the above 'w' rule, so 'a' should be ignored @@ -874,6 +883,11 @@ class AaTest_propose_file_rules(AATest): profile['include']['abstractions/base'] = True profile['include']['abstractions/bash'] = True profile['include']['abstractions/enchant'] = True # includes abstractions/aspell + # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/** w,')) profile['file'].add(FileRule.parse('/dev/null rwk,')) @@ -919,6 +933,11 @@ class AaTest_propose_file_rules_with_absolute_includes(AATest): profile['include'][abs_include1] = False profile['include'][abs_include2] = False profile['include'][abs_include3] = False + # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. + profile['inc_ie'].add(IncludeRule.parse('include ')) + profile['inc_ie'].add(IncludeRule.parse('include "%s"' % abs_include1)) + profile['inc_ie'].add(IncludeRule.parse('include "%s"' % abs_include2)) + profile['inc_ie'].add(IncludeRule.parse('include "%s"' % abs_include3)) rule_obj = FileRule(params[0], params[1], None, FileRule.ALL, owner=False, log_event=True) proposals = propose_file_rules(profile, rule_obj) From 0cf15d54b64c7f856944b34dbab9546120bd734f Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Wed, 13 May 2020 22:38:01 +0200 Subject: [PATCH 02/19] aa-mergeprof: drop separate code asking for include rules ... because this is now done via IncludeRule, and keeping the separate code would mean asking twice. Note that the user interface changes slightly. The old workflow was 1 - #include 2 - #include 3 - #include [select 2 and (A)dd, then get the next question for the remainder] 1 - #include 2 - #include [(A)dd another one, or (I)gnore the remainder] The new workflow will ask separately for each abstraction, and you can (A)dd or (I)gnore it. This is probably easier to understand because it's basically a yes/no question. --- utils/aa-mergeprof | 33 --------------------------------- utils/apparmor/aa.py | 36 ------------------------------------ 2 files changed, 69 deletions(-) diff --git a/utils/aa-mergeprof b/utils/aa-mergeprof index f606c3e78..c4cc3d62d 100755 --- a/utils/aa-mergeprof +++ b/utils/aa-mergeprof @@ -23,7 +23,6 @@ import apparmor.cleanprofile as cleanprofile import apparmor.ui as aaui from apparmor.common import AppArmorException -from apparmor.regex import re_match_include # setup exception handling @@ -130,38 +129,6 @@ class Merge(object): log_dict = {'merge': other.aa} apparmor.aa.loadincludes() - done = False - - #Add the file-wide includes from the other profile to the user profile - options = [] - for inc in other.filelist[other.filename]['include'].keys(): - if not inc in self.user.filelist[self.user.filename]['include'].keys(): - if inc.startswith('/'): - options.append('#include "%s"' %inc) - else: - options.append('#include <%s>' %inc) - - default_option = 1 - - q = aaui.PromptQuestion() - q.options = options - q.selected = default_option - 1 - q.headers = [_('File includes'), _('Select the ones you wish to add')] - q.functions = ['CMD_ALLOW', 'CMD_IGNORE_ENTRY', 'CMD_ABORT', 'CMD_FINISHED'] - q.default = 'CMD_ALLOW' - - while not done and options: - ans, selected = q.promptUser() - if ans == 'CMD_IGNORE_ENTRY': - done = True - elif ans == 'CMD_ALLOW': - selection = options[selected] - inc = re_match_include(selection) - self.user.filelist[self.user.filename]['include'][inc] = True - options.pop(selected) - aaui.UI_Info(_('Adding %s to the file.') % selection) - elif ans == 'CMD_FINISHED': - return if not apparmor.aa.sev_db: apparmor.aa.sev_db = apparmor.severity.Severity(apparmor.aa.CONFDIR + '/severity.db', _('unknown')) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 669b76c9c..bf081af5b 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1092,42 +1092,6 @@ def ask_the_questions(log_dict): aa[profile][hat] = ProfileStorage(profile, hat, 'mergeprof ask_the_questions() - missing hat') aa[profile][hat]['profile'] = False - #Add the includes from the other profile to the user profile - done = False - - options = [] - for inc in log_dict[aamode][profile][hat]['include'].keys(): - if not inc in aa[profile][hat]['include'].keys(): - if inc.startswith('/'): - options.append('#include "%s"' %inc) - else: - options.append('#include <%s>' %inc) - - default_option = 1 - - q = aaui.PromptQuestion() - q.options = options - q.selected = default_option - 1 - q.headers = [_('File includes'), _('Select the ones you wish to add')] - q.functions = ['CMD_ALLOW', 'CMD_IGNORE_ENTRY', 'CMD_ABORT', 'CMD_FINISHED'] - q.default = 'CMD_ALLOW' - - while not done and options: - ans, selected = q.promptUser() - if ans == 'CMD_IGNORE_ENTRY': - done = True - elif ans == 'CMD_ALLOW': - selection = options[selected] - inc = re_match_include(selection) - deleted = delete_all_duplicates(aa[profile][hat], inc, ruletypes) - aa[profile][hat]['include'][inc] = True - options.pop(selected) - aaui.UI_Info(_('Adding %s to the file.') % selection) - if deleted: - aaui.UI_Info(_('Deleted %s previous matching profile entries.') % deleted) - elif ans == 'CMD_FINISHED': - return - # check for and ask about conflicting exec modes ask_conflict_mode(profile, hat, aa[profile][hat], log_dict[aamode][profile][hat]) From 3f0f7154f7e1b8c0db52a014c91767a56d59b670 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 14 May 2020 21:56:44 +0200 Subject: [PATCH 03/19] Let aa-cleanprof remove duplicate preamble rules Technically, this is done in the new function delete_preamble_duplicates() in ProfileList. Also add some tests to ensure this works as expected. --- utils/apparmor/cleanprofile.py | 8 +++++++- utils/apparmor/profile_list.py | 13 +++++++++++++ utils/test/cleanprof_test.in | 2 ++ utils/test/test-profile-list.py | 27 +++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/utils/apparmor/cleanprofile.py b/utils/apparmor/cleanprofile.py index 7414b772d..8a147544f 100644 --- a/utils/apparmor/cleanprofile.py +++ b/utils/apparmor/cleanprofile.py @@ -46,9 +46,15 @@ class CleanProf(object): def remove_duplicate_rules(self, program): #Process the profile of the program + + deleted = 0 + + # remove duplicate rules from the preamble + deleted += self.profile.active_profiles.delete_preamble_duplicates(self.profile.filename) + #Process every hat in the profile individually file_includes = list(self.profile.filelist[self.profile.filename]['include'].keys()) - deleted = 0 + for hat in sorted(self.profile.aa[program].keys()): #The combined list of includes from profile and the file includes = list(self.profile.aa[program][hat]['include'].keys()) + file_includes diff --git a/utils/apparmor/profile_list.py b/utils/apparmor/profile_list.py index 11266f538..e34949761 100644 --- a/utils/apparmor/profile_list.py +++ b/utils/apparmor/profile_list.py @@ -116,6 +116,19 @@ class ProfileList: self.files[filename]['inc_ie'].add(inc_rule) + def delete_preamble_duplicates(self, filename): + ''' Delete duplicates in the preamble of the given profile file ''' + + if not self.files.get(filename): + raise AppArmorBug('%s not listed in ProfileList files' % filename) + + deleted = 0 + + for r_type in ['abi', 'inc_ie']: # TODO: don't hardcode + deleted += self.files[filename][r_type].delete_duplicates(None) # None means not to check includes -- TODO check if this makes sense for all preamble rule types + + return deleted + def get_raw(self, filename, depth=0): ''' Get the preamble for the given profile filename (in original formatting) ''' if not self.files.get(filename): diff --git a/utils/test/cleanprof_test.in b/utils/test/cleanprof_test.in index 4ea34a0e5..593c0b179 100644 --- a/utils/test/cleanprof_test.in +++ b/utils/test/cleanprof_test.in @@ -3,6 +3,8 @@ #include if exists + #include if exists + include if exists alias /foo -> /bar , diff --git a/utils/test/test-profile-list.py b/utils/test/test-profile-list.py index 9b8e2942c..a404a7b3f 100644 --- a/utils/test/test-profile-list.py +++ b/utils/test/test-profile-list.py @@ -149,6 +149,21 @@ class TestAdd_inc_ie(AATest): self.pl.add_inc_ie('/etc/apparmor.d/bin.foo', 'tunables/global') # str insteadd of IncludeRule self.assertEqual(list(self.pl.files.keys()), []) + def test_dedup_inc_ie_1(self): + self.pl.add_inc_ie('/etc/apparmor.d/bin.foo', IncludeRule.parse('include ')) + self.pl.add_inc_ie('/etc/apparmor.d/bin.foo', IncludeRule.parse('#include if exists # comment')) + self.pl.add_inc_ie('/etc/apparmor.d/bin.foo', IncludeRule.parse(' #include ')) + deleted = self.pl.delete_preamble_duplicates('/etc/apparmor.d/bin.foo') + self.assertEqual(deleted, 2) + self.assertEqual(list(self.pl.files.keys()), ['/etc/apparmor.d/bin.foo']) + self.assertEqual(self.pl.get_clean('/etc/apparmor.d/bin.foo'), ['include ', '']) + self.assertEqual(self.pl.get_raw('/etc/apparmor.d/bin.foo'), ['include ', '']) + + def test_dedup_error_1(self): + with self.assertRaises(AppArmorBug): + self.pl.delete_preamble_duplicates('/file/not/found') + self.assertEqual(list(self.pl.files.keys()), []) + class TestAdd_abi(AATest): def AASetup(self): self.pl = ProfileList() @@ -173,6 +188,15 @@ class TestAdd_abi(AATest): self.pl.add_abi('/etc/apparmor.d/bin.foo', 'abi/4.19') # str insteadd of AbiRule self.assertEqual(list(self.pl.files.keys()), []) + def test_dedup_abi_1(self): + self.pl.add_abi('/etc/apparmor.d/bin.foo', AbiRule.parse('abi ,')) + self.pl.add_abi('/etc/apparmor.d/bin.foo', AbiRule.parse(' abi , # comment')) + self.assertEqual(list(self.pl.files.keys()), ['/etc/apparmor.d/bin.foo']) + deleted = self.pl.delete_preamble_duplicates('/etc/apparmor.d/bin.foo') + self.assertEqual(deleted, 1) + self.assertEqual(self.pl.get_clean_first('/etc/apparmor.d/bin.foo'), ['abi ,', '']) # TODO switch to get_clean() once merged + self.assertEqual(self.pl.get_raw('/etc/apparmor.d/bin.foo'), ['abi ,', '']) + class TestAdd_alias(AATest): def AASetup(self): self.pl = ProfileList() @@ -210,6 +234,9 @@ class TestAdd_alias(AATest): self.pl.add_alias('/etc/apparmor.d/bin.foo', '/foo', None) # target None insteadd of str self.assertEqual(list(self.pl.files.keys()), []) +# def test_dedup_alias_1(self): +# TODO: implement after fixing alias handling (when a profile has two aliases with the same path on the left side) + class TestGet(AATest): def AASetup(self): self.pl = ProfileList() From d799195e0ac93dd7b9cc1974d5b4ec1010b10d6f Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 16 May 2020 21:59:33 +0200 Subject: [PATCH 04/19] move is_skippable_file() from aa.py to common.py IncludeRuleset needs to import it (next commit), which is impossible when it lives in aa.py (would cause recursive import loop because aa.py imports IncludeRuleset) --- utils/apparmor/aa.py | 18 +----------------- utils/apparmor/common.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index bf081af5b..fdf4070b5 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -32,7 +32,7 @@ from copy import deepcopy from apparmor.aare import AARE -from apparmor.common import (AppArmorException, AppArmorBug, open_file_read, valid_path, hasher, +from apparmor.common import (AppArmorException, AppArmorBug, is_skippable_file, open_file_read, valid_path, hasher, split_name, open_file_write, DebugLogger) import apparmor.ui as aaui @@ -1625,22 +1625,6 @@ def collapse_log(hashlog, ignore_null_profiles=True): return log_dict -def is_skippable_file(path): - """Returns True if filename matches something to be skipped (rpm or dpkg backup files, hidden files etc.) - The list of skippable files needs to be synced with apparmor initscript and libapparmor _aa_is_blacklisted() - path: filename (with or without directory)""" - - basename = os.path.basename(path) - - if not basename or basename[0] == '.' or basename == 'README': - return True - - skippable_suffix = ('.dpkg-new', '.dpkg-old', '.dpkg-dist', '.dpkg-bak', '.dpkg-remove', '.pacsave', '.pacnew', '.rpmnew', '.rpmsave', '.orig', '.rej', '~') - if basename.endswith(skippable_suffix): - return True - - return False - def is_skippable_dir(path): if re.search('^(.*/)?(disable|cache|cache\.d|force-complain|lxc|\.git)/?$', path): return True diff --git a/utils/apparmor/common.py b/utils/apparmor/common.py index 1091e1999..bbe28343b 100644 --- a/utils/apparmor/common.py +++ b/utils/apparmor/common.py @@ -172,6 +172,22 @@ def get_directory_contents(path): files.sort() return files +def is_skippable_file(path): + """Returns True if filename matches something to be skipped (rpm or dpkg backup files, hidden files etc.) + The list of skippable files needs to be synced with apparmor initscript and libapparmor _aa_is_blacklisted() + path: filename (with or without directory)""" + + basename = os.path.basename(path) + + if not basename or basename[0] == '.' or basename == 'README': + return True + + skippable_suffix = ('.dpkg-new', '.dpkg-old', '.dpkg-dist', '.dpkg-bak', '.dpkg-remove', '.pacsave', '.pacnew', '.rpmnew', '.rpmsave', '.orig', '.rej', '~') + if basename.endswith(skippable_suffix): + return True + + return False + def open_file_read(path, encoding='UTF-8'): '''Open specified file read-only''' return open_file_anymode('r', path, encoding) From f3f597ff0b6c06d20e9d5350d2535adcdf0b3b4c Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 16 May 2020 22:36:05 +0200 Subject: [PATCH 05/19] IncludeRule: add get_full_paths() This function returns a list of paths for an include rule. This can be - a single path if the include file is a file and exists - a single path if the include file doesn't exist, but doesn't have 'if exists' (this will cause a 'file not found' error when used) - a list of paths if the include is a directory - an empty list if the rule has 'if exists' and the file doesn't exist Also add some tests for get_full_paths() --- utils/apparmor/rule/include.py | 31 +++++++++++++++++++++++- utils/test/test-include.py | 44 +++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/utils/apparmor/rule/include.py b/utils/apparmor/rule/include.py index 44b1adb1e..448cfb139 100644 --- a/utils/apparmor/rule/include.py +++ b/utils/apparmor/rule/include.py @@ -13,8 +13,9 @@ # ---------------------------------------------------------------------- from apparmor.regex import RE_INCLUDE, re_match_include_parse -from apparmor.common import AppArmorBug, AppArmorException, type_is_str +from apparmor.common import AppArmorBug, AppArmorException, is_skippable_file, type_is_str from apparmor.rule import BaseRule, BaseRuleset, parse_comment +import os # setup module translations from apparmor.translations import init_translation @@ -124,6 +125,34 @@ class IncludeRule(BaseRule): _('Include'), self.get_clean(), ] + def get_full_paths(self, profile_dir): + ''' get list of full paths of an include (can contain multiple paths if self.path is a directory) ''' + + # currently this section is based on aa.py get_include_path() (with variable names changed) + # TODO: improve/fix logic to honor magic vs. quoted include paths + if self.path.startswith('/'): + full_path = self.path + else: + full_path = os.path.join(profile_dir, self.path) + + files = [] + + if os.path.isdir(full_path): + for path in os.listdir(full_path): + if is_skippable_file(path): + continue + + file_name = os.path.join(full_path, path) + if os.path.isfile(file_name): # only add files, but not subdirectories etc. + files.append(file_name) + + elif os.path.exists(full_path): + files.append(full_path) + + elif self.ifexists == False: + files.append(full_path) # add full_path even if it doesn't exist on disk. Might cause a 'file not found' error later. + + return files class IncludeRuleset(BaseRuleset): '''Class to handle and store a collection of include rules''' diff --git a/utils/test/test-include.py b/utils/test/test-include.py index 036ae1238..ba2b58ea3 100644 --- a/utils/test/test-include.py +++ b/utils/test/test-include.py @@ -15,7 +15,10 @@ import unittest from collections import namedtuple -from common_test import AATest, setup_all_loops +from common_test import AATest, setup_all_loops, write_file + +import os +import shutil from apparmor.rule.include import IncludeRule, IncludeRuleset #from apparmor.rule import BaseRule @@ -336,6 +339,45 @@ class IncludeLogprofHeaderTest(AATest): obj = IncludeRule._parse(params) self.assertEqual(obj.logprof_header(), expected) +class IncludeFullPathsTest(AATest): + def AASetup(self): + self.createTmpdir() + + #copy the local profiles to the test directory + self.profile_dir = '%s/profiles' % self.tmpdir + shutil.copytree('../../profiles/apparmor.d/', self.profile_dir, symlinks=True) + + inc_dir = os.path.join(self.profile_dir, 'abstractions/inc.d') + os.mkdir(inc_dir, 0o755) + write_file(inc_dir, 'incfoo', '/incfoo r,') + write_file(inc_dir, 'incbar', '/incbar r,') + write_file(inc_dir, 'README', '# README') # gets skipped + + sub_dir = os.path.join(self.profile_dir, 'abstractions/inc.d/subdir') # gets skipped + os.mkdir(sub_dir, 0o755) + + empty_dir = os.path.join(self.profile_dir, 'abstractions/empty.d') + os.mkdir(empty_dir, 0o755) + + tests = [ + # @@ will be replaced with self.profile_dir + ('include ', ['@@/abstractions/base'] ), +# ('include "foo"', ['@@/foo'] ), # TODO: adjust logic to honor quoted vs. magic paths (and allow quoted relative paths in re_match_include_parse()) + ('include "/foo/bar"', ['/foo/bar'] ), + ('include ', ['@@/abstractions/inc.d/incfoo', '@@/abstractions/inc.d/incbar'] ), + ('include ', [] ), + ('include ', ['@@/abstractions/not_found'] ), + ('include if exists ', [] ), + ] + + def _run_test(self, params, expected): + exp2 = [] + for path in expected: + exp2.append(path.replace('@@', self.profile_dir)) + + obj = IncludeRule._parse(params) + self.assertEqual(obj.get_full_paths(self.profile_dir), exp2) + ## --- tests for IncludeRuleset --- # class IncludeRulesTest(AATest): From 057c916032e5e52093b4531dfbebeb75e52c2361 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 13:53:19 +0200 Subject: [PATCH 06/19] parse_profile_data(): call load_include() for IncludeRule rule_obj.get_full_paths() handles directories and non-existing files (depending on 'if exists'), so we can simply feed the resulting list into load_include() --- utils/apparmor/aa.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index fdf4070b5..7d2247ec0 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1933,10 +1933,18 @@ def parse_profile_data(data, file, do_include): # IncludeRule can handle 'include' and 'include if exists' - place it after the "old" 'include' handling so that it only catches 'include if exists' for now elif IncludeRule.match(line): + rule_obj = IncludeRule.parse(line) if profile: - profile_data[profile][hat]['inc_ie'].add(IncludeRule.parse(line)) + profile_data[profile][hat]['inc_ie'].add(rule_obj) else: - active_profiles.add_inc_ie(file, IncludeRule.parse(line)) + active_profiles.add_inc_ie(file, rule_obj) + + for incname in rule_obj.get_full_paths(profile_dir): + # include[] keys can be a) 'abstractions/foo' and b) '/full/path' + if incname.startswith(profile_dir): + incname = incname.replace('%s/' % profile_dir, '') + + load_include(incname) elif NetworkRule.match(line): if not profile: From 0cfef21713cdf331f65e761d2e2d16acee5b3be6 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 16 May 2020 23:24:11 +0200 Subject: [PATCH 07/19] IncludeRuleset: add get_all_full_paths() This function returns a list of full paths of all includes. Also add some tests. --- utils/apparmor/rule/include.py | 10 +++++++++- utils/test/test-include.py | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/utils/apparmor/rule/include.py b/utils/apparmor/rule/include.py index 448cfb139..f7736b115 100644 --- a/utils/apparmor/rule/include.py +++ b/utils/apparmor/rule/include.py @@ -156,4 +156,12 @@ class IncludeRule(BaseRule): class IncludeRuleset(BaseRuleset): '''Class to handle and store a collection of include rules''' - pass + + def get_all_full_paths(self, profile_dir): + ''' get full path of all includes ''' + + paths = [] + for rule_obj in self.rules: + paths += rule_obj.get_full_paths(profile_dir) + + return paths diff --git a/utils/test/test-include.py b/utils/test/test-include.py index ba2b58ea3..3d7e67753 100644 --- a/utils/test/test-include.py +++ b/utils/test/test-include.py @@ -381,6 +381,15 @@ class IncludeFullPathsTest(AATest): ## --- tests for IncludeRuleset --- # class IncludeRulesTest(AATest): + def AASetup(self): + self.createTmpdir() + + #copy the local profiles to the test directory + self.profile_dir = '%s/profiles' % self.tmpdir + shutil.copytree('../../profiles/apparmor.d/', self.profile_dir, symlinks=True) + + write_file(self.profile_dir, 'baz', '/baz r,') + def test_empty_ruleset(self): ruleset = IncludeRuleset() ruleset_2 = IncludeRuleset() @@ -389,6 +398,7 @@ class IncludeRulesTest(AATest): self.assertEqual([], ruleset_2.get_raw(2)) self.assertEqual([], ruleset_2.get_clean(2)) self.assertEqual([], ruleset_2.get_clean_unsorted(2)) + self.assertEqual([], ruleset.get_all_full_paths(self.profile_dir)) def test_ruleset_1(self): ruleset = IncludeRuleset() @@ -415,12 +425,18 @@ class IncludeRulesTest(AATest): '', ] + expected_fullpaths = [ + os.path.join(self.profile_dir, 'foo'), + '/bar' + ] + for rule in rules: ruleset.add(IncludeRule.parse(rule)) self.assertEqual(expected_raw, ruleset.get_raw()) self.assertEqual(expected_clean, ruleset.get_clean()) self.assertEqual(expected_clean_unsorted, ruleset.get_clean_unsorted()) + self.assertEqual(expected_fullpaths, ruleset.get_all_full_paths(self.profile_dir)) def test_ruleset_2(self): ruleset = IncludeRuleset() @@ -455,12 +471,19 @@ class IncludeRulesTest(AATest): '', ] + expected_fullpaths = [ + os.path.join(self.profile_dir, 'baz'), + os.path.join(self.profile_dir, 'foo'), + '/bar', + ] + for rule in rules: ruleset.add(IncludeRule.parse(rule)) self.assertEqual(expected_raw, ruleset.get_raw()) self.assertEqual(expected_clean, ruleset.get_clean()) self.assertEqual(expected_clean_unsorted, ruleset.get_clean_unsorted()) + self.assertEqual(expected_fullpaths, ruleset.get_all_full_paths(self.profile_dir)) class IncludeGlobTestAATest(AATest): def setUp(self): From fa67815698cfed6f3a88111250bb2c54d94a676e Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 16 May 2020 23:49:07 +0200 Subject: [PATCH 08/19] Convert get_file_perms() to use inc_ie / IncludeRule --- utils/apparmor/aa.py | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 7d2247ec0..0021027bd 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -2357,7 +2357,7 @@ def get_file_perms(profile, path, audit, deny): perms = profile['file'].get_perms_for_path(path, audit, deny) - includelist = list(profile['include'].keys()) + includelist = profile['inc_ie'].get_all_full_paths(profile_dir) checked = [] while includelist: @@ -2367,25 +2367,27 @@ def get_file_perms(profile, path, audit, deny): continue checked.append(incname) - if os.path.isdir(get_include_path(incname)): - includelist += include_dir_filelist(profile_dir, incname) - else: - incperms = include[incname][incname]['file'].get_perms_for_path(path, audit, deny) + # include[] keys can be a) 'abstractions/foo' and b) '/full/path' + if incname.startswith(profile_dir): + incname = incname.replace('%s/' % profile_dir, '') - for allow_or_deny in ['allow', 'deny']: - for owner_or_all in ['all', 'owner']: - for perm in incperms[allow_or_deny][owner_or_all]: - perms[allow_or_deny][owner_or_all].add(perm) + incperms = include[incname][incname]['file'].get_perms_for_path(path, audit, deny) - if 'a' in perms[allow_or_deny][owner_or_all] and 'w' in perms[allow_or_deny][owner_or_all]: - perms[allow_or_deny][owner_or_all].remove('a') # a is a subset of w, so remove it + for allow_or_deny in ['allow', 'deny']: + for owner_or_all in ['all', 'owner']: + for perm in incperms[allow_or_deny][owner_or_all]: + perms[allow_or_deny][owner_or_all].add(perm) - for incpath in incperms['paths']: - perms['paths'].add(incpath) + if 'a' in perms[allow_or_deny][owner_or_all] and 'w' in perms[allow_or_deny][owner_or_all]: + perms[allow_or_deny][owner_or_all].remove('a') # a is a subset of w, so remove it - for childinc in include[incname][incname]['include'].keys(): - if childinc not in checked: - includelist += [childinc] + for incpath in incperms['paths']: + perms['paths'].add(incpath) + + for childinc in include[incname][incname]['inc_ie'].rules: + for childinc_file in childinc.get_full_paths(profile_dir): + if childinc_file not in checked: + includelist += [childinc_file] return perms From 9f1b2f4014ef27c5e7a17acadd03221387bb9809 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 00:21:32 +0200 Subject: [PATCH 09/19] collapse_log(): avoid accidently initializing aa[profile] ... or calling is_known_rule() on events for non-existing hats. It's the usual hasher() "fun" again - accessing a non-existing element will create its parent. In theory this commit might be worth a backport. In practise, it doesn't cause any visible problem. However, starting with the next commit, it will cause lots of test errors. Also add a missing is_known_rule() call for dbus rules, which might have caused similar hasher() "fun". --- utils/apparmor/aa.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 0021027bd..9ce8bca5f 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1555,6 +1555,11 @@ def collapse_log(hashlog, ignore_null_profiles=True): profile, hat = split_name(hashlog[aamode][full_profile]['final_name']) # XXX limited to two levels to avoid an Exception on nested child profiles or nested null-* # TODO: support nested child profiles + # used to avoid to accidently initialize aa[profile][hat] or calling is_known_rule() on events for a non-existing profile + hat_exists = False + if aa.get(profile) and aa[profile].get(hat): + hat_exists = True + if True: if not log_dict[aamode][profile].get(hat): # with execs in ix mode, we already have ProfileStorage initialized and should keep the content it already has @@ -1570,13 +1575,13 @@ def collapse_log(hashlog, ignore_null_profiles=True): file_event = FileRule(path, mode, None, FileRule.ALL, owner=owner, log_event=True) - if not is_known_rule(aa[profile][hat], 'file', file_event): + if not hat_exists or not is_known_rule(aa[profile][hat], 'file', file_event): log_dict[aamode][profile][hat]['file'].add(file_event) # TODO: check for existing rules with this path, and merge them into one rule for cap in hashlog[aamode][full_profile]['capability'].keys(): cap_event = CapabilityRule(cap, log_event=True) - if not is_known_rule(aa[profile][hat], 'capability', cap_event): + if not hat_exists or not is_known_rule(aa[profile][hat], 'capability', cap_event): log_dict[aamode][profile][hat]['capability'].add(cap_event) dbus = hashlog[aamode][full_profile]['dbus'] @@ -1599,20 +1604,21 @@ def collapse_log(hashlog, ignore_null_profiles=True): else: raise AppArmorBug('unexpected dbus access: %s') - log_dict[aamode][profile][hat]['dbus'].add(dbus_event) + if not hat_exists or not is_known_rule(aa[profile][hat], 'dbus', dbus_event): + log_dict[aamode][profile][hat]['dbus'].add(dbus_event) nd = hashlog[aamode][full_profile]['network'] for family in nd.keys(): for sock_type in nd[family].keys(): net_event = NetworkRule(family, sock_type, log_event=True) - if not is_known_rule(aa[profile][hat], 'network', net_event): + if not hat_exists or not is_known_rule(aa[profile][hat], 'network', net_event): log_dict[aamode][profile][hat]['network'].add(net_event) ptrace = hashlog[aamode][full_profile]['ptrace'] for peer in ptrace.keys(): for access in ptrace[peer].keys(): ptrace_event = PtraceRule(access, peer, log_event=True) - if not is_known_rule(aa[profile][hat], 'ptrace', ptrace_event): + if not hat_exists or not is_known_rule(aa[profile][hat], 'ptrace', ptrace_event): log_dict[aamode][profile][hat]['ptrace'].add(ptrace_event) sig = hashlog[aamode][full_profile]['signal'] @@ -1620,7 +1626,7 @@ def collapse_log(hashlog, ignore_null_profiles=True): for access in sig[peer].keys(): for signal in sig[peer][access].keys(): signal_event = SignalRule(access, signal, peer, log_event=True) - if not is_known_rule(aa[profile][hat], 'signal', signal_event): + if not hat_exists or not is_known_rule(aa[profile][hat], 'signal', signal_event): log_dict[aamode][profile][hat]['signal'].add(signal_event) return log_dict From 6f1245ede951dae8a5ebcba5cb01cfd015c283b3 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 00:29:05 +0200 Subject: [PATCH 10/19] Convert is_known_rule() to use inc_ie / IncludeRule While on it, also add a missing check to avoid checking the same file twice. --- utils/apparmor/aa.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 9ce8bca5f..5192d3d8b 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -2339,22 +2339,27 @@ def is_known_rule(profile, rule_type, rule_obj): if profile[rule_type].is_covered(rule_obj, False): return True - includelist = list(profile['include'].keys()) + includelist = profile['inc_ie'].get_all_full_paths(profile_dir) checked = [] while includelist: incname = includelist.pop(0) + + if incname in checked: + continue checked.append(incname) - if os.path.isdir(get_include_path(incname)): - includelist += include_dir_filelist(profile_dir, incname) - else: - if include[incname][incname][rule_type].is_covered(rule_obj, False): - return True + # include[] keys can be a) 'abstractions/foo' and b) '/full/path' + if incname.startswith(profile_dir): + incname = incname.replace('%s/' % profile_dir, '') - for childinc in include[incname][incname]['include'].keys(): - if childinc not in checked: - includelist += [childinc] + if include[incname][incname][rule_type].is_covered(rule_obj, False): + return True + + for childinc in include[incname][incname]['inc_ie'].rules: + for childinc_file in childinc.get_full_paths(profile_dir): + if childinc_file not in checked: + includelist += [childinc_file] return False From cfdbcb20ccc619110bcc9bee117399e212bdc5f2 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 13:11:03 +0200 Subject: [PATCH 11/19] add include_list_recursive() This function returns a list of all includes in a profile and its included files. It's split off from is_known_rule() and get_file_perms() which shared lots of common code to recursively get the include list. These functions now use include_list_recursive(). --- utils/apparmor/aa.py | 56 ++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 5192d3d8b..de4e33c08 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -2333,22 +2333,39 @@ def write_profile(profile, is_attachment=False): original_aa[profile] = deepcopy(aa[profile]) +def include_list_recursive(profile): + ''' get a list of all includes in a profile and its included files ''' + + includelist = profile['inc_ie'].get_all_full_paths(profile_dir) + full_list = [] + + while includelist: + incname = includelist.pop(0) + + if incname in full_list: + continue + full_list.append(incname) + + # include[] keys can be a) 'abstractions/foo' and b) '/full/path' + if incname.startswith(profile_dir): + incname = incname.replace('%s/' % profile_dir, '') + + for childinc in include[incname][incname]['inc_ie'].rules: + for childinc_file in childinc.get_full_paths(profile_dir): + if childinc_file not in full_list: + includelist += [childinc_file] + + return full_list + def is_known_rule(profile, rule_type, rule_obj): # XXX get rid of get() checks after we have a proper function to initialize a profile if profile.get(rule_type, False): if profile[rule_type].is_covered(rule_obj, False): return True - includelist = profile['inc_ie'].get_all_full_paths(profile_dir) - checked = [] - - while includelist: - incname = includelist.pop(0) - - if incname in checked: - continue - checked.append(incname) + includelist = include_list_recursive(profile) + for incname in includelist: # include[] keys can be a) 'abstractions/foo' and b) '/full/path' if incname.startswith(profile_dir): incname = incname.replace('%s/' % profile_dir, '') @@ -2356,11 +2373,6 @@ def is_known_rule(profile, rule_type, rule_obj): if include[incname][incname][rule_type].is_covered(rule_obj, False): return True - for childinc in include[incname][incname]['inc_ie'].rules: - for childinc_file in childinc.get_full_paths(profile_dir): - if childinc_file not in checked: - includelist += [childinc_file] - return False def get_file_perms(profile, path, audit, deny): @@ -2368,16 +2380,9 @@ def get_file_perms(profile, path, audit, deny): perms = profile['file'].get_perms_for_path(path, audit, deny) - includelist = profile['inc_ie'].get_all_full_paths(profile_dir) - checked = [] - - while includelist: - incname = includelist.pop(0) - - if incname in checked: - continue - checked.append(incname) + includelist = include_list_recursive(profile) + for incname in includelist: # include[] keys can be a) 'abstractions/foo' and b) '/full/path' if incname.startswith(profile_dir): incname = incname.replace('%s/' % profile_dir, '') @@ -2395,11 +2400,6 @@ def get_file_perms(profile, path, audit, deny): for incpath in incperms['paths']: perms['paths'].add(incpath) - for childinc in include[incname][incname]['inc_ie'].rules: - for childinc_file in childinc.get_full_paths(profile_dir): - if childinc_file not in checked: - includelist += [childinc_file] - return perms def propose_file_rules(profile_obj, rule_obj): From 6161641bcf08b1ef4aa84a4a298ac46c39550a5d Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 00:50:30 +0200 Subject: [PATCH 12/19] Convert match_includes() to use inc_ie / IncludeRule --- utils/apparmor/aa.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index de4e33c08..a85128bc3 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1400,8 +1400,14 @@ def match_includes(profile, rule_type, rule_obj): newincludes = [] for incname in include.keys(): + # TODO: improve/fix logic to honor magic vs. quoted include paths + if incname.startswith('/'): + is_magic = False + else: + is_magic = True + # never propose includes that are already in the profile (shouldn't happen because of is_known_rule()) - if profile and profile['include'].get(incname, False): + if profile and profile['inc_ie'].is_covered(IncludeRule(incname, False, is_magic)): continue # XXX type check should go away once we init all profiles correctly From c8fa3dec0ae16bb27df728d5300f381e35301958 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 01:25:08 +0200 Subject: [PATCH 13/19] aa-cleanprof: don't check profile includes against preamble includes remove_duplicate_rules() composed the 'includes' list from a) the include rules in the profile and b) the file_includes (preamble includes), and then checked the profile includes against this combination of profile and preamble includes. Checking profile includes against preamble includes is wrong, and the only reason why this went unnoticed for years is that preamble include files (like tunables) won't work inside a profile and therefore never appear there. In theory this might be a backport candidate, even if this shouldn't cause a real-world bug. --- utils/apparmor/cleanprofile.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/utils/apparmor/cleanprofile.py b/utils/apparmor/cleanprofile.py index 8a147544f..bfdac5c3c 100644 --- a/utils/apparmor/cleanprofile.py +++ b/utils/apparmor/cleanprofile.py @@ -53,11 +53,8 @@ class CleanProf(object): deleted += self.profile.active_profiles.delete_preamble_duplicates(self.profile.filename) #Process every hat in the profile individually - file_includes = list(self.profile.filelist[self.profile.filename]['include'].keys()) - for hat in sorted(self.profile.aa[program].keys()): - #The combined list of includes from profile and the file - includes = list(self.profile.aa[program][hat]['include'].keys()) + file_includes + includes = list(self.profile.aa[program][hat]['include'].keys()) #If different files remove duplicate includes in the other profile if not self.same_file: From 706138ed67838c352724c64ca8d9a191aa2adec3 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 01:49:02 +0200 Subject: [PATCH 14/19] Convert remove_duplicate_rules() to use inc_ie / IncludeRule Besides switching to 'inc_ie', also drop the old code that removed duplicate 'include' rules. --- utils/apparmor/cleanprofile.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/utils/apparmor/cleanprofile.py b/utils/apparmor/cleanprofile.py index bfdac5c3c..9a85be990 100644 --- a/utils/apparmor/cleanprofile.py +++ b/utils/apparmor/cleanprofile.py @@ -54,18 +54,14 @@ class CleanProf(object): #Process every hat in the profile individually for hat in sorted(self.profile.aa[program].keys()): - includes = list(self.profile.aa[program][hat]['include'].keys()) - - #If different files remove duplicate includes in the other profile - if not self.same_file: - if self.other.aa[program].get(hat): # carefully avoid to accidently initialize self.other.aa[program][hat] - for inc in includes: - if self.other.aa[program][hat]['include'].get(inc, False): - self.other.aa[program][hat]['include'].pop(inc) - deleted += 1 + includes = self.profile.aa[program][hat]['inc_ie'].get_all_full_paths(apparmor.profile_dir) #Clean up superfluous rules from includes in the other profile for inc in includes: + # apparmor.include[] keys can be a) 'abstractions/foo' and b) '/full/path' + if inc.startswith(apparmor.profile_dir): + inc = inc.replace('%s/' % apparmor.profile_dir, '') + if not self.profile.include.get(inc, {}).get(inc, False): apparmor.load_include(inc) if self.other.aa[program].get(hat): # carefully avoid to accidently initialize self.other.aa[program][hat] From f0fa69a010cbc0941ec10bb4526458769eb77a86 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 12:55:20 +0200 Subject: [PATCH 15/19] Convert compare_profiles() to use inc_ie / IncludeRule --- utils/apparmor/cleanprofile.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/utils/apparmor/cleanprofile.py b/utils/apparmor/cleanprofile.py index 9a85be990..a6bd2cb17 100644 --- a/utils/apparmor/cleanprofile.py +++ b/utils/apparmor/cleanprofile.py @@ -32,12 +32,8 @@ class CleanProf(object): def compare_profiles(self): deleted = 0 - other_file_includes = list(self.other.filelist[self.other.filename]['include'].keys()) - #Remove the duplicate file-level includes from other - for rule in self.profile.filelist[self.profile.filename]['include'].keys(): - if rule in other_file_includes: - self.other.filelist[self.other.filename]['include'].pop(rule) + deleted += self.other.active_profiles.delete_preamble_duplicates(self.other.filename) for profile in self.profile.aa.keys(): deleted += self.remove_duplicate_rules(profile) From 5cfab9b81b052189d5424bcda3bb6ae2eb9a7bac Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 15:53:53 +0200 Subject: [PATCH 16/19] serialize_profile(): write preamble without checking filelist filelist is getting emptier and emptier, and checking against a nearly-empty filelist would result in not writing the preamble. --- utils/apparmor/aa.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index a85128bc3..0a8b820a5 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -2278,11 +2278,10 @@ def serialize_profile(profile_data, name, options): else: prof_filename = get_profile_filename_from_profile_name(name, True) - if filelist.get(prof_filename, False): - data += active_profiles.get_clean_first(prof_filename, 0) - data += write_list_vars(filelist[prof_filename], 0) + data += active_profiles.get_clean_first(prof_filename, 0) + data += write_list_vars(filelist[prof_filename], 0) - data += active_profiles.get_clean(prof_filename, 0) + data += active_profiles.get_clean(prof_filename, 0) #Here should be all the profiles from the files added write after global/common stuff for prof in sorted(active_profiles.profiles_in_file(prof_filename)): From 60e8c1ff4127f15b011a2104ae5789f79bf190c3 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 13:20:11 +0200 Subject: [PATCH 17/19] No longer write to filelist['include] or profile['include'] All code is migrated to IncludeRuleset / 'inc_ie'], so there's no point in filling 'include' anymore. --- utils/apparmor/aa.py | 14 -------------- utils/test/test-aa.py | 17 ----------------- 2 files changed, 31 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 0a8b820a5..2529f3e82 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -442,8 +442,6 @@ def create_new_profile(localfile, is_stub=False): local_profile = hasher() local_profile[localfile] = ProfileStorage('NEW', localfile, 'create_new_profile()') local_profile[localfile]['flags'] = 'complain' - local_profile[localfile]['include']['abstractions/base'] = 1 - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. local_profile[localfile]['inc_ie'].add(IncludeRule('abstractions/base', False, True)) if os.path.exists(localfile) and os.path.isfile(localfile): @@ -454,8 +452,6 @@ def create_new_profile(localfile, is_stub=False): local_profile[localfile]['file'].add(FileRule(interpreter_path, None, 'ix', FileRule.ALL, owner=False)) if abstraction: - local_profile[localfile]['include'][abstraction] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. local_profile[localfile]['inc_ie'].add(IncludeRule(abstraction, False, True)) handle_binfmt(local_profile[localfile], interpreter_path) @@ -574,8 +570,6 @@ def autodep(bin_name, pname=''): if os.path.isfile(profile_dir + '/tunables/global'): if not filelist.get(file, False): filelist[file] = hasher() - filelist[file]['include']['tunables/global'] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. active_profiles.add_inc_ie(file, IncludeRule('tunables/global', False, True)) write_profile_ui_feedback(pname) @@ -951,8 +945,6 @@ def ask_exec(hashlog): aa[profile][hat]['file'].add(FileRule(interpreter_path, None, 'ix', FileRule.ALL, owner=False)) if abstraction: - aa[profile][hat]['include'][abstraction] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. aa[profile][hat]['inc_ie'].add(IncludeRule(abstraction, False, True)) handle_binfmt(aa[profile][hat], interpreter_path) @@ -1194,8 +1186,6 @@ def ask_rule_questions(prof_events, profile_name, the_profile, r_types): if inc: deleted = delete_all_duplicates(the_profile, inc, r_types) - the_profile['include'][inc] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. the_profile['inc_ie'].add(IncludeRule.parse(selection)) aaui.UI_Info(_('Adding %s to profile.') % selection) @@ -1924,14 +1914,10 @@ def parse_profile_data(data, file, do_include): include_name = re_match_include(line) if profile: - profile_data[profile][hat]['include'][include_name] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. profile_data[profile][hat]['inc_ie'].add(IncludeRule.parse(line)) else: if not filelist.get(file): filelist[file] = hasher() - filelist[file]['include'][include_name] = True - # currently store in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. active_profiles.add_inc_ie(file, IncludeRule.parse(line)) # If include is a directory diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index d412e3977..bbd277a2a 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -150,12 +150,8 @@ class AaTest_create_new_profile(AATest): self.assertEqual(set(profile[program][program]['file'].get_clean()), {'%s mr,' % program, ''}) if exp_abstraction: - self.assertEqual(set(profile[program][program]['include'].keys()), {exp_abstraction, 'abstractions/base'}) - # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. self.assertEqual(profile[program][program]['inc_ie'].get_clean(), ['include ', 'include <%s>' % exp_abstraction, '']) else: - self.assertEqual(set(profile[program][program]['include'].keys()), {'abstractions/base'}) - # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. self.assertEqual(profile[program][program]['inc_ie'].get_clean(), ['include ', '']) class AaTest_get_interpreter_and_abstraction(AATest): @@ -834,10 +830,6 @@ class AaTest_get_file_perms_2(AATest): apparmor.aa.load_include('abstractions/aspell') profile = apparmor.aa.ProfileStorage('/test', '/test', 'test-aa.py') - profile['include']['abstractions/base'] = True - profile['include']['abstractions/bash'] = True - profile['include']['abstractions/enchant'] = True # includes abstractions/aspell - # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. profile['inc_ie'].add(IncludeRule.parse('include ')) profile['inc_ie'].add(IncludeRule.parse('include ')) profile['inc_ie'].add(IncludeRule.parse('include ')) @@ -880,10 +872,6 @@ class AaTest_propose_file_rules(AATest): apparmor.aa.user_globs['/no/thi*ng'] = AARE('/no/thi*ng', True) profile = apparmor.aa.ProfileStorage('/test', '/test', 'test-aa.py') - profile['include']['abstractions/base'] = True - profile['include']['abstractions/bash'] = True - profile['include']['abstractions/enchant'] = True # includes abstractions/aspell - # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. profile['inc_ie'].add(IncludeRule.parse('include ')) profile['inc_ie'].add(IncludeRule.parse('include ')) profile['inc_ie'].add(IncludeRule.parse('include ')) @@ -929,11 +917,6 @@ class AaTest_propose_file_rules_with_absolute_includes(AATest): apparmor.aa.load_include(abs_include3) profile = apparmor.aa.ProfileStorage('/test', '/test', 'test-aa.py') - profile['include']['abstractions/base'] = False - profile['include'][abs_include1] = False - profile['include'][abs_include2] = False - profile['include'][abs_include3] = False - # currently stored in both 'include' (above) and 'inc_ie' (below). TODO: Drop 'include' once all code using it is migrated. profile['inc_ie'].add(IncludeRule.parse('include ')) profile['inc_ie'].add(IncludeRule.parse('include "%s"' % abs_include1)) profile['inc_ie'].add(IncludeRule.parse('include "%s"' % abs_include2)) From cefc64522dc06fe01d4b75d1989c4adddbf560e2 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 13:25:16 +0200 Subject: [PATCH 18/19] Drop now unused 'include' from ProfileStorage and ProfileList --- utils/apparmor/profile_list.py | 1 - utils/apparmor/profile_storage.py | 6 ------ 2 files changed, 7 deletions(-) diff --git a/utils/apparmor/profile_list.py b/utils/apparmor/profile_list.py index e34949761..24f5f6629 100644 --- a/utils/apparmor/profile_list.py +++ b/utils/apparmor/profile_list.py @@ -47,7 +47,6 @@ class ProfileList: self.files[filename] = { 'abi': AbiRuleset(), 'alias': {}, - 'include': {}, # not filled, but avoids errors in is_known_rule() and some other functions when aa-mergeprof asks about the preamble 'inc_ie': IncludeRuleset(), 'profiles': [], } diff --git a/utils/apparmor/profile_storage.py b/utils/apparmor/profile_storage.py index dd8fced15..e6a00cacb 100644 --- a/utils/apparmor/profile_storage.py +++ b/utils/apparmor/profile_storage.py @@ -61,7 +61,6 @@ class ProfileStorage: for rule in ruletypes: data[rule] = ruletypes[rule]['ruleset']() - data['include'] = dict() data['lvar'] = dict() data['filename'] = '' @@ -141,7 +140,6 @@ class ProfileStorage: # "old" write functions for rule types not implemented as *Rule class yet write_functions = { - 'include': write_includes, 'lvar': write_list_vars, 'mount': write_mount, 'pivot_root': write_pivot_root, @@ -151,7 +149,6 @@ class ProfileStorage: write_order = [ 'abi', 'lvar', - 'include', 'inc_ie', 'rlimit', 'capability', @@ -218,9 +215,6 @@ def write_list_vars(ref, depth): return data -def write_includes(prof_data, depth): - return [] # now handled via 'inc_ie' / IncludeRulset - def var_transform(ref): data = [] for value in sorted(ref): From 1d2e710b26bfb1036f948e7d68c059982aa1bcff Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 17 May 2020 14:02:23 +0200 Subject: [PATCH 19/19] parse_profile_data(): drop old code for parsing include rules With this, all include rules (with and without 'if exists') get handled by the code that handles IncludeRule. --- utils/apparmor/aa.py | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 2529f3e82..b0074a3c6 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1909,27 +1909,6 @@ def parse_profile_data(data, file, do_include): else: active_profiles.add_abi(file, AbiRule.parse(line)) - elif re_match_include(line): - # Include files - include_name = re_match_include(line) - - if profile: - profile_data[profile][hat]['inc_ie'].add(IncludeRule.parse(line)) - else: - if not filelist.get(file): - filelist[file] = hasher() - active_profiles.add_inc_ie(file, IncludeRule.parse(line)) - - # If include is a directory - if os.path.isdir(get_include_path(include_name)): - for file_name in include_dir_filelist(profile_dir, include_name): - if not include.get(file_name, False): - load_include(file_name) - else: - if not include.get(include_name, False): - load_include(include_name) - - # IncludeRule can handle 'include' and 'include if exists' - place it after the "old" 'include' handling so that it only catches 'include if exists' for now elif IncludeRule.match(line): rule_obj = IncludeRule.parse(line) if profile: