[Trusty,SRU,1/2] vfs: Commit to never having exectuables on proc and sysfs.

Message ID 20170904175434.7071-2-kleber.souza@canonical.com
State New
Headers show
Series
  • [Trusty,SRU,1/2] vfs: Commit to never having exectuables on proc and sysfs.
Related show

Commit Message

Kleber Sacilotto de Souza Sept. 4, 2017, 5:54 p.m.
From: "Eric W. Biederman" <ebiederm@xmission.com>

CVE-2016-10044

Today proc and sysfs do not contain any executable files.  Several
applications today mount proc or sysfs without noexec and nosuid and
then depend on there being no exectuables files on proc or sysfs.
Having any executable files show on proc or sysfs would cause
a user space visible regression, and most likely security problems.

Therefore commit to never allowing executables on proc and sysfs by
adding a new flag to mark them as filesystems without executables and
enforce that flag.

Test the flag where MNT_NOEXEC is tested today, so that the only user
visible effect will be that exectuables will be treated as if the
execute bit is cleared.

The filesystems proc and sysfs do not currently incoporate any
executable files so this does not result in any user visible effects.

This makes it unnecessary to vet changes to proc and sysfs tightly for
adding exectuable files or changes to chattr that would modify
existing files, as no matter what the individual file say they will
not be treated as exectuable files by the vfs.

Not having to vet changes to closely is important as without this we
are only one proc_create call (or another goof up in the
implementation of notify_change) from having problematic executables
on proc.  Those mistakes are all too easy to make and would create
a situation where there are security issues or the assumptions of
some program having to be broken (and cause userspace regressions).

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
(backported from commit 90f8572b0f021fdd1baa68e00a8c30482ee9e5f4)
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 fs/exec.c           | 10 ++++++++--
 fs/open.c           |  2 +-
 fs/proc/root.c      |  2 ++
 fs/sysfs/mount.c    |  3 +++
 include/linux/fs.h  |  2 ++
 kernel/sys.c        |  3 +--
 mm/mmap.c           |  4 ++--
 mm/nommu.c          |  2 +-
 security/security.c |  2 +-
 9 files changed, 21 insertions(+), 9 deletions(-)

Comments

