[PATCH 0/4] Relocate execve() sanity checks
Eric W. Biederman
ebiederm at xmission.com
Tue May 19 18:42:28 UTC 2020
Kees Cook <keescook at chromium.org> writes:
> On Tue, May 19, 2020 at 12:41:27PM -0500, Eric W. Biederman wrote:
>> Kees Cook <keescook at 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 -- 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.
More information about the Linux-security-module-archive