From 90500d38ab8d5f8c3d3261486fa83c1b6d4ba176 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 17 Sep 2024 09:23:30 +0000 Subject: [PATCH] Merge utils: fixes when handling owner file rules Fixes: https://gitlab.com/apparmor/apparmor/-/issues/429 Fixes: https://gitlab.com/apparmor/apparmor/-/issues/430 Closes #429 and #430 MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1320 Approved-by: John Johansen Merged-by: John Johansen (cherry picked from commit 1940b1b7cd779f8ae2892054278c5f6fc7b15f8c) Signed-off-by: Georgia Garcia --- utils/apparmor/rule/file.py | 19 ++++++++++++++++--- utils/test/test-file.py | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/utils/apparmor/rule/file.py b/utils/apparmor/rule/file.py index 081657b20..fc4f6a99c 100644 --- a/utils/apparmor/rule/file.py +++ b/utils/apparmor/rule/file.py @@ -26,6 +26,7 @@ allow_exec_transitions = ('ix', 'ux', 'Ux', 'px', 'Px', 'cx', 'Cx') # 2 chars - allow_exec_fallback_transitions = ('pix', 'Pix', 'cix', 'Cix', 'pux', 'PUx', 'cux', 'CUx') # 3 chars - len relevant for split_perms() deny_exec_transitions = ('x') file_permissions = ('m', 'r', 'w', 'a', 'l', 'k', 'link', 'subset') # also defines the write order +implicit_all_permissions = ('m', 'r', 'w', 'l', 'k') class FileRule(BaseRule): @@ -228,7 +229,7 @@ class FileRule(BaseRule): if self.all_paths and self.all_perms and not path and not perms and not target: return ('%s%s%sfile,%s' % (space, self.modifiers_str(), owner, self.comment)) # plain 'file,' rule elif not self.all_paths and not self.all_perms and path and perms: - return ('%s%s%s%s%s%s,%s' % (space, self.modifiers_str(), file_keyword, owner, path_and_perms, target, self.comment)) + return ('%s%s%s%s%s%s,%s' % (space, self.modifiers_str(), owner, file_keyword, path_and_perms, target, self.comment)) else: raise AppArmorBug('Invalid combination of path and perms in file rule - either specify path and perms, or none of them') @@ -356,9 +357,21 @@ class FileRule(BaseRule): old_mode = '' if self.original_perms: - original_perms_all = self._join_given_perms(self.original_perms['allow']['all'], None) + original_perms_set = {} + for who in ['all', 'owner']: + original_perms_set[who] = {} + original_perms_set[who]['perms'] = self.original_perms['allow'][who] + original_perms_set[who]['exec_perms'] = None + + if self.original_perms['allow'][who] == FileRule.ALL: + original_perms_set[who]['perms'] = set(implicit_all_permissions) + original_perms_set[who]['exec_perms'] = 'ix' + + original_perms_all = self._join_given_perms(original_perms_set['all']['perms'], + original_perms_set['all']['exec_perms']) original_perms_owner = self._join_given_perms( - self.original_perms['allow']['owner'] - self.original_perms['allow']['all'], None) # only list owner perms that are not covered by other perms + original_perms_set['owner']['perms'] - original_perms_set['all']['perms'], # only list owner perms that are not covered by other perms + original_perms_set['owner']['exec_perms']) if original_perms_all and original_perms_owner: old_mode = '%s + owner %s' % (original_perms_all, original_perms_owner) diff --git a/utils/test/test-file.py b/utils/test/test-file.py index dc908b4c2..62b3cf5ed 100644 --- a/utils/test/test-file.py +++ b/utils/test/test-file.py @@ -406,6 +406,9 @@ class WriteFileTest(AATest): (' deny file /foo r,', 'deny file /foo r,'), (' deny file /foo wr,', 'deny file /foo rw,'), (' allow file /foo Pxrm -> bar,', 'allow file /foo mrPx -> bar,'), + (' deny owner file /foo r,', 'deny owner file /foo r,'), + (' deny owner file /foo wr,', 'deny owner file /foo rw,'), + (' allow owner file /foo Pxrm -> bar,', 'allow owner file /foo mrPx -> bar,'), (' deny owner /foo r,', 'deny owner /foo r,'), (' deny owner /foo wr,', 'deny owner /foo rw,'), (' allow owner /foo Pxrm -> bar,', 'allow owner /foo mrPx -> bar,'), @@ -420,6 +423,9 @@ class WriteFileTest(AATest): (' deny file r /foo,', 'deny file r /foo,'), (' deny file wr /foo ,', 'deny file rw /foo,'), (' allow file Pxmr /foo -> bar,', 'allow file mrPx /foo -> bar,'), + (' deny owner file r /foo ,', 'deny owner file r /foo,'), + (' deny owner file wr /foo ,', 'deny owner file rw /foo,'), + (' allow owner file Pxrm /foo -> bar,', 'allow owner file mrPx /foo -> bar,'), (' deny owner r /foo ,', 'deny owner r /foo,'), (' deny owner wr /foo ,', 'deny owner rw /foo,'), (' allow owner Pxrm /foo -> bar,', 'allow owner mrPx /foo -> bar,'), @@ -864,6 +870,16 @@ class FileLogprofHeaderTest(AATest): obj.original_perms = {'allow': {'all': set(), 'owner': set()}} self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('New Mode'), _('rw')]) + def test_implicit_original_perms(self): + obj = FileRule.create_instance('/foo rw,') + obj.original_perms = {'allow': {'all': FileRule.ALL, 'owner': set()}} + self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('Old Mode'), _('mrwlkix'), _('New Mode'), _('rw')]) + + def test_owner_implicit_original_perms(self): + obj = FileRule.create_instance('/foo rw,') + obj.original_perms = {'allow': {'all': set(), 'owner': FileRule.ALL}} + self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('Old Mode'), _('owner mrwlkix'), _('New Mode'), _('rw')]) + class FileEditHeaderTest(AATest): def _run_test(self, params, expected):