[apparmor] [patch] Make sure aa-cleanprof de-duplicates capability rules

[Kshitij Gupta]

Hello,

On Sun, Apr 26, 2015 at 12:41 AM, Kshitij Gupta <kgupta8592 at gmail.com>
wrote:

> Hello,
>
> On Wed, Apr 15, 2015 at 12:34 AM, Christian Boltz <apparmor at cboltz.de>
> wrote:
>
>> Hello,
>>
>> Am Montag, 13. April 2015 schrieb Steve Beattie:
>> > On Tue, Apr 14, 2015 at 12:50:26AM +0200, Christian Boltz wrote:
>> > > Am Montag, 13. April 2015 schrieb Steve Beattie:
>> > > > On Sun, Apr 12, 2015 at 03:32:25AM +0200, Christian Boltz wrote:
>> > > > > CleanProf.remove_duplicate_rules() didn't call
>> > > > >
>> > > > >   $profile['capability'].delete_duplicates()
>> > > > >
>> > > > > because aa-cleanprof sets same_file=True.
>> > > > >
>> > > > > Fix this by calling delete_duplicates(None) so that it
>> > > > > only checks the profile against itsself.
>> > > > >
>> > > > > [ 43-cleanprof-do-in-profile-run.diff ]
>> > > > >
>> > > > > === modified file 'utils/apparmor/cleanprofile.py'
>> > > > > --- utils/apparmor/cleanprofile.py      2014-12-16 22:13:25
>> > > > > +0000
>> > > > > +++ utils/apparmor/cleanprofile.py      2015-04-11 22:35:00
>> > > > > +0000
>> > > > > @@ -67,6 +67,8 @@
>> > > > >
>> > > > >              #Clean the duplicates of caps in other profile
>> > > > >
>> > > > >              if not self.same_file:
>> > > > >                  deleted +=
>> > > > >
>> > > > >self.other.aa[program][hat]['capability'].delete_duplicates(self.
>> > > > >pro
>> > > > >file.aa[program][hat]['capability'])
>> > > > >
>> > > > > +            else:
>> > > > > +                deleted +=
>> > > > > self.other.aa[program][hat]['capability'].delete_duplicates(None
>> > > > > )>
>> > > >
>> > > > This patch does not seem to do what you claim it does:
>> > > Did you also apply 42-in-profile-deduplication.diff before testing?
>> > > Without that, there's no in-profile deduplication (removing lines
>> > > covered by includes should work without patch 42).
>> >
>> > I didn't initially (nothing in this patch description called out
>> > that it depended on that one. However, when I tried path 42 without
>> > patch 43 applied, the testing that I did showed that it deleted the
>> > in-profile duplicated capability, so I'm still not clear on why this
>> > patch is necessary.
>>
>> The strange thing is that it's clearly necessary for me - I just tested
>> without it, and it didn't remove in-profile duplicates.
>>
>> Both your test-profile (the one mentioned below) and the one Steve used
> had their duplicate rules removed by the tool in my bzr branch which had I
> pulled afresh from upstream (at revision 3018).
>
> Note: I'm testing on Kubuntu 14.10 with Python 3.4 if that matters.
>
> Upon revisiting the profiles(during discussion over IRC) and disabling the
includes from both profiles, the duplicate rules were _not_ removed from
the profile in both the sample profiles presented.

With the above mentioned patch applied the duplicate rules _were_ removed.

So, the patch looks fine to me and works for me.

Thanks for the patch.

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>.

Regards,

Kshitij Gupta


Thanks.
>
> Regards,
>
> Kshitij Gupta
>
>
>
>> Note that I'm testing with all my pending patches applied [1], however I
>> think only patch 42 is related to cleanprof.
>>
>> My test profile:
>>
>> # cat usr.bin.echo
>> /usr/bin/echo {
>>    audit capability chown, # drop (1)
>>    capability dac_override, # drop
>>    deny capability dac_override,
>>    capability dac_override, # drop
>>    audit capability chown, # drop (2)
>>    deny capability chown, # drop
>>    audit deny capability chown,
>>    capability, # drop
>>    audit capability,
>> }
>>
>> Without patch 43, aa-cleanprof doesn't remove any of those rules.
>> With patch 43, aa-cleanprof shrinks the profile to
>>
>>  /usr/bin/echo {
>>   audit deny capability chown,
>>   deny capability dac_override,
>>
>>   audit capability,
>> }
>>
>>
>> Regards,
>>
>> Christian Boltz
>>
>> [1] all pending patches means:
>>     30-logparser-change-mask-only-for-path-events.diff
>>     31-enable-testloops-for-nosetests.diff
>>     33-fix-add-to-variable-and-add-tests.diff
>>     35-fix-serialize_profile_from_old_profiles-variable-add.diff
>>     36-fix-crash-in-serialize_profile_from_old_profiles.diff
>>     39-aatest-maxdiff.diff
>>     41-add-baserule-tests.diff
>>     42-in-profile-deduplication.diff
>>     43-cleanprof-do-in-profile-run.diff
>>
>> --
>> > > dank meiner Versionitis verwende ich längst die 10.1 ;-)
>> > Das Spielchen habe ich auch mitgemacht - von 6.0 bis 9.3. Nu reichts,
>> > man soll schließlich arbeiten mit dem Ding.
>> Zum Arbeiten braucht es kein unsupportetes Supplementary!
>> [>>Christian Boltz, > Christian Lepper & Marcus Meissner in suse-laptop]
>>
>>
>> --
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at:
>> https://lists.ubuntu.com/mailman/listinfo/apparmor
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150426/7d50622e/attachment.html>