Colin King Sept. 5, 2017, 9:29 a.m. | #1
On 04/09/17 18:54, Kleber Sacilotto de Souza wrote:
> From: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> CVE-2016-10044
> 
> Today proc and sysfs do not contain any executable files.  Several
> applications today mount proc or sysfs without noexec and nosuid and
> then depend on there being no exectuables files on proc or sysfs.
> Having any executable files show on proc or sysfs would cause
> a user space visible regression, and most likely security problems.
> 
> Therefore commit to never allowing executables on proc and sysfs by
> adding a new flag to mark them as filesystems without executables and
> enforce that flag.
> 
> Test the flag where MNT_NOEXEC is tested today, so that the only user
> visible effect will be that exectuables will be treated as if the
> execute bit is cleared.
> 
> The filesystems proc and sysfs do not currently incoporate any
> executable files so this does not result in any user visible effects.
> 
> This makes it unnecessary to vet changes to proc and sysfs tightly for
> adding exectuable files or changes to chattr that would modify
> existing files, as no matter what the individual file say they will
> not be treated as exectuable files by the vfs.
> 
> Not having to vet changes to closely is important as without this we
> are only one proc_create call (or another goof up in the
> implementation of notify_change) from having problematic executables
> on proc.  Those mistakes are all too easy to make and would create
> a situation where there are security issues or the assumptions of
> some program having to be broken (and cause userspace regressions).
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> (backported from commit 90f8572b0f021fdd1baa68e00a8c30482ee9e5f4)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
>  fs/exec.c           | 10 ++++++++--
>  fs/open.c           |  2 +-
>  fs/proc/root.c      |  2 ++
>  fs/sysfs/mount.c    |  3 +++
>  include/linux/fs.h  |  2 ++
>  kernel/sys.c        |  3 +--
>  mm/mmap.c           |  4 ++--
>  mm/nommu.c          |  2 +-
>  security/security.c |  2 +-
>  9 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index dd7ab64bd5a2..f96daeca68a9 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -107,6 +107,12 @@ bool path_nosuid(const struct path *path)
>  }
>  EXPORT_SYMBOL(path_nosuid);
>  
> +bool path_noexec(const struct path *path)
> +{
> +	return (path->mnt->mnt_flags & MNT_NOEXEC) ||
> +	       (path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);
> +}
> +
>  /*
>   * Note that a shared library must be both readable and executable due to
>   * security reasons.
> @@ -140,7 +146,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
>  		goto exit;
>  
>  	error = -EACCES;
> -	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (path_noexec(&file->f_path))
>  		goto exit;
>  
>  	fsnotify_open(file);
> @@ -798,7 +804,7 @@ struct file *open_exec(const char *name)
>  	if (!S_ISREG(file_inode(file)->i_mode))
>  		goto exit;
>  
> -	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (path_noexec(&file->f_path))
>  		goto exit;
>  
>  	fsnotify_open(file);
> diff --git a/fs/open.c b/fs/open.c
> index 6dcf14157e27..d8aa5ebbbf84 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -344,7 +344,7 @@ retry:
>  		 * with the "noexec" flag.
>  		 */
>  		res = -EACCES;
> -		if (path.mnt->mnt_flags & MNT_NOEXEC)
> +		if (path_noexec(&path))
>  			goto out_path_release;
>  	}
>  
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index c198775a5617..3ce05378b960 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -139,6 +139,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
>  		}
>  
>  		sb->s_flags |= MS_ACTIVE;
> +		/* User space would break if executables appear on proc */
> +		sb->s_iflags |= SB_I_NOEXEC;
>  	}
>  
>  	return dget(sb->s_root);
> diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
> index f4d0799b5779..d705cabe9d6a 100644
> --- a/fs/sysfs/mount.c
> +++ b/fs/sysfs/mount.c
> @@ -139,6 +139,9 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
>  		}
>  		sb->s_flags |= MS_ACTIVE;
>  	}
> +	if (sb->s_root)
> +		/* Userspace would break if executables appear on sysfs */
> +		sb->s_root->d_sb->s_iflags |= SB_I_NOEXEC;
>  
>  	return dget(sb->s_root);
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7e76dbc028d5..67a0bb3f3b19 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1221,6 +1221,7 @@ extern struct list_head super_blocks;
>  extern spinlock_t sb_lock;
>  
>  /* sb->s_iflags */
> +#define SB_I_NOEXEC	0x00000002	/* Ignore executables on this fs */
>  #define SB_I_NOSUID	0x00000004	/* Ignore suid on this fs */
>  
>  /* Possible states of 'frozen' field */
> @@ -2831,5 +2832,6 @@ static inline bool dir_relax(struct inode *inode)
>  }
>  
>  extern bool path_nosuid(const struct path *path);
> +extern bool path_noexec(const struct path *path);
>  
>  #endif /* _LINUX_FS_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 17b7417d0bf6..3b663a209691 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1648,8 +1648,7 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  	 * overall picture.
>  	 */
>  	err = -EACCES;
> -	if (!S_ISREG(inode->i_mode)	||
> -	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
>  		goto exit;
>  
>  	err = inode_permission(inode, MAY_EXEC);
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 72f35bce0f16..8cbdd057bf83 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1261,7 +1261,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>  	 *  mounted, in which case we dont add PROT_EXEC.)
>  	 */
>  	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
> -		if (!(file && (file->f_path.mnt->mnt_flags & MNT_NOEXEC)))
> +		if (!(file && path_noexec(&file->f_path)))
>  			prot |= PROT_EXEC;
>  
>  	if (!len)
> @@ -1341,7 +1341,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>  		case MAP_PRIVATE:
>  			if (!(file->f_mode & FMODE_READ))
>  				return -EACCES;
> -			if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
> +			if (path_noexec(&file->f_path)) {
>  				if (vm_flags & VM_EXEC)
>  					return -EPERM;
>  				vm_flags &= ~VM_MAYEXEC;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 4efb31662bd1..e3053616ffad 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1031,7 +1031,7 @@ static int validate_mmap_request(struct file *file,
>  
>  		/* handle executable mappings and implied executable
>  		 * mappings */
> -		if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
> +		if (path_noexec(&file->f_path)) {
>  			if (prot & PROT_EXEC)
>  				return -EPERM;
>  		}
> diff --git a/security/security.c b/security/security.c
> index ae6eba6b010d..79d239fdba70 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -719,7 +719,7 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
>  	 * ditto if it's not on noexec mount, except that on !MMU we need
>  	 * BDI_CAP_EXEC_MMAP (== VM_MAYEXEC) in this case
>  	 */
> -	if (!(file->f_path.mnt->mnt_flags & MNT_NOEXEC)) {
> +	if (!path_noexec(&file->f_path)) {
>  #ifndef CONFIG_MMU
>  		unsigned long caps = 0;
>  		struct address_space *mapping = file->f_mapping;
> 
Looks like a sane backport to me.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch

diff --git a/fs/exec.c b/fs/exec.c
index dd7ab64bd5a2..f96daeca68a9 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -107,6 +107,12 @@  bool path_nosuid(const struct path *path)
 }
 EXPORT_SYMBOL(path_nosuid);
 
+bool path_noexec(const struct path *path)
+{
+	return (path->mnt->mnt_flags & MNT_NOEXEC) ||
+	       (path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);
+}
+
 /*
  * Note that a shared library must be both readable and executable due to
  * security reasons.
@@ -140,7 +146,7 @@  SYSCALL_DEFINE1(uselib, const char __user *, library)
 		goto exit;
 
 	error = -EACCES;
-	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+	if (path_noexec(&file->f_path))
 		goto exit;
 
 	fsnotify_open(file);
@@ -798,7 +804,7 @@  struct file *open_exec(const char *name)
 	if (!S_ISREG(file_inode(file)->i_mode))
 		goto exit;
 
-	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+	if (path_noexec(&file->f_path))
 		goto exit;
 
 	fsnotify_open(file);
diff --git a/fs/open.c b/fs/open.c
index 6dcf14157e27..d8aa5ebbbf84 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -344,7 +344,7 @@  retry:
 		 * with the "noexec" flag.
 		 */
 		res = -EACCES;
-		if (path.mnt->mnt_flags & MNT_NOEXEC)
+		if (path_noexec(&path))
 			goto out_path_release;
 	}
 
diff --git a/fs/proc/root.c b/fs/proc/root.c
index c198775a5617..3ce05378b960 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -139,6 +139,8 @@  static struct dentry *proc_mount(struct file_system_type *fs_type,
 		}
 
 		sb->s_flags |= MS_ACTIVE;
+		/* User space would break if executables appear on proc */
+		sb->s_iflags |= SB_I_NOEXEC;
 	}
 
 	return dget(sb->s_root);
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index f4d0799b5779..d705cabe9d6a 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -139,6 +139,9 @@  static struct dentry *sysfs_mount(struct file_system_type *fs_type,
 		}
 		sb->s_flags |= MS_ACTIVE;
 	}
