From 38b1e3d30f39953afac69e2ac6110db46f0c752c Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Fri, 3 Apr 2015 17:20:14 +0200 Subject: [PATCH] rewrite set_profile_flags() to use write_header() Changes in set_profile_flags(): - rewrite set_profile_flags to use parse_profile_start_line() and write_header(). - replace the silent failure for non-existing files with a proper exception (using lazy programming - the check is done by removing the "if os.path.isfile()" check, open_file_read then raises the exception ;-) - comment out regex_hat_flag and the code that was supposed to handle hat flags, which were totally broken. We'll need another patch to fix it, and we also need to decide if we want to do that because it introduces a behaviour change (currently, aa-complain etc. don't change hat flags). The tests for set_profile_flags() are also updated: - prepend a space to comments because write_header always adds a space between '{' and the comment - remove a test with superfluous quotes that are no longer kept (that's just a profile cleanup, so dropping that test is the easiest way) - update test_set_flags_10 and test_set_flags_12 to use the correct profile name - enable the tests for invalid (empty) flags - update the test for a non-existing file Note: test_set_flags_10, test_set_flags_12 and test_set_flags_nochange_09 will fail with this patch applied. The next patch will fix that. Acked-by: Steve Beattie --- utils/apparmor/aa.py | 72 +++++++++++++++++++------------------------ utils/test/test-aa.py | 37 ++++++++++------------ 2 files changed, 48 insertions(+), 61 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 261254357..f349ee674 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -649,49 +649,39 @@ def change_profile_flags(filename, program, flag, set_flag): def set_profile_flags(prof_filename, program, newflags): """Reads the old profile file and updates the flags accordingly""" - regex_bin_flag = re.compile('^(\s*)("?(/.+?)"??|(profile\s+"?(.+?)"??))\s+((flags=)?\((.*)\)\s+)?\{\s*(#.*)?$') - # TODO: use RE_PROFILE_START (only difference: doesn't have a match group for the leading space) - # TODO: also use the global regex for matching the hat # 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 - regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)\s*(#.*)?$') - if os.path.isfile(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) - shutil.copymode(prof_filename, temp_file.name) - with open_file_write(temp_file.name) as f_out: - for line in f_in: - comment = '' - if '#' in line: - comment = '#' + line.split('#', 1)[1].rstrip() - match = regex_bin_flag.search(line) - if not line.strip() or line.strip().startswith('#'): - pass - elif match: - matches = match.groups() - space = matches[0] - profile = matches[1] # profile name including quotes and "profile" keyword - if matches[2]: - binary = matches[2] - else: - binary = matches[4] - flag = matches[6] or 'flags=' - flags = matches[7] - if binary == program or program is None: - if newflags: - line = '%s%s %s(%s) {%s\n' % (space, profile, flag, newflags, comment) - else: - line = '%s%s {%s\n' % (space, profile, comment) - else: - match = regex_hat_flag.search(line) - if match: - hat, flags = match.groups()[:2] - if newflags: - line = '%s flags=(%s) {%s\n' % (hat, newflags, comment) - else: - line = '%s {%s\n' % (hat, comment) - f_out.write(line) - os.rename(temp_file.name, prof_filename) + # TODO: use RE_PROFILE_HAT_DEF for matching the hat (regex_hat_flag is totally broken!) + #regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)\s*(#.*)?$') + with open_file_read(prof_filename) as f_in: + temp_file = tempfile.NamedTemporaryFile('w', prefix=prof_filename, suffix='~', delete=False, dir=profile_dir) + shutil.copymode(prof_filename, temp_file.name) + with open_file_write(temp_file.name) as f_out: + for line in f_in: + if RE_PROFILE_START.search(line): + matches = parse_profile_start_line(line, prof_filename) + space = matches['leadingspace'] or '' + profile = matches['profile'] + + if profile == program or program is None: + header_data = { + 'attachment': matches['attachment'] or '', + 'flags': newflags, + 'profile_keyword': matches['profile_keyword'], + 'header_comment': matches['comment'] or '', + } + line = write_header(header_data, len(space)/2, profile, False, True) + line = '%s\n' % line[0] + #else: + # match = regex_hat_flag.search(line) + # if match: + # hat, flags = match.groups()[:2] + # if newflags: + # line = '%s flags=(%s) {%s\n' % (hat, newflags, comment) + # else: + # line = '%s {%s\n' % (hat, comment) + f_out.write(line) + os.rename(temp_file.name, prof_filename) 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 5e90d78a9..5018fe875 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -119,6 +119,9 @@ class AaTest_set_profile_flags(AaTestWithTempdir): else: expected_flags = '' + if comment: + comment = ' %s' % comment + dummy_profile_content = ' #include \n capability chown,\n /bar r,' prof_template = '%s%s%s {%s\n%s\n%s\n}\n' old_prof = prof_template % (whitespace, profile, old_flags, comment, more_rules, dummy_profile_content) @@ -150,10 +153,8 @@ class AaTest_set_profile_flags(AaTestWithTempdir): def test_set_flags_nochange_09(self): self._test_set_flags('profile xy /foo', 'flags=(complain)', 'complain', profile_name='xy /foo') # XXX profile_name should be 'xy' def test_set_flags_nochange_10(self): - self._test_set_flags('profile "/foo"', 'flags=(complain)', 'complain') # superfluous quotes are kept - def test_set_flags_nochange_11(self): self._test_set_flags('profile "/foo bar"', 'flags=(complain)', 'complain', profile_name='/foo bar') - #def test_set_flags_nochange_12(self): + #def test_set_flags_nochange_11(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}') @@ -177,11 +178,11 @@ class AaTest_set_profile_flags(AaTestWithTempdir): 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 /foo') # XXX profile_name should be just 'xy' + 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 "/foo bar') # profile name should just be 'xy' + self._test_set_flags('profile xy "/foo bar"', 'flags=(complain)', 'audit', profile_name='xy') # XXX regex_hat_flag in set_profile_flags() is totally broken - it matches for ' ' and ' X ', but doesn't match for ' ^foo {' @@ -190,16 +191,15 @@ class AaTest_set_profile_flags(AaTestWithTempdir): # self._test_set_flags(' ^hat', '', 'audit') - # XXX current code/regex doesn't check for empty flags - #def test_set_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): - # 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_01(self): + with self.assertRaises(AppArmorBug): + self._test_set_flags('/foo', '()', None, check_new_flags=False) + def test_set_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_other_profile(self): # test behaviour if the file doesn't contain the specified /foo profile @@ -214,11 +214,8 @@ class AaTest_set_profile_flags(AaTestWithTempdir): self.assertEqual(orig_prof, real_new_prof) def test_set_flags_file_not_found(self): - # XXX this exits silently - # the easiest solution would be to drop the - # if os.path.isfile(prof_filename): - # check and let open_file_read raise an exception - set_profile_flags('%s/file-not-found' % self.tmpdir, '/foo', 'audit') + with self.assertRaises(IOError): + set_profile_flags('%s/file-not-found' % self.tmpdir, '/foo', 'audit') class AaTest_is_skippable_file(AATest):