Re: [PATCH v7 2/7] exec: Move S_ISREG() check earlier
From: Eric W. Biederman <hidden>
Date: 2020-08-11 19:31:41
Also in:
linux-api, linux-fsdevel, linux-security-module, lkml
Possibly related (same subject, not in this thread)
- 2020-07-23 · [PATCH v7 2/7] exec: Move S_ISREG() check earlier · Mickaël Salaün <mic@digikod.net>
Mickaël Salaün [off-list ref] writes:
From: Kees Cook <redacted>
The execve(2)/uselib(2) syscalls have always rejected non-regular
files. Recently, it was noticed that a deadlock was introduced when trying
to execute pipes, as the S_ISREG() test was happening too late. This was
fixed in commit 73601ea5b7b1 ("fs/open.c: allow opening only regular files
during execve()"), but it was added after inode_permission() had already
run, which meant LSMs could see bogus attempts to execute non-regular
files.
Move the test into the other inode type checks (which already look
for other pathological conditions[1]). Since there is no need to use
FMODE_EXEC while we still have access to "acc_mode", also switch the
test to MAY_EXEC.
Also include a comment with the redundant S_ISREG() checks at the end of
execve(2)/uselib(2) to note that they are present to avoid any mistakes.The comment is:
+ /* + * may_open() has already checked for this, so it should be + * impossible to trip now. But we need to be extra cautious + * and check again at the very end too. + */
Those comments scare me. Why do you need to be extra cautious? How can the file type possibly change between may_open and anywhere? The type of a file is immutable after it's creation. If the comment said check just in case something went wrong with code maintenance I could understand but that isn't what the comment says. Also the fallthrough change below really should be broken out into it's own change.
quoted hunk
My notes on the call path, and related arguments, checks, etc: 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 S_ISREG() 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) /* old location of FMODE_EXEC vs S_ISREG() test */ security_file_open(f) open() [1] https://lore.kernel.org/lkml/202006041910.9EF0C602@keescook/ (local) Signed-off-by: Mickaël Salaün <mic@digikod.net> Signed-off-by: Kees Cook <redacted> Link: https://lore.kernel.org/r/20200605160013.3954297-3-keescook@chromium.org (local) --- fs/exec.c | 14 ++++++++++++-- fs/namei.c | 6 ++++-- fs/open.c | 6 ------ 3 files changed, 16 insertions(+), 10 deletions(-)diff --git a/fs/exec.c b/fs/exec.c index d7c937044d10..bdc6a6eb5dce 100644 --- a/fs/exec.c +++ b/fs/exec.c@@ -141,8 +141,13 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; + /* + * may_open() has already checked for this, so it should be + * impossible to trip now. But we need to be extra cautious + * and check again at the very end too. + */ error = -EACCES; - if (!S_ISREG(file_inode(file)->i_mode)) + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode))) goto exit; if (path_noexec(&file->f_path))@@ -886,8 +891,13 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) if (IS_ERR(file)) goto out; + /* + * may_open() has already checked for this, so it should be + * impossible to trip now. But we need to be extra cautious + * and check again at the very end too. + */ err = -EACCES; - if (!S_ISREG(file_inode(file)->i_mode)) + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode))) goto exit; if (path_noexec(&file->f_path))diff --git a/fs/namei.c b/fs/namei.c index 72d4219c93ac..a559ad943970 100644 --- a/fs/namei.c +++ b/fs/namei.c@@ -2849,16 +2849,18 @@ static int may_open(const struct path *path, int acc_mode, int flag) case S_IFLNK: return -ELOOP; case S_IFDIR: - if (acc_mode & MAY_WRITE) + if (acc_mode & (MAY_WRITE | MAY_EXEC)) return -EISDIR; break; case S_IFBLK: case S_IFCHR: if (!may_open_dev(path)) return -EACCES; - /*FALLTHRU*/ + fallthrough;
^^^^^^^^^^^ That is an unrelated change and should be sent separately.
quoted hunk
case S_IFIFO: case S_IFSOCK: + if (acc_mode & MAY_EXEC) + return -EACCES; flag &= ~O_TRUNC; break; }diff --git a/fs/open.c b/fs/open.c index 6cd48a61cda3..623b7506a6db 100644 --- a/fs/open.c +++ b/fs/open.c@@ -784,12 +784,6 @@ static int do_dentry_open(struct file *f, return 0; } - /* Any file opened for execve()/uselib() has to be a regular file. */ - if (unlikely(f->f_flags & FMODE_EXEC && !S_ISREG(inode->i_mode))) { - error = -EACCES; - goto cleanup_file; - } - if (f->f_mode & FMODE_WRITE && !special_file(inode->i_mode)) { error = get_write_access(inode); if (unlikely(error))