Kees Cook <keescook@xxxxxxxxxxxx> writes: > On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote: >> Kees Cook <keescook@xxxxxxxxxxxx> 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
