LinuxLists.cc - [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O

[permalink] [raw]

Subject: 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"
>
>

2020-05-05 17:42:28

by Randy Dunlap

[permalink] [raw]

Subject: 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

2020-05-12 21:51:15

[permalink] [raw]

Subject: 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 <linux/bitops.h>
> #include <linux/init_task.h>
> #include <linux/uaccess.h>
> +#include <linux/sysctl.h>
>
> #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_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 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;
> +}

More naming nits: I think this should be called may_openexec() to match
the other may_*() functions.

> +
> /**
> * inode_permission - Check for access rights to a given inode
> * @inode: Inode to check permission on
> - * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
> + * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, %MAY_OPENEXEC,
> + * %MAY_EXECMOUNT)
> *
> * Check for read/write/execute permissions on an inode. We use fs[ug]id for
> * this, letting us set arbitrary permissions for filesystem access without
> @@ -454,6 +535,10 @@ int inode_permission(struct inode *inode, int mask)
> if (retval)
> return retval;
>
> + retval = omayexec_inode_permission(inode, mask);
> + if (retval)
> + return retval;
> +
> return security_inode_permission(inode, mask);
> }
> EXPORT_SYMBOL(inode_permission);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 79435fca6c3e..39c80a64d054 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -83,6 +83,9 @@ extern int sysctl_protected_symlinks;
> extern int sysctl_protected_hardlinks;
> extern int sysctl_protected_fifos;
> extern int sysctl_protected_regular;
> +#ifndef CONFIG_OMAYEXEC_STATIC
> +extern int sysctl_omayexec_enforce;
> +#endif

Now there's no need to wrap this in ifdef.

>
> typedef __kernel_rwf_t rwf_t;
>
> @@ -3545,6 +3548,8 @@ int proc_nr_dentry(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos);
> int proc_nr_inodes(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos);
> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
> + size_t *lenp, loff_t *ppos);
> int __init get_filesystem_list(char *buf);
>
> #define __FMODE_EXEC ((__force int) FMODE_EXEC)
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8a176d8727a3..29bbf79f444c 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1892,6 +1892,15 @@ static struct ctl_table fs_table[] = {
> .extra1 = SYSCTL_ZERO,
> .extra2 = &two,
> },
> +#ifndef CONFIG_OMAYEXEC_STATIC
> + {
> + .procname = "open_mayexec_enforce",
> + .data = &sysctl_omayexec_enforce,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_omayexec,

This can just be min/max of 0/3 with a new macadmin handler.

> + },
> +#endif
> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
> {
> .procname = "binfmt_misc",
> 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.
> +
> + 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"
> --
> 2.26.2
>

Otherwise, yeah, the intent here looks good to me.

--
Kees Cook

2020-05-13 11:25:57

[permalink] [raw]

Subject: 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/[email protected]/
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 <linux/bitops.h>
>> #include <linux/init_task.h>
>> #include <linux/uaccess.h>
>> +#include <linux/sysctl.h>
>>
>> #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_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;

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

OK

>
>> +
>> +/**
>> + * 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;
>> +}
>
> More naming nits: I think this should be called may_openexec() to match
> the other may_*() functions.

Other *_inode_permission() functions have a similar meaning and the same
signature. The may_*() functions have various signatures. What do the
filesystem folks prefer?

>
>> +
>> /**
>> * inode_permission - Check for access rights to a given inode
>> * @inode: Inode to check permission on
>> - * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
>> + * @mask: Right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, %MAY_OPENEXEC,
>> + * %MAY_EXECMOUNT)
>> *
>> * Check for read/write/execute permissions on an inode. We use fs[ug]id for
>> * this, letting us set arbitrary permissions for filesystem access without
>> @@ -454,6 +535,10 @@ int inode_permission(struct inode *inode, int mask)
>> if (retval)
>> return retval;
>>
>> + retval = omayexec_inode_permission(inode, mask);
>> + if (retval)
>> + return retval;
>> +
>> return security_inode_permission(inode, mask);
>> }
>> EXPORT_SYMBOL(inode_permission);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 79435fca6c3e..39c80a64d054 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -83,6 +83,9 @@ extern int sysctl_protected_symlinks;
>> extern int sysctl_protected_hardlinks;
>> extern int sysctl_protected_fifos;
>> extern int sysctl_protected_regular;
>> +#ifndef CONFIG_OMAYEXEC_STATIC
>> +extern int sysctl_omayexec_enforce;
>> +#endif
>
> Now there's no need to wrap this in ifdef.

Right, if the sysctl can't be disabled with a kernel configuration.

>
>>
>> typedef __kernel_rwf_t rwf_t;
>>
>> @@ -3545,6 +3548,8 @@ int proc_nr_dentry(struct ctl_table *table, int write,
>> void __user *buffer, size_t *lenp, loff_t *ppos);
>> int proc_nr_inodes(struct ctl_table *table, int write,
>> void __user *buffer, size_t *lenp, loff_t *ppos);
>> +int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
>> + size_t *lenp, loff_t *ppos);
>> int __init get_filesystem_list(char *buf);
>>
>> #define __FMODE_EXEC ((__force int) FMODE_EXEC)
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 8a176d8727a3..29bbf79f444c 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1892,6 +1892,15 @@ static struct ctl_table fs_table[] = {
>> .extra1 = SYSCTL_ZERO,
>> .extra2 = &two,
>> },
>> +#ifndef CONFIG_OMAYEXEC_STATIC
>> + {
>> + .procname = "open_mayexec_enforce",
>> + .data = &sysctl_omayexec_enforce,
>> + .maxlen = sizeof(int),
>> + .mode = 0600,
>> + .proc_handler = proc_omayexec,
>
> This can just be min/max of 0/3 with a new macadmin handler.

OK

>
>> + },
>> +#endif
>> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
>> {
>> .procname = "binfmt_misc",
>> 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.
>> +
>> + 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"
>> --
>> 2.26.2
>>
>
> Otherwise, yeah, the intent here looks good to me.
>

2020-05-13 15:41:35

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]>
> Reviewed-by: Thibaut Sautereau <[email protected]>
> Cc: Aleksa Sarai <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> ---

> 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)
<snip>
> +#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().

2020-05-13 23:29:38

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]>
> > Reviewed-by: Thibaut Sautereau <[email protected]>
> > Cc: Aleksa Sarai <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > ---
>
> > 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)
> <snip>
> > +#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 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;

> 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

Eek, no, this should be centralized in the VFS, not per-filesystem, but
I do see that it might be possible for a filesystem to actually do the
MAY_OPENEXEC test internally, so the two-pass check wouldn't be needed.
But... I think that can't happen until _everything_ can do the single
pass check, so we always have to make the second call too.

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

Is there any need to teach anything about MAY_OPENEXEC? It'll show up
for the LSMs as (MAY_OPEN | MAY_EXEC).

--
Kees Cook

2020-05-14 03:06:56

[permalink] [raw]

Subject: 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

2020-05-14 10:14:44

by David Laight

[permalink] [raw]

Subject: 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)

2020-05-14 12:24:00

[permalink] [raw]

Subject: 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 <[email protected]> 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.

2020-05-14 14:44:11

[permalink] [raw]

Subject: 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 <[email protected]> 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

2020-05-14 15:47:21

[permalink] [raw]

Subject: 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 <[email protected]> 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

2020-05-14 15:54:07

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]> 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).

2020-05-14 16:13:27

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]> 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?

2020-05-14 19:18:07

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]> 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.

2020-05-14 19:23:26

[permalink] [raw]

Subject: 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 <[email protected]> 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 <[email protected]>
>>> Reviewed-by: Thibaut Sautereau <[email protected]>
>>> Cc: Aleksa Sarai <[email protected]>
>>> Cc: Al Viro <[email protected]>
>>> Cc: Kees Cook <[email protected]>
>>> ---
>>
>>> 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)
>> <snip>
>>> +#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().

2020-05-15 01:39:55

by Tetsuo Handa

[permalink] [raw]

Subject: 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.

2020-05-15 08:05:39

[permalink] [raw]

Subject: 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 <[email protected]> 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

2020-05-15 08:45:50

by Florian Weimer

[permalink] [raw]

Subject: 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

2020-05-15 11:08:33

[permalink] [raw]

Subject: 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 <[email protected]> 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).

2020-05-15 14:39:13

[permalink] [raw]

Subject: 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

2020-05-15 14:45:39

by Florian Weimer

[permalink] [raw]

Subject: 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

2020-05-15 15:49:25

[permalink] [raw]

Subject: 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 <[email protected]> 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

2020-05-15 15:54:19

[permalink] [raw]

Subject: 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

2020-05-15 18:28:29

[permalink] [raw]

Subject: 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 <[email protected]> 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 tied to mount namespaces. This is interesting, but it requires more
complexity. We can start by the current sysctl's bits to manage all the
mount namespaces.

>
>>> 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?"
> :)
>

Good :)

2020-05-18 07:28:53

by Florian Weimer

[permalink] [raw]

Subject: 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

2020-05-19 02:26:12

by Aleksa Sarai

[permalink] [raw]

Subject: 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 <[email protected]> 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
<https://www.cyphar.com/>

Attachments:

(No filename) (3.65 kB)
signature.asc (235.00 B)
Download all attachments

2020-05-19 10:15:25