[PATCH v3] fs: Treat foreign mounts as nosuid

[PATCH v3] fs: Treat foreign mounts as nosuid

[Andy Lutomirski]

If a process gets access to a mount from a different namespace user
namespace, that process should not be able to take advantage of
setuid files or selinux entrypoints from that filesystem.
Technically, trusting mounts created by the same or ancestor user
namespaces ought to be safe, but it's simpler to distrust all
foreign mounts.

This will make it safer to allow more complex filesystems to be
mounted in non-root user namespaces.

This does not remove the need for MNT_LOCK_NOSUID.  The setuid,
setgid, and file capability bits can no longer be abused if code in
a user namespace were to clear nosuid on an untrusted filesystem,
but this patch, by itself, is insufficient to protect the system
from abuse of files that, when execed, would increase MAC privilege.

As a more concrete explanation, any task that can manipulate a
vfsmount associated with a given user namespace already has
capabilities in that namespace and all of its descendents.  If they
can cause a malicious setuid, setgid, or file-caps executable to
appear in that mount, then that executable will only allow them to
elevate privileges in exactly the set of namespaces in which they
are already privileges.

On the other hand, if they can cause a malicious executable to
appear with a dangerous MAC label, running it could change the
caller's security context in a way that should not have been
possible, even inside the namespace in which the task is confined.

As a hardening measure, this would have made CVE-2014-5207 much
more difficult to exploit.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

Changes from v2:
 - Fix comment typo.

Changes from v1:
 - Treat all foreign mounts as nosuid, not just non-self-or-ancestor
   userns mounts.

 fs/exec.c                |  2 +-
 fs/namespace.c           | 13 +++++++++++++
 include/linux/mount.h    |  1 +
 security/commoncap.c     |  2 +-
 security/selinux/hooks.c |  4 ++--
 5 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a2b42a98c743..ac0bb22aa3ed 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1267,7 +1267,7 @@ int prepare_binprm(struct linux_binprm *bprm)
 	bprm->cred->euid = current_euid();
 	bprm->cred->egid = current_egid();
 
-	if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) &&
+	if (mnt_may_suid(bprm->file->f_path.mnt) &&
 	    !task_no_new_privs(current) &&
 	    kuid_has_mapping(bprm->cred->user_ns, inode->i_uid) &&
 	    kgid_has_mapping(bprm->cred->user_ns, inode->i_gid)) {
diff --git a/fs/namespace.c b/fs/namespace.c
index ef42d9bee212..4df0b393c29d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3019,6 +3019,19 @@ found:
 	return visible;
 }
 
