Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Kees Cook <keescook@chromium.org>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
        Eric Biggers <ebiggers3@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        linux-fsdevel@vger.kernel.org,
        linux-security-module@vger.kernel.org, linux-api@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20200518055457.12302-1-keescook@chromium.org>
        <87a724t153.fsf@x220.int.ebiederm.org>
        <202005190918.D2BD83F7C@keescook>
        <87o8qjstyw.fsf@x220.int.ebiederm.org>
        <202005191052.0A6B1D5843@keescook>
Date:   Tue, 19 May 2020 13:42:28 -0500
In-Reply-To: <202005191052.0A6B1D5843@keescook> (Kees Cook's message of "Tue,
        19 May 2020 10:56:08 -0700")
Message-ID: <87sgfvrckr.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 0/4] Relocate execve() sanity checks
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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