--- v4
+++ v6
@@ -1,199 +1,84 @@
-Enable to either propagate the mount options from the underlying VFS
-mount to prevent execution, or to propagate the file execute permission.
-This may allow a script interpreter to check execution permissions
-before reading commands from a file.
+From: Kees Cook <keescook@chromium.org>
-The main goal is to be able to protect the kernel by restricting
-arbitrary syscalls that an attacker could perform with a crafted binary
-or certain script languages. It also improves multilevel isolation
-by reducing the ability of an attacker to use side channels with
-specific code. These restrictions can natively be enforced for ELF
-binaries (with the noexec mount option) but require this kernel
-extension to properly handle scripts (e.g., Python, Perl).
+The path_noexec() check, like the regular file check, was happening too
+late, letting LSMs see impossible execve()s. Check it earlier as well
+in may_open() and collect the redundant fs/exec.c path_noexec() test
+under the same robustness comment as the S_ISREG() check.
-Add a new sysctl fs.open_mayexec_enforce to control this behavior.
-Indeed, because of compatibility with installed systems, only the system
-administrator is able to check that this new enforcement is in line with
-the system mount points and file permissions. A following patch adds
-documentation.
+My notes on the call path, and related arguments, checks, etc:
-Signed-off-by: Mickaël Salaün <mic@digikod.net>
-Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
-Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
-Cc: Aleksa Sarai <cyphar@cyphar.com>
-Cc: Al Viro <viro@zeniv.linux.org.uk>
-Cc: Kees Cook <keescook@chromium.org>
+do_open_execat()
+ struct open_flags open_exec_flags = {
+ .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
+ .acc_mode = MAY_EXEC,
+ ...
+ do_filp_open(dfd, filename, open_flags)
+ path_openat(nameidata, open_flags, flags)
+ file = alloc_empty_file(open_flags, current_cred());
+ do_open(nameidata, file, open_flags)
+ may_open(path, acc_mode, open_flag)
+ /* new location of MAY_EXEC vs path_noexec() test */
+ inode_permission(inode, MAY_OPEN | acc_mode)
+ security_inode_permission(inode, acc_mode)
+ vfs_open(path, file)
+ do_dentry_open(file, path->dentry->d_inode, open)
+ security_file_open(f)
+ open()
+ /* old location of path_noexec() test */
+
+Signed-off-by: Kees Cook <keescook@chromium.org>
+Acked-by: Mickaël Salaün <mic@digikod.net>
+Link: https://lore.kernel.org/r/20200605160013.3954297-4-keescook@chromium.org
---
+ fs/exec.c | 12 ++++--------
+ fs/namei.c | 4 ++++
+ 2 files changed, 8 insertions(+), 8 deletions(-)
-Changes since v3:
-* Update comment with O_MAYEXEC.
-
-Changes since v2:
-* Cosmetic changes.
-
-Changes since v1:
-* Move code from Yama to the FS subsystem (suggested by Kees Cook).
-* Make omayexec_inode_permission() static (suggested by Jann Horn).
-* Use mode 0600 for the sysctl.
-* Only match regular files (not directories nor other types), which
- follows the same semantic as commit 73601ea5b7b1 ("fs/open.c: allow
- opening only regular files during execve()").
----
- fs/namei.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-
- include/linux/fs.h | 3 ++
- kernel/sysctl.c | 7 +++++
- 3 files changed, 81 insertions(+), 1 deletion(-)
-
+diff --git a/fs/exec.c b/fs/exec.c
+index bdc6a6eb5dce..4eea20c27b01 100644
+--- a/fs/exec.c
++++ b/fs/exec.c
+@@ -147,10 +147,8 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
+ * and check again at the very end too.
+ */
+ error = -EACCES;
+- if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)))
+- goto exit;
+-
+- if (path_noexec(&file->f_path))
++ if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
++ path_noexec(&file->f_path)))
+ goto exit;
+
+ fsnotify_open(file);
+@@ -897,10 +895,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
+ * and check again at the very end too.
+ */
+ err = -EACCES;
+- if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)))
+- goto exit;
+-
+- if (path_noexec(&file->f_path))
++ if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
++ path_noexec(&file->f_path)))
+ goto exit;
+
+ err = deny_write_access(file);
diff --git a/fs/namei.c b/fs/namei.c
-index 33b6d372e74a..a3ac4470da43 100644
+index a559ad943970..ddc9b25540fe 100644
--- a/fs/namei.c
+++ b/fs/namei.c
-@@ -39,6 +39,7 @@
- #include <linux/bitops.h>
- #include <linux/init_task.h>
- #include <linux/uaccess.h>
-+#include <linux/sysctl.h>
+@@ -2863,6 +2863,10 @@ static int may_open(const struct path *path, int acc_mode, int flag)
+ return -EACCES;
+ flag &= ~O_TRUNC;
+ break;
++ case S_IFREG:
++ if ((acc_mode & MAY_EXEC) && path_noexec(path))
++ return -EACCES;
++ break;
+ }
- #include "internal.h"
- #include "mount.h"
-@@ -411,10 +412,40 @@ static int sb_permission(struct super_block *sb, struct inode *inode, int mask)
- return 0;
- }
-
-+#define OMAYEXEC_ENFORCE_NONE 0
-+#define OMAYEXEC_ENFORCE_MOUNT (1 << 0)
-+#define OMAYEXEC_ENFORCE_FILE (1 << 1)
-+#define _OMAYEXEC_LAST OMAYEXEC_ENFORCE_FILE
-+#define _OMAYEXEC_MASK ((_OMAYEXEC_LAST << 1) - 1)
-+
-+/**
-+ * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
-+ *
-+ * @inode: Inode to check permission on
-+ * @mask: Right to check for (%MAY_OPENEXEC, %MAY_EXECMOUNT, %MAY_EXEC)
-+ *
-+ * Returns 0 if access is permitted, -EACCES otherwise.
-+ */
-+static inline int omayexec_inode_permission(struct inode *inode, int mask)
-+{
-+ if (!(mask & MAY_OPENEXEC))
-+ return 0;
-+
-+ if ((sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT) &&
-+ !(mask & MAY_EXECMOUNT))
-+ return -EACCES;
-+
-+ if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
-+ return generic_permission(inode, MAY_EXEC);
-+
-+ return 0;
-+}
-+
- /**
- * inode_permission - Check for access rights to a given inode
- * @inode: Inode to check permission on
-- * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
-+ * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, %MAY_OPENEXEC,
-+ * %MAY_EXECMOUNT)
- *
- * Check for read/write/execute permissions on an inode. We use fs[ug]id for
- * this, letting us set arbitrary permissions for filesystem access without
-@@ -454,10 +485,48 @@ int inode_permission(struct inode *inode, int mask)
- if (retval)
- return retval;
-
-+ retval = omayexec_inode_permission(inode, mask);
-+ if (retval)
-+ return retval;
-+
- return security_inode_permission(inode, mask);
- }
- EXPORT_SYMBOL(inode_permission);
-
-+/*
-+ * Handle open_mayexec_enforce sysctl
-+ */
-+#ifdef CONFIG_SYSCTL
-+int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
-+ size_t *lenp, loff_t *ppos)
-+{
-+ int error;
-+
-+ if (write) {
-+ struct ctl_table table_copy;
-+ int tmp_mayexec_enforce;
-+
-+ if (!capable(CAP_MAC_ADMIN))
-+ return -EPERM;
-+ tmp_mayexec_enforce = *((int *)table->data);
-+ table_copy = *table;
-+ /* Do not erase sysctl_omayexec_enforce. */
-+ table_copy.data = &tmp_mayexec_enforce;
-+ error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
-+ if (error)
-+ return error;
-+ if ((tmp_mayexec_enforce | _OMAYEXEC_MASK) != _OMAYEXEC_MASK)
-+ return -EINVAL;
-+ *((int *)table->data) = tmp_mayexec_enforce;
-+ } else {
-+ error = proc_dointvec(table, write, buffer, lenp, ppos);
-+ if (error)
-+ return error;
-+ }
-+ return 0;
-+}
-+#endif
-+
- /**
- * path_get - get a reference to a path
- * @path: path to get the reference to
-@@ -922,6 +991,7 @@ int sysctl_protected_symlinks __read_mostly = 0;
- int sysctl_protected_hardlinks __read_mostly = 0;
- int sysctl_protected_fifos __read_mostly;
- int sysctl_protected_regular __read_mostly;
-+int sysctl_omayexec_enforce __read_mostly = OMAYEXEC_ENFORCE_NONE;
-
- /**
- * may_follow_link - Check symlink following for unsafe situations
-diff --git a/include/linux/fs.h b/include/linux/fs.h
-index 79435fca6c3e..d44650a674ad 100644
---- a/include/linux/fs.h
-+++ b/include/linux/fs.h
-@@ -83,6 +83,7 @@ extern int sysctl_protected_symlinks;
- extern int sysctl_protected_hardlinks;
- extern int sysctl_protected_fifos;
- extern int sysctl_protected_regular;
-+extern int sysctl_omayexec_enforce;
-
- typedef __kernel_rwf_t rwf_t;
-
-@@ -3545,6 +3546,8 @@ int proc_nr_dentry(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp, loff_t *ppos);
- int proc_nr_inodes(struct ctl_table *table, int write,
- void __user *buffer, size_t *lenp, loff_t *ppos);
-+int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
-+ size_t *lenp, loff_t *ppos);
- int __init get_filesystem_list(char *buf);
-
- #define __FMODE_EXEC ((__force int) FMODE_EXEC)
-diff --git a/kernel/sysctl.c b/kernel/sysctl.c
-index 8a176d8727a3..911afa69f84c 100644
---- a/kernel/sysctl.c
-+++ b/kernel/sysctl.c
-@@ -1892,6 +1892,13 @@ static struct ctl_table fs_table[] = {
- .extra1 = SYSCTL_ZERO,
- .extra2 = &two,
- },
-+ {
-+ .procname = "open_mayexec_enforce",
-+ .data = &sysctl_omayexec_enforce,
-+ .maxlen = sizeof(int),
-+ .mode = 0600,
-+ .proc_handler = proc_omayexec,
-+ },
- #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
- {
- .procname = "binfmt_misc",
+ error = inode_permission(inode, MAY_OPEN | acc_mode);
--
-2.26.2
+2.27.0