From ef0d67582475dc3c1af203b10a5c697aa55f2ee9 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Mon, 13 Apr 2020 15:13:12 +0200 Subject: [PATCH] Get rid of is_covered_aare_compat() This function was introduced as a temporary (ahem...) solution in 95404bb2f390ef743e289dc1da9a381bad168fbe but was never really correct. It checked against other_value.regex (as a string!) and, while this was somewhat generous in the results, could have unintended side effects. Better error out on the safe side and add/keep a few superfluous rules than having a wrong match in is_covered() and miss to add/keep a rule that would be needed. The perfect solution would be to really compare one AARE against the other as the parser does. I'm not too keen to implement this in python, and will wait until someone provides this function (which the parser already has) via libapparmor ;-) --- utils/apparmor/rule/__init__.py | 9 --------- utils/apparmor/rule/dbus.py | 14 +++++++------- utils/apparmor/rule/ptrace.py | 2 +- utils/apparmor/rule/signal.py | 2 +- utils/test/test-dbus.py | 6 +++--- utils/test/test-ptrace.py | 6 +++--- utils/test/test-signal.py | 6 +++--- 7 files changed, 18 insertions(+), 27 deletions(-) diff --git a/utils/apparmor/rule/__init__.py b/utils/apparmor/rule/__init__.py index 08eb3108e..8191db547 100644 --- a/utils/apparmor/rule/__init__.py +++ b/utils/apparmor/rule/__init__.py @@ -192,15 +192,6 @@ class BaseRule(object): # still here? -> then it is covered return True - def _is_covered_aare_compat(self, self_value, self_all, other_value, other_all, cond_name): - '''check if other_* is covered by self_* - for AARE - Note: this function checks against other_value.regex, which is not really correct, but avoids overly strict results when matching one regex against another - ''' - if type(other_value) == AARE: - other_value = other_value.regex - - return self._is_covered_aare(self_value, self_all, other_value, other_all, cond_name) - def _is_covered_aare(self, self_value, self_all, other_value, other_all, cond_name): '''check if other_* is covered by self_* - for AARE''' diff --git a/utils/apparmor/rule/dbus.py b/utils/apparmor/rule/dbus.py index 2510bba20..2d5fe6d47 100644 --- a/utils/apparmor/rule/dbus.py +++ b/utils/apparmor/rule/dbus.py @@ -240,25 +240,25 @@ class DbusRule(BaseRule): if not self._is_covered_list(self.access, self.all_access, other_rule.access, other_rule.all_access, 'access'): return False - if not self._is_covered_aare_compat(self.bus, self.all_buses, other_rule.bus, other_rule.all_buses, 'bus'): + if not self._is_covered_aare(self.bus, self.all_buses, other_rule.bus, other_rule.all_buses, 'bus'): return False - if not self._is_covered_aare_compat(self.path, self.all_paths, other_rule.path, other_rule.all_paths, 'path'): + if not self._is_covered_aare(self.path, self.all_paths, other_rule.path, other_rule.all_paths, 'path'): return False - if not self._is_covered_aare_compat(self.name, self.all_names, other_rule.name, other_rule.all_names, 'name'): + if not self._is_covered_aare(self.name, self.all_names, other_rule.name, other_rule.all_names, 'name'): return False - if not self._is_covered_aare_compat(self.interface, self.all_interfaces, other_rule.interface, other_rule.all_interfaces, 'interface'): + if not self._is_covered_aare(self.interface, self.all_interfaces, other_rule.interface, other_rule.all_interfaces, 'interface'): return False - if not self._is_covered_aare_compat(self.member, self.all_members, other_rule.member, other_rule.all_members, 'member'): + if not self._is_covered_aare(self.member, self.all_members, other_rule.member, other_rule.all_members, 'member'): return False - if not self._is_covered_aare_compat(self.peername, self.all_peernames, other_rule.peername, other_rule.all_peernames, 'peername'): + if not self._is_covered_aare(self.peername, self.all_peernames, other_rule.peername, other_rule.all_peernames, 'peername'): return False - if not self._is_covered_aare_compat(self.peerlabel, self.all_peerlabels, other_rule.peerlabel, other_rule.all_peerlabels, 'peerlabel'): + if not self._is_covered_aare(self.peerlabel, self.all_peerlabels, other_rule.peerlabel, other_rule.all_peerlabels, 'peerlabel'): return False # still here? -> then it is covered diff --git a/utils/apparmor/rule/ptrace.py b/utils/apparmor/rule/ptrace.py index a82d06a48..db230c5c3 100644 --- a/utils/apparmor/rule/ptrace.py +++ b/utils/apparmor/rule/ptrace.py @@ -138,7 +138,7 @@ class PtraceRule(BaseRule): if not self._is_covered_list(self.access, self.all_access, other_rule.access, other_rule.all_access, 'access'): return False - if not self._is_covered_aare_compat(self.peer, self.all_peers, other_rule.peer, other_rule.all_peers, 'peer'): + if not self._is_covered_aare(self.peer, self.all_peers, other_rule.peer, other_rule.all_peers, 'peer'): return False # still here? -> then it is covered diff --git a/utils/apparmor/rule/signal.py b/utils/apparmor/rule/signal.py index e37fec8d7..af83f920d 100644 --- a/utils/apparmor/rule/signal.py +++ b/utils/apparmor/rule/signal.py @@ -188,7 +188,7 @@ class SignalRule(BaseRule): if not self._is_covered_list(self.signal, self.all_signals, other_rule.signal, other_rule.all_signals, 'signal'): return False - if not self._is_covered_aare_compat(self.peer, self.all_peers, other_rule.peer, other_rule.all_peers, 'peer'): + if not self._is_covered_aare(self.peer, self.all_peers, other_rule.peer, other_rule.all_peers, 'peer'): return False # still here? -> then it is covered diff --git a/utils/test/test-dbus.py b/utils/test/test-dbus.py index 3275d7572..09e7d10af 100644 --- a/utils/test/test-dbus.py +++ b/utils/test/test-dbus.py @@ -606,9 +606,9 @@ class DbusCoveredTest_09(DbusCoveredTest): ('dbus,' , [ False , False , False , False ]), ('dbus send,' , [ False , False , False , False ]), ('dbus send member=/foo/bar,' , [ False , False , True , True ]), - ('dbus send member=/foo/*,' , [ False , False , True , True ]), - ('dbus send member=/**,' , [ False , False , True , True ]), - ('dbus send member=/what/*,' , [ False , False , True , True ]), + ('dbus send member=/foo/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('dbus send member=/**,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('dbus send member=/what/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() ('dbus member=/foo/bar,' , [ False , False , False , False ]), ('dbus send, # comment' , [ False , False , False , False ]), ('allow dbus send,' , [ False , False , False , False ]), diff --git a/utils/test/test-ptrace.py b/utils/test/test-ptrace.py index b8e9723ce..6705b2f1c 100644 --- a/utils/test/test-ptrace.py +++ b/utils/test/test-ptrace.py @@ -368,9 +368,9 @@ class PtraceCoveredTest_07(PtraceCoveredTest): ('ptrace,' , [ False , False , False , False ]), ('ptrace read,' , [ False , False , False , False ]), ('ptrace read peer=/foo/bar,' , [ False , False , True , True ]), - ('ptrace read peer=/foo/*,' , [ False , False , True , True ]), - ('ptrace read peer=/**,' , [ False , False , True , True ]), - ('ptrace read peer=/what/*,' , [ False , False , True , True ]), + ('ptrace read peer=/foo/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('ptrace read peer=/**,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('ptrace read peer=/what/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() ('ptrace peer=/foo/bar,' , [ False , False , False , False ]), ('ptrace read, # comment' , [ False , False , False , False ]), ('allow ptrace read,' , [ False , False , False , False ]), diff --git a/utils/test/test-signal.py b/utils/test/test-signal.py index 06d2fd167..1a0a7b736 100644 --- a/utils/test/test-signal.py +++ b/utils/test/test-signal.py @@ -416,9 +416,9 @@ class SignalCoveredTest_08(SignalCoveredTest): ('signal,' , [ False , False , False , False ]), ('signal send,' , [ False , False , False , False ]), ('signal send peer=/foo/bar,' , [ False , False , True , True ]), - ('signal send peer=/foo/*,' , [ False , False , True , True ]), - ('signal send peer=/**,' , [ False , False , True , True ]), - ('signal send peer=/what/*,' , [ False , False , True , True ]), + ('signal send peer=/foo/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('signal send peer=/**,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() + ('signal send peer=/what/*,' , [ False , False , False , False ]), # TODO: wildcard vs. wildcard never matches in is_covered_aare() ('signal peer=/foo/bar,' , [ False , False , False , False ]), ('signal send, # comment' , [ False , False , False , False ]), ('allow signal send,' , [ False , False , False , False ]),