[apparmor] [patch 19/21] Add the ability to mediate signals.

[John Johansen]

On 03/21/2014 05:48 PM, Seth Arnold wrote:
> On Mon, Mar 17, 2014 at 04:29:29PM -0700, john.johansen at canonical.com wrote:
>> Add signal rules and make sure the parser encodes support for them
>> if the supported feature set reports supporting them.
>>
>> The current format of the signal rule is
>>
>>   [audit] [deny] signal [<signal_perms>] [<signal_set>] <target_profile>,
>>
>>   signal_perm  := 'send'|'receive'|'r'|'w'|'rw'
>>   signal_perms := <signal_perm> | '(' <signal_perm> ([,]<signal_perm>)* ')'
>>   signal := ("hup"|"int"|"quit"|"ill"|"trap"|"abrt"|"bus"|"fpe"|"kill"|
>>              "usr1"|"segv"|"usr2"|"pipe"|"alrm"|"term"|"tkflt"|"chld"|
> 
> Note that the signal you've got in here as "tkflt" should actually be
> "stkflt" here and throughout the code.
> 
>>              "cont"|"stop"|"stp"|"ttin"|"ttou"|"urg"|"xcpu"|"xfsz"|"vtalrm"|
>>              "prof"|"winch"|"io"|"pwr"|"sys"|"emt"|"exists")
>>   signal_set   := set=<signal> | '(' <signal> ([,]<signal>)* ')'
>>
>>
>> it does not currently follow the peer=() format, and there is some question
>> as to whether it should or not. Input welcome.
> 
> The peer=() stuff would feel so useless on these rules. I won't whine if
> we don't do it, there's no "local" interfaces that could logically have
> the same names, unlike dbus or networking.
> 
heh, I really don't like the peer=() syntax and it really is pointless on
this type of rule. That said going without it introduces an inconsistency.

currently we have

  signal (send,receive) set=(kill) /profile/foo,

which doesn't feel right either

  signal (send,receive) set=(kill) label=/profile/foo,

might be better.

  signal (send,receive) set=(kill) peer=(/profile/foo),

could work.

But I really dislike

  signal (send,receive) set=(kill) peer=(label=/profile/foo),

I could be convinced to go with it for consistency but basically  
we need to look at it a little more, and decide what we want to do.


>> +rules:  rules opt_prefix signal_rule
>> +	{
>> +		if ($2.owner)
>> +			yyerror(_("owner prefix not allowed on signal rules"));
>> +		if ($2.deny && $2.audit) {
>> +			$3->deny = 1;
>> +		} else if ($2.deny) {
>> +			$3->deny = 1;
>> +			$3->audit = $3->mode;
>> +		} else if ($2.audit) {
>> +			$3->audit = $3->mode;
>> +		}
>> +		$1->rule_ents.push_back($3);
>> +		$$ = $1;
>> +	}
> 
> Why not support 'owner'? Granted it'd only make sense for root processes
> and it is almost redundant with capability kill, but it seems like it
> could be useful for someone.
> 
Because the of way permission are structure in the policydb we can't support
owner yet, and have a consistent clean way to map to the newer permission format.
It will come, but I delayed most of the extended permission work for this cycle
because its just more work (sure a lot of its done but more is needed), and more
to verify.

> 
>> +void signal_rule::move_conditionals(struct cond_entry *conds)
>> +{
>> +	struct cond_entry *cond_ent;
>> +
>> +	list_for_each(conds, cond_ent) {
>> +		/* for now disallow keyword 'in' (list) */
>> +		if (!cond_ent->eq)
>> +			yyerror("keyword \"in\" is not allowed in signal rules\n");
>> +		/* use alternation instead????
>> +		 * set=(kill,usr1) vs. set={kill,usr1}
> 
> Hrm. Dunno. Sorry.
> 
Its () because set can be multi-valued. I was messed up and thinking we only
get a single signal at a time, but we could theoretically allow for multiple
and it should still be okay.

Which means we might want to update the backend encoding a little bit to
support this if it ever is actually allowed.

> 
>> +int signal_rule::gen_policy_re(Profile &prof)
>> +{
>> +	std::ostringstream buffer;
>> +	std::string buf;
>> +
>> +	pattern_t ptype;
>> +	int pos;
>> +
>> +	/* Currently do not generate the rules if the kernel doesn't support
>> +	 * it. We may want to switch this so that a compile could be
>> +	 * used for full support on kernels that don't support the feature
>> +	 */
>> +	if (!kernel_supports_signal) {
>> +		warn_once(prof.name);
>> +		return RULE_NOT_SUPPORTED;
>> +	}
>> +
>> +	if (signals.size() == 0) {
>> +		/* not conditional on signal set, so will generate a label
>> +		 * rule as well
>> +		 */
>> +		buffer << "(" << "\\x" << std::setfill('0') << std::setw(2) << std::hex << AA_CLASS_LABEL << "\\x" << AA_CLASS_SIGNAL << "|";
>> +	}
> 
> Why the AA_CLASS_LABEL here?
> 
there is a double encoding going on here. Because this rule isn't conditional
on the signal. Basically a straight up label check will allow the signal, as
well as going through the signal.

The reason for the split encoding is a little more complicated but it basically
works out to efficiency, and some compatibility with older rules having their
label check encoded not at the start.