From 20161471beec438f40e733f33740c3a41751ce79 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 1 Oct 2016 20:04:42 +0200 Subject: [PATCH] [24/38] Add propose_file_rules() to propose globbed etc. file rules in aa-logprof aa.py: - add propose_file_rules() - will propose matching paths from existing rules in the profile or one of the includes - save user_globs if user selects '(N)ew' (will be re-used when proposing rules) - change user_globs to a dict so that it can carry the human-readable path and an AARE object for it - change order_globs() to ensure the original path (given as parameter) is always the last item in the resulting list - add a ruletype switch to ask_the_questions() so that it uses propose_file_rules() for file events (I don't like this ruletype-specific solution too much, but everything else would make things even more complicated) Also keep aa-mergeprof ask_the_questions() in sync with aa.py. In FileRule, add original_perms (might be set by propose_file_rules()) Finally, add some tests to ensure propose_file_rules() does what it promises. Acked-by: Steve Beattie --- utils/aa-mergeprof | 10 ++++-- utils/apparmor/aa.py | 61 ++++++++++++++++++++++++++++++++++--- utils/apparmor/rule/file.py | 2 ++ utils/test/test-aa.py | 43 +++++++++++++++++++++++++- 4 files changed, 109 insertions(+), 7 deletions(-) diff --git a/utils/aa-mergeprof b/utils/aa-mergeprof index bddc88f3b..4f2e1f657 100755 --- a/utils/aa-mergeprof +++ b/utils/aa-mergeprof @@ -25,7 +25,9 @@ import apparmor.cleanprofile as cleanprofile import apparmor.ui as aaui from apparmor.aa import (add_to_options, available_buttons, combine_name, delete_duplicates, - get_profile_filename, is_known_rule, match_includes, selection_to_rule_obj) + get_profile_filename, is_known_rule, match_includes, + propose_file_rules, selection_to_rule_obj) +from apparmor.aare import AARE from apparmor.common import AppArmorException from apparmor.regex import re_match_include @@ -655,7 +657,10 @@ class Merge(object): if newincludes: options += list(map(lambda inc: '#include <%s>' % inc, sorted(set(newincludes)))) - options.append(rule_obj.get_clean()) + if ruletype == 'file' and rule_obj.path: + options += propose_file_rules(aa[profile][hat], rule_obj) + else: + options.append(rule_obj.get_clean()) done = False while not done: @@ -759,6 +764,7 @@ class Merge(object): edit_rule_obj.store_edit(newpath) options, default_option = add_to_options(options, edit_rule_obj.get_raw()) + apparmor.aa.user_globs[newpath] = AARE(newpath, True) else: done = False diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index efff22686..9a202e271 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -31,6 +31,8 @@ import apparmor.severity from copy import deepcopy +from apparmor.aare import AARE + from apparmor.common import (AppArmorException, AppArmorBug, open_file_read, valid_path, hasher, open_file_write, convert_regexp, DebugLogger) @@ -97,7 +99,8 @@ existing_profiles = dict() seen_events = 0 # was our # To store the globs entered by users so they can be provided again -user_globs = [] +# format: user_globs['/foo*'] = AARE('/foo*') +user_globs = {} # The key for representing bare "file," rules ALL = '\0ALL' @@ -1479,11 +1482,19 @@ def update_repo_profile(profile): # To-Do return None -def order_globs(globs, path): +def order_globs(globs, original_path): """Returns the globs in sorted order, more specific behind""" # To-Do # ATM its lexicographic, should be done to allow better matches later - return sorted(globs) + + globs = sorted(globs) + + # make sure the original path is always the last option + if original_path in globs: + globs.remove(original_path) + globs.append(original_path) + + return globs def ask_the_questions(): found = 0 @@ -1529,7 +1540,10 @@ def ask_the_questions(): if newincludes: options += list(map(lambda inc: '#include <%s>' % inc, sorted(set(newincludes)))) - options.append(rule_obj.get_clean()) + if ruletype == 'file' and rule_obj.path: + options += propose_file_rules(aa[profile][hat], rule_obj) + else: + options.append(rule_obj.get_clean()) seen_events += 1 @@ -1642,6 +1656,7 @@ def ask_the_questions(): edit_rule_obj.store_edit(newpath) options, default_option = add_to_options(options, edit_rule_obj.get_raw()) + user_globs[newpath] = AARE(newpath, True) else: done = False @@ -3792,6 +3807,44 @@ def get_file_perms(profile, path, audit, deny): return perms +def propose_file_rules(profile_obj, rule_obj): + '''Propose merged file rules based on the existing profile and the log events + - permissions get merged + - matching paths from existing rules, common_glob() and user_globs get proposed + - IMPORTANT: modifies rule_obj.original_perms and rule_obj.perms''' + options = [] + original_path = rule_obj.path.regex + + merged_rule_obj = deepcopy(rule_obj) # make sure not to modify the original rule object (with exceptions, see end of this function) + + existing_perms = get_file_perms(profile_obj, rule_obj.path, False, False) + for perm in existing_perms['allow']['all']: # XXX also handle owner-only perms + merged_rule_obj.perms.add(perm) + merged_rule_obj.raw_rule = None + + pathlist = {original_path} | existing_perms['paths'] | set(glob_common(original_path)) + + for user_glob in user_globs: + if user_globs[user_glob].match(original_path): + pathlist.add(user_glob) + + pathlist = order_globs(pathlist, original_path) + + # paths in existing rules that match the original path + for path in pathlist: + merged_rule_obj.store_edit(path) + merged_rule_obj.raw_rule = None + options.append(merged_rule_obj.get_clean()) + + merged_rule_obj.exec_perms = None + + rule_obj.original_perms = existing_perms + if rule_obj.perms != merged_rule_obj.perms: + rule_obj.perms = merged_rule_obj.perms + rule_obj.raw_rule = None + + return options + def reload_base(bin_path): if not check_for_apparmor(): return None diff --git a/utils/apparmor/rule/file.py b/utils/apparmor/rule/file.py index 8305fe4ae..4e0ac9a71 100644 --- a/utils/apparmor/rule/file.py +++ b/utils/apparmor/rule/file.py @@ -82,6 +82,8 @@ class FileRule(BaseRule): if self.perms and 'a' in self.perms and 'w' in self.perms: raise AppArmorException("Conflicting permissions found: 'a' and 'w'") + self.original_perms = None # might be set by aa-logprof / aa.py propose_file_rules() + if exec_perms is None: self.exec_perms = None elif exec_perms == self.ANY_EXEC: diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index bd227adac..69a2c3248 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -21,7 +21,8 @@ import apparmor.aa # needed to set global vars in some tests from apparmor.aa import (check_for_apparmor, get_output, get_reqs, get_interpreter_and_abstraction, create_new_profile, get_profile_flags, set_profile_flags, is_skippable_file, is_skippable_dir, parse_profile_start, parse_profile_data, separate_vars, store_list_var, write_header, - var_transform, serialize_parse_profile_start, get_file_perms) + var_transform, serialize_parse_profile_start, get_file_perms, propose_file_rules) +from apparmor.aare import AARE from apparmor.common import AppArmorException, AppArmorBug from apparmor.rule.file import FileRule @@ -799,6 +800,46 @@ class AaTest_get_file_perms_2(AATest): perms = get_file_perms(profile, params, False, False) # only testing with audit and deny = False self.assertEqual(perms, expected) +class AaTest_propose_file_rules(AATest): + tests = [ + # log event path and perms expected proposals + (['/usr/share/common-licenses/foo/bar', 'w'], ['/usr/share/common*/foo/* rw,', '/usr/share/common-licenses/** rw,', '/usr/share/common-licenses/foo/bar rw,'] ), + (['/dev/null', 'wk'], ['/dev/null rwk,'] ), + (['/foo/bar', 'rw'], ['/foo/bar rw,'] ), + (['/usr/lib/ispell/', 'w'], ['/usr/lib{,32,64}/** rw,', '/usr/lib/ispell/ rw,'] ), + (['/usr/lib/aspell/some.so', 'k'], ['/usr/lib/aspell/* mrk,', '/usr/lib/aspell/*.so mrk,', '/usr/lib{,32,64}/** mrk,', '/usr/lib/aspell/some.so mrk,'] ), + ] + + def _run_test(self, params, expected): + 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) + + # load the abstractions we need in the test + apparmor.aa.profiledir = self.profile_dir + apparmor.aa.load_include('abstractions/base') + apparmor.aa.load_include('abstractions/bash') + apparmor.aa.load_include('abstractions/enchant') + apparmor.aa.load_include('abstractions/aspell') + + # add some user_globs ('(N)ew') to simulate a professional aa-logprof user (and to make sure that part of the code also gets tested) + apparmor.aa.user_globs['/usr/share/common*/foo/*'] = AARE('/usr/share/common*/foo/*', True) + apparmor.aa.user_globs['/no/thi*ng'] = AARE('/no/thi*ng', True) + + profile = apparmor.aa.profile_storage('/test', '/test', 'test-aa.py') + profile['include']['abstractions/base'] = True + profile['include']['abstractions/bash'] = True + profile['include']['abstractions/enchant'] = True # includes abstractions/aspell + + profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/** w,')) + profile['file'].add(FileRule.parse('/dev/null rwk,')) + profile['file'].add(FileRule.parse('/foo/bar rwix,')) + + rule_obj = FileRule(params[0], params[1], None, FileRule.ALL, owner=False, log_event=True) + proposals = propose_file_rules(profile, rule_obj) + self.assertEqual(proposals, expected) setup_all_loops(__name__) if __name__ == '__main__':