userns: Better restrictions on when proc and sysfs can be mounted

Don't allow mounting sysfs unless the caller has CAP_SYS_ADMIN rights over the net namespace. The principle here is if you create or have capabilities over it you can mount it, otherwise you get to live with what other people have mounted.

Instead of testing this with a straight forward ns_capable call, perform this check the long and torturous way with kobject helpers, this keeps direct knowledge of namespaces out of sysfs, and preserves the existing sysfs abstractions.

Signed-off-by "Eric W. Biederman" <[email protected]>

Don't allow mounting sysfs unless the caller has CAP_SYS_ADMIN rights over the net namespace. The principle here is if you create or have capabilities over it you can mount it, otherwise you get to live with what other people have mounted.

Instead of testing this with a straight forward ns_capable call, perform this check the long and torturous way with kobject helpers, this keeps direct knowledge of namespaces out of sysfs, and preserves the existing sysfs abstractions.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/sysfs/mount.c           |   12 +++++++++---
 include/linux/kobject_ns.h |    2 ++
 lib/kobject.c              |   12 ++++++++++++
 net/core/net-sysfs.c       |    8 ++++++++
 4 files changed, 31 insertions(+), 3 deletions(-)

fs/sysfs/mount.c +9 -3

index 4a2da3a..8c69ef4 100644
@@ -112,9 +112,15 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
 	struct super_block *sb;
 	int error;
 
-	if (!(flags & MS_KERNMOUNT) && !capable(CAP_SYS_ADMIN) &&
-	    !fs_fully_visible(fs_type))
-		return ERR_PTR(-EPERM);
+	if (!(flags & MS_KERNMOUNT)) {
+		if (!capable(CAP_SYS_ADMIN) && !fs_fully_visible(fs_type))
+			return ERR_PTR(-EPERM);
+
+		for (type = KOBJ_NS_TYPE_NONE; type < KOBJ_NS_TYPES; type++) {
+			if (!kobj_ns_current_may_mount(type))
+				return ERR_PTR(-EPERM);
+		}
+	}
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)

include/linux/kobject_ns.h +2 -0

