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 <[email protected]> > > 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 >> [email protected] >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/apparmor -- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
