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
