[RFC PATCH v1 0/2] The Long Lost TUN LSM Hooks

[RFC PATCH v1 0/2] The Long Lost TUN LSM Hooks

[Paul Moore]

While drivers in general aren't good places for LSM hooks the TUN driver is
a bit different because of how it handles sockets and network traffic.  The
problem lies in the fact that the TUN driver creates a sock structure to use
when sending network traffic but the sock is never put through the same LSM
setup/control as other sock structures on the system which makes enforcing
security on TUN generated traffic difficult for some LSMs.  This patch set
adds three new LSM hooks, all specific to the TUN driver (none of the existing
hooks made sense, trust me we tried), to control and monitor the creating and
attachment of TUN devices.

The necessary support for SELinux is also included in this patch with support
for Smack and TOMOYO absent; although I suspect there will be no changes needed
for either of those LSMs.

--

NOTE: These patches are truly RFC, please let me know how you feel about these
      new hooks.  I've booted a kernel with these changes but I'm having some
      problems today with my Rawhide/KVM test machine so I'm having mixed luck
      testing this - no regressions, but no real TUN testing either.

---

Paul Moore (2):
      selinux: Support for the new TUN LSM hooks
      lsm: Add hooks to the TUN driver


 drivers/net/tun.c                          |   41 ++++++++-------
 include/linux/security.h                   |   34 +++++++++++++
 security/commoncap.c                       |   26 ++++++++++
 security/security.c                        |   18 +++++++
 security/selinux/hooks.c                   |   76 +++++++++++++++++++++++++++-
 security/selinux/include/av_inherit.h      |    1 
 security/selinux/include/av_permissions.h  |   22 ++++++++
 security/selinux/include/class_to_string.h |    1 
 security/selinux/include/flask.h           |    1 
 security/selinux/include/security.h        |    2 +
 security/selinux/selinuxfs.c               |    3 +
 security/selinux/ss/services.c             |    3 +
 12 files changed, 207 insertions(+), 21 deletions(-)

[RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Paul Moore]

The TUN driver lacks any LSM hooks which makes it difficult for LSM modules,
such as SELinux, to enforce access controls on network traffic generated by
TUN users; this is particularly problematic for virtualization apps such as
QEMU and KVM.  This patch adds three new LSM hooks designed to control the
creation and attachment of TUN devices, the hooks are:

 * security_tun_dev_create()
   Provides access control for the creation of new TUN devices

 * security_tun_dev_post_create()
   Provides the ability to create the necessary socket LSM state for newly
   created TUN devices

 * security_tun_dev_attach()
   Provides access control for attaching to existing, persistent TUN devices
   and the ability to update the TUN device's socket LSM state as necessary
---

 drivers/net/tun.c        |   41 +++++++++++++++++++++++------------------
 include/linux/security.h |   34 ++++++++++++++++++++++++++++++++++
 security/commoncap.c     |   26 ++++++++++++++++++++++++++
 security/security.c      |   18 ++++++++++++++++++
 4 files changed, 101 insertions(+), 18 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 027f7ab..f26c1fe 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -130,28 +130,22 @@ static inline struct tun_sock *tun_sk(struct sock *sk)
 static int tun_attach(struct tun_struct *tun, struct file *file)
 {
 	struct tun_file *tfile = file->private_data;
-	const struct cred *cred = current_cred();
-	int err;
+	int err = 0;
 
 	ASSERT_RTNL();
 
-	/* Check permissions */
-	if (((tun->owner != -1 && cred->euid != tun->owner) ||
-	     (tun->group != -1 && !in_egroup_p(tun->group))) &&
-		!capable(CAP_NET_ADMIN))
-		return -EPERM;
-
 	netif_tx_lock_bh(tun->dev);
 
-	err = -EINVAL;
-	if (tfile->tun)
+	if (tfile->tun) {
+		err = -EINVAL;
 		goto out;
+	}
 
-	err = -EBUSY;
-	if (tun->tfile)
+	if (tun->tfile) {
+		err = -EBUSY;
 		goto out;
+	}
 
-	err = 0;
 	tfile->tun = tun;
 	tun->tfile = tfile;
 	dev_hold(tun->dev);
@@ -922,6 +916,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 	struct sock *sk;
 	struct tun_struct *tun;
 	struct net_device *dev;
+	const struct cred *cred = current_cred();
 	int err;
 
 	dev = __dev_get_by_name(net, ifr->ifr_name);
@@ -935,6 +930,13 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		else
 			return -EINVAL;
 
+		if ((tun->owner != -1 && cred->euid != tun->owner) ||
+		    (tun->group != -1 && !in_egroup_p(tun->group)))
+			return -EPERM;
+		err = security_tun_dev_attach(tun->sk);
+		if (err < 0)
+			return err;
+
 		err = tun_attach(tun, file);
 		if (err < 0)
 			return err;
@@ -943,10 +945,9 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		char *name;
 		unsigned long flags = 0;
 
-		err = -EINVAL;
-
-		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
+		err = security_tun_dev_create();
+		if (err < 0)
+			return err;
 
 		/* Set dev type */
 		if (ifr->ifr_flags & IFF_TUN) {
@@ -957,8 +958,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 			/* TAP device */
 			flags |= TUN_TAP_DEV;
 			name = "tap%d";
-		} else
+		} else {
+			err = -EINVAL;
 			goto failed;
+		}
 
 		if (*ifr->ifr_name)
 			name = ifr->ifr_name;
