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

[Christian Boltz]

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? ;-)

> > 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()):


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]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160301/8e857e41/attachment.pgp>