[apparmor] [PATCH 2/4] parser: Add support for DBus rules

[Tyler Hicks]

On 2013-07-30 00:14:28, John Johansen wrote:
> On 07/29/2013 03:40 PM, Seth Arnold wrote:
> > On Sat, Jul 27, 2013 at 02:45:16AM -0700, Tyler Hicks wrote:
> >> This patch implements the parsing of DBus rules.
> >>
> 
> << snip >>
> 
> >> +}
> >> +
> >> +static void copy_conditionals(struct dbus_entry *ent, struct cond_entry *conds)
> >> +{
> >> +	struct cond_entry *cond_ent;
> >> +
> >> +	list_for_each(conds, cond_ent) {
> >> +		char **ent_member = NULL;
> >> +
> >> +		/* for now disallow keyword 'in' (list) */
> >> +		if (!cond_ent->eq)
> >> +			yyerror("keyword \"in\" is not allowed in dbus rules\n");
> >> +		if (list_len(cond_ent->vals) > 1)
> >> +			yyerror("dbus conditional \"%s\" only supports a single value\n",
> >> +				cond_ent->name);
> >> +
> >> +		if (strcmp(cond_ent->name, "bus") == 0) {
> >> +			ent_member = &ent->bus;
> >> +		} else if (strcmp(cond_ent->name, "name") == 0) {
> >> +			ent_member = &ent->name;
> >> +		} else if (strcmp(cond_ent->name, "label") == 0) {
> >> +			ent_member = &ent->peer_label;
> >> +		} else if (strcmp(cond_ent->name, "path") == 0) {
> >> +			ent_member = &ent->path;
> >> +		} else if (strcmp(cond_ent->name, "interface") == 0) {
> >> +			ent_member = &ent->interface;
> >> +		} else if (strcmp(cond_ent->name, "member") == 0) {
> >> +			ent_member = &ent->member;
> >> +		} else {
> >> +			yyerror("invalid dbus conditional \"%s\"\n",
> >> +				cond_ent->name);
> >> +		}
> >> +
> >> +		if (ent_member) {
> >> +			if (*ent_member) {
> >> +				yyerror("dbus conditional \"%s\" can only be specified once\n",
> >> +					cond_ent->name);
> >> +			}
> >> +
> >> +			(*ent_member) = cond_ent->vals->value;
> >> +		}
> >> +
> >> +		cond_ent->vals->value = NULL;
> >> +	}
> >> +}
> > 
> > This is a bit intricate :) I've read through it a few times and I'm
> > not convinced I understand it. It seems a strange place to put the 'in'
> > check, and the pointer handling seems more complicated than ideal. I'm
> > sorry though, I don't have a concrete suggestion for improvement.
> > 
> 
> The 'in' check is handled here because the code is using a common parsing
> pattern routine, to pick up the entries.  So either this needs to be
> handled in the common parsing code by setting up/passing in context,
> or in post.
> 
> This is just handling the semantic check post rule match. And the check
> is here instead of have a large code block hanging off of the yacc rule.
> 
> As for pushing the semantics down into the generic pattern, this is
> difficult to do with yacc/bison. You can pass context, but we currently
> don't, and the context can only be updated once a certain match is
> achieved and reduced. So handling setting up meaningful context for
> this to be pushed into the generic pattern is probably more work than
> its worth.
> 
> There will be some patches to start push profile/block context down but
> this is will be less work and a lot more useful.

Thanks for replying, John.

I unfortunately didn't make it clear before, but John wrote a good
amount of this patch (and some of the others) and then I've
updated/refined/changed them over time. This 'in' check came from his
original patch, so I'm glad he jumped in. :)

> 
> As for the pointer handling sure its more complicated than you might
> like but isn't that always the case with pointers? I don't see a better
> way without standard templates to achieve the abstraction here

This goofy pointer handling was one of my changes. I wanted a way to do
a single check to see if we'd be clobbering an existing conditional
value (as with '/t { dbus bus=b1 bus=b2, }') and error out when that
happens.

Seth is right, it looks more complicated than it should. I've come up
with a better way of doing this. I'll reply with a v2 patch.

Tyler

> 
> 
> << snip >>
> 
> >> +dbus_perm: TOK_VALUE
> >> +	{
> >> +		if (strcmp($1, "bind") == 0)
> >> +			$$ = AA_DBUS_BIND;
> >> +		else if (strcmp($1, "send") == 0 || strcmp($1, "write") == 0)
> >> +			$$ = AA_DBUS_SEND;
> >> +		else if (strcmp($1, "receive") == 0 || strcmp($1, "read") == 0)
> >> +			$$ = AA_DBUS_RECEIVE;
> > 
> > Is this code still needed? You've got explicit tokens lower down...
> > 
> sadly yes, though perhaps we could abstract this more.
> 
> This is the difference between a bare keyword
> 
>   dbus send blah...,
> 
> and the keyword showing up in a permission list
> 
>   dbus (send receive) blah....
> 
> >> +		else if ($1) {
> >> +			parse_dbus_mode($1, &$$, 1);
> >> +		} else
> >> +			$$ = 0;
> >> +
> >> +		if ($1)
> >> +			free($1);
> >> +	}
> >> +	| TOK_BIND { $$ = AA_DBUS_BIND; }
> >> +	| TOK_SEND { $$ = AA_DBUS_SEND; }
> >> +	| TOK_RECEIVE { $$ = AA_DBUS_RECEIVE; }
> >> +	| TOK_READ { $$ = AA_DBUS_RECEIVE; }
> >> +	| TOK_WRITE { $$ = AA_DBUS_SEND; }
> >> +	| TOK_MODE
> >> +	{
> >> +		parse_dbus_mode($1, &$$, 1);
> >> +	}
> >> +
> >> +dbus_perms: { /* nothing */ $$ = 0; }
> >> +	| dbus_perms dbus_perm { $$ = $1 | $2; }
> >> +	| dbus_perms TOK_COMMA dbus_perm { $$ = $1 | $3; }
> >> +
> >> +opt_dbus_perm: { /* nothing */ $$ = 0; }
> >> +	| dbus_perm  { $$ = $1; }
> >> +	| TOK_OPENPAREN dbus_perms TOK_CLOSEPAREN { $$ = $2; }
> >> +
> >> +dbus_rule: TOK_DBUS opt_dbus_perm opt_conds opt_cond_list TOK_END_OF_RULE
> >> +	{
> >> +		struct dbus_entry *ent;
> >> +
> >> +		ent = new_dbus_entry($2, $3, $4);
> >> +		if (!ent) {
> >> +			yyerror(_("Memory allocation error."));
> >> +		}
> >> +		$$ = ent;
> >> +	}
> >> +
> >>  hat_start: TOK_CARET {}
> >>  	| TOK_HAT {}
> >>  
> > 
> > Thanks!
> > 
> > 
> > 
> 
> 
> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- 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/20130730/67248150/attachment-0001.pgp>