index f66b065..df32d25 100644
@@ -39,6 +39,7 @@ enum kobj_ns_type {
  */
 struct kobj_ns_type_operations {
 	enum kobj_ns_type type;
+	bool (*current_may_mount)(void);
 	void *(*grab_current_ns)(void);
 	const void *(*netlink_ns)(struct sock *sk);
 	const void *(*initial_ns)(void);

@@ -50,6 +51,7 @@ int kobj_ns_type_registered(enum kobj_ns_type type);
 const struct kobj_ns_type_operations *kobj_child_ns_ops(struct kobject *parent);
 const struct kobj_ns_type_operations *kobj_ns_ops(struct kobject *kobj);
 
+bool kobj_ns_current_may_mount(enum kobj_ns_type type);
 void *kobj_ns_grab_current(enum kobj_ns_type type);
 const void *kobj_ns_netlink(enum kobj_ns_type type, struct sock *sk);
 const void *kobj_ns_initial(enum kobj_ns_type type);

lib/kobject.c +12 -0

index 4a1f33d..1853bd9 100644
@@ -915,6 +915,18 @@ const struct kobj_ns_type_operations *kobj_ns_ops(struct kobject *kobj)
 	return kobj_child_ns_ops(kobj->parent);
 }
 
+bool kobj_ns_current_may_mount(enum kobj_ns_type type)
+{
+	bool may_mount = false;
+
+	spin_lock(&kobj_ns_type_lock);
+	if ((type > KOBJ_NS_TYPE_NONE) && (type < KOBJ_NS_TYPES) &&
+	    kobj_ns_ops_tbl[type])
+		may_mount = kobj_ns_ops_tbl[type]->current_may_mount();
+	spin_unlock(&kobj_ns_type_lock);
+
+	return may_mount;
+}
 
 void *kobj_ns_grab_current(enum kobj_ns_type type)
 {

net/core/net-sysfs.c +7 -0

index 981fed3..0b0ff95 100644
@@ -1157,6 +1157,13 @@ static void remove_queue_kobjects(struct net_device *net)
 #endif
 }
 
+static bool net_current_may_mount(void)
+{
+	struct net *net = current->nsproxy->net_ns;
+
+	return !ns_capable(net->user_ns, CAP_SYS_ADMIN);
+}
+
 static void *net_grab_current_ns(void)
 {
 	struct net *ns = current->nsproxy->net_ns;

In kobj_ns_current_may_mount the default should be to allow the mount. The test is only for a single kobj_ns_type at a time, and unless there is a reason to prevent it the mounting sysfs should be allowed. Subsystems that are not registered can't have are not involved so can't have a reason to prevent mounting sysfs.

This is a bug-fix to:
commit 7dc5dbc879bd0779924b5132a48b731a0bc04a1e
Author: Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>
Date: Mon Mar 25 20:07:01 2013 -0700

sysfs: Restrict mounting sysfs

Don't allow mounting sysfs unless the caller has CAP_SYS_ADMIN rights over the net namespace. The principle here is if you create or have capabilities over it you can mount it, otherwise you get to live with what other people have mounted.

Instead of testing this with a straight forward ns_capable call, perform this check the long and torturous way with kobject helpers, this keeps direct knowledge of namespaces out of sysfs, and preserves the existing sysfs abstractions.

Acked-by Greg Kroah-Hartman <[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

That came in via the userns tree during the 3.12 merge window.

Reported-by James Hogan <[email protected]>

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Quoted 3 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Possibly dumb question: does this check whether the pre-existing mount has hidepid set?

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Quoted 2 lines

Possibly dumb question: does this check whether the pre-existing mount has hidepid set?

Not currently.

It may be worth doing something with respect to hidepid. I forget what hidepid tries to do, and I need to dash. But feel free to cook up a follow on patch.

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Quoted 2 lines

Possibly dumb question: does this check whether the pre-existing mount has hidepid set?

Not currently.

It may be worth doing something with respect to hidepid. I forget what hidepid tries to do, and I need to dash. But feel free to cook up a follow on patch.

Quoted 8 lines · ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected] (Eric W. Biederman) writes:

Quoted 6 lines · Andy Lutomirski

Quoted 3 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Quoted 8 lines

Quoted 2 lines

Possibly dumb question: does this check whether the pre-existing mount has hidepid set?

Not currently.

It may be worth doing something with respect to hidepid. I forget what hidepid tries to do, and I need to dash. But feel free to cook up a follow on patch.

So I have thought about this a bit more.

hidepid hides the processes that ptrace_may_access will fail on.

You can only reach the point where an unprivileged mount of a pid namespace is possible if you have created both a user namespace and a pid namespace. Which means the creator of the pid namespace will be capable of ptracing all of the other processes in the pid namespace (ignoring setns).

So I don't see a point of worry about hidepid or the hidepid gid on child pid namespaces. The cases it is attempting to protecting against really don't exist.

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namespace.c                 |   37 +++++++++++++++++++++++++------------
 fs/proc/root.c                 |    7 +++++--
 fs/sysfs/mount.c               |    3 ++-
 include/linux/fs.h             |    1 +
 include/linux/user_namespace.h |    4 ----
 kernel/user.c                  |    2 --
 kernel/user_namespace.c        |    2 --
 7 files changed, 33 insertions(+), 23 deletions(-)

fs/namespace.c +9 -9

index 64627f8..877e427 100644
@@ -2867,25 +2867,38 @@ bool current_chrooted(void)
 	return chrooted;
 }
 
-void update_mnt_policy(struct user_namespace *userns)
+bool fs_fully_visible(struct file_system_type *type)
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mount *mnt;
+	bool visible = false;
 
-	down_read(&namespace_sem);
+	if (unlikely(!ns))
+		return false;
+
+	namespace_lock();
 	list_for_each_entry(mnt, &ns->list, mnt_list) {
-		switch (mnt->mnt.mnt_sb->s_magic) {
-		case SYSFS_MAGIC:
-			userns->may_mount_sysfs = true;
-			break;
-		case PROC_SUPER_MAGIC:
-			userns->may_mount_proc = true;
-			break;
+		struct mount *child;
+		if (mnt->mnt.mnt_sb->s_type != type)
+			continue;

fs/namespace.c +6 -3

 		}
-		if (userns->may_mount_sysfs && userns->may_mount_proc)
-			break;
+		visible = true;
+		goto found;
+	next:	;
 	}
-	up_read(&namespace_sem);
+found:
+	namespace_unlock();
+	return visible;
 }
 
 static void *mntns_get(struct task_struct *task)