@@ -989,6 +992,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 		tun->sk = sk;
 		container_of(sk, struct tun_sock, sk)->tun = tun;
 
+		security_tun_dev_post_create(sk);
+
 		tun_net_init(dev);
 
 		if (strchr(dev->name, '%')) {
diff --git a/include/linux/security.h b/include/linux/security.h
index 5eff459..67f5d91 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -91,6 +91,9 @@ struct seq_file;
 extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
 extern int cap_netlink_recv(struct sk_buff *skb, int cap);
 
+extern int cap_tun_dev_create(void);
+extern int cap_tun_dev_attach(void);
+
 extern unsigned long mmap_min_addr;
 /*
  * Values used in the task_security_ops calls
@@ -974,6 +977,17 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Sets the connection's peersid to the secmark on skb.
  * @req_classify_flow:
  *	Sets the flow's sid to the openreq sid.
+ * @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.
+ *	@tun_sk contains the newly created sock 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
+ *	associated with the TUN device's sock structure.
+ *	@tun_sk contains the existing sock structure.
  *
  * Security hooks for XFRM operations.
  *
@@ -1572,6 +1586,9 @@ struct security_operations {
 	void (*inet_csk_clone) (struct sock *newsk, const struct request_sock *req);
 	void (*inet_conn_established) (struct sock *sk, struct sk_buff *skb);
 	void (*req_classify_flow) (const struct request_sock *req, struct flowi *fl);
+	int (*tun_dev_create)(void);
+	void (*tun_dev_post_create)(struct sock *tun_sk);
+	int (*tun_dev_attach)(struct sock *tun_sk);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -2557,6 +2574,9 @@ void security_inet_csk_clone(struct sock *newsk,
 			const struct request_sock *req);
 void security_inet_conn_established(struct sock *sk,
 			struct sk_buff *skb);
+int security_tun_dev_create(void);
+void security_tun_dev_post_create(struct sock *tun_sk);
+int security_tun_dev_attach(struct sock *tun_sk);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct socket *sock,
@@ -2707,6 +2727,20 @@ static inline void security_inet_conn_established(struct sock *sk,
 			struct sk_buff *skb)
 {
 }
+
+static inline int security_tun_dev_create(void)
+{
+	return cap_tun_dev_create();
+}
+
+static inline void security_tun_dev_post_create(struct sock *tun_sk)
+{
+}
+
+static inline int security_tun_dev_attach(struct sock *tun_sk)
+{
+	return cap_tun_dev_attach();
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/commoncap.c b/security/commoncap.c
index 48b7e02..07125a6 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -984,3 +984,29 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
 		cap_sys_admin = 1;
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
+
+/**
+ * cap_tun_dev_create - Determine if creation of a new TUN device is allowed
+ *
+ * Determine if the user is allowed to create a new TUN device, historically
+ * this has always required the CAP_NET_ADMIN permission.
+ */
+int cap_tun_dev_create(void)
+{
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+	return 0;
+}
+
+/**
+ * cap_tun_dev_attach - Determine if attaching to an TUN device is allowed
+ *
+ * Determine if the user is allowed to attach to an existing persistent TUN
+ * device, historically this has always required the CAP_NET_ADMIN permission.
+ */
+int cap_tun_dev_attach(void)
+{
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+	return 0;
+}
diff --git a/security/security.c b/security/security.c
index dc7674f..14ebf82 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1112,6 +1112,24 @@ void security_inet_conn_established(struct sock *sk,
 	security_ops->inet_conn_established(sk, skb);
 }
 
+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 *tun_sk)
+{
+	return security_ops->tun_dev_post_create(tun_sk);
+}
+EXPORT_SYMBOL(security_tun_dev_post_create);
+
+int security_tun_dev_attach(struct sock *tun_sk)
+{
+	return security_ops->tun_dev_attach(tun_sk);
+}
+EXPORT_SYMBOL(security_tun_dev_attach);
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Eric Paris]

