| Message ID | 20200518055457.12302-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Relocate execve() sanity checks | expand |
Kees Cook <keescook@chromium.org> writes: > Hi, > > While looking at the code paths for the proposed O_MAYEXEC flag, I saw > some things that looked like they should be fixed up. > > exec: Change uselib(2) IS_SREG() failure to EACCES > This just regularizes the return code on uselib(2). > > exec: Relocate S_ISREG() check > This moves the S_ISREG() check even earlier than it was already. > > exec: Relocate path_noexec() check > This adds the path_noexec() check to the same place as the > S_ISREG() check. > > fs: Include FMODE_EXEC when converting flags to f_mode > This seemed like an oversight, but I suspect there is some > reason I couldn't find for why FMODE_EXEC doesn't get set in > f_mode and just stays in f_flags. So I took a look at this series. I think the belt and suspenders approach of adding code in open and then keeping it in exec and uselib is probably wrong. My sense of the situation is a belt and suspenders approach is more likely to be confusing and result in people making mistakes when maintaining the code than to actually be helpful. Eric
On Tue, May 19, 2020 at 10:06:32AM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > > Hi, > > > > While looking at the code paths for the proposed O_MAYEXEC flag, I saw > > some things that looked like they should be fixed up. > > > > exec: Change uselib(2) IS_SREG() failure to EACCES > > This just regularizes the return code on uselib(2). > > > > exec: Relocate S_ISREG() check > > This moves the S_ISREG() check even earlier than it was already. > > > > exec: Relocate path_noexec() check > > This adds the path_noexec() check to the same place as the > > S_ISREG() check. > > > > fs: Include FMODE_EXEC when converting flags to f_mode > > This seemed like an oversight, but I suspect there is some > > reason I couldn't find for why FMODE_EXEC doesn't get set in > > f_mode and just stays in f_flags. > > So I took a look at this series. > > I think the belt and suspenders approach of adding code in open and then > keeping it in exec and uselib is probably wrong. My sense of the > situation is a belt and suspenders approach is more likely to be > confusing and result in people making mistakes when maintaining the code > than to actually be helpful. This is why I added the comments in fs/exec.c's redundant checks. When I was originally testing this series, I had entirely removed the checks in fs/exec.c, but then had nightmares about some kind of future VFS paths that would somehow bypass do_open() and result in execve() working on noexec mounts, there by allowing for the introduction of a really nasty security bug. The S_ISREG test is demonstrably too late (as referenced in the series), 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. -Kees [1] https://lore.kernel.org/lkml/202005142343.D580850@keescook/
Kees Cook <keescook@chromium.org> writes: > On Tue, May 19, 2020 at 10:06:32AM -0500, Eric W. Biederman wrote: >> Kees Cook <keescook@chromium.org> writes: >> >> > Hi, >> > >> > While looking at the code paths for the proposed O_MAYEXEC flag, I saw >> > some things that looked like they should be fixed up. >> > >> > exec: Change uselib(2) IS_SREG() failure to EACCES >> > This just regularizes the return code on uselib(2). >> > >> > exec: Relocate S_ISREG() check >> > This moves the S_ISREG() check even earlier than it was already. >> > >> > exec: Relocate path_noexec() check >> > This adds the path_noexec() check to the same place as the >> > S_ISREG() check. >> > >> > fs: Include FMODE_EXEC when converting flags to f_mode >> > This seemed like an oversight, but I suspect there is some >> > reason I couldn't find for why FMODE_EXEC doesn't get set in >> > f_mode and just stays in f_flags. >> >> So I took a look at this series. >> >> I think the belt and suspenders approach of adding code in open and then >> keeping it in exec and uselib is probably wrong. My sense of the >> situation is a belt and suspenders approach is more likely to be >> confusing and result in people making mistakes when maintaining the code >> than to actually be helpful. > > This is why I added the comments in fs/exec.c's redundant checks. When I > was originally testing this series, I had entirely removed the checks in > fs/exec.c, but then had nightmares about some kind of future VFS paths > that would somehow bypass do_open() and result in execve() working on > noexec mounts, there by allowing for the introduction of a really nasty > security bug. > > The S_ISREG test is demonstrably too late (as referenced in the series), Yes. The open of a pipe very much happens when it should not. The deadlock looks like part of the cred_guard_mutex mess. I think I introduced an alternate solution for the specific code paths in the backtrace when I introduced exec_update_mutex. The fact that cred_guard_mutex is held over open, while at the same time cred_guard_mutex is grabbed on open files is very questionable. Until my most recent patchset feeding exec /proc/self/maps would also deadlock this way. > 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. Eric
On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > 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).
Kees Cook <keescook@chromium.org> writes: > On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: >> Kees Cook <keescook@chromium.org> writes: >> > 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. 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. 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. I am wondering if we need something distinct to request the type of the file being opened versus execute permissions. 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. Eric
On Tue, May 19, 2020 at 01:42:28PM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: > >> Kees Cook <keescook@chromium.org> writes: > >> > 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. ;) > 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). > 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). 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. 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. 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. 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. > 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. > 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
On 5/19/20 2:17 PM, Kees Cook wrote: > On Tue, May 19, 2020 at 01:42:28PM -0500, Eric W. Biederman wrote: >> Kees Cook <keescook@chromium.org> writes: >> >>> On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: >>>> Kees Cook <keescook@chromium.org> writes: >>>>> 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. ;) > >> 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). > >> 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. > >> 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 >> 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 >