Merge branch 'cboltz-change-flags' into 'master'

prevent that aa-complain etc. overwrites flags in child profiles if they differ from the main profile

See merge request apparmor/apparmor!150

Acked-by: John Johansen <john.johansen@canonical.com>
This commit is contained in:
John Johansen 2018-08-07 09:59:39 +00:00
commit 9dab5f07f4
5 changed files with 173 additions and 137 deletions

View file

@ -49,7 +49,7 @@ from apparmor.regex import (RE_PROFILE_START, RE_PROFILE_END, RE_PROFILE_LINK,
RE_PROFILE_UNIX, RE_RULE_HAS_COMMA, RE_HAS_COMMENT_SPLIT,
strip_quotes, parse_profile_start_line, re_match_include )
from apparmor.profile_storage import (ProfileStorage, ruletypes, write_alias,
from apparmor.profile_storage import (ProfileStorage, add_or_remove_flag, ruletypes, write_alias,
write_includes, write_list_vars )
import apparmor.rules as aarules
@ -561,7 +561,7 @@ def activate_repo_profiles(url, profiles, complain):
write_profile(pname)
if complain:
fname = get_profile_filename(pname)
set_profile_flags(profile_dir + fname, 'complain')
change_profile_flags(profile_dir + fname, None, 'complain', True)
aaui.UI_Info(_('Setting %s to complain mode.') % pname)
except Exception as e:
sys.stderr.write(_("Error activating profiles: %s") % e)
@ -623,43 +623,16 @@ def get_profile_flags(filename, program):
raise AppArmorException(_('%s contains no profile') % filename)
def change_profile_flags(filename, program, flag, set_flag):
old_flags = get_profile_flags(filename, program)
newflags = []
if old_flags:
# Flags maybe white-space and/or , separated
old_flags = old_flags.split(',')
if not isinstance(old_flags, str):
for i in old_flags:
newflags += i.split()
else:
newflags = old_flags.split()
#newflags = [lambda x:x.strip(), oldflags]
if set_flag:
if flag not in newflags:
newflags.append(flag)
else:
if flag in newflags:
newflags.remove(flag)
newflags = ','.join(newflags)
set_profile_flags(filename, program, newflags)
def set_profile_flags(prof_filename, program, newflags):
def change_profile_flags(prof_filename, program, flag, set_flag):
"""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
if newflags and newflags.strip() == '':
raise AppArmorBug('New flags for %s contain only whitespace' % prof_filename)
if not flag or flag.strip() == '':
raise AppArmorBug('New flag for %s is empty' % prof_filename)
with open_file_read(prof_filename) as f_in:
temp_file = tempfile.NamedTemporaryFile('w', prefix=prof_filename, suffix='~', delete=False, dir=profile_dir)
@ -670,6 +643,8 @@ def set_profile_flags(prof_filename, program, newflags):
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)
@ -693,6 +668,8 @@ def set_profile_flags(prof_filename, program, newflags):
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
@ -706,9 +683,9 @@ def set_profile_flags(prof_filename, program, newflags):
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

@ -16,7 +16,7 @@
from apparmor.aamode import AA_LINK_SUBSET
from apparmor.common import AppArmorBug, AppArmorException, hasher
from apparmor.common import AppArmorBug, AppArmorException, hasher, type_is_str
from apparmor.rule.capability import CapabilityRuleset
from apparmor.rule.change_profile import ChangeProfileRuleset
@ -70,9 +70,9 @@ class ProfileStorage:
data['attachment'] = ''
data['flags'] = ''
data['external'] = False
data['header_comment'] = '' # currently only set by set_profile_flags()
data['header_comment'] = '' # currently only set by change_profile_flags()
data['initial_comment'] = ''
data['profile_keyword'] = False # currently only set by set_profile_flags()
data['profile_keyword'] = False # currently only set by change_profile_flags()
data['profile'] = False # profile or hat?
data['allow'] = dict()
@ -159,6 +159,33 @@ class ProfileStorage:
return data
def split_flags(flags):
'''split the flags given as string into a sorted, de-duplicated list'''
if flags is None:
flags = ''
# Flags may be whitespace and/or comma separated
flags_list = flags.replace(',', ' ').split()
# sort and remove duplicates
return sorted(set(flags_list))
def add_or_remove_flag(flags, flag_to_change, set_flag):
'''add (if set_flag == True) or remove the given flag_to_change to flags'''
if type_is_str(flags) or flags is None:
flags = split_flags(flags)
if set_flag:
if flag_to_change not in flags:
flags.append(flag_to_change)
else:
if flag_to_change in flags:
flags.remove(flag_to_change)
return sorted(flags)
def set_allow_str(allow):
if allow == 'deny':
return 'deny '

View file

@ -89,7 +89,7 @@ class MinitoolsTest(AATest):
self.assertEqual(os.path.islink('%s/%s' % (force_complain_dir, os.path.basename(self.local_profilename))), True,
'Failed to create a symlink for %s in force-complain'%self.local_profilename)
self.assertEqual(apparmor.get_profile_flags(self.local_profilename, self.test_path), 'audit,complain',
self.assertEqual(apparmor.get_profile_flags(self.local_profilename, self.test_path), 'audit, complain',
'Complain flag could not be set in profile %s'%self.local_profilename)
#Remove complain flag first i.e. set to enforce mode

View file

@ -19,7 +19,7 @@ import sys
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, set_options_audit_mode, set_options_owner_mode, is_skippable_file, is_skippable_dir,
get_profile_flags, change_profile_flags, set_options_audit_mode, set_options_owner_mode, is_skippable_file, is_skippable_dir,
parse_profile_start, parse_profile_data, separate_vars, store_list_var, write_header,
get_file_perms, propose_file_rules)
from apparmor.aare import AARE
@ -226,16 +226,13 @@ class AaTest_get_profile_flags(AaTestWithTempdir):
with self.assertRaises(AppArmorException):
self._test_get_flags('/no-such-profile flags=(complain)', 'complain')
class AaTest_set_profile_flags(AaTestWithTempdir):
def _test_set_flags(self, profile, old_flags, new_flags, whitespace='', comment='',
class AaTest_change_profile_flags(AaTestWithTempdir):
def _test_change_profile_flags(self, profile, old_flags, flags_to_change, set_flag, expected_flags, whitespace='', comment='',
more_rules='', expected_more_rules='@-@-@',
expected_flags='@-@-@', check_new_flags=True, profile_name='/foo'):
check_new_flags=True, profile_name='/foo'):
if old_flags:
old_flags = ' %s' % old_flags
if expected_flags == '@-@-@':
expected_flags = new_flags
if expected_flags:
expected_flags = ' flags=(%s)' % (expected_flags)
else:
@ -253,152 +250,150 @@ class AaTest_set_profile_flags(AaTestWithTempdir):
new_prof = prof_template % (whitespace, profile, expected_flags, comment, expected_more_rules, dummy_profile_content)
self.file = write_file(self.tmpdir, 'profile', old_prof)
set_profile_flags(self.file, profile_name, new_flags)
change_profile_flags(self.file, profile_name, flags_to_change, set_flag)
if check_new_flags:
real_new_prof = read_file(self.file)
self.assertEqual(new_prof, real_new_prof)
# tests that actually don't change the flags
def test_set_flags_nochange_01(self):
self._test_set_flags('/foo', '', '')
def test_set_flags_nochange_02(self):
self._test_set_flags('/foo', '( complain )', ' complain ', whitespace=' ')
def test_set_flags_nochange_03(self):
self._test_set_flags('/foo', '(complain)', 'complain')
def test_set_flags_nochange_04(self):
self._test_set_flags('/foo', 'flags=(complain)', 'complain')
def test_set_flags_nochange_05(self):
self._test_set_flags('/foo', 'flags=(complain, audit)', 'complain, audit', whitespace=' ')
def test_set_flags_nochange_06(self):
self._test_set_flags('/foo', 'flags=(complain, audit)', 'complain, audit', whitespace=' ', comment='# a comment')
def test_set_flags_nochange_07(self):
self._test_set_flags('/foo', 'flags=(complain, audit)', 'complain, audit', whitespace=' ', more_rules=' # a comment\n#another comment')
def test_set_flags_nochange_08(self):
self._test_set_flags('profile /foo', 'flags=(complain)', 'complain')
def test_set_flags_nochange_09(self):
self._test_set_flags('profile xy /foo', 'flags=(complain)', 'complain', profile_name='xy')
def test_set_flags_nochange_10(self):
self._test_set_flags('profile "/foo bar"', 'flags=(complain)', 'complain', profile_name='/foo bar')
def test_set_flags_nochange_11(self):
self._test_set_flags('/foo', '(complain)', 'complain', profile_name=None)
#def test_set_flags_nochange_12(self):
# XXX changes the flags for the child profile (which happens to have the same profile name) to 'complain'
# self._test_set_flags('/foo', 'flags=(complain)', 'complain', more_rules=' profile /foo {\n}')
def test_change_profile_flags_nochange_02(self):
self._test_change_profile_flags('/foo', '( complain )', 'complain', True, 'complain', whitespace=' ')
def test_change_profile_flags_nochange_03(self):
self._test_change_profile_flags('/foo', '(complain)', 'complain', True, 'complain')
def test_change_profile_flags_nochange_04(self):
self._test_change_profile_flags('/foo', 'flags=(complain)', 'complain', True, 'complain')
def test_change_profile_flags_nochange_05(self):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', 'complain', True, 'audit, complain', whitespace=' ')
def test_change_profile_flags_nochange_06(self):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', 'complain', True, 'audit, complain', whitespace=' ', comment='# a comment')
def test_change_profile_flags_nochange_07(self):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', 'audit', True, 'audit, complain', whitespace=' ', more_rules=' # a comment\n#another comment')
def test_change_profile_flags_nochange_08(self):
self._test_change_profile_flags('profile /foo', 'flags=(complain)', 'complain', True, 'complain')
def test_change_profile_flags_nochange_09(self):
self._test_change_profile_flags('profile xy /foo', 'flags=(complain)', 'complain', True, 'complain', profile_name='xy')
def test_change_profile_flags_nochange_10(self):
self._test_change_profile_flags('profile "/foo bar"', 'flags=(complain)', 'complain', True, 'complain', profile_name='/foo bar')
def test_change_profile_flags_nochange_11(self):
self._test_change_profile_flags('/foo', '(complain)', 'complain', True, 'complain', profile_name=None)
def test_change_profile_flags_nochange_12(self):
# XXX changes the flags for the child profile (which happens to have the same profile name) to 'complain'
self._test_change_profile_flags('/foo', 'flags=(complain)', 'complain', True, 'complain', more_rules=' profile /foo {\n}', expected_more_rules=' profile /foo flags=(complain) {\n}')
# tests that change the flags
def test_set_flags_01(self):
self._test_set_flags('/foo', '', 'audit')
def test_set_flags_02(self):
self._test_set_flags('/foo', '( complain )', 'audit ', whitespace=' ')
def test_set_flags_04(self):
self._test_set_flags('/foo', '(complain)', 'audit')
def test_set_flags_05(self):
self._test_set_flags('/foo', 'flags=(complain)', 'audit')
def test_set_flags_06(self):
self._test_set_flags('/foo', 'flags=(complain, audit)', None, whitespace=' ')
def test_set_flags_07(self):
self._test_set_flags('/foo', 'flags=(complain, audit)', '', expected_flags=None)
def test_set_flags_08(self):
self._test_set_flags('/foo', '( complain )', 'audit ', whitespace=' ', profile_name=None)
def test_set_flags_09(self):
self._test_set_flags('profile /foo', 'flags=(complain)', 'audit')
def test_set_flags_10(self):
self._test_set_flags('profile xy /foo', 'flags=(complain)', 'audit', profile_name='xy')
def test_set_flags_11(self):
self._test_set_flags('profile "/foo bar"', 'flags=(complain)', 'audit', profile_name='/foo bar')
def test_set_flags_12(self):
self._test_set_flags('profile xy "/foo bar"', 'flags=(complain)', 'audit', profile_name='xy')
def test_set_flags_13(self):
self._test_set_flags('/foo', '(audit)', '')
def test_change_profile_flags_01(self):
self._test_change_profile_flags('/foo', '', 'audit', True, 'audit')
def test_change_profile_flags_02(self):
self._test_change_profile_flags('/foo', '( complain )', 'audit', True, 'audit, complain', whitespace=' ')
def test_change_profile_flags_04(self):
self._test_change_profile_flags('/foo', '(complain)', 'audit', True, 'audit, complain')
def test_change_profile_flags_05(self):
self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', True, 'audit, complain')
def test_change_profile_flags_06(self):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', 'complain', False, 'audit', whitespace=' ')
def test_change_profile_flags_07(self):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', 'audit', False, 'complain')
def test_change_profile_flags_08(self):
self._test_change_profile_flags('/foo', '( complain )', 'audit', True, 'audit, complain', whitespace=' ', profile_name=None)
def test_change_profile_flags_09(self):
self._test_change_profile_flags('profile /foo', 'flags=(complain)', 'audit', True, 'audit, complain')
def test_change_profile_flags_10(self):
self._test_change_profile_flags('profile xy /foo', 'flags=(complain)', 'audit', True, 'audit, complain', profile_name='xy')
def test_change_profile_flags_11(self):
self._test_change_profile_flags('profile "/foo bar"', 'flags=(complain)', 'audit', True, 'audit, complain', profile_name='/foo bar')
def test_change_profile_flags_12(self):
self._test_change_profile_flags('profile xy "/foo bar"', 'flags=(complain)', 'audit', True, 'audit, complain', profile_name='xy')
def test_change_profile_flags_13(self):
self._test_change_profile_flags('/foo', '(audit)', 'audit', False, '')
# test handling of hat flags
def test_set_flags_with_hat_01(self):
self._test_set_flags('/foo', 'flags=(complain)', 'audit',
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) {\n}\n'
)
def test_set_flags_with_hat_02(self):
self._test_set_flags('/foo', 'flags=(complain)', 'audit',
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 {\n}\n',
expected_more_rules='\n ^foobar flags=(audit) {\n}\n'
more_rules='\n ^foobar flags=(audit) {\n}\n',
expected_more_rules='\n ^foobar {\n}\n'
)
def test_set_flags_with_hat_03(self):
self._test_set_flags('/foo', 'flags=(complain)', 'audit',
more_rules='\n^foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost!
expected_more_rules='\n^foobar flags=(audit) { # comment\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',
expected_more_rules='\n^foobar flags=(attach_disconnected, audit) { # comment\n}\n'
)
def test_set_flags_with_hat_04(self):
self._test_set_flags('/foo', '', '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'
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',
expected_more_rules='\n hat foobar flags=(attach_disconnected, audit) { # comment\n}\n'
)
def test_set_flags_with_hat_05(self):
self._test_set_flags('/foo', '(audit)', '',
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'
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',
expected_more_rules='\n hat foobar flags=(attach_disconnected) { # comment\n}\n'
)
# test handling of child profiles
def test_set_flags_with_child_01(self):
self._test_set_flags('/foo', 'flags=(complain)', 'audit',
def test_change_profile_flags_with_child_01(self):
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) {\n}\n'
)
#def test_set_flags_with_child_02(self):
def test_change_profile_flags_with_child_02(self):
# XXX child profile flags aren't changed if profile parameter is not None
#self._test_set_flags('/foo', 'flags=(complain)', 'audit',
# more_rules='\n profile /bin/bar {\n}\n',
# expected_more_rules='\n profile /bin/bar flags=(audit) {\n}\n'
#)
self._test_change_profile_flags('/foo', 'flags=(complain)', 'audit', True, 'audit, complain',
more_rules='\n profile /bin/bar {\n}\n',
expected_more_rules='\n profile /bin/bar {\n}\n' # flags(audit) should be added
)
def test_set_flags_invalid_01(self):
def test_change_profile_flags_invalid_01(self):
with self.assertRaises(AppArmorBug):
self._test_set_flags('/foo', '()', None, check_new_flags=False)
def test_set_flags_invalid_02(self):
self._test_change_profile_flags('/foo', '()', None, False, '', check_new_flags=False)
def test_change_profile_flags_invalid_02(self):
with self.assertRaises(AppArmorBug):
self._test_set_flags('/foo', 'flags=()', None, check_new_flags=False)
def test_set_flags_invalid_03(self):
with self.assertRaises(AppArmorException):
self._test_set_flags('/foo', '( )', '', check_new_flags=False)
def test_set_flags_invalid_04(self):
self._test_change_profile_flags('/foo', 'flags=()', None, True, '', check_new_flags=False)
def test_change_profile_flags_invalid_03(self):
with self.assertRaises(AppArmorBug):
self._test_set_flags('/foo', 'flags=(complain, audit)', ' ', check_new_flags=False) # whitespace-only newflags
self._test_change_profile_flags('/foo', '( )', '', True, '', check_new_flags=False)
def test_change_profile_flags_invalid_04(self):
with self.assertRaises(AppArmorBug):
self._test_change_profile_flags('/foo', 'flags=(complain, audit)', ' ', True, 'audit, complain', check_new_flags=False) # whitespace-only newflags
def test_set_flags_other_profile(self):
def test_change_profile_flags_other_profile(self):
# test behaviour if the file doesn't contain the specified /foo profile
orig_prof = '/no-such-profile flags=(complain) {\n}'
self.file = write_file(self.tmpdir, 'profile', orig_prof)
with self.assertRaises(AppArmorBug):
set_profile_flags(self.file, '/foo', 'audit')
with self.assertRaises(AppArmorException):
change_profile_flags(self.file, '/foo', 'audit', True)
# the file should not be changed
real_new_prof = read_file(self.file)
self.assertEqual(orig_prof, real_new_prof)
def test_set_flags_no_profile_found(self):
def test_change_profile_flags_no_profile_found(self):
# test behaviour if the file doesn't contain any profile
orig_prof = '# /comment flags=(complain) {\n# }'
self.file = write_file(self.tmpdir, 'profile', orig_prof)
with self.assertRaises(AppArmorBug):
set_profile_flags(self.file, None, 'audit')
with self.assertRaises(AppArmorException):
change_profile_flags(self.file, None, 'audit', True)
# the file should not be changed
real_new_prof = read_file(self.file)
self.assertEqual(orig_prof, real_new_prof)
def test_set_flags_file_not_found(self):
def test_change_profile_flags_file_not_found(self):
with self.assertRaises(IOError):
set_profile_flags('%s/file-not-found' % self.tmpdir, '/foo', 'audit')
change_profile_flags('%s/file-not-found' % self.tmpdir, '/foo', 'audit', True)
class AaTest_set_options_audit_mode(AATest):
tests = [

View file

@ -13,7 +13,7 @@ import unittest
from common_test import AATest, setup_all_loops
from apparmor.common import AppArmorBug
from apparmor.profile_storage import ProfileStorage, var_transform
from apparmor.profile_storage import ProfileStorage, add_or_remove_flag, split_flags, var_transform
class TestUnknownKey(AATest):
def AASetup(self):
@ -35,6 +35,43 @@ class TestUnknownKey(AATest):
with self.assertRaises(AppArmorBug):
self.storage['foo'] = 'bar'
class AaTest_add_or_remove_flag(AATest):
tests = [
# existing flag(s) flag to change add or remove? expected flags
([ [], 'complain', True ], ['complain'] ),
([ [], 'complain', False ], [] ),
([ ['complain'], 'complain', True ], ['complain'] ),
([ ['complain'], 'complain', False ], [] ),
([ [], 'audit', True ], ['audit'] ),
([ [], 'audit', False ], [] ),
([ ['complain'], 'audit', True ], ['audit', 'complain'] ),
([ ['complain'], 'audit', False ], ['complain'] ),
([ '', 'audit', True ], ['audit'] ),
([ None, 'audit', False ], [] ),
([ 'complain', 'audit', True ], ['audit', 'complain'] ),
([ ' complain ', 'audit', False ], ['complain'] ),
]
def _run_test(self, params, expected):
new_flags = add_or_remove_flag(params[0], params[1], params[2])
self.assertEqual(new_flags, expected)
class AaTest_split_flags(AATest):
tests = [
(None , [] ),
('' , [] ),
(' ' , [] ),
(' , ' , [] ),
('complain' , ['complain'] ),
(' complain attach_disconnected' , ['attach_disconnected', 'complain'] ),
(' complain , attach_disconnected' , ['attach_disconnected', 'complain'] ),
(' complain , , audit , , ' , ['audit', 'complain'] ),
]
def _run_test(self, params, expected):
split = split_flags(params)
self.assertEqual(split, expected)
class AaTest_var_transform(AATest):
tests = [
(['foo', ''], '"" foo' ),