On Tue, Aug 4, 2009 at 5:21 PM, Paul Moore<paul.moore@hp.com> wrote:
> The TUN driver lacks any LSM hooks which makes it difficult for LSM modules,
> such as SELinux, to enforce access controls on network traffic generated by
> TUN users; this is particularly problematic for virtualization apps such as
> QEMU and KVM.  This patch adds three new LSM hooks designed to control the
> creation and attachment of TUN devices, the hooks are:
>

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 48b7e02..07125a6 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -984,3 +984,29 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
>                cap_sys_admin = 1;
>        return __vm_enough_memory(mm, pages, cap_sys_admin);
>  }
> +
> +/**
> + * cap_tun_dev_create - Determine if creation of a new TUN device is allowed
> + *
> + * Determine if the user is allowed to create a new TUN device, historically
> + * this has always required the CAP_NET_ADMIN permission.
> + */
> +int cap_tun_dev_create(void)
> +{
> +       if (!capable(CAP_NET_ADMIN))
> +               return -EPERM;
> +       return 0;
> +}
> +
> +/**
> + * cap_tun_dev_attach - Determine if attaching to an TUN device is allowed
> + *
> + * Determine if the user is allowed to attach to an existing persistent TUN
> + * device, historically this has always required the CAP_NET_ADMIN permission.
> + */
> +int cap_tun_dev_attach(void)
> +{
> +       if (!capable(CAP_NET_ADMIN))
> +               return -EPERM;
> +       return 0;
> +}
> diff --git a/security/security.c b/security/security.c
> index dc7674f..14ebf82 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1112,6 +1112,24 @@ void security_inet_conn_established(struct sock *sk,
>        security_ops->inet_conn_established(sk, skb);
>  }
>
> +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 *tun_sk)
> +{
> +       return security_ops->tun_dev_post_create(tun_sk);
> +}
> +EXPORT_SYMBOL(security_tun_dev_post_create);
> +
> +int security_tun_dev_attach(struct sock *tun_sk)
> +{
> +       return security_ops->tun_dev_attach(tun_sk);
> +}
> +EXPORT_SYMBOL(security_tun_dev_attach);
> +
>  #endif /* CONFIG_SECURITY_NETWORK */
>
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM

You also must add cap_tun_dev_post_create in security/capability.c and
you need to call set_to_cap_if_null() for all 3 of these new
functions.  I believe you'll see a wonderful null pointer panic if you
tried with selinux=0.....

-Eric

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Serge E. Hallyn]

Quoting Paul Moore (paul.moore@hp.com):
...
>  static int tun_attach(struct tun_struct *tun, struct file *file)
>  {
>  	struct tun_file *tfile = file->private_data;
> -	const struct cred *cred = current_cred();
> -	int err;
> +	int err = 0;
> 
>  	ASSERT_RTNL();
> 
> -	/* Check permissions */
> -	if (((tun->owner != -1 && cred->euid != tun->owner) ||
> -	     (tun->group != -1 && !in_egroup_p(tun->group))) &&
> -		!capable(CAP_NET_ADMIN))
> -		return -EPERM;
...

> @@ -935,6 +930,13 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
>  		else
>  			return -EINVAL;
> 
> +		if ((tun->owner != -1 && cred->euid != tun->owner) ||
> +		    (tun->group != -1 && !in_egroup_p(tun->group)))
> +			return -EPERM;
> +		err = security_tun_dev_attach(tun->sk);
> +		if (err < 0)
> +			return err;
> +

...

> +/**
> + * cap_tun_dev_attach - Determine if attaching to an TUN device is allowed
> + *
> + * Determine if the user is allowed to attach to an existing persistent TUN
> + * device, historically this has always required the CAP_NET_ADMIN permission.
> + */
> +int cap_tun_dev_attach(void)
> +{
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +	return 0;
> +}

The checks before and after this patch are not equivalent.  Post-patch,
one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
you only needed those if current_cred() did not own the tun device.  Is
that intentional?

Also as Eric said this patch needs to set the cap_ hooks.  This patch
isn't yet introducing the selinux hooks, so iiuc actually this patch should
always oops if CONFIG_SECURITY=y.

-serge

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Paul Moore]

On Wednesday 05 August 2009 10:13:50 am Serge E. Hallyn wrote:
> Quoting Paul Moore (paul.moore@hp.com):

[NOTE: my email has been out all day due to some mysterious FS issue so my 
apologies for not replying sooner]

...

> The checks before and after this patch are not equivalent.  Post-patch,
> one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
> you only needed those if current_cred() did not own the tun device.  Is
> that intentional?

Nope, just a goof on my part; I misread the booleans and haven't fully tested 
the patch yet so it slipped out, thanks for catching it.  This brings up a 
good point, would we rather move the TUN owner/group checks into the cap_tun_* 
functions or move the capable() call back into the TUN driver?  The answer 
wasn't clear to me when I was looking at the code before and the uniqueness of 
the TUN driver doesn't help much in this regard.

> Also as Eric said this patch needs to set the cap_ hooks.  This patch
> isn't yet introducing the selinux hooks, so iiuc actually this patch should
> always oops if CONFIG_SECURITY=y.

Yep, another symptom of not enough testing as I mentioned out in the original 
posting, thanks to both of you for pointing this out ... now somebody just 
needs to fix Rawhide so I can actually get a KVM instance running :)

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Serge E. Hallyn]

