[apparmor] [patch] Fix wrong usage of write_prof_data in serialize_profile_from_old_profile()

[Kshitij Gupta]

Hello,

On Wed, Mar 2, 2016 at 12:42 AM, Christian Boltz <apparmor at cboltz.de> wrote:

> Hello,
>
> Am Montag, 22. Februar 2016, 02:16:28 CET schrieb Kshitij Gupta:
> > On Sat, Dec 26, 2015 at 9:07 PM, Christian Boltz wrote:
> > > write_prof_data[hat] is correct (it only contains one profile, see
> > > also bug 1528139), write_prof_data[profile][hat] is not and returns
> > > an empty (sub)hasher.
> >
> > Hmm...
> >
> > Reading the comments near the initialisation of write_prof_data:
> > # XXX this will explode if a file contains multiple profiles, see
> > https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1528139
> > # XXX fixing this needs lots of write_prof_data[hat] ->
> > write_prof_data[profile][hat] changes (and of course also a change in
> > the calling code)
> >
> > So, basically a part of the logic below is correct in that it accessed
> > the hat from the profile, which will again need to be added once
> > write_prof_data supports multiple profiles I'm guessing?
>
> Well, it's a very small part of the function that uses
> write_prof_data[profile][hat]. In theory that would be better, but doing
> this everywhere would also mean we have to do lots of changes in the
> function and also in the calling code.
>
> serialize_profile_from_old_profile() is (IMHO) broken beyond repair, so I
> don't want to spend too much time on it - but this patch is needed to
> a) fix the rare places where [profile][hat] is accidently used and
> b) get towards a more strict profile_storage()
>
> > Why not copy write_prof_data[hat] to write_prof_data[profile][hat] for
> > the time-being?
> > That might seem hack-ish though.
>
> As discussed on IRC some days ago, it would also lead to funny[tm]
> problems if someone has a hat named ^capability or ^network or something
> like that ;-)
>
> So - how do you like the patch as is? ;-)
>
> Well okay. I'm fine with it, its a step in the right direction. Hope there
is no regression ;-)

> > This affects RE_PROFILE_START and RE_PROFILE_BARE_FILE_ENTRY.
> > >
> > > I propose this patch for trunk, 2.10 and 2.9.
> > >
> > >
> > > [
> > > 46-serialize_profile_from_old_profile-fix-wrong-access-to-write_prof
> > > _data.diff ]
> > >
> > > === modified file ./utils/apparmor/aa.py
> > > --- utils/apparmor/aa.py        2015-12-06 19:36:00.818745321 +0100
> > > +++ utils/apparmor/aa.py        2015-12-08 18:59:09.625261162 +0100
> > > @@ -3718,7 +3718,7 @@
> > >
> > >              if RE_PROFILE_START.search(line):
> > >                  (profile, hat, attachment, flags, in_contained_hat,
> > >
> > > correct) = serialize_parse_profile_start(
> > > -                        line, prof_filename, None, profile, hat,
> > > write_prof_data[profile][hat]['profile'],
> > > write_prof_data[profile][hat]['external'], correct)
> > > +                        line, prof_filename, None, profile, hat,
> > > write_prof_data[hat]['profile'], write_prof_data[hat]['external'],
> > > correct)>
> > >                  if not write_prof_data[hat]['name'] == profile:
> > >                      correct = False
> > >
> > > @@ -3954,7 +3954,7 @@
> > >
> > >                  if matches[0]:
> > >                      audit = mode
> > >
> > > -                path_rule =
> > > write_prof_data[profile][hat][allow]['path'][ALL]
> > > +                path_rule =
> > > write_prof_data[hat][allow]['path'][ALL]
> > >
> > >                  if path_rule.get('mode', set()) & mode and \
> > >
> > >                     (not audit or path_rule.get('audit', set()) &
> > >                     audit)
> > >
> > > and \
> > >
> > >                     path_rule.get('file_prefix', set()):
>
> Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>for trunk, 2.9 and 2.10

>
> Regards,
>
> Christian Boltz
> --
> In A.D. 1582 Pope Gregory XIII [...] changed the rules about calculating
> leap years to account for this.
> Similarly, in A.D. 2013 Rockchip hardware engineers found that the new
> Gregorian calendar still contained flaws, and that the month of November
> should be counted up to 31 days instead.
> [https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> commit/?id=f076ef44a44d02ed91543f820c14c2c7dff53716]
>
Wow! *_*


>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>


-- 
Regards,

Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160302/b16ba585/attachment.html>