linux-kernel - Re: NULL pointer dereference in selinux_ip_postroute

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 08 Aug 2012 16:35:08 -0400
From:	Paul Moore <paul@...l-moore.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Eric Paris <eparis@...isplace.org>,
	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 Wednesday, August 08, 2012 10:09:38 PM Eric Dumazet wrote:
> On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
> > Seems wrong.  We shouldn't ever need ifdef CONFIG_SECURITY in core
> > code.
> 
> Sure but it seems include file misses an accessor for this.
> 
> We could add it on a future cleanup patch, as Paul mentioned.

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 CONF_SECURITY then security_sk_alloc() is a static
> > 
> > inline return 0;   I guess the question is "Where did the sk come
> > from"?  Why wasn't security_sk_alloc() called when it was allocated?
> > Should it have been updated at some time and that wasn't done either?
> > Seems wrong to be putting packets on the queue for a socket where the
> > security data was never allocated and was never set to its proper
> > state.
> 
> 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.

-- 
paul moore
www.paul-moore.com

--
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/