fs/proc/root.c +5 -2

index 38bd5d4..45e5fb7 100644
@@ -110,8 +110,11 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
 		ns = task_active_pid_ns(current);
 		options = data;
 
-		if (!current_user_ns()->may_mount_proc ||
-		    !ns_capable(ns->user_ns, CAP_SYS_ADMIN))
+		if (!capable(CAP_SYS_ADMIN) && !fs_fully_visible(fs_type))
+			return ERR_PTR(-EPERM);
+
+		/* Does the mounter have privilege over the pid namespace? */
+		if (!ns_capable(ns->user_ns, CAP_SYS_ADMIN))
 			return ERR_PTR(-EPERM);
 	}
 

fs/sysfs/mount.c +2 -1

index afd8327..4a2da3a 100644
@@ -112,7 +112,8 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
 	struct super_block *sb;
 	int error;
 
-	if (!(flags & MS_KERNMOUNT) && !current_user_ns()->may_mount_sysfs)
+	if (!(flags & MS_KERNMOUNT) && !capable(CAP_SYS_ADMIN) &&
+	    !fs_fully_visible(fs_type))
 		return ERR_PTR(-EPERM);
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);

include/linux/fs.h +1 -0

index 9818747..3050c62 100644
@@ -1897,6 +1897,7 @@ extern int vfs_ustat(dev_t, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
 extern bool our_mnt(struct vfsmount *mnt);
+extern bool fs_fully_visible(struct file_system_type *);
 
 extern int current_umask(void);
 

include/linux/user_namespace.h +0 -4

index b6b215f..4ce0093 100644
@@ -26,8 +26,6 @@ struct user_namespace {
 	kuid_t			owner;
 	kgid_t			group;
 	unsigned int		proc_inum;
-	bool			may_mount_sysfs;
-	bool			may_mount_proc;
 };
 
 extern struct user_namespace init_user_ns;

@@ -84,6 +82,4 @@ static inline void put_user_ns(struct user_namespace *ns)
 
 #endif
 
-void update_mnt_policy(struct user_namespace *userns);
-
 #endif /* _LINUX_USER_H */

kernel/user.c +0 -2

index 69b4c3d..5bbb919 100644
@@ -51,8 +51,6 @@ struct user_namespace init_user_ns = {
 	.owner = GLOBAL_ROOT_UID,
 	.group = GLOBAL_ROOT_GID,
 	.proc_inum = PROC_USER_INIT_INO,
-	.may_mount_sysfs = true,
-	.may_mount_proc = true,
 };
 EXPORT_SYMBOL_GPL(init_user_ns);
 

kernel/user_namespace.c +0 -2

index d8c30db..d58ad1e 100644
@@ -97,8 +97,6 @@ int create_user_ns(struct cred *new)
 
 	set_cred_user_ns(new, ns);
 
-	update_mnt_policy(ns);
-
 	return 0;
 }
 

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

Quoted 2 lines · Gao feng

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

I have been wondering the same. Was quite some illogical surprise that we have to be doing overlay mounts. This is the exact opposite from what anyone would expect.

Hi Eric,

