Message-ID: <20121205114339.GA26649@redhat.com>
Date:	Wed, 5 Dec 2012 13:43:39 +0200
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	Paul Moore <pmoore@...hat.com>, netdev@...r.kernel.org,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov
Subject: Re: [RFC PATCH 2/2] tun: fix LSM/SELinux labeling of tun/tap devices

On Wed, Dec 05, 2012 at 02:17:30PM +0800, Jason Wang wrote:
> On 12/04/2012 11:24 PM, Michael S. Tsirkin wrote:
> > On Tue, Dec 04, 2012 at 09:24:43PM +0800, Jason Wang wrote:
> >> On Monday, December 03, 2012 11:22:29 AM Paul Moore wrote:
> >>> On Monday, December 03, 2012 06:15:42 PM Jason Wang wrote:
> >>>> On 11/30/2012 06:06 AM, Paul Moore wrote:
> >>>>> This patch corrects some problems with LSM/SELinux that were introduced
> >>>>> with the multiqueue patchset.  The problem stems from the fact that the
> >>>>> multiqueue work changed the relationship between the tun device and its
> >>>>> associated socket; before the socket persisted for the life of the
> >>>>> device, however after the multiqueue changes the socket only persisted
> >>>>> for the life of the userspace connection (fd open).  For non-persistent
> >>>>> devices this is not an issue, but for persistent devices this can cause
> >>>>> the tun device to lose its SELinux label.
> >>>>>
> >>>>> We correct this problem by adding an opaque LSM security blob to the
> >>>>> tun device struct which allows us to have the LSM security state, e.g.
> >>>>> SELinux labeling information, persist for the lifetime of the tun
> >>>>> device.
> >>> ...
> >>>
> >>>>> -static int selinux_tun_dev_attach(struct sock *sk)
> >>>>> +static int selinux_tun_dev_attach(struct sock *sk, void *security)
> >>>>>
> >>>>>  {
> >>>>>
> >>>>> +	struct tun_security_struct *tunsec = security;
> >>>>>
> >>>>>  	struct sk_security_struct *sksec = sk->sk_security;
> >>>>>  	u32 sid = current_sid();
> >>>>>  	int err;
> >>>>>
> >>>>> +	/* we don't currently perform any NetLabel based labeling here ...
> >>>>>
> >>>>>  	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> >>>>>  	
> >>>>>  			   TUN_SOCKET__RELABELFROM, NULL);
> >>>>>  	
> >>>>>  	if (err)
> >>>>>  	
> >>>>>  		return err;
> >>>>>
> >>>>> -	err = avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET,
> >>>>> +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> >>>>>
> >>>>>  			   TUN_SOCKET__RELABELTO, NULL);
> >>>>>  	
> >>>>>  	if (err)
> >>>>>  	
> >>>>>  		return err;
> >>>>>
> >>>>> -	sksec->sid = sid;
> >>>>> +	sksec->sid = tunsec->sid;
> >>>>> +	sksec->sclass = SECCLASS_TUN_SOCKET;
> >>>> I'm not sure whether this is correct, looks like we need to differ between
> >>>> TUNSETQUEUE and TUNSETIFF. When userspace call TUNSETIFF for persistent
> >>>> device, looks like we need change the sid of tunsec like in the past.
> >>> It may be that I'm misunderstanding TUNSETQUEUE and/or TUNSETIFF.  Can you
> >>> elaborate as to why they should be different?
> >> If I understand correctly, before multiqueue patchset, TUNSETIFF is used to:
> >>
> >> 1) Create the tun/tap network device
> >> 2) For persistent device, re-attach the fd to the network device / socket. In 
> >> this case, we call selinux_tun_dev_attch() to relabel the socket sid (in fact 
> >> also the device's since the socket were persistent also) to the sid of process 
> >> that calls TUNSETIFF.
> >>
> >> So, after the changes of multiqueue, we need try to preserve those policy. The 
> >> interesting part is the introducing of TUNSETQUEUE, it's used to attach more 
> >> file descriptors/sockets to a tun/tap device after at least one file descriptor 
> >> were attached to the tun/tap device through TUNSETIFF. So I think maybe we 
> >> need differ those two ioctls. This patch looks fine for TUNSETQUEUE, but for 
> >> TUNSETIFF, we need relabel the tunsec to the process that calling TUNSETIFF 
> >> for persistent device?
> > Basically, it looks like currently once you get a tun fd,
> > you can attach it to any device even if normally
> > selinux would prevent you from accessing it.
> 
> Yes some checking during TUNSETQUEUE is missed.
> > If we reuse selinux_tun_dev_attach, we won't need to
> > change selinux policy, with a new capability we will need to change it
> > to allow libvirt to do TUNSETQUEUE.
> >
> 
> Also needed for qemu too since it may call TUNSETQUEUE when guest wants
> to change the number of queues.

Hmm that's nasty. If you allow qemu to do TUNSETQUEUE then how
do you prevent it from attaching to some other tun?
Maybe we can extend TUNSETQUEUE or add another ioctl to
mark a queue active/inactive? Probably control transmit
and receive being active separately as well.

> >> btw. Current code does allow calling TUNSETQUEUE to a persistent tun/tap 
> >> device with no file attached. It should be a bug and need to be fixed.
> > Is this a problem? You can always
> > attach
> > set queue
> > detach
> >
> > and it would be hard to prevent this ...
> 
> Currently, the following steps is allowed:
> 
> 1. fd1 = open("/dev/net/tun");
> 2. tunsetiff(fd1, "tap0");
> 3. tunsetpersistent("tap0");
> 4. close(fd1);
> 5. fd2 = open("/dev/net/tun");
> 6. tunsetqueue(fd2, "tap0);

Allowed for libvirt, right? Not for qemu.

> Looks like step 6 should be forbidden since:
> 
> - no fd/sockets were attached to the device, we need use TUNSETIFF
> instead to keep the API as we used do in single queue tun
> - we need update the security information in tun_struct just like what
> we discussed in this mail
> - it may also miss checks in TUNSETIFF

We need to fix the checks anyway. Basically if you allow
a queue to be the only fd that is attached
(and I don't see how you can prevent this since
userspace does not expect close to fail),
I don't see why one way to get to this state
should be legal and another illegal.

The rest is implementation detail.

> >>> One thing that I think we probably should change is the relabelto/from
> >>> permissions in the function above (selinux_tun_dev_attach()); in the case
> >>> where the socket does not yet have a label, e.g. 'sksec->sid == 0', we
> >>> should probably skip the relabel permissions since we want to assign the
> >>> TUN device label regardless in this case.
> >> I'm not familiar with the selinux, have a quick glance of the code, looks like 
> >> the label has been initialized to SECINITSID_KERNEL in 
> >> selinux_socket_post_create().
> >>
> >> Thanks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists