Message-ID: <20121206141220.GA32521@redhat.com>
Date:	Thu, 6 Dec 2012 16:12:20 +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 v2 3/3] tun: fix LSM/SELinux labeling of tun/tap
 devices

On Thu, Dec 06, 2012 at 09:51:13PM +0800, Jason Wang wrote:
> On Thursday, December 06, 2012 12:33:25 PM Michael S. Tsirkin wrote:
> > On Wed, Dec 05, 2012 at 03:26:19PM -0500, 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.  In the process we tweak the LSM hooks to work with this new
> > > approach to TUN device/socket labeling and introduce a new LSM hook,
> > > security_tun_dev_create_queue(), to approve requests to create a new
> > > TUN queue via TUNSETQUEUE.
> > > 
> > > The SELinux code has been adjusted to match the new LSM hooks, the
> > > other LSMs do not make use of the LSM TUN controls.  This patch makes
> > > use of the recently added "tun_socket:create_queue" permission to
> > > restrict access to the TUNSETQUEUE operation.  On older SELinux
> > > policies which do not define the "tun_socket:create_queue" permission
> > > the access control decision for TUNSETQUEUE will be handled according
> > > to the SELinux policy's unknown permission setting.
> > > 
> > > Signed-off-by: Paul Moore <pmoore@...hat.com>
> > 
> > OK so just to verify: this can be used to ensure that qemu
> > process that has the queue fd can only attach it to
> > a specific device, right?
> 
> I think it can't.
> And I'm not sure whether we need selinux help to do this. 

Well without selinux I doi not see a problem.
If you can do SETQUEUE you can do SETIFF too and then
you can attach to tap.

> Looks like we can do this without selinux through:
> 
> 1. Don't assign a NULL pointer to tfile->tun during file detaching

So you detach from tun but keep a pointer to it? Not good.

> 2. Compare the ifr_name and the name of tfil->tun, if not equal, return -EINVAL
> 3. Set a special flag in tun_detach_all() to notify the fd is not usable, and 
> can't be used for future attaching.
> 
> Afther this, only the device that the fd is first attched through (TUNSETIFF or 
> TUNSETQUEUE) is allowed to be attached again.

This looks like a hard-coded security policy.
The problem is not detach the problem is attach,
we should solve it there.

> > 
> > > ---
> > > 
> > >  drivers/net/tun.c                 |   26 +++++++++++++---
> > >  include/linux/security.h          |   59
> > >  +++++++++++++++++++++++++++++-------- security/capability.c            
> > >  |   24 +++++++++++++--
> > >  security/security.c               |   28 ++++++++++++++----
> > >  security/selinux/hooks.c          |   50 ++++++++++++++++++++++++-------
> > >  security/selinux/include/objsec.h |    4 +++
> > >  6 files changed, 153 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index 14a0454..fb8148b 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -182,6 +182,7 @@ struct tun_struct {
> > > 
> > >  	struct hlist_head flows[TUN_NUM_FLOW_ENTRIES];
> > >  	struct timer_list flow_gc_timer;
> > >  	unsigned long ageing_time;
> > > 
> > > +	void *security;
> > > 
> > >  };
> > >  
> > >  static inline u32 tun_hashfn(u32 rxhash)
> > > 
> > > @@ -465,6 +466,10 @@ static int tun_attach(struct tun_struct *tun, struct
> > > file *file)> 
> > >  	struct tun_file *tfile = file->private_data;
> > >  	int err;
> > > 
> > > +	err = security_tun_dev_attach(tfile->socket.sk, tun->security);
> > > +	if (err < 0)
> > > +		goto out;
> > > +
> > > 
> > >  	err = -EINVAL;
> > >  	if (rcu_dereference_protected(tfile->tun, lockdep_rtnl_is_held()))
> > >  	
> > >  		goto out;
> > > 
> > > @@ -1348,6 +1353,7 @@ static void tun_free_netdev(struct net_device *dev)
> > > 
> > >  	struct tun_struct *tun = netdev_priv(dev);
> > >  	
> > >  	tun_flow_uninit(tun);
> > > 
> > > +	security_tun_dev_free_security(tun->security);
> > > 
> > >  	free_netdev(dev);
> > >  
> > >  }
> > > 
> > > @@ -1534,7 +1540,7 @@ static int tun_set_iff(struct net *net, struct file
> > > *file, struct ifreq *ifr)> 
> > >  		if (tun_not_capable(tun))
> > >  		
> > >  			return -EPERM;
> > > 
> > > -		err = security_tun_dev_attach(tfile->socket.sk);
> > > +		err = security_tun_dev_open(tun->security);
> > > 
> > >  		if (err < 0)
> > >  		
> > >  			return err;
> > > 
> > > @@ -1587,7 +1593,9 @@ static int tun_set_iff(struct net *net, struct file
> > > *file, struct ifreq *ifr)> 
> > >  		spin_lock_init(&tun->lock);
> > > 
> > > -		security_tun_dev_post_create(&tfile->sk);
> > > +		err = security_tun_dev_alloc_security(&tun->security);
> > > +		if (err < 0)
> > > +			goto err_free_dev;
> > > 
> > >  		tun_net_init(dev);
> > > 
> > > @@ -1767,12 +1775,18 @@ static int tun_set_queue(struct file *file, struct
> > > ifreq *ifr)> 
> > >  		tun = netdev_priv(dev);
> > >  		if (dev->netdev_ops != &tap_netdev_ops &&
> > > 
> > > -			dev->netdev_ops != &tun_netdev_ops)
> > > +			dev->netdev_ops != &tun_netdev_ops) {
> > > 
> > >  			ret = -EINVAL;
> > > 
> > > -		else if (tun_not_capable(tun))
> > > +			goto unlock;
> > > +		}
> > > +		if (tun_not_capable(tun)) {
> > > 
> > >  			ret = -EPERM;
> > > 
> > > -		else
> > > -			ret = tun_attach(tun, file);
> > > +			goto unlock;
> > > +		}
> > > +		ret = security_tun_dev_create_queue(tun->security);
> > > +		if (ret < 0)
> > > +			goto unlock;
> > > +		ret = tun_attach(tun, file);
> > > 
> > >  	} else if (ifr->ifr_flags & IFF_DETACH_QUEUE)
> > >  	
> > >  		__tun_detach(tfile, false);
> > >  	
> > >  	else
> > > 
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 05e88bd..8ea923b 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -983,17 +983,29 @@ static inline void security_free_mnt_opts(struct
> > > security_mnt_opts *opts)> 
> > >   *	tells the LSM to decrement the number of secmark labeling rules loaded
> > >   * @req_classify_flow:
> > >   *	Sets the flow's sid to the openreq sid.
> > > 
> > > + * @tun_dev_alloc_security:
> > > + *	This hook allows a module to allocate a security structure for a TUN
> > > + *	device.
> > > + *	@security pointer to a security structure pointer.
> > > + *	Returns a zero on success, negative values on failure.
> > > + * @tun_dev_free_security:
> > > + *	This hook allows a module to free the security structure for a TUN
> > > + *	device.
> > > + *	@security pointer to the TUN device's security structure
> > > 
> > >   * @tun_dev_create:
> > >   *	Check permissions prior to creating a new TUN device.
> > > 
> > > - * @tun_dev_post_create:
> > > - *	This hook allows a module to update or allocate a per-socket security
> > > - *	structure.
> > > - *	@sk contains the newly created sock structure.
> > > + * @tun_dev_create_queue:
> > > + *	Check permissions prior to creating a new TUN device queue.
> > > + *	@security pointer to the TUN device's security structure.
> > > 
> > >   * @tun_dev_attach:
> > > - *	Check permissions prior to attaching to a persistent TUN device.  This
> > > - *	hook can also be used by the module to update any security state
> > > + *	This hook can be used by the module to update any security state
> > > 
> > >   *	associated with the TUN device's sock structure.
> > >   *	@sk contains the existing sock structure.
> > > 
> > > + *	@security pointer to the TUN device's security structure.
> > > + * @tun_dev_open:
> > > + *	This hook can be used by the module to update any security state
> > > + *	associated with the TUN device's security structure.
> > > + *	@security pointer to the TUN devices's security structure.
> > > 
> > >   *
> > >   * Security hooks for XFRM operations.
> > >   *
> > > 
> > > @@ -1613,9 +1625,12 @@ struct security_operations {
> > > 
> > >  	void (*secmark_refcount_inc) (void);
> > >  	void (*secmark_refcount_dec) (void);
> > >  	void (*req_classify_flow) (const struct request_sock *req, struct flowi
> > >  	*fl);> 
> > > -	int (*tun_dev_create)(void);
> > > -	void (*tun_dev_post_create)(struct sock *sk);
> > > -	int (*tun_dev_attach)(struct sock *sk);
> > > +	int (*tun_dev_alloc_security) (void **security);
> > > +	void (*tun_dev_free_security) (void *security);
> > > +	int (*tun_dev_create) (void);
> > > +	int (*tun_dev_create_queue) (void *security);
> > > +	int (*tun_dev_attach) (struct sock *sk, void *security);
> > > +	int (*tun_dev_open) (void *security);
> > > 
> > >  #endif	/* CONFIG_SECURITY_NETWORK */
> > >  
> > >  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> > > 
> > > @@ -2553,9 +2568,12 @@ void security_inet_conn_established(struct sock
> > > *sk,
> > > 
> > >  int security_secmark_relabel_packet(u32 secid);
> > >  void security_secmark_refcount_inc(void);
> > >  void security_secmark_refcount_dec(void);
> > > 
> > > +int security_tun_dev_alloc_security(void **security);
> > > +void security_tun_dev_free_security(void *security);
> > > 
> > >  int security_tun_dev_create(void);
> > > 
> > > -void security_tun_dev_post_create(struct sock *sk);
> > > -int security_tun_dev_attach(struct sock *sk);
> > > +int security_tun_dev_create_queue(void *security);
> > > +int security_tun_dev_attach(struct sock *sk, void *security);
> > > +int security_tun_dev_open(void *security);
> > > 
> > >  #else	/* CONFIG_SECURITY_NETWORK */
> > >  static inline int security_unix_stream_connect(struct sock *sock,
> > > 
> > > @@ -2720,16 +2738,31 @@ static inline void
> > > security_secmark_refcount_dec(void)> 
> > >  {
> > >  }
> > > 
> > > +static inline int security_tun_dev_alloc_security(void **security)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void security_tun_dev_free_security(void *security)
> > > +{
> > > +}
> > > +
> > > 
> > >  static inline int security_tun_dev_create(void)
> > >  {
> > >  
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > -static inline void security_tun_dev_post_create(struct sock *sk)
> > > +static inline int security_tun_dev_create_queue(void *security)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static inline int security_tun_dev_attach(struct sock *sk, void
> > > *security)
> > > 
> > >  {
> > > 
> > > +	return 0;
> > > 
> > >  }
> > > 
> > > -static inline int security_tun_dev_attach(struct sock *sk)
> > > +static inline int security_tun_dev_open(void *security)
> > > 
> > >  {
> > >  
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > diff --git a/security/capability.c b/security/capability.c
> > > index b14a30c..bf4cbf2 100644
> > > --- a/security/capability.c
> > > +++ b/security/capability.c
> > > @@ -704,16 +704,31 @@ static void cap_req_classify_flow(const struct
> > > request_sock *req,> 
> > >  {
> > >  }
> > > 
> > > +static int cap_tun_dev_alloc_security(void **security)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static void cap_tun_dev_free_security(void *security)
> > > +{
> > > +}
> > > +
> > > 
> > >  static int cap_tun_dev_create(void)
> > >  {
> > >  
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > -static void cap_tun_dev_post_create(struct sock *sk)
> > > +static int cap_tun_dev_create_queue(void *security)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static int cap_tun_dev_attach(struct sock *sk, void *security)
> > > 
> > >  {
> > > 
> > > +	return 0;
> > > 
> > >  }
> > > 
> > > -static int cap_tun_dev_attach(struct sock *sk)
> > > +static int cap_tun_dev_open(void *security)
> > > 
> > >  {
> > >  
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > @@ -1044,8 +1059,11 @@ void __init security_fixup_ops(struct
> > > security_operations *ops)> 
> > >  	set_to_cap_if_null(ops, secmark_refcount_inc);
> > >  	set_to_cap_if_null(ops, secmark_refcount_dec);
> > >  	set_to_cap_if_null(ops, req_classify_flow);
> > > 
> > > +	set_to_cap_if_null(ops, tun_dev_alloc_security);
> > > +	set_to_cap_if_null(ops, tun_dev_free_security);
> > > 
> > >  	set_to_cap_if_null(ops, tun_dev_create);
> > > 
> > > -	set_to_cap_if_null(ops, tun_dev_post_create);
> > > +	set_to_cap_if_null(ops, tun_dev_create_queue);
> > > +	set_to_cap_if_null(ops, tun_dev_open);
> > > 
> > >  	set_to_cap_if_null(ops, tun_dev_attach);
> > >  
> > >  #endif	/* CONFIG_SECURITY_NETWORK */
> > >  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> > > 
> > > diff --git a/security/security.c b/security/security.c
> > > index 8dcd4ae..4d82654 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1244,24 +1244,42 @@ void security_secmark_refcount_dec(void)
> > > 
> > >  }
> > >  EXPORT_SYMBOL(security_secmark_refcount_dec);
> > > 
> > > +int security_tun_dev_alloc_security(void **security)
> > > +{
> > > +	return security_ops->tun_dev_alloc_security(security);
> > > +}
> > > +EXPORT_SYMBOL(security_tun_dev_alloc_security);
> > > +
> > > +void security_tun_dev_free_security(void *security)
> > > +{
> > > +	security_ops->tun_dev_free_security(security);
> > > +}
> > > +EXPORT_SYMBOL(security_tun_dev_free_security);
> > > +
> > > 
> > >  int security_tun_dev_create(void)
> > >  {
> > >  
> > >  	return security_ops->tun_dev_create();
> > >  
> > >  }
> > >  EXPORT_SYMBOL(security_tun_dev_create);
> > > 
> > > -void security_tun_dev_post_create(struct sock *sk)
> > > +int security_tun_dev_create_queue(void *security)
> > > 
> > >  {
> > > 
> > > -	return security_ops->tun_dev_post_create(sk);
> > > +	return security_ops->tun_dev_create_queue(security);
> > > 
> > >  }
> > > 
> > > -EXPORT_SYMBOL(security_tun_dev_post_create);
> > > +EXPORT_SYMBOL(security_tun_dev_create_queue);
> > > 
> > > -int security_tun_dev_attach(struct sock *sk)
> > > +int security_tun_dev_attach(struct sock *sk, void *security)
> > > 
> > >  {
> > > 
> > > -	return security_ops->tun_dev_attach(sk);
> > > +	return security_ops->tun_dev_attach(sk, security);
> > > 
> > >  }
> > >  EXPORT_SYMBOL(security_tun_dev_attach);
> > > 
> > > +int security_tun_dev_open(void *security)
> > > +{
> > > +	return security_ops->tun_dev_open(security);
> > > +}
> > > +EXPORT_SYMBOL(security_tun_dev_open);
> > > +
> > > 
> > >  #endif	/* CONFIG_SECURITY_NETWORK */
> > >  
> > >  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> > > 
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 61a5336..f1efb08 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -4399,6 +4399,24 @@ static void selinux_req_classify_flow(const struct
> > > request_sock *req,> 
> > >  	fl->flowi_secid = req->secid;
> > >  
> > >  }
> > > 
> > > +static int selinux_tun_dev_alloc_security(void **security)
> > > +{
> > > +	struct tun_security_struct *tunsec;
> > > +
> > > +	tunsec = kzalloc(sizeof(*tunsec), GFP_KERNEL);
> > > +	if (!tunsec)
> > > +		return -ENOMEM;
> > > +	tunsec->sid = current_sid();
> > > +
> > > +	*security = tunsec;
> > > +	return 0;
> > > +}
> > > +
> > > +static void selinux_tun_dev_free_security(void *security)
> > > +{
> > > +	kfree(security);
> > > +}
> > > +
> > > 
> > >  static int selinux_tun_dev_create(void)
> > >  {
> > >  
> > >  	u32 sid = current_sid();
> > > 
> > > @@ -4414,8 +4432,17 @@ static int selinux_tun_dev_create(void)
> > > 
> > >  			    NULL);
> > >  
> > >  }
> > > 
> > > -static void selinux_tun_dev_post_create(struct sock *sk)
> > > +static int selinux_tun_dev_create_queue(void *security)
> > > 
> > >  {
> > > 
> > > +	struct tun_security_struct *tunsec = security;
> > > +
> > > +	return avc_has_perm(current_sid(), tunsec->sid, SECCLASS_TUN_SOCKET,
> > > +			    TUN_SOCKET__CREATE_QUEUE, NULL);
> > > +}
> > > +
> > > +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;
> > >  	
> > >  	/* we don't currently perform any NetLabel based labeling here and it
> > > 
> > > @@ -4425,20 +4452,19 @@ static void selinux_tun_dev_post_create(struct
> > > sock *sk)> 
> > >  	 * cause confusion to the TUN user that had no idea network labeling
> > >  	 * protocols were being used */
> > > 
> > > -	/* see the comments in selinux_tun_dev_create() about why we don't use
> > > -	 * the sockcreate SID here */
> > > -
> > > -	sksec->sid = current_sid();
> > > +	sksec->sid = tunsec->sid;
> > > 
> > >  	sksec->sclass = SECCLASS_TUN_SOCKET;
> > > 
> > > +
> > > +	return 0;
> > > 
> > >  }
> > > 
> > > -static int selinux_tun_dev_attach(struct sock *sk)
> > > +static int selinux_tun_dev_open(void *security)
> > > 
> > >  {
> > > 
> > > -	struct sk_security_struct *sksec = sk->sk_security;
> > > +	struct tun_security_struct *tunsec = security;
> > > 
> > >  	u32 sid = current_sid();
> > >  	int err;
> > > 
> > > -	err = avc_has_perm(sid, sksec->sid, SECCLASS_TUN_SOCKET,
> > > +	err = avc_has_perm(sid, tunsec->sid, SECCLASS_TUN_SOCKET,
> > > 
> > >  			   TUN_SOCKET__RELABELFROM, NULL);
> > >  	
> > >  	if (err)
> > >  	
> > >  		return err;
> > > 
> > > @@ -4446,8 +4472,7 @@ static int selinux_tun_dev_attach(struct sock *sk)
> > > 
> > >  			   TUN_SOCKET__RELABELTO, NULL);
> > >  	
> > >  	if (err)
> > >  	
> > >  		return err;
> > > 
> > > -
> > > -	sksec->sid = sid;
> > > +	tunsec->sid = sid;
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > @@ -5642,9 +5667,12 @@ static struct security_operations selinux_ops = {
> > > 
> > >  	.secmark_refcount_inc =		selinux_secmark_refcount_inc,
> > >  	.secmark_refcount_dec =		selinux_secmark_refcount_dec,
> > >  	.req_classify_flow =		selinux_req_classify_flow,
> > > 
> > > +	.tun_dev_alloc_security =	selinux_tun_dev_alloc_security,
> > > +	.tun_dev_free_security =	selinux_tun_dev_free_security,
> > > 
> > >  	.tun_dev_create =		selinux_tun_dev_create,
> > > 
> > > -	.tun_dev_post_create = 		selinux_tun_dev_post_create,
> > > +	.tun_dev_create_queue =		selinux_tun_dev_create_queue,
> > > 
> > >  	.tun_dev_attach =		selinux_tun_dev_attach,
> > > 
> > > +	.tun_dev_open =			selinux_tun_dev_open,
> > > 
> > >  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> > >  
> > >  	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,
> > > 
> > > diff --git a/security/selinux/include/objsec.h
> > > b/security/selinux/include/objsec.h index 26c7eee..aa47bca 100644
> > > --- a/security/selinux/include/objsec.h
> > > +++ b/security/selinux/include/objsec.h
> > > @@ -110,6 +110,10 @@ struct sk_security_struct {
> > > 
> > >  	u16 sclass;			/* sock security class */
> > >  
> > >  };
> > > 
> > > +struct tun_security_struct {
> > > +	u32 sid;			/* SID for the tun device sockets */
> > > +};
> > > +
> > > 
> > >  struct key_security_struct {
> > >  
> > >  	u32 sid;	/* SID of key */
> > >  
> > >  };
--
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