Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Kees Cook]

On Fri, May 15, 2020 at 01:04:08PM +0200, MickaÃl SalaÃn wrote:
> 
> On 15/05/2020 10:01, Kees Cook wrote:
> > On Thu, May 14, 2020 at 09:16:13PM +0200, MickaÃl SalaÃn wrote:
> >> On 14/05/2020 18:10, Stephen Smalley wrote:
> >>> On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> >>>> So, it looks like adding FMODE_EXEC into f_flags in do_open() is needed in
> >>>> addition to injecting MAY_EXEC into acc_mode in do_open()? Hmmm
> >>>
> >>> Just do both in build_open_flags() and be done with it? Looks like he
> >>> was already setting FMODE_EXEC in patch 1 so we just need to teach
> >>> AppArmor/TOMOYO to check for it and perform file execute checking in
> >>> that case if !current->in_execve?
> >>
> >> I can postpone the file permission check for another series to make this
> >> one simpler (i.e. mount noexec only). Because it depends on the sysctl
> >> setting, it is OK to add this check later, if needed. In the meantime,
> >> AppArmor and Tomoyo could be getting ready for this.
> > 
> > So, after playing around with this series, investigating Stephen's
> > comments, digging through the existing FMODE_EXEC uses, and spending a
> > bit more time thinking about Lev and Aleksa's dislike of the sysctls, I've
> > got a much more radically simplified solution that I think could work.
> 
> Not having a sysctl would mean that distros will probably have to patch
> script interpreters to remove the use of O_MAYEXEC. Or distros would
> have to exclude newer version of script interpreters because they
> implement O_MAYEXEC. Or distros would have to patch their kernel to
> implement themselves the sysctl knob I'm already providing. Sysadmins
> may not control the kernel build nor the user space build, they control
> the system configuration (some mount point options and some file
> execution permissions) but I guess that a distro update breaking a
> running system is not acceptable. Either way, unfortunately, I think it
> doesn't help anyone to not have a controlling sysctl. The same apply for
> access-control LSMs relying on a security policy which can be defined by
> sysadmins.
> 
> Your commits enforce file exec checks, which is a good thing from a
> security point of view, but unfortunately that would requires distros to
> update all the packages providing shared objects once the dynamic linker
> uses O_MAYEXEC.

I used to agree with this, but I'm now convinced now that the sysctls are
redundant and will ultimately impede adoption. In looking at what levels
the existing (CLIP OS, Chrome OS) and future (PEP 578) implementations
have needed to do to meaningfully provide the protection, it seems
like software will not be using this flag out of the blue. It'll need
careful addition way beyond the scope of just a sysctl. (As in, I don't
think using O_MAYEXEC is going to just get added without thought to all
interpreters. And developers that DO add it will want to know that the
system will behave in the specified way: having it be off by default
will defeat the purpose of adding the flag for the end users.)

I think it boils down to deciding how to control enforcement: should it
be up to the individual piece of software, or should it be system-wide?
Looking at the patches Chrome OS has made to the shell (and the
accompanying system changes), and Python's overall plans, it seems to
me that the requirements for meaningfully using this flag is going to
be very software-specific.

Now, if the goal is to try to get O_MAYEXEC into every interpreter as
widely as possible without needing to wait for the software-specific
design changes, then I can see the reason to want a default-off global
sysctl. (Though in that case, I suspect it needs to be tied to userns or
something to support containers with different enforcement levels.)

> > Maybe I've missed some earlier discussion that ruled this out, but I
> > couldn't find it: let's just add O_EXEC and be done with it. It actually
> > makes the execve() path more like openat2() and is much cleaner after
> > a little refactoring. Here are the results, though I haven't emailed it
> > yet since I still want to do some more testing:
> > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1
> > 
> > I look forward to flames! ;)
> > 
> 
> Like Florian said, O_EXEC is for execute-only (which obviously doesn't
> work for scripts):
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> On the other hand, the semantic of O_MAYEXEC is complementary to other
> O_* flags. It is inspired by the VM_MAYEXEC flag.

Ah! I see now -- it's intended to be like the O_*ONLY flags. I
misunderstood what Florian meant. Okay, sure that's a good enough reason
for me to retain the O_MAYEXEC name. (And then I think this distinction
from O_EXEC needs to be well documented.)

> The O_EXEC flag is specified for open(2). openat2(2) is Linux-specific
> and it is highly unlikely that new flags will be added to open(2) or
> openat(2) because of compatibility issues.

Agreed. (Which in my mind is further rationale that a sysctl isn't
wanted here: adding O_MAYEXEC will need to be very intentional.)

> FYI, musl implements O_EXEC on Linux with O_PATH:
> https://www.openwall.com/lists/musl/2013/02/22/1
> https://git.musl-libc.org/cgit/musl/commit/?id=6d05d862975188039e648273ceab350d9ab5b69e
> 
> However, the O_EXEC flag/semantic could be useful for the dynamic
> linkers, i.e. to only be able to map files in an executable (and
> read-only) way. If this is OK, then we may want to rename O_MAYEXEC to
> something like O_INTERPRET. This way we could have two new flags for
> sightly (but important) different use cases. The sysctl bitfield could
> be extended to manage both of these flags.

If it's not O_EXEC, then I do like keeping "EXEC" in the flag name,
since it has direct relation to noexec and exec-bit. I'm fine with
O_MAYEXEC -- I just couldn't find the rationale for why it _shouldn't_
be O_EXEC. (Which is now well understood -- thanks to you you and
Florian!)

> Other than that, the other commits are interesting. I'm a bit worried
> about the implication of the f_flags/f_mode change though.

That's an area I also didn't see why FMODE_EXEC wasn't retained in
f_mode. Especially given the nature of the filtering out FMODE_NONOTIFY
in build_open_flags(). Why would FMODE_NONOTIFY move to f_mode, but not
FMODE_EXEC?

> From a practical point of view, I'm also wondering how you intent to
> submit this series on LKML without conflicting with the current
> O_MAYEXEC series (versions, changesâ). I would like you to keep the
> warnings from my patches about other ways to execute/interpret code and
> the threat model (patch 1/6 and 5/6).

I don't intend it to conflict -- I wanted to have actual code written
out to share as a basis for discussion. I didn't want to talk about
"maybe we can try $foo", but rather "here's $foo; what do y'all think?"
:)

-- 
Kees Cook