[apparmor] [PATCH 1/5] parser: Add dbus eavesdrop permission support to apparmor_parser

[Tyler Hicks]

On 2013-11-19 18:49:28, Seth Arnold wrote:
> On Tue, Nov 19, 2013 at 06:16:21PM -0800, Tyler Hicks wrote:
> > Allows for the policy writer to grant permission to eavesdrop on the
> > specified bus. Some example rules for granting the eavesdrop permission
> > are:
> > 
> >   # Grant send, receive, bind, and eavesdrop
> >   dbus,
> > 
> >   # Grant send, receive, bind, and eavesdrop on the session bus
> >   dbus bus=session,
> > 
> >   # Grant send and eavesdrop on the system bus
> >   dbus (send eavesdrop) bus=system,
> > 
> >   # Grant eavesdrop on any bus
> >   dbus eavesdrop,
> > 
> > Eavesdropping rules can contain the bus conditional. Any other
> > conditionals are not compatible with eavesdropping rules and the parser
> > will return an error.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> 
> Acked-by: Seth Arnold <seth.arnold at canonical.com>

Thanks!

> 
> Small notes inline..
> 
> > ---
> >  libraries/libapparmor/src/apparmor.h |  1 +
> >  parser/dbus.c                        | 14 +++++++++++---
> >  parser/immunix.h                     |  3 ++-
> >  parser/parser_lex.l                  |  1 +
> >  parser/parser_misc.c                 |  1 +
> >  parser/parser_regex.c                |  7 +++++++
> >  parser/parser_yacc.y                 |  4 ++++
> >  7 files changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libraries/libapparmor/src/apparmor.h b/libraries/libapparmor/src/apparmor.h
> > index 21c9e20..7648eae 100644
> > --- a/libraries/libapparmor/src/apparmor.h
> > +++ b/libraries/libapparmor/src/apparmor.h
> > @@ -50,6 +50,7 @@ __BEGIN_DECLS
> >  
> >  #define AA_DBUS_SEND		AA_MAY_WRITE
> >  #define AA_DBUS_RECEIVE		AA_MAY_READ
> > +#define AA_DBUS_EAVESDROP	(1 << 5)
> >  #define AA_DBUS_BIND		AA_MAY_BIND
> 
> I'd rather this use AA_MAY_LOCK; if it is going to shadow the value, I
> think it should be done explicitly, as the other three are.

I agree that this needs some cleanup. I'm not sure that I agree that it
should use AA_MAY_LOCK.

I'm having trouble answering why AA_DBUS_EAVESDROP needs to shadow the
value of AA_MAY_LOCK. There's no relation between the two macros other
than their value. If AA_MAY_LOCK changes, why should AA_DBUS_EAVESDROP
change?

Also, note that all of those AA_MAY_* macros are simply defined above
using raw values. AA_MAY_WRITE is not tied in any way to the parser's
AA_MAY_WRITE in parser/immunix.h.

I'm going to give this some more thought and then send out a cleanup
patch sometime today.

> 
> >  out:
> > @@ -184,6 +190,8 @@ void print_dbus_entry(struct dbus_entry *ent)
> >  		fprintf(stderr, "receive ");
> >  	if (ent->mode & AA_DBUS_BIND)
> >  		fprintf(stderr, "bind ");
> > +	if (ent->mode & AA_DBUS_EAVESDROP)
> > +		fprintf(stderr, "eavesdrop ");
> >  	fprintf(stderr, ")");
> >  
> >  	if (ent->bus)
> > diff --git a/parser/immunix.h b/parser/immunix.h
> > index f5064e8..c53d18f 100644
> > --- a/parser/immunix.h
> > +++ b/parser/immunix.h
> > @@ -42,10 +42,11 @@
> >  
> >  #define AA_DBUS_SEND			AA_MAY_WRITE
> >  #define AA_DBUS_RECEIVE			AA_MAY_READ
> > +#define AA_DBUS_EAVESDROP		(1 << 5)
> >  #define AA_DBUS_BIND			(1 << 6)
> 
> Much the same here, but also AA_DBUS_BIND could be AA_EXEC_MMAP.

Again, I see AA_DBUS_BIND and AA_EXEC_MMAP as two different classes of
permissions with no real relation. It just so happens that
AA_DBUS_SEND/AA_MAY_WRITE and AA_DBUS_RECEIVE/AA_MAY_READ do have some
relation so they muddy the water a bit.

Tyler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20131120/dcdf990b/attachment.pgp>