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

[Tyler Hicks]

On 2014-09-05 10:36:32, John Johansen wrote:
> 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

Ok, well lets treat that as a separate issue. Stick my acked-by on your
patch, commit it to trunk, and then I'll send out a fix for this issue.

Thanks!

Tyler

> 
> > 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/10bfa2bb/attachment.pgp>