Date:	Wed, 8 Aug 2012 16:51:56 -0400
From:	Eric Paris <eparis@...isplace.org>
To:	Paul Moore <paul@...l-moore.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	John Stultz <johnstul@...ibm.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	lkml <linux-kernel@...r.kernel.org>,
	James Morris <james.l.morris@...cle.com>,
	selinux@...ho.nsa.gov, Eric Dumazet <edumazet@...gle.com>,
	john.johansen@...onical.com
Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore <paul@...l-moore.com> wrote:
> On Wednesday, August 08, 2012 10:09:38 PM Eric Dumazet wrote:

> Actually, the issue is that the shared socket doesn't have an init/alloc
> function to do the LSM allocation like we do with other sockets so Eric's
> patch does it as part of ip_send_unicast_reply().
>
> If we look at the relevant part of Eric's patch:
>
>  +#ifdef CONFIG_SECURITY
>  +       if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
>  +                       goto out;
>  +#endif
>
> ... if we were to remove the CONFIG_SECURITY conditional we would end up
> calling security_sk_alloc() each time through in the CONFIG_SECURITY=n case as
> sk->sk_security would never be initialized to a non-NULL value.  In the
> CONFIG_SECURITY=y case it should only be called once as security_sk_alloc()
> should set sk->sk_security to a LSM blob.

Ifndef SECURITY this turns into (because security_sk_alloc is a static
inline in that case)

if (!sk->sk_security && 0)
        goto out;

Which I'd hope the compiler would optimize.  So that only leaves us
caring about the case there CONFIG_SECURITY is true.  In that case if
we need code which does if !alloc'd then alloc it seems we broke the
model of everything else in the code and added a branch needlessly.

Could we add a __init function which does the security_sk_alloc() in
the same file where we declared them?

>> IMHO it seems wrong to even care about security for internal sockets.
>>
>> They are per cpu, shared for all users on the machine.
>
> The issue, from a security point of view, is that these sockets are sending
> network traffic; even if it is just resets and timewait ACKs, it is still
> network traffic and the LSMs need to be able to enforce security policy on
> this traffic.  After all, what would you say if your firewall let these same
> packets pass without any filtering?
>
> The issue I'm struggling with at present is how should we handle this traffic
> from a LSM perspective.  The label based LSMs, e.g. SELinux and Smack, use the
> LSM blob assigned to locally generated outbound traffic to identify the
> traffic and apply the security policy, so not only do we have to resolve the
> issue of ensuring the traffic is labeled correctly, we have to do it with a
> shared socket (although the patch didn't change the shared nature of the
> socket).
>
> For those who are interested, I think the reasonable labeling solution here is
> to go with SECINITSID_KERNEL/kernel_t for SELinux and likely the ambient label
> for Smack as in both the TCP reset and timewait ACK there shouldn't be any
> actual user data present.

I'm willing to accept that argument from an SELinux perspective.  I'd
also accept the argument that it is private and do something similar
to what we do with IS_PRIVATE on inodes.  Although sockets probably
don't have a good field to use...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists