On Fri, May 15, 2020 at 01:04:08PM +0200, Mickaël Salaün wrote:

On 15/05/2020 10:01, Kees Cook wrote:

quoted

On Thu, May 14, 2020 at 09:16:13PM +0200, Mickaël Salaün wrote:

quoted

On 14/05/2020 18:10, Stephen Smalley wrote:

quoted

On Thu, May 14, 2020 at 11:45 AM Kees Cook [off-list ref] wrote:

quoted

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.)

quoted

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