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

[Tyler Hicks]

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.

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?

> 
> 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.

> 
> 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.

$ 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:

$ 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140905/825b2181/attachment.pgp>