Hello, (back from the openSUSE Conference, which was once more interesting and following the "have a lot of fun..." motto :-) )
BTW: One of the talks was about a new "MySQL Firewall" which does whitelisting of allowed queries - basically AppArmor for MySQL ;-) (Link to the video on request.) Am Samstag, 25. Juni 2016, 15:59:06 CEST schrieb Tyler Hicks: > https://launchpad.net/bugs/1584069 > > This patch adds support for the safe and unsafe exec modes for > change_profile rules. The logic is pretty simple at this point because > the kernel's default for exec modes changed in newer versions. > Therefore, this patch simply retains any specified exec mode in > parsed rules. If an exec mode is not specified in a rule, there is no > attempt to force the usage of "safe" because older kernels do not > support it. Looks like I was too busy with other things in the last weeks ;-) Thanks for the patch! Some comments inline - nothing terribly wrong, but nevertheless I'm looking forward for v2 ;-) > Signed-off-by: Tyler Hicks <tyhi...@canonical.com> > --- > utils/apparmor/regex.py | 2 + > utils/apparmor/rule/change_profile.py | 30 +++++++- > utils/test/test-change_profile.py | 135 > +++++++++++++++++++-------------- > utils/test/test-parser-simple-tests.py | 5 ++ > 4 files changed, 112 insertions(+), 60 deletions(-) > > diff --git a/utils/apparmor/regex.py b/utils/apparmor/regex.py > index 756c3e0..75d1042 100644 > --- a/utils/apparmor/regex.py > +++ b/utils/apparmor/regex.py > @@ -30,6 +30,7 @@ RE_PROFILE_NAME = '(?P<%s>(\S+|"[^"]+"))' > # string without spaces, or RE_PATH = '/\S+|"/[^"]+"' > # filename (starting with '/') without spaces, or quoted filename. > RE_PROFILE_PATH = '(?P<%s>(' + RE_PATH + '))' # quoted or > unquoted filename. %s is the match group name RE_PROFILE_PATH_OR_VAR > = '(?P<%s>(' + RE_PATH + '|@{\S+}\S*|"@{\S+}[^"]*"))' # quoted or > unquoted filename or variable. %s is the match group name > +RE_SAFE_OR_UNSAFE = '(?P<%s>(safe|unsafe))' It's unlikely that we'll need the RE_SAFE_OR_UNSAFE sniplet more than once in the same regex, so you can embed the 'execmode' name directly instead of ... > RE_PROFILE_END = re.compile('^\s*\}' + RE_EOL) > RE_PROFILE_CAP = re.compile(RE_AUDIT_DENY + > 'capability(?P<capability>(\s+\S+)+)?' + RE_COMMA_EOL) @@ -77,6 +78,7 > @@ RE_PROFILE_START = re.compile( > RE_PROFILE_CHANGE_PROFILE = re.compile( > RE_AUDIT_DENY + > 'change_profile' + > + '(\s+' + RE_SAFE_OR_UNSAFE % 'execmode' + ')?' + # optionally ... inserting it here using % 'execmode'. (We have rule types where RE_PROFILE_PATH and/or RE_PROFILE_PATH_OR_VAR can appear more than once, and that's the reason for using %s in them.) > diff --git a/utils/apparmor/rule/change_profile.py > b/utils/apparmor/rule/change_profile.py index 6063b70..cddc09a 100644 > --- a/utils/apparmor/rule/change_profile.py > +++ b/utils/apparmor/rule/change_profile.py > @@ -46,6 +46,13 @@ class ChangeProfileRule(BaseRule): > comment=comment, > log_event=log_event) > > + if execmode: > + if execmode != 'safe' and execmode != 'unsafe': > + raise AppArmorException('Unknown exec mode (%s) in > change_profile rule' % execmode) I'd vote for AppArmorBug - if something passes in an invalid value here, that's clearly a bug. (The parsing regex makes sure that only safe/ unsafe/None comes in, so parsing a profile should never hit this.) > + elif not execcond or > execcond == ChangeProfileRule.ALL: + raise > AppArmorException('Exec condition is required when unsafe or safe > keywords are present') > + self.execmode = execmode Just for the records - for this case, AppArmorException makes sense. > def is_covered_localvars(self, other_rule): > '''check if other_rule is covered by this rule object''' > > + if self.execmode != other_rule.execmode: > + return False The default execmode is 'safe', so 'safe' and '' / None should be considered as equal. > @@ -139,6 +156,9 @@ class ChangeProfileRule(BaseRule): > if not type(rule_obj) == ChangeProfileRule: > raise AppArmorBug('Passed non-change_profile rule: %s' % > str(rule_obj)) > > + if self.execmode != rule_obj.execmode: > + return False Same here. (Maybe add a TODO that strict mode should behave different - I'll add this flag to the function in the FileRule patches and will adjust the change_profile code afterwards.) > @@ -150,10 +170,12 @@ class ChangeProfileRule(BaseRule): > return True > > def logprof_header_localvars(self): > + execmode_txt = logprof_value_or_all(self.execmode, > None) Same again ;-) Also, did you really test this in all variants? I'd guess not specifying an exec mode will result in Exec Mode: or even Exec Mode: None 'safe' / 'unsafe' would be better choices ;-) Also, you don't need to use logprof_value_or_all() - with None as second parameter, it will just return the first parameter (self.execmode) so you can use that directly. (There's some special handling for sets, lists and tuples, but that doesn't matter here.) So basically you should do if self.execmode: execmode_txt = self.execmode else execmode_txt = 'safe' > @@ -144,20 +153,20 @@ class ChangeProfileFromInit(ChangeProfileTest): > class InvalidChangeProfileInit(AATest): I'm not quoting your changes because KMail made them unreadable by changing the linebreaks ;-) Nevertheless, please add another test that includes an invalid execmode, for example (['maybe' , '/foo', '/bar' ] , AppArmorBug), # invalid execmode (should be one line, but I won't do manual linebreaks in the whole mail just to get it right here ;-) > @@ -248,6 +257,8 @@ class > ChangeProfileCoveredTest_01(ChangeProfileCoveredTest): > + (' change_profile safe /foo,' , [ False , > False , False , False ]), After handling 'safe' and None/'' as equal, you'll need to adjust this test ;-) > ChangeProfileCoveredTest_02(ChangeProfileCoveredTest): ( > + ( 'change_profile safe /foo -> /bar,' , [ False , False , False , False ]), Same here. > +class ChangeProfileCoveredTest_06(ChangeProfileCoveredTest): > + rule = 'change_profile safe /foo,' > > + tests = [ > + # rule equal > strict equal covered covered exact + ( 'deny > change_profile /foo,' , [ False , False , False > , False ]), + ('audit deny change_profile /foo,' , > [ False , False , False , False ]), + ( > 'change_profile /foo,' , [ False , False , > False , False ]), # XXX should covered be true here? + > ( 'deny change_profile /bar,' , [ False , False > , False , False ]), + ( 'deny change_profile,' > , [ False , False , False , False ]), + > ] and here ;-) > @@ -333,7 +356,7 @@ class ChangeProfileCoveredTest_Invalid(AATest): > def test_borked_obj_is_covered_2(self): You might want to add another testcase that sets an invalid testobj.execmode. > class ChangeProfileLogprofHeaderTest(AATest): > tests = [ > + ('change_profile,', [ > _('Exec Mode'), None, _('Exec Condition'), _('ALL'), _('Target Profile'), _('ALL'), ]), Ah, here you confirm that Exec Condition: None will be displayed in aa-logprof or aa-mergeprof. As I already wrote in the comment about logprof_headers_localvars(), you should avoid that ;-) (and adjust the tests afterwards) > diff --git a/utils/test/test-parser-simple-tests.py > b/utils/test/test-parser-simple-tests.py index 304ff98..66b77ab > 100644 > --- a/utils/test/test-parser-simple-tests.py > +++ b/utils/test/test-parser-simple-tests.py > @@ -47,6 +47,11 @@ exception_not_raised = [ > 'capability/bad_3.sd', > 'capability/bad_4.sd', > 'change_hat/bad_parsing.sd', > + > + # The tools don't detect conflicting change_profile exec modes > + 'change_profile/onx_conflict_unsafe1.sd', > + 'change_profile/onx_conflict_unsafe2.sd', OK for now, but having conflict detection would be nice. Regards, Christian Boltz -- [Evolution - Message-ID] Oh ja... Apropos: die libcamel (die fuer diesen Muell verantwortlich ist) ist, aehm. "interessant" zu lesen... Und NEIN! Ich habe keine Lust, den Muell zu fixen. Es sei denn, man zahlt mir Schmerzensgeld. [David Haller in suse-linux]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