+	if (sb->s_root)
+		/* Userspace would break if executables appear on sysfs */
+		sb->s_root->d_sb->s_iflags |= SB_I_NOEXEC;
 
 	return dget(sb->s_root);
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7e76dbc028d5..67a0bb3f3b19 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1221,6 +1221,7 @@  extern struct list_head super_blocks;
 extern spinlock_t sb_lock;
 
 /* sb->s_iflags */
+#define SB_I_NOEXEC	0x00000002	/* Ignore executables on this fs */
 #define SB_I_NOSUID	0x00000004	/* Ignore suid on this fs */
 
 /* Possible states of 'frozen' field */
@@ -2831,5 +2832,6 @@  static inline bool dir_relax(struct inode *inode)
 }
 
 extern bool path_nosuid(const struct path *path);
+extern bool path_noexec(const struct path *path);
 
 #endif /* _LINUX_FS_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 17b7417d0bf6..3b663a209691 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1648,8 +1648,7 @@  static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
 	 * overall picture.
 	 */
 	err = -EACCES;
-	if (!S_ISREG(inode->i_mode)	||
-	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+	if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
 		goto exit;
 
 	err = inode_permission(inode, MAY_EXEC);
diff --git a/mm/mmap.c b/mm/mmap.c
index 72f35bce0f16..8cbdd057bf83 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1261,7 +1261,7 @@  unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	 *  mounted, in which case we dont add PROT_EXEC.)
 	 */
 	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
-		if (!(file && (file->f_path.mnt->mnt_flags & MNT_NOEXEC)))
+		if (!(file && path_noexec(&file->f_path)))
 			prot |= PROT_EXEC;
 
 	if (!len)
@@ -1341,7 +1341,7 @@  unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 		case MAP_PRIVATE:
 			if (!(file->f_mode & FMODE_READ))
 				return -EACCES;
-			if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
+			if (path_noexec(&file->f_path)) {
 				if (vm_flags & VM_EXEC)
 					return -EPERM;
 				vm_flags &= ~VM_MAYEXEC;
diff --git a/mm/nommu.c b/mm/nommu.c
index 4efb31662bd1..e3053616ffad 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1031,7 +1031,7 @@  static int validate_mmap_request(struct file *file,
 
 		/* handle executable mappings and implied executable
 		 * mappings */
-		if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
+		if (path_noexec(&file->f_path)) {
 			if (prot & PROT_EXEC)
 				return -EPERM;
 		}
diff --git a/security/security.c b/security/security.c
index ae6eba6b010d..79d239fdba70 100644
--- a/security/security.c
+++ b/security/security.c
@@ -719,7 +719,7 @@  static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
 	 * ditto if it's not on noexec mount, except that on !MMU we need
 	 * BDI_CAP_EXEC_MMAP (== VM_MAYEXEC) in this case
 	 */
-	if (!(file->f_path.mnt->mnt_flags & MNT_NOEXEC)) {
+	if (!path_noexec(&file->f_path)) {
 #ifndef CONFIG_MMU
 		unsigned long caps = 0;
 		struct address_space *mapping = file->f_mapping;