From b00aab0843803bfcfcc6e17109aed23e111a3c60 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Wed, 25 Jul 2018 23:22:33 +0200 Subject: [PATCH] let change_profile_flags() change flags in child profiles ... instead of overwriting them with the flags of the main profile. This fixes a longstanding issue with aa-complain, aa-enforce and aa-audit which broke the flags of child profiles and hats if they differed from the main profile. It also fixes several issues documented in the tests (which obviously need adjustment to match the fixed behaviour). Also change the "no profile found" cases to AppArmorException - errors in a profile are not worth triggering AppArmorBug ;-) --- utils/apparmor/aa.py | 16 ++++++---------- utils/test/test-aa.py | 27 ++++++++++++--------------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 58e68f7d3..b70b3f12b 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -624,17 +624,9 @@ def get_profile_flags(filename, program): raise AppArmorException(_('%s contains no profile') % filename) def change_profile_flags(prof_filename, program, flag, set_flag): - old_flags = get_profile_flags(prof_filename, program) - - newflags = add_or_remove_flag(old_flags, flag, set_flag) - - newflags = ', '.join(newflags) - """Reads the old profile file and updates the flags accordingly""" # TODO: count the number of matching lines (separated by profile and hat?) and return it # so that code calling this function can make sure to only report success if there was a match - # TODO: existing (unrelated) flags of hats and child profiles are overwritten - ideally, we should - # keep them and only add or remove a given flag # TODO: change child profile flags even if program is specified found = False @@ -651,6 +643,8 @@ def change_profile_flags(prof_filename, program, flag, set_flag): matches = parse_profile_start_line(line, prof_filename) space = matches['leadingspace'] or '' profile = matches['profile'] + old_flags = matches['flags'] + newflags = ', '.join(add_or_remove_flag(old_flags, flag, set_flag)) if (matches['attachment'] is not None): profile_glob = AARE(matches['attachment'], True) @@ -674,6 +668,8 @@ def change_profile_flags(prof_filename, program, flag, set_flag): space = matches.group('leadingspace') or '' hat_keyword = matches.group('hat_keyword') hat = matches.group('hat') + old_flags = matches['flags'] + newflags = ', '.join(add_or_remove_flag(old_flags, flag, set_flag)) comment = matches.group('comment') or '' if comment: comment = ' %s' % comment @@ -687,9 +683,9 @@ def change_profile_flags(prof_filename, program, flag, set_flag): if not found: if program is None: - raise AppArmorBug("%(file)s doesn't contain a valid profile (syntax error?)" % {'file': prof_filename}) + raise AppArmorException("%(file)s doesn't contain a valid profile (syntax error?)" % {'file': prof_filename}) else: - raise AppArmorBug("%(file)s doesn't contain a valid profile for %(profile)s (syntax error?)" % {'file': prof_filename, 'profile': program}) + raise AppArmorException("%(file)s doesn't contain a valid profile for %(profile)s (syntax error?)" % {'file': prof_filename, 'profile': program}) def profile_exists(program): """Returns True if profile exists, False otherwise""" diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index 8e3f16905..2bccae256 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -310,32 +310,32 @@ class AaTest_change_profile_flags(AaTestWithTempdir): def test_set_flags_with_hat_01(self): self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', True, 'audit, complain', more_rules='\n ^foobar {\n}\n', - expected_more_rules='\n ^foobar flags=(audit, complain) {\n}\n' # XXX complain should not be added to the child profile + expected_more_rules='\n ^foobar flags=(audit) {\n}\n' ) def test_change_profile_flags_with_hat_02(self): self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', False, 'complain', profile_name=None, more_rules='\n ^foobar flags=(audit) {\n}\n', - expected_more_rules='\n ^foobar flags=(complain) {\n}\n' # XXX complain should NOT be added to child profile + expected_more_rules='\n ^foobar {\n}\n' ) def test_change_profile_flags_with_hat_03(self): self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', True, 'audit, complain', - more_rules='\n^foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost! - expected_more_rules='\n^foobar flags=(audit, complain) { # comment\n}\n' # XXX complain should not be added + more_rules='\n^foobar (attach_disconnected) { # comment\n}\n', + expected_more_rules='\n^foobar flags=(attach_disconnected, audit) { # comment\n}\n' ) def test_change_profile_flags_with_hat_04(self): self._test_change_profile_flags('/foo', '', 'audit', True, 'audit', - more_rules='\n hat foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost! - expected_more_rules='\n hat foobar flags=(audit) { # comment\n}\n' + more_rules='\n hat foobar (attach_disconnected) { # comment\n}\n', + expected_more_rules='\n hat foobar flags=(attach_disconnected, audit) { # comment\n}\n' ) def test_change_profile_flags_with_hat_05(self): self._test_change_profile_flags('/foo', '(audit)', 'audit', False, '', - more_rules='\n hat foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost - expected_more_rules='\n hat foobar { # comment\n}\n' + more_rules='\n hat foobar (attach_disconnected) { # comment\n}\n', + expected_more_rules='\n hat foobar flags=(attach_disconnected) { # comment\n}\n' ) # test handling of child profiles @@ -343,7 +343,7 @@ class AaTest_change_profile_flags(AaTestWithTempdir): self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', True, 'audit, complain', profile_name=None, more_rules='\n profile /bin/bar {\n}\n', - expected_more_rules='\n profile /bin/bar flags=(audit, complain) {\n}\n' # XXX complain should not be added + expected_more_rules='\n profile /bin/bar flags=(audit) {\n}\n' ) def test_change_profile_flags_with_child_02(self): @@ -355,16 +355,13 @@ class AaTest_change_profile_flags(AaTestWithTempdir): def test_change_profile_flags_invalid_01(self): - with self.assertRaises(AppArmorException): - # XXX new flag 'None' should raise AppArmorBug + with self.assertRaises(AppArmorBug): self._test_change_profile_flags('/foo', '()', None, False, '', check_new_flags=False) def test_change_profile_flags_invalid_02(self): - with self.assertRaises(AppArmorException): - # XXX new flag 'None' should raise AppArmorBug + with self.assertRaises(AppArmorBug): self._test_change_profile_flags('/foo', 'flags=()', None, True, '', check_new_flags=False) def test_change_profile_flags_invalid_03(self): - with self.assertRaises(AppArmorException): - # XXX empty new flag should raise AppArmorBug + with self.assertRaises(AppArmorBug): self._test_change_profile_flags('/foo', '( )', '', True, '', check_new_flags=False) def test_change_profile_flags_invalid_04(self): with self.assertRaises(AppArmorBug):