+bool mnt_may_suid(struct vfsmount *mnt)
+{
+	/*
+	 * Foreign mounts (accessed via fchdir or through /proc
+	 * symlinks) are always treated as if they are nosuid.  This
+	 * prevents namespaces from trusting potentially unsafe
+	 * suid/sgid bits, file caps, or security labels that originate
+	 * in other namespaces.
+	 */
+	return real_mount(mnt)->mnt_ns == current->nsproxy->mnt_ns &&
+		!(mnt->mnt_flags & MNT_NOSUID);
+}
+
 static void *mntns_get(struct task_struct *task)
 {
 	struct mnt_namespace *ns = NULL;
diff --git a/include/linux/mount.h b/include/linux/mount.h
index 9262e4bf0cc3..b7b84bafe09b 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -80,6 +80,7 @@ extern void mntput(struct vfsmount *mnt);
 extern struct vfsmount *mntget(struct vfsmount *mnt);
 extern struct vfsmount *mnt_clone_internal(struct path *path);
 extern int __mnt_is_readonly(struct vfsmount *mnt);
+extern bool mnt_may_suid(struct vfsmount *mnt);
 
 struct file_system_type;
 extern struct vfsmount *vfs_kern_mount(struct file_system_type *type,
diff --git a/security/commoncap.c b/security/commoncap.c
index bab0611afc1e..52b3eed065e0 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -443,7 +443,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	if (!file_caps_enabled)
 		return 0;
 
-	if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+	if (!mnt_may_suid(bprm->file->f_path.mnt))
 		return 0;
 
 	dentry = dget(bprm->file->f_dentry);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b0e940497e23..2089fd0d539e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2139,7 +2139,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 		 */
 		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
 			return -EPERM;
-		if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+		if (!mnt_may_suid(bprm->file->f_path.mnt))
 			return -EACCES;
 	} else {
 		/* Check for a default transition on this program. */
@@ -2153,7 +2153,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 	ad.type = LSM_AUDIT_DATA_PATH;
 	ad.u.path = bprm->file->f_path;
 
-	if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) ||
+	if (!mnt_may_suid(bprm->file->f_path.mnt) ||
 	    (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))
 		new_tsec->sid = old_tsec->sid;
 
-- 
1.9.3

Re: [PATCH v3] fs: Treat foreign mounts as nosuid

[Serge E. Hallyn]

Quoting Andy Lutomirski (luto@amacapital.net):
> If a process gets access to a mount from a different namespace user
> namespace, that process should not be able to take advantage of
> setuid files or selinux entrypoints from that filesystem.
> Technically, trusting mounts created by the same or ancestor user
> namespaces ought to be safe, but it's simpler to distrust all
> foreign mounts.
> 
> This will make it safer to allow more complex filesystems to be
> mounted in non-root user namespaces.
> 
> This does not remove the need for MNT_LOCK_NOSUID.  The setuid,
> setgid, and file capability bits can no longer be abused if code in
> a user namespace were to clear nosuid on an untrusted filesystem,
> but this patch, by itself, is insufficient to protect the system
> from abuse of files that, when execed, would increase MAC privilege.
> 
> As a more concrete explanation, any task that can manipulate a
> vfsmount associated with a given user namespace already has
> capabilities in that namespace and all of its descendents.  If they
> can cause a malicious setuid, setgid, or file-caps executable to
> appear in that mount, then that executable will only allow them to
> elevate privileges in exactly the set of namespaces in which they
> are already privileges.
> 
> On the other hand, if they can cause a malicious executable to
> appear with a dangerous MAC label, running it could change the
> caller's security context in a way that should not have been
> possible, even inside the namespace in which the task is confined.
> 
> As a hardening measure, this would have made CVE-2014-5207 much
> more difficult to exploit.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> ---
> 
> Changes from v2:
>  - Fix comment typo.
> 
> Changes from v1:
>  - Treat all foreign mounts as nosuid, not just non-self-or-ancestor
>    userns mounts.
> 
>  fs/exec.c                |  2 +-
>  fs/namespace.c           | 13 +++++++++++++
>  include/linux/mount.h    |  1 +
>  security/commoncap.c     |  2 +-
>  security/selinux/hooks.c |  4 ++--
>  5 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index a2b42a98c743..ac0bb22aa3ed 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1267,7 +1267,7 @@ int prepare_binprm(struct linux_binprm *bprm)
>  	bprm->cred->euid = current_euid();
>  	bprm->cred->egid = current_egid();
>  
> -	if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) &&
> +	if (mnt_may_suid(bprm->file->f_path.mnt) &&
>  	    !task_no_new_privs(current) &&
>  	    kuid_has_mapping(bprm->cred->user_ns, inode->i_uid) &&
>  	    kgid_has_mapping(bprm->cred->user_ns, inode->i_gid)) {
> diff --git a/fs/namespace.c b/fs/namespace.c
> index ef42d9bee212..4df0b393c29d 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3019,6 +3019,19 @@ found:
>  	return visible;
>  }
>  
> +bool mnt_may_suid(struct vfsmount *mnt)
> +{
> +	/*
> +	 * Foreign mounts (accessed via fchdir or through /proc
> +	 * symlinks) are always treated as if they are nosuid.  This
> +	 * prevents namespaces from trusting potentially unsafe
> +	 * suid/sgid bits, file caps, or security labels that originate
> +	 * in other namespaces.
> +	 */
> +	return real_mount(mnt)->mnt_ns == current->nsproxy->mnt_ns &&
> +		!(mnt->mnt_flags & MNT_NOSUID);
> +}
> +
>  static void *mntns_get(struct task_struct *task)
>  {
>  	struct mnt_namespace *ns = NULL;
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index 9262e4bf0cc3..b7b84bafe09b 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -80,6 +80,7 @@ extern void mntput(struct vfsmount *mnt);
>  extern struct vfsmount *mntget(struct vfsmount *mnt);
>  extern struct vfsmount *mnt_clone_internal(struct path *path);
>  extern int __mnt_is_readonly(struct vfsmount *mnt);
> +extern bool mnt_may_suid(struct vfsmount *mnt);
>  
>  struct file_system_type;
>  extern struct vfsmount *vfs_kern_mount(struct file_system_type *type,
> diff --git a/security/commoncap.c b/security/commoncap.c
> index bab0611afc1e..52b3eed065e0 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -443,7 +443,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
>  	if (!file_caps_enabled)
>  		return 0;
>  
> -	if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
> +	if (!mnt_may_suid(bprm->file->f_path.mnt))
>  		return 0;
>  
>  	dentry = dget(bprm->file->f_dentry);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b0e940497e23..2089fd0d539e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2139,7 +2139,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>  		 */
>  		if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
>  			return -EPERM;
> -		if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
> +		if (!mnt_may_suid(bprm->file->f_path.mnt))
>  			return -EACCES;
>  	} else {
>  		/* Check for a default transition on this program. */
> @@ -2153,7 +2153,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
>  	ad.type = LSM_AUDIT_DATA_PATH;
>  	ad.u.path = bprm->file->f_path;
>  
> -	if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) ||
> +	if (!mnt_may_suid(bprm->file->f_path.mnt) ||
>  	    (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))
>  		new_tsec->sid = old_tsec->sid;
>  
> -- 
> 1.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] fs: Treat foreign mounts as nosuid

