On 06/27/2016 03:27 PM, Christian Boltz wrote: > 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.
That's not always true. The default is 'safe' on very new kernels that
support stacking. In all other cases, the 'safe' is dropped and the
default kernel behavior is 'unsafe'.
>
>> @@ -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
'Exec Mode: None' is what is printed. There are even tests below that
prove it.
I didn't want to bubble up a default execmode behavior in the python
utils because, as mentioned above, the default behavior is confusing. My
preference is for the tools to mostly ignore safe/unsafe and let the
parser and kernel figure it out.
If a policy author has explicitly chosen safe or unsafe, then the Python
utils should preserve that decision.
> 'safe' / 'unsafe' would be better choices ;-)
I disagree unless we want to duplicate the parser's logic of checking
the kernel features and deciding what the default execmode is.
> 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.
Yikes. I'd hope not. After writing this patch, I've come to the
conclusion that the utils are reimplementing way too much of what the
parser is already doing. This patch took way too long to essentially
rewrite in Python what I've already developed and tested in the parser.
Tyler
>
>
> Regards,
>
> Christian Boltz
>
>
>
signature.asc
Description: OpenPGP digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
