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 ;-)
This commit is contained in:
Christian Boltz 2018-07-25 23:22:33 +02:00
parent d26ffbdd29
commit b00aab0843
Failed to generate hash of commit
2 changed files with 18 additions and 25 deletions

View file

@ -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"""

View file

@ -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):