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