On 5/19/20 2:17 PM, Kees Cook wrote:

On Tue, May 19, 2020 at 01:42:28PM -0500, Eric W. Biederman wrote:

quoted

Kees Cook [off-list ref] writes:

quoted

On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote:

quoted

Kees Cook [off-list ref] writes:

quoted

and given the LSM hooks, I think the noexec check is too late as well.
(This is especially true for the coming O_MAYEXEC series, which will
absolutely need those tests earlier as well[1] -- the permission checking
is then in the correct place: during open, not exec.) I think the only
question is about leaving the redundant checks in fs/exec.c, which I
think are a cheap way to retain a sense of robustness.

The trouble is when someone passes through changes one of the permission
checks for whatever reason (misses that they are duplicated in another
location) and things then fail in some very unexpected way.

Do you think this series should drop the "late" checks in fs/exec.c?
Honestly, the largest motivation for me to move the checks earlier as
I've done is so that other things besides execve() can use FMODE_EXEC
during open() and receive the same sanity-checking as execve() (i.e the
O_MAYEXEC series -- the details are still under discussion but this
cleanup will be needed regardless).

I think this series should drop the "late" checks in fs/exec.c  It feels
less error prone, and it feels like that would transform this into
something Linus would be eager to merge because series becomes a cleanup
that reduces line count.

Yeah, that was my initial sense too. I just started to get nervous about
removing the long-standing exec sanity checks. ;)

quoted

I haven't been inside of open recently enough to remember if the
location you are putting the check fundamentally makes sense.  But the
O_MAYEXEC bits make a pretty strong case that something of the sort
needs to happen.

Right. I *think* it's correct place for now, based on my understanding
of the call graph (which is why I included it in the commit logs).

quoted

I took a quick look but I can not see clearly where path_noexec
and the regular file tests should go.

I do see that you have code duplication with faccessat which suggests
that you haven't put the checks in the right place.

Yeah, I have notes on the similar call sites (which I concluded, perhaps
wrongly) to ignore:

do_faccessat()
    user_path_at(dfd, filename, lookup_flags, &path);
    if (acc_mode & MAY_EXEC .... path_noexec()
    inode_permission(inode, mode | MAY_ACCESS);

This appears to be strictly advisory, and the path_noexec() test is
there to, perhaps, avoid surprises when doing access() then fexecve()?
I would note, however, that that path-based LSMs appear to have no hook
in this call graph at all. I was expecting a call like:

	security_file_permission(..., mode | MAY_ACCESS)

but I couldn't find one (or anything like it), so only
inode_permission() is being tested (which means also the existing
execve() late tests are missed, and the newly added S_ISREG() test from
do_dentry_open() is missed).

sadly correct, its something that we intend to fix but haven't gotten
to it

prctl_set_mm_exe_file()
    err = -EACCESS;
    if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
        goto exit;
    err = inode_permission(inode, MAY_EXEC);

This is similar (no path-based LSM hooks present, only inode_permission()
used for permission checking), but it is at least gated by CAP_SYS_ADMIN.

dito here

And this bring me to a related question from my review: does
dentry_open() intentionally bypass security_inode_permission()? I.e. it
calls vfs_open() not do_open():

openat2(dfd, char * filename, open_how)
    build_open_flags(open_how, open_flags)
    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)
                    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)
                        if (unlikely(f->f_flags & FMODE_EXEC && !S_ISREG(inode->i_mode))) ...
                        security_file_open(f)
                                /* path-based LSMs check for open here
				 * and use FMODE_* flags to determine how a file
                                 * is being opened. */
                        open()

vs

dentry_open(path, flags, cred)
        f = alloc_empty_file(flags, cred);
        vfs_open(path, f);

I would expect dentry_open() to mostly duplicate a bunch of
path_openat(), but it lacks the may_open() call, etc.

I really got the feeling that there was some new conceptual split needed
inside do_open() where the nameidata details have been finished, after
we've gained the "file" information, but before we've lost the "path"
information. For example, may_open(path, ...) has no sense of "file",
though it does do the inode_permission() call.

yes that would be nice, sadly the path hooks are really a bolted on after thought

Note also that may_open() is used in do_tmpfile() too, and has a comment
implying it needs to be checking only a subset of the path details. So
I'm not sure how to split things up.

/me neither anymore

So, that's why I put the new checks just before the may_open() call in
do_open(): it's the most central, positions itself correctly for dealing
with O_MAYEXEC, and doesn't appear to make any existing paths worse.

quoted

I am wondering if we need something distinct to request the type of the
file being opened versus execute permissions.

Well, this is why I wanted to centralize it -- the knowledge of how a
file is going to be used needs to be tested both by the core VFS
(S_ISREG, path_noexec) and the LSMs. Things were inconsistent before.

yep

quoted

All I know is being careful and putting the tests in a good logical
place makes the code more maintainable, whereas not being careful
results in all kinds of sharp corners that might be exploitable.
So I think it is worth digging in and figuring out where those checks
should live.  Especially so that code like faccessat does not need
to duplicate them.

I think this is the right place with respect to execve(), though I think
there are other cases that could use to be improved (or at least made
more consistent).

-Kees