[apparmor] [patch] Don't store exec modes in transtions[]

[Steve Beattie]

On Sun, Mar 20, 2016 at 02:58:45PM +0100, Christian Boltz wrote:
> Am Samstag, 19. März 2016, 11:55:09 CET schrieb Steve Beattie:
> > On Sun, Feb 21, 2016 at 03:00:06PM +0100, Christian Boltz wrote:
> > > exec choices are stored in transitions[], but that's never used
> > > (and I don't see a need for it), therefore stop storing it.
> > > 
> > > 
> > > [ 73-exec-transitions.diff ]
> > > 
> > > === modified file 'utils/apparmor/aa.py'
> > > --- utils/apparmor/aa.py        2016-02-20 12:32:36 +0000
> > > +++ utils/apparmor/aa.py        2016-02-21 13:50:24 +0000
> > > @@ -1205,7 +1205,6 @@
> > > 
> > >                          context_new = context_new + '^%s' % hat
> > >                      
> > >                      context_new = context_new + ' -> %s' %
> > >                      exec_target
> > > 
> > > -                    # ans_new = transitions.get(context_new, '')  #
> > > XXX ans meant here?> 
> > >                      combinedmode = set()
> > >                      combinedaudit = set()
> > >                      ## Check return Value Consistency
> > > 
> > > @@ -1415,7 +1414,6 @@
> > > 
> > >                                          exec_mode = exec_mode -
> > >                                          (apparmor.aamode.AA_EXEC_U
> > >                                          NSAFE |
> > >                                          AA_OTHER(apparmor.aamode.A
> > >                                          A_EXEC_UNSAFE))>                                  
> > >                                  else:
> > >                                      ans = 'INVALID'
> > > 
> > > -                        transitions[context_new] = ans
> > > 
> > >                          regex_options =
> > >                          re.compile('CMD_(ix|px|cx|nx|pix|cix|nix)'
> > >                          )
> > 
> > >                          if regex_options.search(ans):
> > Are you sure about that? I see in handle_children():
> > 
> >  
> > http://bazaar.launchpad.net/~apparmor-dev/apparmor/master/view/head:/
> > utils/apparmor/aa.py#L1075
> > 
> >   1075                ans = transitions.get(context, 'XXXINVALIDXXX')
> >   1076
> >   1077		      while ans not in ['CMD_ADDHAT', 'CMD_USEDEFAULT',
> > 'CMD_DENY']:
> > 
> > and transitions is a global hasher() object.
> > 
> > But I've only looked at this cursorily, so don't claim any real
> > understanding of what's going on (or not going on) here.
> 
> 'transitions' is currently used for storing two not-too-related things:
> 
> The code you quoted is about hats, and it's looking for 'CMD_ADDHAT', 
> 'CMD_USEDEFAULT' and 'CMD_DENY'. That part is useful and will stay.
> (Maybe we should rename 'transitions' to a better name (hat_choices?), 
> but that's a cosmetic issue.)

I *think* based on reading tea leaves here that the transitions
structure was to store responses so that if the same hat/exec
transition came up again, the previous answer for that hat/exec could
be used and querying the user could be skipped; hence the generic name.

> The code I want to remove stores the exec choice (CMD_ix, CMD_px etc.) - 
> and that information isn't used anywhere (= storing it is superfluous).
> Instead, profile_known_exec() is used to check if we need to ask for 
> adding an exec rule or if we already have one.

Okay, it looks like the exec() handling code is using
profile_known_exec() to determine if there's already been a relevant
permission/deny added. And you are correct, transitions is not being
queried here, so there's no point in saving to it. Therefore,

Acked-by: Steve Beattie <steve at nxnw.org>

Thanks.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160329/b5f780d1/attachment.pgp>