Quoted 58 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namespace.c                 |   37 +++++++++++++++++++++++++------------
 fs/proc/root.c                 |    7 +++++--
 fs/sysfs/mount.c               |    3 ++-
 include/linux/fs.h             |    1 +
 include/linux/user_namespace.h |    4 ----
 kernel/user.c                  |    2 --
 kernel/user_namespace.c        |    2 --
 7 files changed, 33 insertions(+), 23 deletions(-)

fs/namespace.c +9 -9

index 64627f8..877e427 100644
@@ -2867,25 +2867,38 @@ bool current_chrooted(void)
 	return chrooted;
 }
 
-void update_mnt_policy(struct user_namespace *userns)
+bool fs_fully_visible(struct file_system_type *type)
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mount *mnt;
+	bool visible = false;
 
-	down_read(&namespace_sem);
+	if (unlikely(!ns))
+		return false;
+
+	namespace_lock();
 	list_for_each_entry(mnt, &ns->list, mnt_list) {
-		switch (mnt->mnt.mnt_sb->s_magic) {
-		case SYSFS_MAGIC:
-			userns->may_mount_sysfs = true;
-			break;
-		case PROC_SUPER_MAGIC:
-			userns->may_mount_proc = true;
-			break;
+		struct mount *child;
+		if (mnt->mnt.mnt_sb->s_type != type)
+			continue;

fs/namespace.c bool current_chrooted(void) · line 2893

Quoted 10 lines

fs/namespace.c +10 -0

+
+		/* This mount is not fully visible if there are any child mounts
+		 * that cover anything except for empty directories.
+		 */
+		list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
+			struct inode *inode = child->mnt_mountpoint->d_inode;
+			if (!S_ISDIR(inode->i_mode))
+				goto next;
+			if (inode->i_nlink != 2)
+				goto next;

I met a problem that proc filesystem failed to mount in user namespace, The problem is the i_nlink of sysctl entries under proc filesystem is not
2. it always is 1 even it's a directory, see proc_sys_make_inode. and for btrfs, the i_nlink for an empty dir is 2 too. it seems like depends on the filesystem itself,not depends on vfs. In my system binfmt_misc is mounted on /proc/sys/fs/binfmt_misc, and the i_nlink of this directory's inode is 1.

btw, I'm not quite understand what's the inode->i_nlink != 2 here means? is this directory empty? as I know, when we create a file(not dir) under a dir, the i_nlink of this dir will not increase.

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

Quoted 72 lines · Gao feng

Hi Eric,

Quoted 68 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---
 fs/namespace.c                 |   37 +++++++++++++++++++++++++------------
 fs/proc/root.c                 |    7 +++++--
 fs/sysfs/mount.c               |    3 ++-
 include/linux/fs.h             |    1 +
 include/linux/user_namespace.h |    4 ----
 kernel/user.c                  |    2 --
 kernel/user_namespace.c        |    2 --
 7 files changed, 33 insertions(+), 23 deletions(-)

fs/namespace.c +19 -9

index 64627f8..877e427 100644
@@ -2867,25 +2867,38 @@ bool current_chrooted(void)
 	return chrooted;
 }
 
-void update_mnt_policy(struct user_namespace *userns)
+bool fs_fully_visible(struct file_system_type *type)
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mount *mnt;
+	bool visible = false;
 
-	down_read(&namespace_sem);
+	if (unlikely(!ns))
+		return false;
+
+	namespace_lock();
 	list_for_each_entry(mnt, &ns->list, mnt_list) {
-		switch (mnt->mnt.mnt_sb->s_magic) {
-		case SYSFS_MAGIC:
-			userns->may_mount_sysfs = true;
-			break;
-		case PROC_SUPER_MAGIC:
-			userns->may_mount_proc = true;
-			break;
+		struct mount *child;
+		if (mnt->mnt.mnt_sb->s_type != type)
+			continue;
+
+		/* This mount is not fully visible if there are any child mounts
+		 * that cover anything except for empty directories.
+		 */
+		list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
+			struct inode *inode = child->mnt_mountpoint->d_inode;
+			if (!S_ISDIR(inode->i_mode))
+				goto next;
+			if (inode->i_nlink != 2)
+				goto next;

Quoted 8 lines

I met a problem that proc filesystem failed to mount in user namespace, The problem is the i_nlink of sysctl entries under proc filesystem is not
2. it always is 1 even it's a directory, see proc_sys_make_inode. and for btrfs, the i_nlink for an empty dir is 2 too. it seems like depends on the filesystem itself,not depends on vfs. In my system binfmt_misc is mounted on /proc/sys/fs/binfmt_misc, and the i_nlink of this directory's inode is 1.

Quoted 7 lines

btw, I'm not quite understand what's the inode->i_nlink != 2 here means? is this directory empty? as I know, when we create a file(not dir) under a dir, the i_nlink of this dir will not increase.

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

Any Idea?? or should we need to revert this patch??

Quoted 73 lines · Gao feng

Quoted 72 lines · Gao feng

Hi Eric,

Quoted 68 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namespace.c                 |   37 +++++++++++++++++++++++++------------
 fs/proc/root.c                 |    7 +++++--
 fs/sysfs/mount.c               |    3 ++-
 include/linux/fs.h             |    1 +
 include/linux/user_namespace.h |    4 ----
 kernel/user.c                  |    2 --
 kernel/user_namespace.c        |    2 --
 7 files changed, 33 insertions(+), 23 deletions(-)

fs/namespace.c +19 -9

index 64627f8..877e427 100644
@@ -2867,25 +2867,38 @@ bool current_chrooted(void)
 	return chrooted;
 }
 
-void update_mnt_policy(struct user_namespace *userns)
+bool fs_fully_visible(struct file_system_type *type)
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mount *mnt;
+	bool visible = false;
 
-	down_read(&namespace_sem);
+	if (unlikely(!ns))
+		return false;
+
+	namespace_lock();
 	list_for_each_entry(mnt, &ns->list, mnt_list) {
-		switch (mnt->mnt.mnt_sb->s_magic) {
-		case SYSFS_MAGIC:
-			userns->may_mount_sysfs = true;
-			break;
-		case PROC_SUPER_MAGIC:
-			userns->may_mount_proc = true;
-			break;
+		struct mount *child;
+		if (mnt->mnt.mnt_sb->s_type != type)
+			continue;
+
+		/* This mount is not fully visible if there are any child mounts
+		 * that cover anything except for empty directories.
+		 */
+		list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
+			struct inode *inode = child->mnt_mountpoint->d_inode;
+			if (!S_ISDIR(inode->i_mode))
+				goto next;
+			if (inode->i_nlink != 2)
+				goto next;

Quoted 8 lines

Quoted 8 lines

I met a problem that proc filesystem failed to mount in user namespace, The problem is the i_nlink of sysctl entries under proc filesystem is not
2. it always is 1 even it's a directory, see proc_sys_make_inode. and for btrfs, the i_nlink for an empty dir is 2 too. it seems like depends on the filesystem itself,not depends on vfs. In my system binfmt_misc is mounted on /proc/sys/fs/binfmt_misc, and the i_nlink of this directory's inode is 1.

Yes. 1 is what filesystems that are too lazy to count the number of links to a directory return, and /proc/sys is currently such a filesystem.

Ordinarily nlink == 2 means a directory does not have any subdirectories.

Quoted 9 lines

Quoted 7 lines

btw, I'm not quite understand what's the inode->i_nlink != 2 here means? is this directory empty? as I know, when we create a file(not dir) under a dir, the i_nlink of this dir will not increase.

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

Any Idea?? or should we need to revert this patch??

The patch is mostly doing what it is supposed to be doing.

Now the code is slightly buggy. inode->i_nlink will test to see if a directory has subdirectories but it won't test to see if a directory is empty. Where did my brain go when I was writing that test?

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 118 lines · Eric W. Biederman

Quoted 73 lines · Gao feng

Quoted 72 lines · Gao feng

Hi Eric,

Quoted 68 lines · Eric W. Biederman

Rely on the fact that another flavor of the filesystem is already mounted and do not rely on state in the user namespace.

Verify that the mounted filesystem is not covered in any significant way. I would love to verify that the previously mounted filesystem has no mounts on top but there are at least the directories /proc/sys/fs/binfmt_misc and /sys/fs/cgroup/ that exist explicitly for other filesystems to mount on top of.

Refactor the test into a function named fs_fully_visible and call that function from the mount routines of proc and sysfs. This makes this test local to the filesystems involved and the results current of when the mounts take place, removing a weird threading of the user namespace, the mount namespace and the filesystems themselves.

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 fs/namespace.c                 |   37 +++++++++++++++++++++++++------------
 fs/proc/root.c                 |    7 +++++--
 fs/sysfs/mount.c               |    3 ++-
 include/linux/fs.h             |    1 +
 include/linux/user_namespace.h |    4 ----
 kernel/user.c                  |    2 --
 kernel/user_namespace.c        |    2 --
 7 files changed, 33 insertions(+), 23 deletions(-)

fs/namespace.c +19 -9

index 64627f8..877e427 100644
@@ -2867,25 +2867,38 @@ bool current_chrooted(void)
 	return chrooted;
 }
 
-void update_mnt_policy(struct user_namespace *userns)
+bool fs_fully_visible(struct file_system_type *type)
 {
 	struct mnt_namespace *ns = current->nsproxy->mnt_ns;
 	struct mount *mnt;
+	bool visible = false;
 
-	down_read(&namespace_sem);
+	if (unlikely(!ns))
+		return false;
+
+	namespace_lock();
 	list_for_each_entry(mnt, &ns->list, mnt_list) {
-		switch (mnt->mnt.mnt_sb->s_magic) {
-		case SYSFS_MAGIC:
-			userns->may_mount_sysfs = true;
-			break;
-		case PROC_SUPER_MAGIC:
-			userns->may_mount_proc = true;
-			break;
+		struct mount *child;
+		if (mnt->mnt.mnt_sb->s_type != type)
+			continue;
+
+		/* This mount is not fully visible if there are any child mounts
+		 * that cover anything except for empty directories.
+		 */
+		list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
+			struct inode *inode = child->mnt_mountpoint->d_inode;
+			if (!S_ISDIR(inode->i_mode))
+				goto next;
+			if (inode->i_nlink != 2)
+				goto next;

Quoted 8 lines

Quoted 8 lines

I met a problem that proc filesystem failed to mount in user namespace, The problem is the i_nlink of sysctl entries under proc filesystem is not
2. it always is 1 even it's a directory, see proc_sys_make_inode. and for btrfs, the i_nlink for an empty dir is 2 too. it seems like depends on the filesystem itself,not depends on vfs. In my system binfmt_misc is mounted on /proc/sys/fs/binfmt_misc, and the i_nlink of this directory's inode is 1.

Yes. 1 is what filesystems that are too lazy to count the number of links to a directory return, and /proc/sys is currently such a filesystem.

Ordinarily nlink == 2 means a directory does not have any subdirectories.

Quoted 9 lines

Quoted 7 lines

btw, I'm not quite understand what's the inode->i_nlink != 2 here means? is this directory empty? as I know, when we create a file(not dir) under a dir, the i_nlink of this dir will not increase.

And another question, it looks like if we don't have proc/sys fs mounted, then proc/sys will be failed to be mounted?

Any Idea?? or should we need to revert this patch??

The patch is mostly doing what it is supposed to be doing.

Now the code is slightly buggy. inode->i_nlink will test to see if a directory has subdirectories but it won't test to see if a directory is empty. Where did my brain go when I was writing that test?

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001

Quoted 21 lines · Gao feng

Quoted 12 lines · Eric W. Biederman

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 7 lines

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

Something like this.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001
From: Gao feng <[email protected]>
Date: Wed, 13 Nov 2013 16:06:46 +0800
Subject: [PATCH] userns: introduce new mount option MS_LOCK

After commit 5ff9d8a65ce80efb509ce4e8051394e9ed2cd942
vfs: Lock in place mounts from more privileged users, in userns, the mounts of child mntns which copied from parent mntns is locked and user has no rights to umount/move them, it's too strict.

The core purpose of above commit is trying to prevent unprivileged user from accessing files hidden by mount. This patch introduces a new mount option MS_LOCK, this gives user the capable to mount filesystem as the type of lock if he wants to use mount to hide something.

Quoted 43 lines · Gao feng

Quoted 21 lines · Gao feng

Quoted 12 lines · Eric W. Biederman

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 7 lines

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

Something like this.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001
From: Gao feng <[email protected]>
Date: Wed, 13 Nov 2013 16:06:46 +0800
Subject: [PATCH] userns: introduce new mount option MS_LOCK

After commit 5ff9d8a65ce80efb509ce4e8051394e9ed2cd942
vfs: Lock in place mounts from more privileged users, in userns, the mounts of child mntns which copied from parent mntns is locked and user has no rights to umount/move them, it's too strict.

Quoted 6 lines

The core purpose of above commit is trying to prevent unprivileged user from accessing files hidden by mount. This patch introduces a new mount option MS_LOCK, this gives user the capable to mount filesystem as the type of lock if he wants to use mount to hide something.

This is bad -- if something was secure in old kernels, it needs to stay secure. If you had MS_NOT_A_LOCK, that would be okay, but it might not solve your problem.

Quoted 43 lines · Gao feng

Quoted 21 lines · Gao feng

Quoted 12 lines · Eric W. Biederman

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 7 lines

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

Something like this.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001
From: Gao feng <[email protected]>
Date: Wed, 13 Nov 2013 16:06:46 +0800
Subject: [PATCH] userns: introduce new mount option MS_LOCK

After commit 5ff9d8a65ce80efb509ce4e8051394e9ed2cd942
vfs: Lock in place mounts from more privileged users, in userns, the mounts of child mntns which copied from parent mntns is locked and user has no rights to umount/move them, it's too strict.

Quoted 6 lines

The core purpose of above commit is trying to prevent unprivileged user from accessing files hidden by mount. This patch introduces a new mount option MS_LOCK, this gives user the capable to mount filesystem as the type of lock if he wants to use mount to hide something.

This is bad -- if something was secure in old kernels, it needs to stay secure. If you had MS_NOT_A_LOCK, that would be okay, but it might not solve your problem.

Quoted 51 lines · Andy Lutomirski

Quoted 49 lines · Gao feng

Quoted 21 lines · Gao feng

Quoted 12 lines · Eric W. Biederman

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 7 lines

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

Something like this.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001
From: Gao feng <[email protected]>
Date: Wed, 13 Nov 2013 16:06:46 +0800
Subject: [PATCH] userns: introduce new mount option MS_LOCK

After commit 5ff9d8a65ce80efb509ce4e8051394e9ed2cd942
vfs: Lock in place mounts from more privileged users, in userns, the mounts of child mntns which copied from parent mntns is locked and user has no rights to umount/move them, it's too strict.

The core purpose of above commit is trying to prevent unprivileged user from accessing files hidden by mount. This patch introduces a new mount option MS_LOCK, this gives user the capable to mount filesystem as the type of lock if he wants to use mount to hide something.

Quoted 4 lines

This is bad -- if something was secure in old kernels, it needs to stay secure. If you had MS_NOT_A_LOCK, that would be okay, but it might not solve your problem.

what you mean old kernels here? I saw patch "vfs: Lock in place mounts from more privileged users" is merged into upstream in linux 3.12-rc1, this is not very old. I think there
are not many userspace processes rely on this feature.

Quoted 52 lines · Gao feng

Quoted 51 lines · Andy Lutomirski

Quoted 49 lines · Gao feng

Quoted 21 lines · Gao feng

Quoted 12 lines · Eric W. Biederman

Right now I would rather not have the empty directory exception than remove this code.

The test is a little trickier to write than it might otherwise be because /proc and /sys tend to be slightly imperfect filesystems.

I think the only way to really test that is to call readdir on the directory itself :( I don't like that thought.

I don't know what I was thinking when I wrote that test but I definitely goofed up. Grr!

Quoted 7 lines

I can certainly filter out any directory with nlink > 2. That would be an easy partial step forward.

The real question though is how do I detect directories it is safe to mount on where there will not be files in them. I can't call iterate with the namespace_lock held so things are a bit tricky.

Quoted 7 lines

I know this problem is not easy to be resolved. why not let the user make the decision? maybe we can introduce a new mount option MS_LOCK, if user wants to use mount to hide something, he should use mount with option MS_LOCK. so the unpriviged user can't umount this filesystem and fail to mount the filesystem if one of it's child mount is mounted with MS_LOCK option otherwise he use MS_REC too.

Something like this.

From 437f33ea366623c7a9d557b2e84cae424876a44f Mon Sep 17 00:00:00 2001
From: Gao feng <[email protected]>
Date: Wed, 13 Nov 2013 16:06:46 +0800
Subject: [PATCH] userns: introduce new mount option MS_LOCK

After commit 5ff9d8a65ce80efb509ce4e8051394e9ed2cd942
vfs: Lock in place mounts from more privileged users, in userns, the mounts of child mntns which copied from parent mntns is locked and user has no rights to umount/move them, it's too strict.

The core purpose of above commit is trying to prevent unprivileged user from accessing files hidden by mount. This patch introduces a new mount option MS_LOCK, this gives user the capable to mount filesystem as the type of lock if he wants to use mount to hide something.

Quoted 8 lines

Quoted 4 lines

This is bad -- if something was secure in old kernels, it needs to stay secure. If you had MS_NOT_A_LOCK, that would be okay, but it might not solve your problem.

what you mean old kernels here? I saw patch "vfs: Lock in place mounts from more privileged users" is merged into upstream in linux 3.12-rc1, this is not very old. I think there
are not many userspace processes rely on this feature.

Sort of true. Most people aren't that silly. This feature was added to defend against a theoretical attack that you can use with mount namespaces.

In particular the scenario we are concerned with is:

Suppose the file system looks like:

Suppose there are two filesystems a and b that look like:

a:/usr/
a:/usr/my_very_secret_file
a:/dev/
a:/etc/
a:/lib/

b:/bin/
b:/etc/
b:/games/
b:/include/
b:/lib/
b:/lib32/
b:/local/
b:/sbin/
b:/share/
b:/src/

And filesystem b is mounted on a:/usr hiding a:/usr/my_very_secret_file

So the filesystem looks like:

/usr/
/usr/bin/
/usr/etc/
/usr/games/
/usr/include/
/usr/lib/
/usr/lib32/
/usr/local/
/usr/sbin/
/usr/share/
/usr/src/
/dev/
/etc/
/lib/

Without locking mounts into place an unprivileged user can clone the mount namespace and do "umount /usr" and read /usr/my_very_secret_file.

Most systems don't hide sensitive things with mounts but it is very possible and guarding against is fairly cheap and easy. And while a little annoying it should not be a large impediment to unprivileged user of the user namespace because pivot root still works.

This thread started talking about bugs in fs_fully_visible. And those bugs are fixable and I aim to get to them shortly. At the very least I can lie and test for nlink <= 2 which fixes the regression in mounting proc.

Then I can write the fun version that takes references and drops locks so it can call the internal version of readdir to see if a directory is actually empty.

But the principle remains the same we really don't want to reveal anything that is hidden under a mount on purpose or by mistake. Just because then we don't have to think about those things from a security point of view making everyone's life easier.

Ok,I agree with you that we should make container security by default.

What's your idea that introduces option MS_NOT_A_LOCK just like Andy's advisement?

In libvirt, host creates dev and devpts directories for container,then mount devpts, tmpfs on them and create device nodes inside these dirs for container. and then in container, these filesystems are moved to container's /dev/ /dev/pts directory. We really have no need to lock these mounts. they are just created for container.