On Fri, Nov 28, 2014 at 07:47:29PM +0100, Christian Boltz wrote: > Am Donnerstag, 27. November 2014 schrieb Steve Beattie: > > If we're going to raise an exception that nobody's going to check for, > > wuld it be better to raise AppArmorBug, as that seems more > > appropriate, that either there's a bug in our code or we got a dodgy > > log message with a weird permission mode. Plus we can attach > > information about why things went wrong. Patch follows: > > So we replace an exception that nobody is going to check for with > another exception (with some additional details) that nobody is going to > check for? ;-)
Err? I thought we were going to at some point do some fancy logging/handling of AppArmorBug exceptions, no? So while in the short term, it doesn't make much difference except in the detailed reporting, I thought getting things set up for where you wanted to go in the future made sense. > > Signed-off-by: Steve Beattie <[email protected]> > > --- > > utils/apparmor/aamode.py | 9 ++++++++- > > utils/test/test-aamode.py | 7 +++++++ > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > Index: b/utils/apparmor/aamode.py > > =================================================================== > > --- a/utils/apparmor/aamode.py > > +++ b/utils/apparmor/aamode.py > > @@ -12,6 +12,7 @@ > > # > > # > > --------------------------------------------------------------------- > > - import re > > +from apparmor.common import AppArmorBug > > > > def AA_OTHER(mode): > > other = set() > > @@ -103,11 +104,17 @@ def split_log_mode(mode): > > other = '' > > > > if "::" in mode: > > - user, other = mode.split("::") > > + try: > > + user, other = mode.split("::") > > + except ValueError as e: > > + raise AppArmorBug("Got ValueError '%s' when splitting %s" > > % (str(e), mode)) > > That gives us a better error message than implicitely raising a > ValueError, and isn't measurable in the execution time. So even if I > joked about it above, it's an improvement. > > > else: > > user = mode > > other = mode > > #print ('split_logmode:', user, mode) > > Unrelated: please delete the "#print" line before commiting. I didn't add it, but sure. > > + if ":" in user or ":" in other: > > + raise AppArmorBug("After splitting %s, user (%s) or other > > (%s) contained ':' " % (mode, user, other)) > > This check makes aa-logprof slightly slower (~2% - 1:10 instead of 1:09 > runtime for 20 aa-logprof runs on a 4 MB audit.log). > > Valid data might be worth that - OTOH, the function doesn't check if > user or other contain unknown letters, numbers or even special chars > (splitting "yes!!11!!::no§42" will work successfully, at least until you > try to use the result ;-) > > So I'm undecided if adding this check really makes sense - given that we > only check for ":" and allow any other strange character, it feels like > a drop in the ocean ;-) Well, do we check for sanity on permissions anywhere else that would detect if we got a wonky log entry? Somewhere we should ensure that we don't write out garbage to a profile, right? > > def mode_contains(mode, subset): > > Index: b/utils/test/test-aamode.py > > =================================================================== > > --- a/utils/test/test-aamode.py > > +++ b/utils/test/test-aamode.py > > @@ -12,6 +12,7 @@ > > import unittest > > > > from apparmor.aamode import split_log_mode, sub_str_to_mode > > +from apparmor.common import AppArmorBug > > > > class AamodeTest_split_log_mode(unittest.TestCase): > > def test_split_log_mode_1(self): > > @@ -26,6 +27,12 @@ class AamodeTest_split_log_mode(unittest > > self.assertEqual(split_log_mode('r::w'), ('r', 'w')) > > def test_split_log_mode_6(self): > > self.assertEqual(split_log_mode('rw::rw'), ('rw', 'rw')) > > + def test_split_log_mode_invalid_1(self): > > + with self.assertRaises(AppArmorBug): > > + split_log_mode('r::w::r') > > + def test_split_log_mode_invalid_2(self): > > + with self.assertRaises(AppArmorBug): > > + split_log_mode('r:::r') > > With or without the check for ":": > Acked-by: Christian Boltz <[email protected]> > > (If you don't include the check for ":", you'll also have to remove > test_split_log_mode_invalid_2() test) I'll drop it for now. -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
