Re: Incorrect avc logs for the packet object class

[Paul Moore <paul.moore@hp.com>]

On Thursday 21 May 2009 03:05:54 pm Paul Moore wrote:
> On Thursday 21 May 2009 01:57:09 pm Stephen Smalley wrote:
> > On Thu, 2009-05-21 at 12:26 -0400, Stephen Smalley wrote:
> > > On Thu, 2009-05-21 at 10:37 -0400, Paul Moore wrote:
> > > > On Thursday 21 May 2009 10:14:40 am Stephen Smalley wrote:
> > > > > On Wed, 2009-05-20 at 13:23 -0400, Marshall Miller wrote:
> > > > > > I first noticed this bug on a RHEL 5 system, and I also noticed
> > > > > > it on Ubuntu Jaunty.  I tested this out on Fedora 11 Preview and
> > > > > > it was there also.
> > > > > >
> > > > > > The avc messages for the packet object class sporadically report
> > > > > > incorrect comm/pid info.  It is most apparent when multiple
> > > > > > processes are sending/receiving packets at the same time.  To
> > > > > > demonstrate this, I added an iptables rule such that every packet
> > > > > > being sent is labeled system_u:object_r:dns_client_packet_t:s0
> > > > > > (arbitrarily chosen from existing types).  I then created and
> > > > > > inserted a module that auditallows all packet perms for subj ==
> > > > > > domain and obj == dns_client_packet_t. Then I started the
> > > > > > Software Updater, and when it started downloading packages I ran
> > > > > > firefox.
> > > > >
> > > > > This has always been an issue for the low level networking
> > > > > permission checks that may happen outside of process context;
> > > > > unfortunately, "current" is still defined in that situation and
> > > > > thus avc_audit() cannot distinguish the cases where it has a
> > > > > relevant process context from these situations.  Options for
> > > > > fixing:
> > > > > - Have the callers of avc_has_perm() pass a flag (e.g. via
> > > > > avc_audit_data) when no relevant process context is available, and
> > > > > use that flag within avc_audit() to omit the comm= and pid=
> > > > > information and to pass a NULL audit_context to audit_log_start().
> > > > > - Have avc_audit() internally check for certain classes and
> > > > > permissions (e.g. SECCLASS_PACKET, SECCLASS_PEER, SECCLASS_NETIF,
> > > > > SECCLASS_NODE) known to have this behavior, and likewise omit the
> > > > > comm= and pid= information and pass a NULL context to
> > > > > audit_log_start() in that case. Concern with this approach is that
> > > > > a given class/permission may be used from multiple call sites, some
> > > > > of which may have a valid process context and some of which may
> > > > > not.
> > > >
> > > > For the reasons that Stephen pointed out as well as a concern that it
> > > > would be harder to maintain (we would likely forget to add new
> > > > classes in the future should they arise) I vote for the first
> > > > flag-passing approach.  It should be a relatively small patch; if I
> > > > don't hear any objections I'll start working on it on in the next day
> > > > or two ... I would think we should be able to still get this into
> > > > 2.6.30.
> > >
> > > Thanks, Paul - sounds good.  One further observation here:  in the
> > > past, I only recall seeing this issue crop up on recv-side checking,
> > > not the send-side checking, as most sends happen while still in the
> > > process context of the sender.  I guess what we are seeing here is TCP
> > > re-transmit.  Unfortunately, I don't think that we can distinguish the
> > > cases from our hooks for the packet send checks, and thus we may end up
> > > losing audit information (both at the avc level and at the syscall
> > > audit record level) about the sending process even when that
> > > information would in fact be accurate.
>
> If we still want to do this we would likely need to trim the process
> information (i.e. pid and comm) for everything lower than the socket layer
> which in the case of outgoing packets is pretty much everything.
>
> > ...which might be why we have never "fixed" this behavior.  While the
> > information is unreliable, it is sometimes correct and can be helpful in
> > diagnosing network policy denials.  Depends on what is more important -
> > complete accuracy in the avc denials (although the comm field is
> > inherently unreliable, of course) or providing as much information as
> > possible to debug avc denials.
>
> That is a tough call, and I think the "correct" answer will depend on who
> you ask.
>
> > A simple heuristic would be to check whether the scontext matches the
> > context of the process, i.e. (current_sid() == ssid).  That will
> > eliminate some of the noise but not all of it, and it might suppress
> > auditing of the pid/comm information on some checks that happen in
> > process context but don't take the current task SID as the source.
>
> Although this runs into issues of losing information when the sender
> changes the label of the originating socket (I assume you are talking about
> comparing the socket/packet's SID with current_sid()) and I don't think
> anyone wants to lose audit records.
>
> I thought netfilter had a way to filter packets based on PID, I need to go
> check on that as we could probably use the same mechanism ...

Never mind, looks like that functionality no longer exists ...

static bool owner_mt_check_v0(const struct xt_mtchk_param *par)
{
        const struct ipt_owner_info *info = par->matchinfo;

        if (info->match & (IPT_OWNER_PID | IPT_OWNER_SID | IPT_OWNER_COMM)) {
                printk(KERN_WARNING KBUILD_MODNAME
                       ": PID, SID and command matching is not "
                       "supported anymore\n");
                return false;
        }

        return true;
}

-- 
paul moore
linux @ hp


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.