Quoting Paul Moore (paul.moore@hp.com):
> On Wednesday 05 August 2009 10:13:50 am Serge E. Hallyn wrote:
> > Quoting Paul Moore (paul.moore@hp.com):
> 
> [NOTE: my email has been out all day due to some mysterious FS issue so my 
> apologies for not replying sooner]
> 
> ...
> 
> > The checks before and after this patch are not equivalent.  Post-patch,
> > one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
> > you only needed those if current_cred() did not own the tun device.  Is
> > that intentional?
> 
> Nope, just a goof on my part; I misread the booleans and haven't fully tested 
> the patch yet so it slipped out, thanks for catching it.  This brings up a 
> good point, would we rather move the TUN owner/group checks into the cap_tun_* 
> functions or move the capable() call back into the TUN driver?  The answer 
> wasn't clear to me when I was looking at the code before and the uniqueness of 
> the TUN driver doesn't help much in this regard.

I see the question being asked as:  Does this device belong to
the caller and, if not, is the caller privileged to act
anyway?'  So I think the capable call should be moved back
into the tun driver, followed by a separate security_tun_dev_attach()
check, since that is a separate, restrictive question.

thanks,
-serge

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Paul Moore]

On Wednesday 05 August 2009 10:15:58 pm Serge E. Hallyn wrote:
> Quoting Paul Moore (paul.moore@hp.com):
> > On Wednesday 05 August 2009 10:13:50 am Serge E. Hallyn wrote:
> > > Quoting Paul Moore (paul.moore@hp.com):
> >
> > [NOTE: my email has been out all day due to some mysterious FS issue so
> > my apologies for not replying sooner]
> >
> > ...
> >
> > > The checks before and after this patch are not equivalent.  Post-patch,
> > > one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
> > > you only needed those if current_cred() did not own the tun device.  Is
> > > that intentional?
> >
> > Nope, just a goof on my part; I misread the booleans and haven't fully
> > tested the patch yet so it slipped out, thanks for catching it.  This
> > brings up a good point, would we rather move the TUN owner/group checks
> > into the cap_tun_* functions or move the capable() call back into the TUN
> > driver?  The answer wasn't clear to me when I was looking at the code
> > before and the uniqueness of the TUN driver doesn't help much in this
> > regard.
>
> I see the question being asked as:  Does this device belong to
> the caller and, if not, is the caller privileged to act
> anyway?'  So I think the capable call should be moved back
> into the tun driver, followed by a separate security_tun_dev_attach()
> check, since that is a separate, restrictive question.

Works for me, I'll make the change.

BTW, the main reason for posting the patches in such an early state was to 
solicit feedback on the location and types of hooks added; I've read lots of 
good feedback but nothing regarding the fundamental aspects of the hooks ... 
any comments before I push out v2?

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Serge E. Hallyn]

Quoting Paul Moore (paul.moore@hp.com):
> On Wednesday 05 August 2009 10:15:58 pm Serge E. Hallyn wrote:
> > Quoting Paul Moore (paul.moore@hp.com):
> > > On Wednesday 05 August 2009 10:13:50 am Serge E. Hallyn wrote:
> > > > Quoting Paul Moore (paul.moore@hp.com):
> > >
> > > [NOTE: my email has been out all day due to some mysterious FS issue so
> > > my apologies for not replying sooner]
> > >
> > > ...
> > >
> > > > The checks before and after this patch are not equivalent.  Post-patch,
> > > > one must always have CAP_NET_ADMIN to do the attach, whereas pre-patch
> > > > you only needed those if current_cred() did not own the tun device.  Is
> > > > that intentional?
> > >
> > > Nope, just a goof on my part; I misread the booleans and haven't fully
> > > tested the patch yet so it slipped out, thanks for catching it.  This
> > > brings up a good point, would we rather move the TUN owner/group checks
> > > into the cap_tun_* functions or move the capable() call back into the TUN
> > > driver?  The answer wasn't clear to me when I was looking at the code
> > > before and the uniqueness of the TUN driver doesn't help much in this
> > > regard.
> >
> > I see the question being asked as:  Does this device belong to
> > the caller and, if not, is the caller privileged to act
> > anyway?'  So I think the capable call should be moved back
> > into the tun driver, followed by a separate security_tun_dev_attach()
> > check, since that is a separate, restrictive question.
> 
> Works for me, I'll make the change.
> 
> BTW, the main reason for posting the patches in such an early state was to 
> solicit feedback on the location and types of hooks added; I've read lots of 
> good feedback but nothing regarding the fundamental aspects of the hooks ... 
> any comments before I push out v2?

Oh now that you mention it, yes - I think the security_tun_dev_attach()
should be called again separately after the post_create() hook.

As for more general comments on whether or which tuntap-specific hooks
need to exist, two things.  First, if you have specific requirements
in mind please do share those, otherwise I'm working based on what I
see in Documentation/networking/tuntap.txt and drivers/net/tun.c.  Second,
based on my understanding i think the hooks you have make sense,
but is there any way to relabel a tun socket?  Since they are always
labeled with current_sid(), that seems restrictive...  I see that you
don't want to use sockcreate_sid, but (to use a made-up example not
reflecting reality) a kvm_setup_t task couldn't create a tun sock for
a kvm_run_t task to use, right?

-serge

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Paul Moore]

On Thursday 06 August 2009 11:52:58 am Serge E. Hallyn wrote:
> Quoting Paul Moore (paul.moore@hp.com):
> > BTW, the main reason for posting the patches in such an early state was
> > to solicit feedback on the location and types of hooks added; I've read
> > lots of good feedback but nothing regarding the fundamental aspects of
> > the hooks ... any comments before I push out v2?
>
> Oh now that you mention it, yes - I think the security_tun_dev_attach()
> should be called again separately after the post_create() hook.

Why?  Granted the TUN driver calls tun_attach() in both cases but that doesn't 
necessarily mean the operation from a security point of view is the same.  
Using the SELinux hooks as an example, attaching to an existing TUN device is 
currently treated as a relabel operation; the calling task relabels the 
persistent TUN device to match its own label so that traffic sent over the TUN 
device is labeled using the newly attached calling task's label.  Creating a 
new TUN device is like creating any other object (yes, there are exceptions to 
this but I'm speaking generally here), it inherits the label of the task which 
creates it, performing access control for a relabel operation here just 
doesn't make sense.

Or are you expecting some other form of access control for the attach hook 
which would change this argument?

> As for more general comments on whether or which tuntap-specific hooks
> need to exist, two things.  First, if you have specific requirements
> in mind please do share those, otherwise I'm working based on what I
> see in Documentation/networking/tuntap.txt and drivers/net/tun.c.

Not that haven't already been mentioned.  If something doesn't make sense, let 
me know.

> Second, based on my understanding i think the hooks you have make sense,
> but is there any way to relabel a tun socket?  Since they are always labeled
> with current_sid(), that seems restrictive... 

Not at present, the TUN driver only supports changing the user/group IDs.  I 
am debating adding support to change/view the label of the device/socket 
(TUN{SET,GET}SECCTX perhaps?) but that can happen later and is in no way 
prevented by these patches.  My thinking is that these patches are a 
requirement for us to apply the existing LSM network access controls to 
traffic originating from the TUN driver; depending on how use cases evolve 
with the TUN driver we may want to add additional functionality but this 
should serve as a good base.

> I see that you don't want to use sockcreate_sid, but (to use a made-up
> example not reflecting reality) a kvm_setup_t task couldn't create a tun
> sock for a kvm_run_t task to use, right?

Well, the only time this will really be an issue is when you have one task 
create a new, persistent TUN device and a second task that attaches to the 
existing TUN device and uses it to send traffic.  Sticking with your example, 
if the first task is labeled kvm_setup_t and the second task is labeled 
kvm_run_t then the policy would look something like this:

	# allow kvm_setup_t to create a new TUN device
	allow kvm_setup_t self:tun_socket { create };

	# allow kvm_run_t to use TUN devices created by kvm_setup_t
	allow kvm_run_t kvm_setup_t:tun_socket { relabelfrom };
	allow kvm_run_t self:tun_socket { relabelto };

The policy above has the nice effect of only allowing kvm_run_t to attach to 
existing TUN devices created by kvm_setup_t; it can not create a new TUN 
device or use persistent TUN devices created by other domains.  This should 
also help explain why I think calling the attach() hook after the 
post_create() hook makes little sense given the access controls currently 
proposed.

-- 
paul moore
linux @ hp

Re: [RFC PATCH v1 1/2] lsm: Add hooks to the TUN driver

[Serge E. Hallyn]

Quoting Paul Moore (paul.moore@hp.com):
> On Thursday 06 August 2009 11:52:58 am Serge E. Hallyn wrote:
> > Quoting Paul Moore (paul.moore@hp.com):
> > > BTW, the main reason for posting the patches in such an early state was
> > > to solicit feedback on the location and types of hooks added; I've read
> > > lots of good feedback but nothing regarding the fundamental aspects of
> > > the hooks ... any comments before I push out v2?
> >
> > Oh now that you mention it, yes - I think the security_tun_dev_attach()
> > should be called again separately after the post_create() hook.
> 
> Why?  Granted the TUN driver calls tun_attach() in both cases but that doesn't 
> necessarily mean the operation from a security point of view is the same.  
> Using the SELinux hooks as an example, attaching to an existing TUN device is 
> currently treated as a relabel operation; the calling task relabels the 
> persistent TUN device to match its own label so that traffic sent over the TUN 
> device is labeled using the newly attached calling task's label.  Creating a 
> new TUN device is like creating any other object (yes, there are exceptions to 
> this but I'm speaking generally here), it inherits the label of the task which 
> creates it, performing access control for a relabel operation here just 
> doesn't make sense.
> 
> Or are you expecting some other form of access control for the attach hook 
> which would change this argument?

You're right, since there is no way to create without attaching that doesn't
make sense, regardless of the LSM or policy :)  Nm.

> > As for more general comments on whether or which tuntap-specific hooks
> > need to exist, two things.  First, if you have specific requirements
> > in mind please do share those, otherwise I'm working based on what I
> > see in Documentation/networking/tuntap.txt and drivers/net/tun.c.
> 
> Not that haven't already been mentioned.  If something doesn't make sense, let 
> me know.
> 
> > Second, based on my understanding i think the hooks you have make sense,
> > but is there any way to relabel a tun socket?  Since they are always labeled
> > with current_sid(), that seems restrictive... 
> 
> Not at present, the TUN driver only supports changing the user/group IDs.  I 
> am debating adding support to change/view the label of the device/socket 
> (TUN{SET,GET}SECCTX perhaps?) but that can happen later and is in no way 
> prevented by these patches.  My thinking is that these patches are a 
> requirement for us to apply the existing LSM network access controls to 
> traffic originating from the TUN driver; depending on how use cases evolve 
> with the TUN driver we may want to add additional functionality but this 
> should serve as a good base.
> 
> > I see that you don't want to use sockcreate_sid, but (to use a made-up
> > example not reflecting reality) a kvm_setup_t task couldn't create a tun
> > sock for a kvm_run_t task to use, right?
> 
> Well, the only time this will really be an issue is when you have one task 
> create a new, persistent TUN device and a second task that attaches to the 
> existing TUN device and uses it to send traffic.  Sticking with your example, 
> if the first task is labeled kvm_setup_t and the second task is labeled 
> kvm_run_t then the policy would look something like this:
> 
> 	# allow kvm_setup_t to create a new TUN device
> 	allow kvm_setup_t self:tun_socket { create };
> 
> 	# allow kvm_run_t to use TUN devices created by kvm_setup_t
> 	allow kvm_run_t kvm_setup_t:tun_socket { relabelfrom };
> 	allow kvm_run_t self:tun_socket { relabelto };
> 
> The policy above has the nice effect of only allowing kvm_run_t to attach to 
> existing TUN devices created by kvm_setup_t; it can not create a new TUN 
> device or use persistent TUN devices created by other domains.  This should 
> also help explain why I think calling the attach() hook after the 
> post_create() hook makes little sense given the access controls currently 
> proposed.

And really allowing flexibility in the default label can always be done
without affecting the tun code so never mind.

So I think your hooks make sense as is, given the TUN usage model described
in the docs.

thanks,
-serge

[RFC PATCH v1 2/2] selinux: Support for the new TUN LSM hooks

[Paul Moore]

Add support for the new TUN LSM hooks: security_tun_dev_create(),
security_tun_dev_post_create() and security_tun_dev_attach().  This includes
the addition of a new object class, tun_socket, which represents the socks
associated with TUN devices.  The _tun_dev_create() and _tun_dev_post_create()
hooks are fairly similar to the standard socket functions but _tun_dev_attach()
is a bit special.  The _tun_dev_attach() is unique because it involves a
domain attaching to an existing TUN device and its associated tun_socket
object, an operation which does not exist with standard sockets and most
closely resembles a relabel operation.

This patch also includes a new policy capability, tun_perms, to ensure that
the new access controls do not affect older SELinux policies.

--

NOTE: This relies on some changes to the policy to add the new object class
      and its associated permissions, I will ensure that the policy is sorted
      and merged before pushing this patch upstream.  Also, you will notice
      that the new tun_socket object class simply inherits the base socket
      object class, thoughts?
---

 security/selinux/hooks.c                   |   76 +++++++++++++++++++++++++++-
 security/selinux/include/av_inherit.h      |    1 
 security/selinux/include/av_permissions.h  |   22 ++++++++
 security/selinux/include/class_to_string.h |    1 
 security/selinux/include/flask.h           |    1 
 security/selinux/include/security.h        |    2 +
 security/selinux/selinuxfs.c               |    3 +
 security/selinux/ss/services.c             |    3 +
 8 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 15c2a08..6ba99c2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -13,8 +13,8 @@
  *					   Eric Paris <eparis@redhat.com>
  *  Copyright (C) 2004-2005 Trusted Computer Solutions, Inc.
  *			    <dgoeddel@trustedcs.com>
- *  Copyright (C) 2006, 2007 Hewlett-Packard Development Company, L.P.
- *		Paul Moore <paul.moore@hp.com>
+ *  Copyright (C) 2006, 2007, 2009 Hewlett-Packard Development Company, L.P.
+ *	Paul Moore <paul.moore@hp.com>
  *  Copyright (C) 2007 Hitachi Software Engineering Co., Ltd.
  *		       Yuichi Nakamura <ynakam@hitachisoft.jp>
  *
@@ -4296,6 +4296,75 @@ static void selinux_req_classify_flow(const struct request_sock *req,
 	fl->secid = req->secid;
 }
 
+static int selinux_tun_dev_create(void)
+{
+	u32 sid;
+	int err;
+
+	err = cap_tun_dev_create();
+	if (err)
+		return err;
+
+	if (!selinux_policycap_tunperm)
+		return 0;
+
+	/* we aren't taking into account the "sockcreate" SID since the socket
+	 * that is being created here is not a socket in the traditional sense,
+	 * instead it is a private sock, accessible only to the kernel, and
+	 * representing a wide range of network traffic spanning multiple
+	 * connections unlike traditional sockets - check the TUN driver to
+	 * get a better understand of why this socket is special */
+
+	sid = current_sid();
+	return avc_has_perm(sid, sid, SECCLASS_TUN_SOCKET, TUN_SOCKET__CREATE,
+			    NULL);
+}
+
+static void selinux_tun_dev_post_create(struct sock *tun_sk)
+{
+	struct sk_security_struct *sksec = tun_sk->sk_security;
+
+	/* see the comments in _tun_dev_create() about why we don't use the
+	 * sockcreate SID here */
+
+	/* we don't currently perform any NetLabel based labeling here and it
+	 * isn't clear that we would want to do so anyway: while we could apply
+	 * labeling without the support of the TUN user the resulting labeled
+	 * traffic from the other end of the connection would almost certainly
+	 * cause confusion to the TUN user that had no idea network labeling
+	 * protocols were being used */
+
+	sksec->sid = current_sid();
+	sksec->sclass = SECCLASS_TUN_SOCKET;
+}
+
+static int selinux_tun_dev_attach(struct sock *tun_sk)
+{
+	struct sk_security_struct *tun_sksec = tun_sk->sk_security;
+	u32 sid;
+	int err;
+
+	err = cap_tun_dev_attach();
+	if (err)
+		return err;
+
+	if (!selinux_policycap_tunperm)
+		return 0;
+
+	sid = current_sid();
+	err = avc_has_perm(sid, tun_sksec->sid, SECCLASS_TUN_SOCKET,
+			   TUN_SOCKET__RELABELFROM, NULL);
+	if (err)
+		return err;
+	err = avc_has_perm(sid, sid, SECCLASS_RAWIP_SOCKET,
+			   TUN_SOCKET__RELABELTO, NULL);
+	if (err)
+		return err;
+	tun_sksec->sid = sid;
+
+	return 0;
+}
+
 static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 {
 	int err = 0;
@@ -5464,6 +5533,9 @@ static struct security_operations selinux_ops = {
 	.inet_csk_clone =		selinux_inet_csk_clone,
 	.inet_conn_established =	selinux_inet_conn_established,
 	.req_classify_flow =		selinux_req_classify_flow,
+	.tun_dev_create =		selinux_tun_dev_create,
+	.tun_dev_post_create = 		selinux_tun_dev_post_create,
+	.tun_dev_attach =		selinux_tun_dev_attach,
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	.xfrm_policy_alloc_security =	selinux_xfrm_policy_alloc,
diff --git a/security/selinux/include/av_inherit.h b/security/selinux/include/av_inherit.h
index 8377a4b..abedcd7 100644
--- a/security/selinux/include/av_inherit.h
+++ b/security/selinux/include/av_inherit.h
@@ -15,6 +15,7 @@
    S_(SECCLASS_KEY_SOCKET, socket, 0x00400000UL)
    S_(SECCLASS_UNIX_STREAM_SOCKET, socket, 0x00400000UL)
    S_(SECCLASS_UNIX_DGRAM_SOCKET, socket, 0x00400000UL)
+   S_(SECCLASS_TUN_SOCKET, socket, 0x00400000UL)
    S_(SECCLASS_IPC, ipc, 0x00000200UL)
    S_(SECCLASS_SEM, ipc, 0x00000200UL)
    S_(SECCLASS_MSGQ, ipc, 0x00000200UL)
diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
index d645192..0b41ad5 100644
--- a/security/selinux/include/av_permissions.h
+++ b/security/selinux/include/av_permissions.h
@@ -423,6 +423,28 @@
 #define UNIX_DGRAM_SOCKET__RECV_MSG               0x00080000UL
 #define UNIX_DGRAM_SOCKET__SEND_MSG               0x00100000UL
 #define UNIX_DGRAM_SOCKET__NAME_BIND              0x00200000UL
+#define TUN_SOCKET__IOCTL                         0x00000001UL
+#define TUN_SOCKET__READ                          0x00000002UL
+#define TUN_SOCKET__WRITE                         0x00000004UL
+#define TUN_SOCKET__CREATE                        0x00000008UL
+#define TUN_SOCKET__GETATTR                       0x00000010UL
+#define TUN_SOCKET__SETATTR                       0x00000020UL
+#define TUN_SOCKET__LOCK                          0x00000040UL
+#define TUN_SOCKET__RELABELFROM                   0x00000080UL
+#define TUN_SOCKET__RELABELTO                     0x00000100UL
+#define TUN_SOCKET__APPEND                        0x00000200UL
+#define TUN_SOCKET__BIND                          0x00000400UL
+#define TUN_SOCKET__CONNECT                       0x00000800UL
+#define TUN_SOCKET__LISTEN                        0x00001000UL
+#define TUN_SOCKET__ACCEPT                        0x00002000UL
+#define TUN_SOCKET__GETOPT                        0x00004000UL
+#define TUN_SOCKET__SETOPT                        0x00008000UL
+#define TUN_SOCKET__SHUTDOWN                      0x00010000UL
+#define TUN_SOCKET__RECVFROM                      0x00020000UL
+#define TUN_SOCKET__SENDTO                        0x00040000UL
+#define TUN_SOCKET__RECV_MSG                      0x00080000UL
+#define TUN_SOCKET__SEND_MSG                      0x00100000UL
+#define TUN_SOCKET__NAME_BIND                     0x00200000UL
 #define PROCESS__FORK                             0x00000001UL
 #define PROCESS__TRANSITION                       0x00000002UL
 #define PROCESS__SIGCHLD                          0x00000004UL
diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
index 21ec786..7ab9299 100644
--- a/security/selinux/include/class_to_string.h
+++ b/security/selinux/include/class_to_string.h
@@ -77,3 +77,4 @@
     S_(NULL)
     S_(NULL)
     S_("kernel_service")
+    S_("tun_socket")
diff --git a/security/selinux/include/flask.h b/security/selinux/include/flask.h
index 882f27d..f248500 100644
--- a/security/selinux/include/flask.h
+++ b/security/selinux/include/flask.h
@@ -53,6 +53,7 @@
 #define SECCLASS_PEER                                    68
 #define SECCLASS_CAPABILITY2                             69
 #define SECCLASS_KERNEL_SERVICE                          74
+#define SECCLASS_TUN_SOCKET                              75
 
 /*
  * Security identifier indices for initial entities
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index ca83579..188af8d 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -63,12 +63,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_TUNPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_tunperm;
 
 /*
  * type_datum properties
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index b4fc506..770e059 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"tun_perms"
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 500e6f7..adbe6d5 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_tunperm;
 
 /*
  * This is declared in avc.c
@@ -1593,6 +1594,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_tunperm = ebitmap_get_bit(&policydb.policycaps,
+						   POLICYDB_CAPABILITY_TUNPERM);
 }
 
 extern void selinux_complete_init(void);

Re: [RFC PATCH v1 2/2] selinux: Support for the new TUN LSM hooks

[Eric Paris]

On Tue, Aug 4, 2009 at 5:22 PM, Paul Moore<paul.moore@hp.com> wrote:
> Add support for the new TUN LSM hooks: security_tun_dev_create(),
> security_tun_dev_post_create() and security_tun_dev_attach().  This includes
> the addition of a new object class, tun_socket, which represents the socks
> associated with TUN devices.  The _tun_dev_create() and _tun_dev_post_create()
> hooks are fairly similar to the standard socket functions but _tun_dev_attach()
> is a bit special.  The _tun_dev_attach() is unique because it involves a
> domain attaching to an existing TUN device and its associated tun_socket
> object, an operation which does not exist with standard sockets and most
> closely resembles a relabel operation.
>
> This patch also includes a new policy capability, tun_perms, to ensure that
> the new access controls do not affect older SELinux policies.

I think we finally have the first patch where the 'handle_unknown'
stuff fits better than the policy capabilities work!  First time for
everything!  I'd suggest dropping the policy capability all together
and the checks will be applied when the class and perm is defined in
the loaded policy.  If the class+perm isn't defined in policy the
policy handle_unknown setting will define the result of the security
check.

whoo hoo, saves 32bits of memory and 2 branches on low use operations!

-Eric

Re: [RFC PATCH v1 0/2] The Long Lost TUN LSM Hooks

[James Morris]

On Tue, 4 Aug 2009, Paul Moore wrote:

> While drivers in general aren't good places for LSM hooks the TUN driver is
> a bit different because of how it handles sockets and network traffic.  The
> problem lies in the fact that the TUN driver creates a sock structure to use
> when sending network traffic but the sock is never put through the same LSM
> setup/control as other sock structures on the system which makes enforcing
> security on TUN generated traffic difficult for some LSMs.  This patch set
> adds three new LSM hooks, all specific to the TUN driver (none of the existing
> hooks made sense, trust me we tried), to control and monitor the creating and
> attachment of TUN devices.

Looks ok to me in principle.

For netdev reviewers: we're lacking proper LSM control over tun devices 
because they're not created like normal sockets, and subsequently don't 
get labeled appropriately.  They also behave differently to normal sockets 
and need special handling for security per Paul's notes.


- James
-- 
James Morris
<jmorris@namei.org>