[Seth Forshee]

On Tue, Oct 14, 2014 at 04:54:47PM -0700, Andy Lutomirski wrote:
> If a process gets access to a mount from a different namespace user
> namespace, that process should not be able to take advantage of
> setuid files or selinux entrypoints from that filesystem.
> Technically, trusting mounts created by the same or ancestor user
> namespaces ought to be safe, but it's simpler to distrust all
> foreign mounts.
> 
> This will make it safer to allow more complex filesystems to be
> mounted in non-root user namespaces.
> 
> This does not remove the need for MNT_LOCK_NOSUID.  The setuid,
> setgid, and file capability bits can no longer be abused if code in
> a user namespace were to clear nosuid on an untrusted filesystem,
> but this patch, by itself, is insufficient to protect the system
> from abuse of files that, when execed, would increase MAC privilege.
> 
> As a more concrete explanation, any task that can manipulate a
> vfsmount associated with a given user namespace already has
> capabilities in that namespace and all of its descendents.  If they
> can cause a malicious setuid, setgid, or file-caps executable to
> appear in that mount, then that executable will only allow them to
> elevate privileges in exactly the set of namespaces in which they
> are already privileges.
> 
> On the other hand, if they can cause a malicious executable to
> appear with a dangerous MAC label, running it could change the
> caller's security context in a way that should not have been
> possible, even inside the namespace in which the task is confined.
> 
> As a hardening measure, this would have made CVE-2014-5207 much
> more difficult to exploit.
> 
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>

Now that I'm back from plumbers I've finally had a chance to play around
with this and it seems to be working as expected. I'll be considering
this patch a prerequisite for the next round of fuse userns patches.

Acked-by: Seth Forshee <seth.forshee@canonical.com>