[PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
Enable to forbid access to files open with O_MAYEXEC. Thanks to the
noexec option from the underlying VFS mount, or to the file execute
permission, userspace can enforce these execution policies. This may
allow script interpreters to check execution permission before reading
commands from a file, or dynamic linkers to allow shared object loading.
Add a new sysctl fs.open_mayexec_enforce to enable system administrators
to enforce two complementary security policies according to the
installed system: enforce the noexec mount option, and enforce
executable file permission. Indeed, because of compatibility with
installed systems, only system administrators are able to check that
this new enforcement is in line with the system mount points and file
permissions. A following patch adds documentation.
For tailored Linux distributions, it is possible to enforce such
restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
CONFIG_OMAYEXEC_ENFORCE_FILE.
Being able to restrict execution also enables to protect the kernel by
restricting arbitrary syscalls that an attacker could perform with a
crafted binary or certain script languages. It also improves multilevel
isolation by reducing the ability of an attacker to use side channels
with specific code. These restrictions can natively be enforced for ELF
binaries (with the noexec mount option) but require this kernel
extension to properly handle scripts (e.g., Python, Perl). To get a
consistent execution policy, additional memory restrictions should also
be enforced (e.g. thanks to SELinux).
Signed-off-by: Mickaël Salaün
Reviewed-by: Thibaut Sautereau
Cc: Aleksa Sarai
Cc: Al Viro
Cc: Kees Cook
---
Changes since v4:
* Add kernel configuration options to enforce O_MAYEXEC at build time,
and disable the sysctl in such case (requested by James Morris).
* Reword commit message.
Changes since v3:
* Update comment with O_MAYEXEC.
Changes since v2:
* Cosmetic changes.
Changes since v1:
* Move code from Yama to the FS subsystem (suggested by Kees Cook).
* Make omayexec_inode_permission() static (suggested by Jann Horn).
* Use mode 0600 for the sysctl.
* Only match regular files (not directories nor other types), which
follows the same semantic as commit 73601ea5b7b1 ("fs/open.c: allow
opening only regular files during execve()").
---
fs/namei.c | 87 +-
include/linux/fs.h | 5 +++
kernel/sysctl.c| 9 +
security/Kconfig | 26 ++
4 files changed, 126 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 33b6d372e74a..70f179f6bc6c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -39,6 +39,7 @@
#include
#include
#include
+#include
#include "internal.h"
#include "mount.h"
@@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb, struct
inode *inode, int mask)
return 0;
}
+#define OMAYEXEC_ENFORCE_NONE 0
+#define OMAYEXEC_ENFORCE_MOUNT (1 << 0)
+#define OMAYEXEC_ENFORCE_FILE (1 << 1)
+#define _OMAYEXEC_LAST OMAYEXEC_ENFORCE_FILE
+#define _OMAYEXEC_MASK ((_OMAYEXEC_LAST << 1) - 1)
+
+#ifdef CONFIG_OMAYEXEC_STATIC
+const int sysctl_omayexec_enforce =
+#ifdef CONFIG_OMAYEXEC_ENFORCE_MOUNT
+ OMAYEXEC_ENFORCE_MOUNT |
+#endif
+#ifdef CONFIG_OMAYEXEC_ENFORCE_FILE
+ OMAYEXEC_ENFORCE_FILE |
+#endif
+ OMAYEXEC_ENFORCE_NONE;
+#else /* CONFIG_OMAYEXEC_STATIC */
+int sysctl_omayexec_enforce __read_mostly = OMAYEXEC_ENFORCE_NONE;
+#endif /* CONFIG_OMAYEXEC_STATIC */
+
+/*
+ * Handle open_mayexec_enforce sysctl
+ */
+#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
+int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
+ size_t *lenp, loff_t *ppos)
+{
+ int error;
+
+ if (write) {
+ struct ctl_table table_copy;
+ int tmp_mayexec_enforce;
+
+ if (!capable(CAP_MAC_ADMIN))
+ return -EPERM;
+
+ tmp_mayexec_enforce = *((int *)table->data);
+ table_copy = *table;
+ /* Do not erase sysctl_omayexec_enforce. */
+ table_copy.data = &tmp_mayexec_enforce;
+ error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+
+ if ((tmp_mayexec_enforce | _OMAYEXEC_MASK) != _OMAYEXEC_MASK)
+ return -EINVAL;
+
+ *((int *)table->data) = tmp_mayexec_enforce;
+ } else {
+ error = proc_dointvec(table, write, buffer, lenp, ppos);
+ if (error)
+ return error;
+ }
+ return 0;
+}
+#endif
+
+/**
+ * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
+ *
+ * @inode: Inode to check permission on
+ * @mask: Right to check for (%MAY_OPENEXEC, %MAY_EXECMOUNT, %MAY_EXEC)
+
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 5/5/20 8:31 AM, Mickaël Salaün wrote: > diff --git a/security/Kconfig b/security/Kconfig > index cd3cc7da3a55..d8fac9240d14 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH > If you wish for all usermode helper programs to be disabled, > specify an empty string here (i.e. ""). > > +menuconfig OMAYEXEC_STATIC > + tristate "Configure O_MAYEXEC behavior at build time" > + ---help--- > + Enable to enforce O_MAYEXEC at build time, and disable the dedicated > + fs.open_mayexec_enforce sysctl. That help message is a bit confusing IMO. Does setting/enabling OMAYEXEC_STATIC both enforce O_MAYEXEC at build time and also disable the dedicated sysctl? Or are these meant to be alternatives, one for what Enabling this kconfig symbol does and the other for what Disabling this symbol does? If so, it doesn't say that. > + > + See Documentation/admin-guide/sysctl/fs.rst for more details. > + > +if OMAYEXEC_STATIC > + > +config OMAYEXEC_ENFORCE_MOUNT > + bool "Mount restriction" > + default y > + ---help--- > + Forbid opening files with the O_MAYEXEC option if their underlying > VFS is > + mounted with the noexec option or if their superblock forbids > execution > + of its content (e.g., /proc). > + > +config OMAYEXEC_ENFORCE_FILE > + bool "File permission restriction" > + ---help--- > + Forbid opening files with the O_MAYEXEC option if they are not marked > as > + executable for the current process (e.g., POSIX permissions). > + > +endif # OMAYEXEC_STATIC > + > source "security/selinux/Kconfig" > source "security/smack/Kconfig" > source "security/tomoyo/Kconfig" -- ~Randy Reported-by: Randy Dunlap
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 05/05/2020 17:44, Randy Dunlap wrote: > On 5/5/20 8:31 AM, Mickaël Salaün wrote: >> diff --git a/security/Kconfig b/security/Kconfig >> index cd3cc7da3a55..d8fac9240d14 100644 >> --- a/security/Kconfig >> +++ b/security/Kconfig >> @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH >>If you wish for all usermode helper programs to be disabled, >>specify an empty string here (i.e. ""). >> >> +menuconfig OMAYEXEC_STATIC >> +tristate "Configure O_MAYEXEC behavior at build time" >> +---help--- >> + Enable to enforce O_MAYEXEC at build time, and disable the dedicated >> + fs.open_mayexec_enforce sysctl. > > That help message is a bit confusing IMO. Does setting/enabling > OMAYEXEC_STATIC > both enforce O_MAYEXEC at build time and also disable the dedicated sysctl? Yes. What about this? "Define the O_MAYEXEC policy at build time only. As a side effect, this also disables the fs.open_mayexec_enforce sysctl." > > Or are these meant to be alternatives, one for what Enabling this kconfig > symbol > does and the other for what Disabling this symbol does? If so, it doesn't > say that. > >> + >> + See Documentation/admin-guide/sysctl/fs.rst for more details. >> + >> +if OMAYEXEC_STATIC >> + >> +config OMAYEXEC_ENFORCE_MOUNT >> +bool "Mount restriction" >> +default y >> +---help--- >> + Forbid opening files with the O_MAYEXEC option if their underlying >> VFS is >> + mounted with the noexec option or if their superblock forbids >> execution >> + of its content (e.g., /proc). >> + >> +config OMAYEXEC_ENFORCE_FILE >> +bool "File permission restriction" >> +---help--- >> + Forbid opening files with the O_MAYEXEC option if they are not marked >> as >> + executable for the current process (e.g., POSIX permissions). >> + >> +endif # OMAYEXEC_STATIC >> + >> source "security/selinux/Kconfig" >> source "security/smack/Kconfig" >> source "security/tomoyo/Kconfig" > >
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 5/5/20 9:55 AM, Mickaël Salaün wrote: > > > On 05/05/2020 17:44, Randy Dunlap wrote: >> On 5/5/20 8:31 AM, Mickaël Salaün wrote: >>> diff --git a/security/Kconfig b/security/Kconfig >>> index cd3cc7da3a55..d8fac9240d14 100644 >>> --- a/security/Kconfig >>> +++ b/security/Kconfig >>> @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH >>> If you wish for all usermode helper programs to be disabled, >>> specify an empty string here (i.e. ""). >>> >>> +menuconfig OMAYEXEC_STATIC >>> + tristate "Configure O_MAYEXEC behavior at build time" >>> + ---help--- >>> + Enable to enforce O_MAYEXEC at build time, and disable the dedicated >>> + fs.open_mayexec_enforce sysctl. >> >> That help message is a bit confusing IMO. Does setting/enabling >> OMAYEXEC_STATIC >> both enforce O_MAYEXEC at build time and also disable the dedicated sysctl? > > Yes. What about this? > "Define the O_MAYEXEC policy at build time only. As a side effect, this > also disables the fs.open_mayexec_enforce sysctl." > Yes, much better. Thanks. >> >> Or are these meant to be alternatives, one for what Enabling this kconfig >> symbol >> does and the other for what Disabling this symbol does? If so, it doesn't >> say that. >> >>> + >>> + See Documentation/admin-guide/sysctl/fs.rst for more details. >>> + >>> +if OMAYEXEC_STATIC >>> + >>> +config OMAYEXEC_ENFORCE_MOUNT >>> + bool "Mount restriction" >>> + default y >>> + ---help--- >>> + Forbid opening files with the O_MAYEXEC option if their underlying >>> VFS is >>> + mounted with the noexec option or if their superblock forbids >>> execution >>> + of its content (e.g., /proc). >>> + >>> +config OMAYEXEC_ENFORCE_FILE >>> + bool "File permission restriction" >>> + ---help--- >>> + Forbid opening files with the O_MAYEXEC option if they are not marked >>> as >>> + executable for the current process (e.g., POSIX permissions). >>> + >>> +endif # OMAYEXEC_STATIC >>> + >>> source "security/selinux/Kconfig" >>> source "security/smack/Kconfig" >>> source "security/tomoyo/Kconfig" >> >> -- ~Randy
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Tue, May 05, 2020 at 05:31:53PM +0200, Mickaël Salaün wrote:
> Enable to forbid access to files open with O_MAYEXEC. Thanks to the
> noexec option from the underlying VFS mount, or to the file execute
> permission, userspace can enforce these execution policies. This may
> allow script interpreters to check execution permission before reading
> commands from a file, or dynamic linkers to allow shared object loading.
Some language tailoring. I might change the first sentence to:
Allow for the enforcement of the O_MAYEXEC openat2(2) flag.
> Add a new sysctl fs.open_mayexec_enforce to enable system administrators
> to enforce two complementary security policies according to the
> installed system: enforce the noexec mount option, and enforce
> executable file permission. Indeed, because of compatibility with
> installed systems, only system administrators are able to check that
> this new enforcement is in line with the system mount points and file
> permissions. A following patch adds documentation.
>
> For tailored Linux distributions, it is possible to enforce such
> restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
> The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
> CONFIG_OMAYEXEC_ENFORCE_FILE.
OMAYEXEC feels like the wrong name here. Maybe something closer to the
sysctl name? CONFIG_OPEN_MAYEXEC?
And I think it's not needed to have 3 configs for this. That's a lot of
mess for a corner case option. I think I would model this after other
sysctl CONFIGs, and just call this CONFIG_OPEN_MAYEXEC_DEFAULT.
Is _disabling_ the sysctl needed? This patch gets much smaller without
the ..._STATIC bit. (And can we avoid "static", it means different
things to different people. How about invert the logic and call it
CONFIG_OPEN_MAYEXEC_SYSCTL?)
Further notes below...
> [...]
> diff --git a/fs/namei.c b/fs/namei.c
> index 33b6d372e74a..70f179f6bc6c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -39,6 +39,7 @@
> #include
> #include
> #include
> +#include
>
> #include "internal.h"
> #include "mount.h"
> @@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb, struct
> inode *inode, int mask)
> return 0;
> }
>
> +#define OMAYEXEC_ENFORCE_NONE0
Like the CONFIG, I'd stay close to the sysctl, OPEN_MAYEXEC_ENFORCE_...
> +#define OMAYEXEC_ENFORCE_MOUNT (1 << 0)
> +#define OMAYEXEC_ENFORCE_FILE(1 << 1)
Please use BIT(0), BIT(1)...
> +#define _OMAYEXEC_LAST OMAYEXEC_ENFORCE_FILE
> +#define _OMAYEXEC_MASK ((_OMAYEXEC_LAST << 1) - 1)
> +
> +#ifdef CONFIG_OMAYEXEC_STATIC
> +const int sysctl_omayexec_enforce =
> +#ifdef CONFIG_OMAYEXEC_ENFORCE_MOUNT
> + OMAYEXEC_ENFORCE_MOUNT |
> +#endif
> +#ifdef CONFIG_OMAYEXEC_ENFORCE_FILE
> + OMAYEXEC_ENFORCE_FILE |
> +#endif
> + OMAYEXEC_ENFORCE_NONE;
> +#else /* CONFIG_OMAYEXEC_STATIC */
> +int sysctl_omayexec_enforce __read_mostly = OMAYEXEC_ENFORCE_NONE;
> +#endif /* CONFIG_OMAYEXEC_STATIC */
If you keep CONFIG_OPEN_MAYEXEC_SYSCTL, you could do this in namei.h:
#ifdef CONFIG_OPEN_MAYEXEC_SYSCTL
#define __sysctl_writable __read_mostly
#else
#define __sysctl_write const
#endif
Then with my proposed change to the enforce CONFIG, all of this is
reduced to simply:
int open_mayexec_enforce __sysctl_writable = CONFIG_OPEN_MAYEXEC_DEFAULT;
> +
> +/*
> + * Handle open_mayexec_enforce sysctl
> + */
> +#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
> + size_t *lenp, loff_t *ppos)
> +{
> + int error;
> +
> + if (write) {
> + struct ctl_table table_copy;
> + int tmp_mayexec_enforce;
> +
> + if (!capable(CAP_MAC_ADMIN))
> + return -EPERM;
> +
> + tmp_mayexec_enforce = *((int *)table->data);
> + table_copy = *table;
> + /* Do not erase sysctl_omayexec_enforce. */
> + table_copy.data = &tmp_mayexec_enforce;
> + error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> +
> + if ((tmp_mayexec_enforce | _OMAYEXEC_MASK) != _OMAYEXEC_MASK)
> + return -EINVAL;
> +
> + *((int *)table->data) = tmp_mayexec_enforce;
> + } else {
> + error = proc_dointvec(table, write, buffer, lenp, ppos);
> + if (error)
> + return error;
> + }
> + return 0;
> +}
> +#endif
I don't think any of this is needed. There are no complex bit field
interactions to check for. The sysctl is min=0, max=3. The only thing
special here is checking CAP_MAC_ADMIN. I would just add
proc_dointvec_minmax_macadmin(), like we have for ..._minmax_sysadmin().
> +
> +/**
> + * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
> + *
> + * @inode: Inode to check p
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 14/05/2020 18:10, Stephen Smalley wrote:
> On Thu, May 14, 2020 at 11:45 AM Kees Cook wrote:
>>
>> On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
>>> On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> Like, couldn't just the entire thing just be:
>
> diff --git a/fs/namei.c b/fs/namei.c
> index a320371899cf..0ab18e19f5da 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> acc_mode, int flag)
> break;
> }
>
> + if (unlikely(mask & MAY_OPENEXEC)) {
> + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> + path_noexec(path))
> + return -EACCES;
> + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> + acc_mode |= MAY_EXEC;
> + }
> error = inode_permission(inode, MAY_OPEN | acc_mode);
> if (error)
> return error;
>
FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
reduced to this plus the Kconfig and sysctl changes, the self tests
pass.
I think this makes things much cleaner and correct.
>>>
>>> I think that covers inode-based security modules but not path-based
>>> ones (they don't implement the inode_permission hook). For those, I
>>> would tentatively guess that we need to make sure FMODE_EXEC is set on
>>> the open file and then they need to check for that in their file_open
>>> hooks.
>>
>> I kept confusing myself about what order things happened in, so I made
>> these handy notes about the call graph:
>>
>> openat2(dfd, char * filename, open_how)
>> do_filp_open(dfd, filename, open_flags)
>> path_openat(nameidata, open_flags, flags)
>> do_open(nameidata, file, open_flags)
>> may_open(path, acc_mode, open_flag)
>> inode_permission(inode, MAY_OPEN | acc_mode)
>> security_inode_permission(inode, acc_mode)
>> vfs_open(path, file)
>> do_dentry_open(file, path->dentry->d_inode, open)
>> if (unlikely(f->f_flags & FMODE_EXEC &&
>> !S_ISREG(inode->i_mode))) ...
>> security_file_open(f)
>> open()
>>
>> 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.
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 14/05/2020 01:27, Kees Cook wrote:
> On Wed, May 13, 2020 at 11:37:16AM -0400, Stephen Smalley wrote:
>> On Tue, May 5, 2020 at 11:33 AM Mickaël Salaün wrote:
>>>
>>> Enable to forbid access to files open with O_MAYEXEC. Thanks to the
>>> noexec option from the underlying VFS mount, or to the file execute
>>> permission, userspace can enforce these execution policies. This may
>>> allow script interpreters to check execution permission before reading
>>> commands from a file, or dynamic linkers to allow shared object loading.
>>>
>>> Add a new sysctl fs.open_mayexec_enforce to enable system administrators
>>> to enforce two complementary security policies according to the
>>> installed system: enforce the noexec mount option, and enforce
>>> executable file permission. Indeed, because of compatibility with
>>> installed systems, only system administrators are able to check that
>>> this new enforcement is in line with the system mount points and file
>>> permissions. A following patch adds documentation.
>>>
>>> For tailored Linux distributions, it is possible to enforce such
>>> restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
>>> The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
>>> CONFIG_OMAYEXEC_ENFORCE_FILE.
>>>
>>> Being able to restrict execution also enables to protect the kernel by
>>> restricting arbitrary syscalls that an attacker could perform with a
>>> crafted binary or certain script languages. It also improves multilevel
>>> isolation by reducing the ability of an attacker to use side channels
>>> with specific code. These restrictions can natively be enforced for ELF
>>> binaries (with the noexec mount option) but require this kernel
>>> extension to properly handle scripts (e.g., Python, Perl). To get a
>>> consistent execution policy, additional memory restrictions should also
>>> be enforced (e.g. thanks to SELinux).
>>>
>>> Signed-off-by: Mickaël Salaün
>>> Reviewed-by: Thibaut Sautereau
>>> Cc: Aleksa Sarai
>>> Cc: Al Viro
>>> Cc: Kees Cook
>>> ---
>>
>>> diff --git a/fs/namei.c b/fs/namei.c
>>> index 33b6d372e74a..70f179f6bc6c 100644
>>> --- a/fs/namei.c
>>> +++ b/fs/namei.c
>>> @@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb,
>>> struct inode *inode, int mask)
>>
>>> +#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
>>> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
>>> + size_t *lenp, loff_t *ppos)
>>> +{
>>> + int error;
>>> +
>>> + if (write) {
>>> + struct ctl_table table_copy;
>>> + int tmp_mayexec_enforce;
>>> +
>>> + if (!capable(CAP_MAC_ADMIN))
>>> + return -EPERM;
>>
>> Not fond of using CAP_MAC_ADMIN here (or elsewhere outside of security
>> modules). The ability to set this sysctl is not equivalent to being
>> able to load a MAC policy, set arbitrary MAC labels on
>> processes/files, etc.
>
> That's fair. In that case, perhaps this could just use the existing
> _sysadmin helper? (Though I should note that these perm checks actually
> need to be in the open, not the read/write ... I thought there was a
> series to fix that, but I can't find it now. Regardless, that's
> orthogonal to this series.)
OK, I'll switch to CAP_SYS_ADMIN with proc_dointvec_minmax_sysadmin().
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 2020/05/06 0:31, Mickaël Salaün wrote: > The goal of this patch series is to enable to control script execution > with interpreters help. A new O_MAYEXEC flag, usable through > openat2(2), is added to enable userspace script interpreter to delegate > to the kernel (and thus the system security policy) the permission to > interpret/execute scripts or other files containing what can be seen as > commands. Since TOMOYO considers that any file (even standard input which is connected to keyboard) can provide data which can be interpreted as executable, TOMOYO does not check traditional "execute permission". TOMOYO's execute permission serves as a gate for replacing current process with a new file using execve() syscall. All other calls (e.g. uselib(), open()) are simply treated as opening a file for read/write/append etc. Therefore, On 14/05/2020 18:10, Stephen Smalley wrote:> 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? regarding TOMOYO, I don't think that TOMOYO needs to perform file execute checking if !current->in_execve , even if O_MAYEXEC is introduced.
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On 12/05/2020 23:48, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:53PM +0200, Mickaël Salaün wrote:
>> Enable to forbid access to files open with O_MAYEXEC. Thanks to the
>> noexec option from the underlying VFS mount, or to the file execute
>> permission, userspace can enforce these execution policies. This may
>> allow script interpreters to check execution permission before reading
>> commands from a file, or dynamic linkers to allow shared object loading.
>
> Some language tailoring. I might change the first sentence to:
>
> Allow for the enforcement of the O_MAYEXEC openat2(2) flag.
OK
>
>> Add a new sysctl fs.open_mayexec_enforce to enable system administrators
>> to enforce two complementary security policies according to the
>> installed system: enforce the noexec mount option, and enforce
>> executable file permission. Indeed, because of compatibility with
>> installed systems, only system administrators are able to check that
>> this new enforcement is in line with the system mount points and file
>> permissions. A following patch adds documentation.
>>
>> For tailored Linux distributions, it is possible to enforce such
>> restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
>> The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
>> CONFIG_OMAYEXEC_ENFORCE_FILE.
>
> OMAYEXEC feels like the wrong name here. Maybe something closer to the
> sysctl name? CONFIG_OPEN_MAYEXEC?
>
> And I think it's not needed to have 3 configs for this. That's a lot of
> mess for a corner case option. I think I would model this after other
> sysctl CONFIGs, and just call this CONFIG_OPEN_MAYEXEC_DEFAULT.
OK, I guess you mean to store the default integer value of the sysctl in
this config option.
>
> Is _disabling_ the sysctl needed? This patch gets much smaller without
> the ..._STATIC bit. (And can we avoid "static", it means different
> things to different people. How about invert the logic and call it
> CONFIG_OPEN_MAYEXEC_SYSCTL?)
I added this in response to James's comment:
https://lore.kernel.org/lkml/alpine.lrh.2.21.2005020405210.5...@namei.org/
I'm fine to let the sysctl visible whatever the kernel config is. It
makes the code simpler. I guess tailored security distros already
protect sysctl entries anyway.
>
> Further notes below...
>
>> [...]
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 33b6d372e74a..70f179f6bc6c 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -39,6 +39,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "internal.h"
>> #include "mount.h"
>> @@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb,
>> struct inode *inode, int mask)
>> return 0;
>> }
>>
>> +#define OMAYEXEC_ENFORCE_NONE 0
>
> Like the CONFIG, I'd stay close to the sysctl, OPEN_MAYEXEC_ENFORCE_...
>
>> +#define OMAYEXEC_ENFORCE_MOUNT (1 << 0)
>> +#define OMAYEXEC_ENFORCE_FILE (1 << 1)
>
> Please use BIT(0), BIT(1)...
>
>> +#define _OMAYEXEC_LAST OMAYEXEC_ENFORCE_FILE
>> +#define _OMAYEXEC_MASK ((_OMAYEXEC_LAST << 1) - 1)
>> +
>> +#ifdef CONFIG_OMAYEXEC_STATIC
>> +const int sysctl_omayexec_enforce =
>> +#ifdef CONFIG_OMAYEXEC_ENFORCE_MOUNT
>> +OMAYEXEC_ENFORCE_MOUNT |
>> +#endif
>> +#ifdef CONFIG_OMAYEXEC_ENFORCE_FILE
>> +OMAYEXEC_ENFORCE_FILE |
>> +#endif
>> +OMAYEXEC_ENFORCE_NONE;
>> +#else /* CONFIG_OMAYEXEC_STATIC */
>> +int sysctl_omayexec_enforce __read_mostly = OMAYEXEC_ENFORCE_NONE;
>> +#endif /* CONFIG_OMAYEXEC_STATIC */
>
>
> If you keep CONFIG_OPEN_MAYEXEC_SYSCTL, you could do this in namei.h:
>
> #ifdef CONFIG_OPEN_MAYEXEC_SYSCTL
> #define __sysctl_writable __read_mostly
> #else
> #define __sysctl_writeconst
> #endif
>
> Then with my proposed change to the enforce CONFIG, all of this is
> reduced to simply:
>
> int open_mayexec_enforce __sysctl_writable = CONFIG_OPEN_MAYEXEC_DEFAULT;
Except the position of the const, this is clearer indeed.
>
>> +
>> +/*
>> + * Handle open_mayexec_enforce sysctl
>> + */
>> +#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
>> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
>> +size_t *lenp, loff_t *ppos)
>> +{
>> +int error;
>> +
>> +if (write) {
>> +struct ctl_table table_copy;
>> +int tmp_mayexec_enforce;
>> +
>> +if (!capable(CAP_MAC_ADMIN))
>> +return -EPERM;
>> +
>> +tmp_mayexec_enforce = *((int *)table->data);
>> +table_copy = *table;
>> +/* Do not erase sysctl_omayexec_enforce. */
>> +table_copy.data = &tmp_mayexec_enforce;
>> +error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
>> +if (error)
>> +return error;
>> +
>> +if ((tmp_mayexec_enforce | _OMAYEXEC_MASK) != _OMAYEXEC_MASK)
>> +return -EINVAL;
>> +
>> +*((int
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Tue, May 5, 2020 at 11:33 AM Mickaël Salaün wrote:
>
> Enable to forbid access to files open with O_MAYEXEC. Thanks to the
> noexec option from the underlying VFS mount, or to the file execute
> permission, userspace can enforce these execution policies. This may
> allow script interpreters to check execution permission before reading
> commands from a file, or dynamic linkers to allow shared object loading.
>
> Add a new sysctl fs.open_mayexec_enforce to enable system administrators
> to enforce two complementary security policies according to the
> installed system: enforce the noexec mount option, and enforce
> executable file permission. Indeed, because of compatibility with
> installed systems, only system administrators are able to check that
> this new enforcement is in line with the system mount points and file
> permissions. A following patch adds documentation.
>
> For tailored Linux distributions, it is possible to enforce such
> restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
> The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
> CONFIG_OMAYEXEC_ENFORCE_FILE.
>
> Being able to restrict execution also enables to protect the kernel by
> restricting arbitrary syscalls that an attacker could perform with a
> crafted binary or certain script languages. It also improves multilevel
> isolation by reducing the ability of an attacker to use side channels
> with specific code. These restrictions can natively be enforced for ELF
> binaries (with the noexec mount option) but require this kernel
> extension to properly handle scripts (e.g., Python, Perl). To get a
> consistent execution policy, additional memory restrictions should also
> be enforced (e.g. thanks to SELinux).
>
> Signed-off-by: Mickaël Salaün
> Reviewed-by: Thibaut Sautereau
> Cc: Aleksa Sarai
> Cc: Al Viro
> Cc: Kees Cook
> ---
> diff --git a/fs/namei.c b/fs/namei.c
> index 33b6d372e74a..70f179f6bc6c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb, struct
> inode *inode, int mask)
> +#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
> + size_t *lenp, loff_t *ppos)
> +{
> + int error;
> +
> + if (write) {
> + struct ctl_table table_copy;
> + int tmp_mayexec_enforce;
> +
> + if (!capable(CAP_MAC_ADMIN))
> + return -EPERM;
Not fond of using CAP_MAC_ADMIN here (or elsewhere outside of security
modules). The ability to set this sysctl is not equivalent to being
able to load a MAC policy, set arbitrary MAC labels on
processes/files, etc.
> + * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
> + *
> + * @inode: Inode to check permission on
> + * @mask: Right to check for (%MAY_OPENEXEC, %MAY_EXECMOUNT, %MAY_EXEC)
> + *
> + * Returns 0 if access is permitted, -EACCES otherwise.
> + */
> +static inline int omayexec_inode_permission(struct inode *inode, int mask)
> +{
> + if (!(mask & MAY_OPENEXEC))
> + return 0;
> +
> + if ((sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT) &&
> + !(mask & MAY_EXECMOUNT))
> + return -EACCES;
> +
> + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> + return generic_permission(inode, MAY_EXEC);
> +
> + return 0;
> +}
I'm wondering if this is being done at the wrong level. I would think
that OMAYEXEC_ENFORCE_FILE would mean to check file execute permission
with respect to all mechanisms/policies, including DAC,
filesystem-specific checking (inode->i_op->permission), security
modules, etc. That requires more than just calling
generic_permission() with MAY_EXEC, which only covers the default
DAC/ACL logic; you'd need to take the handling up a level to
inode_permission() and re-map MAY_OPENEXEC to MAY_EXEC for
do_inode_permission() and security_inode_permission() at least.
Alternatively, we can modify each individual filesystem (that
implements its own i_op->permission) and security module to start
handling MAY_OPENEXEC and have them choose to remap it to a file
execute check (or not) independent of the sysctl. Not sure of your
intent. As it stands, selinux_inode_permission() will ignore the new
MAY_OPENEXEC flag until someone updates it. Likewise for Smack.
AppArmor/TOMOYO would probably need to check and handle FMODE_EXEC in
their file_open hooks since they don't implement inode_permission().
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Wed, May 13, 2020 at 11:37:16AM -0400, Stephen Smalley wrote:
> On Tue, May 5, 2020 at 11:33 AM Mickaël Salaün wrote:
> >
> > Enable to forbid access to files open with O_MAYEXEC. Thanks to the
> > noexec option from the underlying VFS mount, or to the file execute
> > permission, userspace can enforce these execution policies. This may
> > allow script interpreters to check execution permission before reading
> > commands from a file, or dynamic linkers to allow shared object loading.
> >
> > Add a new sysctl fs.open_mayexec_enforce to enable system administrators
> > to enforce two complementary security policies according to the
> > installed system: enforce the noexec mount option, and enforce
> > executable file permission. Indeed, because of compatibility with
> > installed systems, only system administrators are able to check that
> > this new enforcement is in line with the system mount points and file
> > permissions. A following patch adds documentation.
> >
> > For tailored Linux distributions, it is possible to enforce such
> > restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
> > The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
> > CONFIG_OMAYEXEC_ENFORCE_FILE.
> >
> > Being able to restrict execution also enables to protect the kernel by
> > restricting arbitrary syscalls that an attacker could perform with a
> > crafted binary or certain script languages. It also improves multilevel
> > isolation by reducing the ability of an attacker to use side channels
> > with specific code. These restrictions can natively be enforced for ELF
> > binaries (with the noexec mount option) but require this kernel
> > extension to properly handle scripts (e.g., Python, Perl). To get a
> > consistent execution policy, additional memory restrictions should also
> > be enforced (e.g. thanks to SELinux).
> >
> > Signed-off-by: Mickaël Salaün
> > Reviewed-by: Thibaut Sautereau
> > Cc: Aleksa Sarai
> > Cc: Al Viro
> > Cc: Kees Cook
> > ---
>
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 33b6d372e74a..70f179f6bc6c 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -411,10 +412,90 @@ static int sb_permission(struct super_block *sb,
> > struct inode *inode, int mask)
>
> > +#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
> > +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
> > + size_t *lenp, loff_t *ppos)
> > +{
> > + int error;
> > +
> > + if (write) {
> > + struct ctl_table table_copy;
> > + int tmp_mayexec_enforce;
> > +
> > + if (!capable(CAP_MAC_ADMIN))
> > + return -EPERM;
>
> Not fond of using CAP_MAC_ADMIN here (or elsewhere outside of security
> modules). The ability to set this sysctl is not equivalent to being
> able to load a MAC policy, set arbitrary MAC labels on
> processes/files, etc.
That's fair. In that case, perhaps this could just use the existing
_sysadmin helper? (Though I should note that these perm checks actually
need to be in the open, not the read/write ... I thought there was a
series to fix that, but I can't find it now. Regardless, that's
orthogonal to this series.)
> > + * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
> > + *
> > + * @inode: Inode to check permission on
> > + * @mask: Right to check for (%MAY_OPENEXEC, %MAY_EXECMOUNT, %MAY_EXEC)
> > + *
> > + * Returns 0 if access is permitted, -EACCES otherwise.
> > + */
> > +static inline int omayexec_inode_permission(struct inode *inode, int mask)
> > +{
> > + if (!(mask & MAY_OPENEXEC))
> > + return 0;
> > +
> > + if ((sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT) &&
> > + !(mask & MAY_EXECMOUNT))
> > + return -EACCES;
> > +
> > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > + return generic_permission(inode, MAY_EXEC);
> > +
> > + return 0;
> > +}
>
> I'm wondering if this is being done at the wrong level. I would think
> that OMAYEXEC_ENFORCE_FILE would mean to check file execute permission
> with respect to all mechanisms/policies, including DAC,
> filesystem-specific checking (inode->i_op->permission), security
> modules, etc. That requires more than just calling
> generic_permission() with MAY_EXEC, which only covers the default
> DAC/ACL logic; you'd need to take the handling up a level to
> inode_permission() and re-map MAY_OPENEXEC to MAY_EXEC for
> do_inode_permission() and security_inode_permission() at least.
Oh, yeah, that's a good point. Does this need to be a two-pass check, or
can MAY_OPENEXEC get expanded to MAY_EXEC here? Actually, why is this so
deep at all? Shouldn't this be in may_open()?
Like, couldn't just the entire thing just be:
diff --git a/fs/namei.c b/fs/namei.c
index a320371899cf..0ab18e19f5da 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2849,6 +2849,13 @@ static int
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> Like, couldn't just the entire thing just be:
>
> diff --git a/fs/namei.c b/fs/namei.c
> index a320371899cf..0ab18e19f5da 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> acc_mode, int flag)
> break;
> }
>
> + if (unlikely(mask & MAY_OPENEXEC)) {
> + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> + path_noexec(path))
> + return -EACCES;
> + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> + acc_mode |= MAY_EXEC;
> + }
> error = inode_permission(inode, MAY_OPEN | acc_mode);
> if (error)
> return error;
>
FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
reduced to this plus the Kconfig and sysctl changes, the self tests
pass.
I think this makes things much cleaner and correct.
--
Kees Cook
RE: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
From: Kees Cook
> Sent: 14 May 2020 04:05
> On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > Like, couldn't just the entire thing just be:
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index a320371899cf..0ab18e19f5da 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > acc_mode, int flag)
> > break;
> > }
> >
> > + if (unlikely(mask & MAY_OPENEXEC)) {
> > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > + path_noexec(path))
> > + return -EACCES;
> > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > + acc_mode |= MAY_EXEC;
> > + }
> > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > if (error)
> > return error;
> >
>
> FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> reduced to this plus the Kconfig and sysctl changes, the self tests
> pass.
>
> I think this makes things much cleaner and correct.
And a summary of that would be right for the 0/n patch email.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
>
> On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > Like, couldn't just the entire thing just be:
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index a320371899cf..0ab18e19f5da 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > acc_mode, int flag)
> > break;
> > }
> >
> > + if (unlikely(mask & MAY_OPENEXEC)) {
> > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > + path_noexec(path))
> > + return -EACCES;
> > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > + acc_mode |= MAY_EXEC;
> > + }
> > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > if (error)
> > return error;
> >
>
> FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> reduced to this plus the Kconfig and sysctl changes, the self tests
> pass.
>
> I think this makes things much cleaner and correct.
I think that covers inode-based security modules but not path-based
ones (they don't implement the inode_permission hook). For those, I
would tentatively guess that we need to make sure FMODE_EXEC is set on
the open file and then they need to check for that in their file_open
hooks.
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
> >
> > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > Like, couldn't just the entire thing just be:
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index a320371899cf..0ab18e19f5da 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > > acc_mode, int flag)
> > > break;
> > > }
> > >
> > > + if (unlikely(mask & MAY_OPENEXEC)) {
> > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > + path_noexec(path))
> > > + return -EACCES;
> > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > + acc_mode |= MAY_EXEC;
> > > + }
> > > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > > if (error)
> > > return error;
> > >
> >
> > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > reduced to this plus the Kconfig and sysctl changes, the self tests
> > pass.
> >
> > I think this makes things much cleaner and correct.
>
> I think that covers inode-based security modules but not path-based
> ones (they don't implement the inode_permission hook). For those, I
> would tentatively guess that we need to make sure FMODE_EXEC is set on
> the open file and then they need to check for that in their file_open
> hooks.
Does there need to be an FMODE_OPENEXEC, or is the presence of
FMODE_OPEN with FMODE_EXEC sufficient?
--
Kees Cook
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
> >
> > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > Like, couldn't just the entire thing just be:
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index a320371899cf..0ab18e19f5da 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > > acc_mode, int flag)
> > > break;
> > > }
> > >
> > > + if (unlikely(mask & MAY_OPENEXEC)) {
> > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > + path_noexec(path))
> > > + return -EACCES;
> > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > + acc_mode |= MAY_EXEC;
> > > + }
> > > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > > if (error)
> > > return error;
> > >
> >
> > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > reduced to this plus the Kconfig and sysctl changes, the self tests
> > pass.
> >
> > I think this makes things much cleaner and correct.
>
> I think that covers inode-based security modules but not path-based
> ones (they don't implement the inode_permission hook). For those, I
> would tentatively guess that we need to make sure FMODE_EXEC is set on
> the open file and then they need to check for that in their file_open
> hooks.
I kept confusing myself about what order things happened in, so I made
these handy notes about the call graph:
openat2(dfd, char * filename, open_how)
do_filp_open(dfd, filename, open_flags)
path_openat(nameidata, open_flags, flags)
do_open(nameidata, file, open_flags)
may_open(path, acc_mode, open_flag)
inode_permission(inode, MAY_OPEN | acc_mode)
security_inode_permission(inode, acc_mode)
vfs_open(path, file)
do_dentry_open(file, path->dentry->d_inode, open)
if (unlikely(f->f_flags & FMODE_EXEC &&
!S_ISREG(inode->i_mode))) ...
security_file_open(f)
open()
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
--
Kees Cook
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Thu, May 14, 2020 at 10:41 AM Kees Cook wrote:
>
> On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> > On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
> > >
> > > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > > Like, couldn't just the entire thing just be:
> > > >
> > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > index a320371899cf..0ab18e19f5da 100644
> > > > --- a/fs/namei.c
> > > > +++ b/fs/namei.c
> > > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > > > acc_mode, int flag)
> > > > break;
> > > > }
> > > >
> > > > + if (unlikely(mask & MAY_OPENEXEC)) {
> > > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > > + path_noexec(path))
> > > > + return -EACCES;
> > > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > > + acc_mode |= MAY_EXEC;
> > > > + }
> > > > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > > > if (error)
> > > > return error;
> > > >
> > >
> > > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > > reduced to this plus the Kconfig and sysctl changes, the self tests
> > > pass.
> > >
> > > I think this makes things much cleaner and correct.
> >
> > I think that covers inode-based security modules but not path-based
> > ones (they don't implement the inode_permission hook). For those, I
> > would tentatively guess that we need to make sure FMODE_EXEC is set on
> > the open file and then they need to check for that in their file_open
> > hooks.
>
> Does there need to be an FMODE_OPENEXEC, or is the presence of
> FMODE_OPEN with FMODE_EXEC sufficient?
I don't think we need an extra flag/mode bit. But note that 1)
FMODE_OPENED isn't set until after security_file_open() is called so
we can't rely on it there, 2) __FMODE_EXEC aka FMODE_EXEC is set in
f_flags not f_mode, 3) FMODE_EXEC was originally introduced for
distributed filesystems so that they could return ETXTBUSY if the file
was opened for write and execute on different nodes, 4) AppArmor and
TOMOYO have special handling of execve based on current->in_execve so
I guess the only overlap would be for uselib(2).
Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
On Thu, May 14, 2020 at 11:45 AM Kees Cook wrote:
>
> On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> > On Wed, May 13, 2020 at 11:05 PM Kees Cook wrote:
> > >
> > > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > > Like, couldn't just the entire thing just be:
> > > >
> > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > index a320371899cf..0ab18e19f5da 100644
> > > > --- a/fs/namei.c
> > > > +++ b/fs/namei.c
> > > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int
> > > > acc_mode, int flag)
> > > > break;
> > > > }
> > > >
> > > > + if (unlikely(mask & MAY_OPENEXEC)) {
> > > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > > + path_noexec(path))
> > > > + return -EACCES;
> > > > + if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > > + acc_mode |= MAY_EXEC;
> > > > + }
> > > > error = inode_permission(inode, MAY_OPEN | acc_mode);
> > > > if (error)
> > > > return error;
> > > >
> > >
> > > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > > reduced to this plus the Kconfig and sysctl changes, the self tests
> > > pass.
> > >
> > > I think this makes things much cleaner and correct.
> >
> > I think that covers inode-based security modules but not path-based
> > ones (they don't implement the inode_permission hook). For those, I
> > would tentatively guess that we need to make sure FMODE_EXEC is set on
> > the open file and then they need to check for that in their file_open
> > hooks.
>
> I kept confusing myself about what order things happened in, so I made
> these handy notes about the call graph:
>
> openat2(dfd, char * filename, open_how)
> do_filp_open(dfd, filename, open_flags)
> path_openat(nameidata, open_flags, flags)
> do_open(nameidata, file, open_flags)
> may_open(path, acc_mode, open_flag)
> inode_permission(inode, MAY_OPEN | acc_mode)
> security_inode_permission(inode, acc_mode)
> vfs_open(path, file)
> do_dentry_open(file, path->dentry->d_inode, open)
> if (unlikely(f->f_flags & FMODE_EXEC &&
> !S_ISREG(inode->i_mode))) ...
> security_file_open(f)
> open()
>
> 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?
How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
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 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. 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! ;) -- Kees Cook
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
On 19/05/2020 04:23, Aleksa Sarai wrote:
> On 2020-05-15, Kees Cook wrote:
>> On Fri, May 15, 2020 at 04:43:37PM +0200, Florian Weimer wrote:
>>> * Kees Cook:
>>>
On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
> * Kees Cook:
>
>> 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 think POSIX specifies O_EXEC in such a way that it does not confer
> read permissions. This seems incompatible with what we are trying to
> achieve here.
I was trying to retain this behavior, since we already make this
distinction between execve() and uselib() with the MAY_* flags:
execve():
struct open_flags open_exec_flags = {
.open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
.acc_mode = MAY_EXEC,
uselib():
static const struct open_flags uselib_flags = {
.open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
.acc_mode = MAY_READ | MAY_EXEC,
I tried to retain this in my proposal, in the O_EXEC does not imply
MAY_READ:
>>>
>>> That doesn't quite parse for me, sorry.
>>>
>>> The point is that the script interpreter actually needs to *read* those
>>> files in order to execute them.
>>
>> I think I misunderstood what you meant (Mickaël got me sorted out
>> now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
>> then yes, this new flag can't be O_EXEC. I was reading the glibc
>> documentation (which treats it as a permission bit flag, not POSIX,
>> which treats it as a complete mode description).
>
> On the other hand, if we had O_EXEC (or O_EXONLY a-la O_RDONLY) then the
> interpreter could re-open the file descriptor as O_RDONLY after O_EXEC
> succeeds. Not ideal, but I don't think it's a deal-breaker.
>
> Regarding O_MAYEXEC, I do feel a little conflicted.
>
> I do understand that its goal is not to be what O_EXEC was supposed to
> be (which is loosely what O_PATH has effectively become), so I think
> that this is not really a huge problem -- especially since you could
> just do O_MAYEXEC|O_PATH if you wanted to disallow reading explicitly.
> It would be nice to have an O_EXONLY concept, but it's several decades
> too late to make it mandatory (and making it optional has questionable
> utility IMHO).
>
> However, the thing I still feel mildly conflicted about is the sysctl. I
> do understand the argument for it (ultimately, whether O_MAYEXEC is
> usable on a system depends on the distribution) but it means that any
> program which uses O_MAYEXEC cannot rely on it to provide the security
> guarantees they expect. Even if the program goes and reads the sysctl
> value, it could change underneath them. If this is just meant to be a
> best-effort protection then this doesn't matter too much, but I just
> feel uneasy about these kinds of best-effort protections.
I think there is a cognitive bias here. There is a difference between
application-centric policies and system policies. For example, openat2
RESOLVE_* flags targets application developers and are self-sufficient:
the kernel provides features (applied to FDs, owned and managed by user
space) which must be known (by the application) to be supported (by the
kernel), otherwise the application may give more privileges than
expected. However, the O_MAYEXEC flag targets system administrators: it
does not make sense to enable an application to know nor enforce the
system(-wide) policy, but only to enable applications to follow this
policy (i.e. best-effort *from the application developer point of
view*). Indeed, access-control such as file executability depends on
multiple layers (e.g. file permission, mount options, ACL, SELinux
policy), most of them managed and enforced in a consistent way by
(multiple parts of) the system.
Applications should not and it does not make sense for them to expect
anything from O_MAYEXEC. This flag only enables the system to enforce a
security policy and that's all. It is really a different use case than
FD management. This feature is meant to extend the system ability thanks
to applications collaboration. Here the sysctl should not be looked at
by applications, the same way an application should not look at the
currently enforced SELinux policy nor the mount options. An application
may be launched differently according to the system-wide policy, but
this is again a system configuration. There is a difference between ABI
compatibility (i.e. does this feature is supported by the kernel?) and
system-wide security policy (what
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: > 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 think POSIX specifies O_EXEC in such a way that it does not confer read permissions. This seems incompatible with what we are trying to achieve here. Thanks, Florian
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
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 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. > > 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. 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. 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. Other than that, the other commits are interesting. I'm a bit worried about the implication of the f_flags/f_mode change though. >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).
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
> * Kees Cook:
>
> > 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 think POSIX specifies O_EXEC in such a way that it does not confer
> read permissions. This seems incompatible with what we are trying to
> achieve here.
I was trying to retain this behavior, since we already make this
distinction between execve() and uselib() with the MAY_* flags:
execve():
struct open_flags open_exec_flags = {
.open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
.acc_mode = MAY_EXEC,
uselib():
static const struct open_flags uselib_flags = {
.open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
.acc_mode = MAY_READ | MAY_EXEC,
I tried to retain this in my proposal, in the O_EXEC does not imply
MAY_READ:
+ /* Should execution permissions be checked on open? */
+ if (flags & O_EXEC) {
+ flags |= __FMODE_EXEC;
+ acc_mode |= MAY_EXEC;
+ }
--
Kees Cook
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 10:43:34AM +0200, Florian Weimer wrote:
>> * Kees Cook:
>>
>> > 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 think POSIX specifies O_EXEC in such a way that it does not confer
>> read permissions. This seems incompatible with what we are trying to
>> achieve here.
>
> I was trying to retain this behavior, since we already make this
> distinction between execve() and uselib() with the MAY_* flags:
>
> execve():
> struct open_flags open_exec_flags = {
> .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> .acc_mode = MAY_EXEC,
>
> uselib():
> static const struct open_flags uselib_flags = {
> .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> .acc_mode = MAY_READ | MAY_EXEC,
>
> I tried to retain this in my proposal, in the O_EXEC does not imply
> MAY_READ:
That doesn't quite parse for me, sorry.
The point is that the script interpreter actually needs to *read* those
files in order to execute them.
Thanks,
Florian
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
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 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 i
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
On Fri, May 15, 2020 at 04:43:37PM +0200, Florian Weimer wrote:
> * Kees Cook:
>
> > On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
> >> * Kees Cook:
> >>
> >> > 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 think POSIX specifies O_EXEC in such a way that it does not confer
> >> read permissions. This seems incompatible with what we are trying to
> >> achieve here.
> >
> > I was trying to retain this behavior, since we already make this
> > distinction between execve() and uselib() with the MAY_* flags:
> >
> > execve():
> > struct open_flags open_exec_flags = {
> > .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> > .acc_mode = MAY_EXEC,
> >
> > uselib():
> > static const struct open_flags uselib_flags = {
> > .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> > .acc_mode = MAY_READ | MAY_EXEC,
> >
> > I tried to retain this in my proposal, in the O_EXEC does not imply
> > MAY_READ:
>
> That doesn't quite parse for me, sorry.
>
> The point is that the script interpreter actually needs to *read* those
> files in order to execute them.
I think I misunderstood what you meant (Mickaël got me sorted out
now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
then yes, this new flag can't be O_EXEC. I was reading the glibc
documentation (which treats it as a permission bit flag, not POSIX,
which treats it as a complete mode description).
--
Kees Cook
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
On 15/05/2020 17:46, Kees Cook wrote: > 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 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 that the different points of view should be the following: - kernel developer: the app *may* behave this way; - user space developer: the app *should* behave this way; - sysadmin: the app *must* behave this way (either enforcing O_MAYEXEC or not). The idea is to push adoption of O_MAYEXEC to upstream interpreters, knowing that it will not break anything on running systems which do not care about this features. However, on systems which want this feature enforced, there will be knowledgeable peoples (i.e. sysadmins who enforced O_MAYEXEC deliberately) to manage it. If we don't give the opportunity to sysadmins to control this feature, no upstream interpreters will adopt it. Only tailored distro will use it, maybe with custom LSM or other way to enforce it anyway, and having a sysctl or not is not an issue neither. I think it would be a missed opportunity to help harden most Linux systems. > > 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. Yes, that is our intention: to make this flag used by most interpreters, without breaking any existing systems. > (Though in that case, I suspect it needs to be tied to userns or > something to support containers with different enforcement levels.) The LSMs already manage security policies, but the sysctl could indeed be ti
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: > I think I misunderstood what you meant (Mickaël got me sorted out > now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE", > then yes, this new flag can't be O_EXEC. I was reading the glibc > documentation (which treats it as a permission bit flag, not POSIX, > which treats it as a complete mode description). I see. I think this part of the manual is actually very Hurd-specific (before the O_ACCMODE description). I'll see if I can make this clearer in the markup. Thanks, Florian
Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)
On 2020-05-15, Kees Cook wrote:
> On Fri, May 15, 2020 at 04:43:37PM +0200, Florian Weimer wrote:
> > * Kees Cook:
> >
> > > On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
> > >> * Kees Cook:
> > >>
> > >> > 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 think POSIX specifies O_EXEC in such a way that it does not confer
> > >> read permissions. This seems incompatible with what we are trying to
> > >> achieve here.
> > >
> > > I was trying to retain this behavior, since we already make this
> > > distinction between execve() and uselib() with the MAY_* flags:
> > >
> > > execve():
> > > struct open_flags open_exec_flags = {
> > > .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> > > .acc_mode = MAY_EXEC,
> > >
> > > uselib():
> > > static const struct open_flags uselib_flags = {
> > > .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> > > .acc_mode = MAY_READ | MAY_EXEC,
> > >
> > > I tried to retain this in my proposal, in the O_EXEC does not imply
> > > MAY_READ:
> >
> > That doesn't quite parse for me, sorry.
> >
> > The point is that the script interpreter actually needs to *read* those
> > files in order to execute them.
>
> I think I misunderstood what you meant (Mickaël got me sorted out
> now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
> then yes, this new flag can't be O_EXEC. I was reading the glibc
> documentation (which treats it as a permission bit flag, not POSIX,
> which treats it as a complete mode description).
On the other hand, if we had O_EXEC (or O_EXONLY a-la O_RDONLY) then the
interpreter could re-open the file descriptor as O_RDONLY after O_EXEC
succeeds. Not ideal, but I don't think it's a deal-breaker.
Regarding O_MAYEXEC, I do feel a little conflicted.
I do understand that its goal is not to be what O_EXEC was supposed to
be (which is loosely what O_PATH has effectively become), so I think
that this is not really a huge problem -- especially since you could
just do O_MAYEXEC|O_PATH if you wanted to disallow reading explicitly.
It would be nice to have an O_EXONLY concept, but it's several decades
too late to make it mandatory (and making it optional has questionable
utility IMHO).
However, the thing I still feel mildly conflicted about is the sysctl. I
do understand the argument for it (ultimately, whether O_MAYEXEC is
usable on a system depends on the distribution) but it means that any
program which uses O_MAYEXEC cannot rely on it to provide the security
guarantees they expect. Even if the program goes and reads the sysctl
value, it could change underneath them. If this is just meant to be a
best-effort protection then this doesn't matter too much, but I just
feel uneasy about these kinds of best-effort protections.
I do wonder if we could require that fexecve(3) can only be done with
file descriptors that have been opened with O_MAYEXEC (obviously this
would also need to be a sysctl -- *sigh*). This would tie in to some of
the magic-link changes I wanted to push (namely, upgrade_mask).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
