[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87o8nhm18l.fsf@x220.int.ebiederm.org>
Date: Tue, 11 Aug 2020 14:27:54 -0500
From: ebiederm@...ssion.com (Eric W. Biederman)
To: Mickaël Salaün <mic@...ikod.net>
Cc: linux-kernel@...r.kernel.org, Aleksa Sarai <cyphar@...har.com>, Alexei
Starovoitov <ast@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, Andrew
Morton <akpm@...ux-foundation.org>, Andy Lutomirski <luto@...nel.org>,
Christian Brauner <christian.brauner@...ntu.com>, Christian Heimes
<christian@...hon.org>, Daniel Borkmann <daniel@...earbox.net>, Deven
Bowers <deven.desai@...ux.microsoft.com>, Dmitry Vyukov
<dvyukov@...gle.com>, Eric Biggers <ebiggers@...nel.org>, Eric Chiang
<ericchiang@...gle.com>, Florian Weimer <fweimer@...hat.com>, James
Morris <jmorris@...ei.org>, Jan Kara <jack@...e.cz>, Jann Horn
<jannh@...gle.com>, Jonathan Corbet <corbet@....net>, Kees Cook
<keescook@...omium.org>, Lakshmi Ramasubramanian
<nramas@...ux.microsoft.com>, Matthew Garrett <mjg59@...gle.com>,
Matthew Wilcox <willy@...radead.org>, Michael Kerrisk
<mtk.manpages@...il.com>, Mimi Zohar <zohar@...ux.ibm.com>, Philippe
Trébuchet <philippe.trebuchet@....gouv.fr>, Scott Shell
<scottsh@...rosoft.com>, Sean Christopherson
<sean.j.christopherson@...el.com>, Shuah Khan <shuah@...nel.org>, Steve
Dower <steve.dower@...hon.org>, Steve Grubb <sgrubb@...hat.com>, Tetsuo
Handa <penguin-kernel@...ove.SAKURA.ne.jp>, Thibaut Sautereau
<thibaut.sautereau@...p-os.org>, Vincent Strubel
<vincent.strubel@....gouv.fr>, kernel-hardening@...ts.openwall.com,
linux-api@...r.kernel.org, linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v7 2/7] exec: Move S_ISREG() check earlier
Mickaël Salaün <mic@...ikod.net> writes:
> From: Kees Cook <keescook@...omium.org>
>
> 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.
> 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/
>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Link: https://lore.kernel.org/r/20200605160013.3954297-3-keescook@chromium.org
> ---
> 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.
> 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))
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
