[apparmor] [patch 18/12] v3 unix socket rules

[John Johansen]

On 09/05/2014 10:23 AM, Tyler Hicks wrote:
> On 2014-09-05 09:25:03, John Johansen wrote:
>> Do not output local permissions for rules that have peer_conditionals
>>
>> while it is not possible to specify a rule with local conditionals with
>> peer conditionals
>> eg.
>>    unix listen peer=(addr=@foo),
>>
>> a rule such as
>>    unix peer=(addr=@foo),
>>
>> is possible, and was setting all permissions for local as well as the peer
>> condition permissions.
>>
>> Currently this means the create permission must be specified in a separate
>> rule from a rule with a peer= condition, if create is to be allowed. This
>> isn't too much of an issue but it does mean rule such as
>>   unix connect peer=(addr=@foo),
> 
> As a policy author, I would not have expected connect to imply create
> permission. Also, our apparmor.d man page states that create will not
> be implied:
> 
>   If a rule is specified with a peer component it will only imply accept
>   (stream), connect (stream), listen, receive and send. It will not
>   imply the create, bind, listen, shutdown, getattr, or setattr
>   permissions.
> 
okay good

> Note that listen is specified in the "will imply" and the "will not
> imply" list. We need to fix that. Which list should it be in?
> 
listen is a local perm, it is specified on a socket before it can accept
connections.

>>
>> Can not imply the ability to create a socket. Which may indeed be the
>> behavior if we wish to enforce that the socket was created in another
>> process and passed in. Is this what we want to do?
> 
> Yes, I think that's what we want. The ability to call socket() would
> allow a confined application to autoload a kernel module that implements
> the network family specified in the socket() params. There could be
> cases where you don't want to allow that autoload to be triggered by a
> confined program but do want the program to inherit a socket fd that was
> opened by a program with greater privs.
> 
good

>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> While I agree with the intent of this patch, I'm not ready to ack it
> just yet because it seems to quietly drop the connect perm instead of
> throwing an error.
> 
it is not so much dropping the accept perm, as mask it it out when a rule
that doesn't specify permissions has its permission mask set to all perms.

If a rule specifies the create perm or any other local perm it should
be screened out sooner than the rule generation, in the move_conds,
move_peer_conds, or constructors for unix_rule


> $ echo "/t { unix create peer=(addr=@foo), }" | ./apparmor_parser -qQ ; echo $?
> 0
> 
> In fact, our error throwing in the case of local perms and peer conds is
> pretty inconsistent right now:
> 
yes :(

> $ for p in create bind listen shutdown getattr setattr; do \
>     echo "/t { unix $p peer=(addr=@foo), }" | ./apparmor_parser -qQM && echo "Quietly dropped: $p"; \
>   done
> Quietly dropped: create
> AppArmor parser error, in stdin line 1: unix socket 'bind' access cannot be used with message rule conditionals
> AppArmor parser error, in stdin line 1: unix socket 'listen' access cannot be used with message rule conditionals
> Quietly dropped: shutdown
> Quietly dropped: getattr
> Quietly dropped: setattr
> 
> I think all of those "Quietly dropped:" lines should result in a parser
> error.
> 
ugh we need a patch to fix that

> Tyler
> 
>>
>> ---
>>
>> === modified file 'parser/af_unix.cc'
>> --- parser/af_unix.cc	2014-09-05 15:49:33 +0000
>> +++ parser/af_unix.cc	2014-09-05 16:13:04 +0000
>> @@ -334,7 +334,7 @@
>>  	}
>>  
>>  	write_to_prot(buffer);
>> -	if (mask & AA_NET_CREATE) {
>> +	if ((mask & AA_NET_CREATE) && !has_peer_conds()) {
>>  		buf = buffer.str();
>>  		if (!prof.policy.rules->add_rule(buf.c_str(), deny,
>>  						 map_perms(AA_NET_CREATE),
>> @@ -355,7 +355,8 @@
>>  		buffer << "\\x00";
>>  
>>  		/* create already masked off */
>> -		if (mask & AA_LOCAL_NET_PERMS & ~AA_LOCAL_NET_CMD) {
>> +		if ((mask & AA_LOCAL_NET_PERMS & ~AA_LOCAL_NET_CMD) &&
>> +		    !has_peer_conds()) {
>>  			buf = buffer.str();
>>  			if (!prof.policy.rules->add_rule(buf.c_str(), deny,
>>  							 map_perms(mask & AA_LOCAL_NET_PERMS & ~AA_LOCAL_NET_CMD),
>> @@ -364,7 +365,7 @@
>>  				goto fail;
>>  		}
>>  
>> -		if (mask & AA_NET_LISTEN) {
>> +		if ((mask & AA_NET_LISTEN) && !has_peer_conds()) {
>>  			std::ostringstream tmp(buffer.str());
>>  			tmp.seekp(0, ios_base::end);
>>  			tmp << "\\x" << std::setfill('0') << std::setw(2) << std::hex << CMD_LISTEN;
>> @@ -377,7 +378,7 @@
>>  							 dfaflags))
>>  				goto fail;
>>  		}
>> -		if (mask & AA_NET_OPT) {
>> +		if ((mask & AA_NET_OPT) && !has_peer_conds()) {
>>  			std::ostringstream tmp(buffer.str());
>>  			tmp.seekp(0, ios_base::end);
>>  			tmp << "\\x" << std::setfill('0') << std::setw(2) << std::hex << CMD_OPT;
>>
>>
>> -- 
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor