[PATCH v2 0/3] Make /proc per net namespace objects belong to container

[PATCH v2 0/3] Make /proc per net namespace objects belong to container

[Dmitry Torokhov]

Currently [almost] all /proc objects belong to the global root, even if
data belongs to a given namespace within a container and (at least for
sysctls) we work around permssions checks to allow container's root to
access the data.

This series changes ownership of net namespace /proc objects
(/proc/net/self/* and /proc/sys/net/*) to be container's root and not
global root when there exists mapping for container's root in user
namespace.

This helps when running Android CTS in a container, but I think it makes
sense regardless.

Changes from V1:

- added fix for crash when !CONFIG_NET_NS (new patch #1)
- addressed Eric'c comments for error handling style in patch #3 and
  added his Ack
- adjusted patch #2 to use the same style of erro handling
- sent out as series instead of separate patches

Dmitry Torokhov (3):
  netns: do not call pernet ops for not yet set up init_net namespace
  proc: make proc entries inherit ownership from parent
  net: make net namespace sysctls belong to container's owner

 fs/proc/generic.c        |  2 ++
 fs/proc/proc_net.c       | 13 +++++++++++++
 fs/proc/proc_sysctl.c    |  5 +++++
 include/linux/sysctl.h   |  4 ++++
 net/core/net_namespace.c | 21 +++++++++++++++++----
 net/sysctl_net.c         | 29 ++++++++++++++++++++---------
 6 files changed, 61 insertions(+), 13 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 1/3] netns: do not call pernet ops for not yet set up init_net namespace

[Dmitry Torokhov]

When CONFIG_NET_NS is disabled, registering pernet operations causes
init() to be called immediately with init_net as an argument. Unfortunately
this leads to some pernet ops, such as proc_net_ns_init() to be called too
early, when init_net namespace has not been fully initialized. This causes
issues when we want to change pernet ops to use more data from the net
namespace in question, for example reference user namespace that owns our
network namespace.

To fix this we could either play game of musical chairs and rearrange init
order, or we could do the same as when CONFIG_NET_NS is enabled, and
postpone calling pernet ops->init() until namespace is set up properly.

Note that we can not simply undo commit ed160e839d2e ("[NET]: Cleanup
pernet operation without CONFIG_NET_NS") and use the same implementations
for __register_pernet_operations() and __unregister_pernet_operations(),
because many pernet ops are marked as __net_initdata and will be discarded,
which wreaks havoc on our ops lists. Here we rely on the fact that we only
use lists until init_net is fully initialized, which happens much earlier
than discarding __net_initdata sections.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 net/core/net_namespace.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 2c2eb1b..1fe5816 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -37,6 +37,8 @@ struct net init_net = {
 };
 EXPORT_SYMBOL(init_net);
 
+static bool init_net_initialized;
+
 #define INITIAL_NET_GEN_PTRS	13 /* +1 for len +2 for rcu_head */
 
 static unsigned int max_gen_ptrs = INITIAL_NET_GEN_PTRS;
@@ -750,6 +752,8 @@ static int __init net_ns_init(void)
 	if (setup_net(&init_net, &init_user_ns))
 		panic("Could not setup the initial network namespace");
 
+	init_net_initialized = true;
+
 	rtnl_lock();
 	list_add_tail_rcu(&init_net.list, &net_namespace_list);
 	rtnl_unlock();
@@ -811,15 +815,24 @@ static void __unregister_pernet_operations(struct pernet_operations *ops)
 static int __register_pernet_operations(struct list_head *list,
 					struct pernet_operations *ops)
 {
+	if (!init_net_initialized) {
+		list_add_tail(&ops->list, list);
+		return 0;
+	}
+
 	return ops_init(ops, &init_net);
 }
 
 static void __unregister_pernet_operations(struct pernet_operations *ops)
 {
-	LIST_HEAD(net_exit_list);
-	list_add(&init_net.exit_list, &net_exit_list);
-	ops_exit_list(ops, &net_exit_list);
-	ops_free_list(ops, &net_exit_list);
+	if (!init_net_initialized) {
+		list_del(&ops->list);
+	} else {
+		LIST_HEAD(net_exit_list);
+		list_add(&init_net.exit_list, &net_exit_list);
+		ops_exit_list(ops, &net_exit_list);
+		ops_free_list(ops, &net_exit_list);
+	}
 }
 
 #endif /* CONFIG_NET_NS */
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 2/3] proc: make proc entries inherit ownership from parent

[Dmitry Torokhov]

There are certain parameters that belong to net namespace and that are
exported in /proc. They should be controllable by the container's owner,
but are currently owned by global root and thus not available.

Let's change proc code to inherit ownership of parent entry, and when
create per-ns "net" proc entry set it up as owned by container's owner.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 fs/proc/generic.c  |  2 ++
 fs/proc/proc_net.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index c633476..bca66d8 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -390,6 +390,8 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
 	atomic_set(&ent->count, 1);
 	spin_lock_init(&ent->pde_unload_lock);
 	INIT_LIST_HEAD(&ent->pde_openers);
+	proc_set_user(ent, (*parent)->uid, (*parent)->gid);
+
 out:
 	return ent;
 }
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index c8bbc68..7ae6b1d 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -21,6 +21,7 @@
 #include <linux/bitops.h>
 #include <linux/mount.h>
 #include <linux/nsproxy.h>
+#include <linux/uidgid.h>
 #include <net/net_namespace.h>
 #include <linux/seq_file.h>
 
@@ -185,6 +186,8 @@ const struct file_operations proc_net_operations = {
 static __net_init int proc_net_ns_init(struct net *net)
 {
 	struct proc_dir_entry *netd, *net_statd;
+	kuid_t uid;
+	kgid_t gid;
 	int err;
 
 	err = -ENOMEM;
@@ -199,6 +202,16 @@ static __net_init int proc_net_ns_init(struct net *net)
 	netd->parent = &proc_root;
 	memcpy(netd->name, "net", 4);
 
+	uid = make_kuid(net->user_ns, 0);
+	if (!uid_valid(uid))
+		uid = netd->uid;
+
+	gid = make_kgid(net->user_ns, 0);
+	if (!gid_valid(gid))
+		gid = netd->gid;
+
+	proc_set_user(netd, uid, gid);
+
 	err = -EEXIST;
 	net_statd = proc_net_mkdir(net, "stat", netd);
 	if (!net_statd)
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

[Dmitry Torokhov]

If net namespace is attached to a user namespace let's make container's
root owner of sysctls affecting said network namespace instead of global
root.

This also allows us to clean up net_ctl_permissions() because we do not
need to fudge permissions anymore for the container's owner since it now
owns the objects in question.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 fs/proc/proc_sysctl.c  |  5 +++++
 include/linux/sysctl.h |  4 ++++
 net/sysctl_net.c       | 29 ++++++++++++++++++++---------
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5e57c3e..28f9085 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -430,6 +430,7 @@ static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, i
 static struct inode *proc_sys_make_inode(struct super_block *sb,
 		struct ctl_table_header *head, struct ctl_table *table)
 {
+	struct ctl_table_root *root = head->root;
 	struct inode *inode;
 	struct proc_inode *ei;
 
@@ -457,6 +458,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 		if (is_empty_dir(head))
 			make_empty_dir_inode(inode);
 	}
+
+	if (root->set_ownership)
+		root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
+
 out:
 	return inode;
 }
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index fa7bc29..55bec2f 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -25,6 +25,7 @@
 #include <linux/rcupdate.h>
 #include <linux/wait.h>
 #include <linux/rbtree.h>
+#include <linux/uidgid.h>
 #include <uapi/linux/sysctl.h>
 
 /* For the /proc/sys support */
@@ -156,6 +157,9 @@ struct ctl_table_root {
 	struct ctl_table_set default_set;
 	struct ctl_table_set *(*lookup)(struct ctl_table_root *root,
 					   struct nsproxy *namespaces);
+	void (*set_ownership)(struct ctl_table_header *head,
+			      struct ctl_table *table,
+			      kuid_t *uid, kgid_t *gid);
 	int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
 };
 
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index ed98c1f..5bc1a3d 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -42,26 +42,37 @@ static int net_ctl_permissions(struct ctl_table_header *head,
 			       struct ctl_table *table)
 {
 	struct net *net = container_of(head->set, struct net, sysctls);
-	kuid_t root_uid = make_kuid(net->user_ns, 0);
-	kgid_t root_gid = make_kgid(net->user_ns, 0);
 
 	/* Allow network administrator to have same access as root. */
-	if (ns_capable(net->user_ns, CAP_NET_ADMIN) ||
-	    uid_eq(root_uid, current_euid())) {
+	if (ns_capable(net->user_ns, CAP_NET_ADMIN)) {
 		int mode = (table->mode >> 6) & 7;
 		return (mode << 6) | (mode << 3) | mode;
 	}
-	/* Allow netns root group to have the same access as the root group */
-	if (in_egroup_p(root_gid)) {
-		int mode = (table->mode >> 3) & 7;
-		return (mode << 3) | mode;
-	}
+
 	return table->mode;
 }
 
+static void net_ctl_set_ownership(struct ctl_table_header *head,
+				  struct ctl_table *table,
+				  kuid_t *uid, kgid_t *gid)
+{
+	struct net *net = container_of(head->set, struct net, sysctls);
+	kuid_t ns_root_uid;
+	kgid_t ns_root_gid;
+
+	ns_root_uid = make_kuid(net->user_ns, 0);
+	if (uid_valid(ns_root_uid))
+		*uid = ns_root_uid;
+
+	ns_root_gid = make_kgid(net->user_ns, 0);
+	if (gid_valid(ns_root_gid))
+		*gid = ns_root_gid;
+}
+
 static struct ctl_table_root net_sysctl_root = {
 	.lookup = net_ctl_header_lookup,
 	.permissions = net_ctl_permissions,
+	.set_ownership = net_ctl_set_ownership,
 };
 
 static int __net_init sysctl_net_init(struct net *net)
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2 0/3] Make /proc per net namespace objects belong to container

[David Miller]

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Wed, 10 Aug 2016 14:35:59 -0700

> Currently [almost] all /proc objects belong to the global root, even if
> data belongs to a given namespace within a container and (at least for
> sysctls) we work around permssions checks to allow container's root to
> access the data.
> 
> This series changes ownership of net namespace /proc objects
> (/proc/net/self/* and /proc/sys/net/*) to be container's root and not
> global root when there exists mapping for container's root in user
> namespace.
> 
> This helps when running Android CTS in a container, but I think it makes
> sense regardless.
> 
> Changes from V1:
> 
> - added fix for crash when !CONFIG_NET_NS (new patch #1)
> - addressed Eric'c comments for error handling style in patch #3 and
>   added his Ack
> - adjusted patch #2 to use the same style of erro handling
> - sent out as series instead of separate patches

Series applied to net-next, thanks.

Re: [PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

[Dmitry Torokhov]

Hi David,

On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> If net namespace is attached to a user namespace let's make container's
> root owner of sysctls affecting said network namespace instead of global
> root.
>
> This also allows us to clean up net_ctl_permissions() because we do not
> need to fudge permissions anymore for the container's owner since it now
> owns the objects in question.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I was looking at linux-next today, and I noticed that, when you merged
my patch, you basically reverted the following commit:

commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
Author: Tyler Hicks <tyhicks@canonical.com>
Date:   Thu Jun 2 23:43:22 2016 -0500

    net: Use ns_capable_noaudit() when determining net sysctl permissions

    The capability check should not be audited since it is only being used
    to determine the inode permissions. A failed check does not indicate a
    violation of security policy but, when an LSM is enabled, a denial audit
    message was being generated.

    The denial audit message caused confusion for some application authors
    because root-running Go applications always triggered the denial. To
    prevent this confusion, the capability check in net_ctl_permissions() is
    switched to the noaudit variant.

    BugLink: https://launchpad.net/bugs/1465724

    Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
    Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>
    Signed-off-by: James Morris <james.l.morris@oracle.com>

Thanks!

> ---
>  fs/proc/proc_sysctl.c  |  5 +++++
>  include/linux/sysctl.h |  4 ++++
>  net/sysctl_net.c       | 29 ++++++++++++++++++++---------
>  3 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 5e57c3e..28f9085 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -430,6 +430,7 @@ static int sysctl_perm(struct ctl_table_header *head, struct ctl_table *table, i
>  static struct inode *proc_sys_make_inode(struct super_block *sb,
>                 struct ctl_table_header *head, struct ctl_table *table)
>  {
> +       struct ctl_table_root *root = head->root;
>         struct inode *inode;
>         struct proc_inode *ei;
>
> @@ -457,6 +458,10 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
>                 if (is_empty_dir(head))
>                         make_empty_dir_inode(inode);
>         }
> +
> +       if (root->set_ownership)
> +               root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> +
>  out:
>         return inode;
>  }
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index fa7bc29..55bec2f 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -25,6 +25,7 @@
>  #include <linux/rcupdate.h>
>  #include <linux/wait.h>
>  #include <linux/rbtree.h>
> +#include <linux/uidgid.h>
>  #include <uapi/linux/sysctl.h>
>
>  /* For the /proc/sys support */
> @@ -156,6 +157,9 @@ struct ctl_table_root {
>         struct ctl_table_set default_set;
>         struct ctl_table_set *(*lookup)(struct ctl_table_root *root,
>                                            struct nsproxy *namespaces);
> +       void (*set_ownership)(struct ctl_table_header *head,
> +                             struct ctl_table *table,
> +                             kuid_t *uid, kgid_t *gid);
>         int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
>  };
>
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index ed98c1f..5bc1a3d 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -42,26 +42,37 @@ static int net_ctl_permissions(struct ctl_table_header *head,
>                                struct ctl_table *table)
>  {
>         struct net *net = container_of(head->set, struct net, sysctls);
> -       kuid_t root_uid = make_kuid(net->user_ns, 0);
> -       kgid_t root_gid = make_kgid(net->user_ns, 0);
>
>         /* Allow network administrator to have same access as root. */
> -       if (ns_capable(net->user_ns, CAP_NET_ADMIN) ||
> -           uid_eq(root_uid, current_euid())) {
> +       if (ns_capable(net->user_ns, CAP_NET_ADMIN)) {
>                 int mode = (table->mode >> 6) & 7;
>                 return (mode << 6) | (mode << 3) | mode;
>         }
> -       /* Allow netns root group to have the same access as the root group */
> -       if (in_egroup_p(root_gid)) {
> -               int mode = (table->mode >> 3) & 7;
> -               return (mode << 3) | mode;
> -       }
> +
>         return table->mode;
>  }
>
> +static void net_ctl_set_ownership(struct ctl_table_header *head,
> +                                 struct ctl_table *table,
> +                                 kuid_t *uid, kgid_t *gid)
> +{
> +       struct net *net = container_of(head->set, struct net, sysctls);
> +       kuid_t ns_root_uid;
> +       kgid_t ns_root_gid;
> +
> +       ns_root_uid = make_kuid(net->user_ns, 0);
> +       if (uid_valid(ns_root_uid))
> +               *uid = ns_root_uid;
> +
> +       ns_root_gid = make_kgid(net->user_ns, 0);
> +       if (gid_valid(ns_root_gid))
> +               *gid = ns_root_gid;
> +}
> +
>  static struct ctl_table_root net_sysctl_root = {
>         .lookup = net_ctl_header_lookup,
>         .permissions = net_ctl_permissions,
> +       .set_ownership = net_ctl_set_ownership,
>  };
>
>  static int __net_init sysctl_net_init(struct net *net)
> --
> 2.8.0.rc3.226.g39d4020
>

-- 
Dmitry

Re: [PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

[David Miller]

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: Thu, 29 Sep 2016 08:46:05 -0700

> Hi David,
> 
> On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> If net namespace is attached to a user namespace let's make container's
>> root owner of sysctls affecting said network namespace instead of global
>> root.
>>
>> This also allows us to clean up net_ctl_permissions() because we do not
>> need to fudge permissions anymore for the container's owner since it now
>> owns the objects in question.
>>
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> I was looking at linux-next today, and I noticed that, when you merged
> my patch, you basically reverted the following commit:
> 
> commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
> Author: Tyler Hicks <tyhicks@canonical.com>
> Date:   Thu Jun 2 23:43:22 2016 -0500
> 
>     net: Use ns_capable_noaudit() when determining net sysctl permissions

Please send me a fixup patch for this, sorry.

Re: [PATCH v2 3/3] net: make net namespace sysctls belong to container's owner

[Dmitry Torokhov]

On Fri, Sep 30, 2016 at 01:21:27AM -0400, David Miller wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date: Thu, 29 Sep 2016 08:46:05 -0700
> 
> > Hi David,
> > 
> > On Wed, Aug 10, 2016 at 2:36 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> If net namespace is attached to a user namespace let's make container's
> >> root owner of sysctls affecting said network namespace instead of global
> >> root.
> >>
> >> This also allows us to clean up net_ctl_permissions() because we do not
> >> need to fudge permissions anymore for the container's owner since it now
> >> owns the objects in question.
> >>
> >> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > I was looking at linux-next today, and I noticed that, when you merged
> > my patch, you basically reverted the following commit:
> > 
> > commit d6e0d306449bcb5fa3c80e7a3edf11d45abf9ae9
> > Author: Tyler Hicks <tyhicks@canonical.com>
> > Date:   Thu Jun 2 23:43:22 2016 -0500
> > 
> >     net: Use ns_capable_noaudit() when determining net sysctl permissions
> 
> Please send me a fixup patch for this, sorry.

Just did, look for <20160930222431.GA30208@dtor-ws>

Thanks.

-- 
Dmitry