Message-ID: <55A8A4B4.3050202@siteground.com>
Date:	Fri, 17 Jul 2015 09:46:12 +0300
From:	Nikolay Borisov <n.borisov@...eground.com>
To:	Seth Forshee <seth.forshee@...onical.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	James Morris <james.l.morris@...cle.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Paul Moore <paul@...l-moore.com>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	Eric Paris <eparis@...isplace.org>
CC:	Andy Lutomirski <luto@...capital.net>,
	linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] fs: Treat foreign mounts as nosuid



On 07/15/2015 10:46 PM, Seth Forshee wrote:
> From: Andy Lutomirski <luto@...capital.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@...capital.net>
> [ saf: Forward ported to 4.2 ]
> Signed-off-by: Seth Forshee <seth.forshee@...onical.com>
> ---
>  fs/exec.c                |  2 +-
>  fs/namespace.c           | 13 +++++++++++++
>  include/linux/mount.h    |  1 +
>  security/commoncap.c     |  2 +-
>  security/selinux/hooks.c |  2 +-
>  5 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index b06623a9347f..ea7311d72cc3 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1295,7 +1295,7 @@ static void bprm_fill_uid(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))
>  		return;
>  
>  	if (task_no_new_privs(current))
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 423001de32a2..2bfd7ca92247 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3252,6 +3252,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);

Maybe check_mnt() from fs/namespace.c can be exported and used here,
instead of open coding it.

> +}
> +
>  static struct ns_common *mntns_get(struct task_struct *task)
>  {
>  	struct ns_common *ns = NULL;
> diff --git a/include/linux/mount.h b/include/linux/mount.h
> index f822c3c11377..54a594d49733 100644
> --- a/include/linux/mount.h
> +++ b/include/linux/mount.h
> @@ -81,6 +81,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 path;
>  extern struct vfsmount *clone_private_mount(struct path *path);
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 175ab497e810..858d86a1b73c 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -437,7 +437,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;
>  	if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns))
>  		return 0;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 564079c5c49d..459e71ddbc9d 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2137,7 +2137,7 @@ static int check_nnp_nosuid(const struct linux_binprm *bprm,
>  			    const struct task_security_struct *new_tsec)
>  {
>  	int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
> -	int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID);
> +	int nosuid = !mnt_may_suid(bprm->file->f_path.mnt);
>  	int rc;
>  
>  	if (!nnp && !nosuid)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists