From 7cfda2772d5dc76e7feb536afc51346d8babca42 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sat, 26 Dec 2020 22:39:11 +0100 Subject: [PATCH 1/2] split off parse_profile_start_to_storage() from parse_profile_data() parse_profile_start_to_storage() converts the result of parse_profile_start() into a ProfileStorage object. No functional change, but parse_profile_data() becomes more readable. Also extend tests to cover parse_profile_start_to_storage(). --- utils/apparmor/aa.py | 39 +++++++++++++++++++++++---------------- utils/test/test-aa.py | 19 ++++++++++++++++++- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index d2d8fb9ee..cde834573 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1812,6 +1812,27 @@ def parse_profile_start(line, file, lineno, profile, hat): return (profile, hat, attachment, xattrs, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) +def parse_profile_start_to_storage(line, file, lineno, profile, hat): + ''' parse a profile start line (using parse_profile_startline()) and convert it to a ProfileStorage ''' + + (profile, hat, attachment, xattrs, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) = parse_profile_start(line, file, lineno, profile, hat) + + prof_storage = ProfileStorage(profile, hat, 'parse_profile_data() profile_start') + + if attachment: + prof_storage['attachment'] = attachment + if pps_set_profile: + prof_storage['profile'] = True + if pps_set_hat_external: + prof_storage['external'] = True + + prof_storage['name'] = profile + prof_storage['filename'] = file + prof_storage['xattrs'] = xattrs + prof_storage['flags'] = flags + + return (profile, hat, in_contained_hat, prof_storage) + def parse_profile_data(data, file, do_include): profile_data = hasher() profile = None @@ -1837,27 +1858,13 @@ def parse_profile_data(data, file, do_include): lastline = None # Starting line of a profile if RE_PROFILE_START.search(line): - (profile, hat, attachment, xattrs, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) = parse_profile_start(line, file, lineno, profile, hat) + (profile, hat, in_contained_hat, prof_storage) = parse_profile_start_to_storage(line, file, lineno, profile, hat) if profile_data[profile].get(hat, False): raise AppArmorException('Profile %(profile)s defined twice in %(file)s, last found in line %(line)s' % { 'file': file, 'line': lineno + 1, 'profile': combine_name(profile, hat) }) - profile_data[profile][hat] = ProfileStorage(profile, hat, 'parse_profile_data() profile_start') - - if attachment: - profile_data[profile][hat]['attachment'] = attachment - if pps_set_profile: - profile_data[profile][hat]['profile'] = True - if pps_set_hat_external: - profile_data[profile][hat]['external'] = True - - # save profile name and filename - profile_data[profile][hat]['name'] = profile - profile_data[profile][hat]['filename'] = file - - profile_data[profile][hat]['xattrs'] = xattrs - profile_data[profile][hat]['flags'] = flags + profile_data[profname] = prof_storage # Save the initial comment if initial_comment: diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index 0c6c7d709..61eaa332c 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -20,7 +20,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, change_profile_flags, set_options_audit_mode, set_options_owner_mode, is_skippable_file, is_skippable_dir, - parse_profile_start, parse_profile_data, write_header, + parse_profile_start, parse_profile_start_to_storage, parse_profile_data, write_header, get_file_perms, propose_file_rules) from apparmor.aare import AARE from apparmor.common import AppArmorException, AppArmorBug @@ -524,6 +524,20 @@ class AaTest_parse_profile_start(AATest): self.assertEqual(parsed, expected) + (profile, hat, in_contained_hat, prof_storage) = parse_profile_start_to_storage(params[0], 'somefile', 1, params[1], params[2]) + + self.assertEqual(profile, expected[0]) + self.assertEqual(hat, expected[1]) + if expected[2] is None: + self.assertEqual(prof_storage['attachment'], '') + else: + self.assertEqual(prof_storage['attachment'], expected[2]) + self.assertEqual(prof_storage['xattrs'], expected[3]) + self.assertEqual(prof_storage['flags'], expected[4]) + self.assertEqual(in_contained_hat, expected[5]) + self.assertEqual(prof_storage['profile'], expected[6]) + self.assertEqual(prof_storage['external'], expected[7]) + class AaTest_parse_profile_start_errors(AATest): tests = [ (('/foo///bar///baz {', None, None), AppArmorException), # XXX deeply nested external hat @@ -535,6 +549,9 @@ class AaTest_parse_profile_start_errors(AATest): with self.assertRaises(expected): parse_profile_start(params[0], 'somefile', 1, params[1], params[2]) + with self.assertRaises(expected): + parse_profile_start_to_storage(params[0], 'somefile', 1, params[1], params[2]) + class AaTest_parse_profile_data(AATest): def test_parse_empty_profile_01(self): prof = parse_profile_data('/foo {\n}\n'.split(), 'somefile', False) From f7a365f89f6eaa481a6a5a72ae674692bcc2153d Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Sun, 14 Feb 2021 20:09:31 +0100 Subject: [PATCH 2/2] Simplify handling of in_contained_hat in_contained_hat is needed to know if we are already in a profile or not. (Simply checking if we are in a hat doesn't work, because something like "profile foo//bar" will set profile and hat at once, and later (wrongfully) expect another "}". However, the way how this variable was set became too complicated. To simplify the code, set in_contained_hat directly in parse_profile_data() RE_PROFILE_START instead of returning it via parse_profile_start() and parse_profile_start_to_storage() Since this change removes a return value from two functions, also adjust the tests accordingly. --- utils/apparmor/aa.py | 20 +++++++++++++------- utils/test/test-aa.py | 27 +++++++++++++-------------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index cde834573..64ffc2708 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1788,7 +1788,6 @@ def parse_profile_start(line, file, lineno, profile, hat): 'profile': profile, 'file': file, 'line': lineno + 1 }) hat = matches['profile'] - in_contained_hat = True pps_set_profile = True pps_set_hat_external = False @@ -1803,19 +1802,18 @@ def parse_profile_start(line, file, lineno, profile, hat): hat = profile pps_set_hat_external = False - in_contained_hat = False pps_set_profile = False attachment = matches['attachment'] flags = matches['flags'] xattrs = matches['xattrs'] - return (profile, hat, attachment, xattrs, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) + return (profile, hat, attachment, xattrs, flags, pps_set_profile, pps_set_hat_external) def parse_profile_start_to_storage(line, file, lineno, profile, hat): ''' parse a profile start line (using parse_profile_startline()) and convert it to a ProfileStorage ''' - (profile, hat, attachment, xattrs, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) = parse_profile_start(line, file, lineno, profile, hat) + (profile, hat, attachment, xattrs, flags, pps_set_profile, pps_set_hat_external) = parse_profile_start(line, file, lineno, profile, hat) prof_storage = ProfileStorage(profile, hat, 'parse_profile_data() profile_start') @@ -1831,7 +1829,7 @@ def parse_profile_start_to_storage(line, file, lineno, profile, hat): prof_storage['xattrs'] = xattrs prof_storage['flags'] = flags - return (profile, hat, in_contained_hat, prof_storage) + return (profile, hat, prof_storage) def parse_profile_data(data, file, do_include): profile_data = hasher() @@ -1858,13 +1856,21 @@ def parse_profile_data(data, file, do_include): lastline = None # Starting line of a profile if RE_PROFILE_START.search(line): - (profile, hat, in_contained_hat, prof_storage) = parse_profile_start_to_storage(line, file, lineno, profile, hat) + # in_contained_hat is needed to know if we are already in a profile or not. (Simply checking if we are in a hat doesn't work, + # because something like "profile foo//bar" will set profile and hat at once, and later (wrongfully) expect another "}". + # The logic is simple and resembles a "poor man's stack" (with limited/hardcoded height). + if profile: + in_contained_hat = True + else: + in_contained_hat = False + + (profile, hat, prof_storage) = parse_profile_start_to_storage(line, file, lineno, profile, hat) if profile_data[profile].get(hat, False): raise AppArmorException('Profile %(profile)s defined twice in %(file)s, last found in line %(line)s' % { 'file': file, 'line': lineno + 1, 'profile': combine_name(profile, hat) }) - profile_data[profname] = prof_storage + profile_data[profile][hat] = prof_storage # Save the initial comment if initial_comment: diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index 61eaa332c..448ee6ae4 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -507,16 +507,16 @@ class AaTest_is_skippable_dir(AATest): class AaTest_parse_profile_start(AATest): tests = [ - # profile start line profile hat profile hat attachment xattrs flags in_contained_hat, pps_set_profile, pps_set_hat_external - (('/foo {', None, None), ('/foo', '/foo', None, None, None, False, False, False)), - (('/foo (complain) {', None, None), ('/foo', '/foo', None, None, 'complain', False, False, False)), - (('profile foo /foo {', None, None), ('foo', 'foo', '/foo', None, None, False, False, False)), # named profile - (('profile /foo {', '/bar', '/bar'), ('/bar', '/foo', None, None, None, True, True, False)), # child profile - (('/foo//bar {', None, None), ('/foo', 'bar', None, None, None, False, False, True )), # external hat - (('profile "/foo" (complain) {', None, None), ('/foo', '/foo', None, None, 'complain', False, False, False)), - (('profile "/foo" xattrs=(user.bar=bar) {', None, None), ('/foo', '/foo', None, 'user.bar=bar', None, False, False, False)), - (('profile "/foo" xattrs=(user.bar=bar user.foo=*) {', None, None), ('/foo', '/foo', None, 'user.bar=bar user.foo=*', None, False, False, False)), - (('/usr/bin/xattrs-test xattrs=(myvalue="foo.bar") {', None, None), ('/usr/bin/xattrs-test', '/usr/bin/xattrs-test', None, 'myvalue="foo.bar"', None, False, False, False)), + # profile start line profile hat profile hat attachment xattrs flags pps_set_profile, pps_set_hat_external + (('/foo {', None, None), ('/foo', '/foo', None, None, None, False, False)), + (('/foo (complain) {', None, None), ('/foo', '/foo', None, None, 'complain', False, False)), + (('profile foo /foo {', None, None), ('foo', 'foo', '/foo', None, None, False, False)), # named profile + (('profile /foo {', '/bar', '/bar'), ('/bar', '/foo', None, None, None, True, False)), # child profile + (('/foo//bar {', None, None), ('/foo', 'bar', None, None, None, False, True )), # external hat + (('profile "/foo" (complain) {', None, None), ('/foo', '/foo', None, None, 'complain', False, False)), + (('profile "/foo" xattrs=(user.bar=bar) {', None, None), ('/foo', '/foo', None, 'user.bar=bar', None, False, False)), + (('profile "/foo" xattrs=(user.bar=bar user.foo=*) {', None, None), ('/foo', '/foo', None, 'user.bar=bar user.foo=*', None, False, False)), + (('/usr/bin/xattrs-test xattrs=(myvalue="foo.bar") {', None, None), ('/usr/bin/xattrs-test', '/usr/bin/xattrs-test', None, 'myvalue="foo.bar"', None, False, False)), ] def _run_test(self, params, expected): @@ -524,7 +524,7 @@ class AaTest_parse_profile_start(AATest): self.assertEqual(parsed, expected) - (profile, hat, in_contained_hat, prof_storage) = parse_profile_start_to_storage(params[0], 'somefile', 1, params[1], params[2]) + (profile, hat, prof_storage) = parse_profile_start_to_storage(params[0], 'somefile', 1, params[1], params[2]) self.assertEqual(profile, expected[0]) self.assertEqual(hat, expected[1]) @@ -534,9 +534,8 @@ class AaTest_parse_profile_start(AATest): self.assertEqual(prof_storage['attachment'], expected[2]) self.assertEqual(prof_storage['xattrs'], expected[3]) self.assertEqual(prof_storage['flags'], expected[4]) - self.assertEqual(in_contained_hat, expected[5]) - self.assertEqual(prof_storage['profile'], expected[6]) - self.assertEqual(prof_storage['external'], expected[7]) + self.assertEqual(prof_storage['profile'], expected[5]) + self.assertEqual(prof_storage['external'], expected[6]) class AaTest_parse_profile_start_errors(AATest): tests = [