On 12/05/2020 23:48, Kees Cook wrote:

On Tue, May 05, 2020 at 05:31:53PM +0200, Mickaël Salaün wrote:

quoted

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

quoted

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.5924@namei.org/ (local)
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...

quoted

[...]

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

quoted

+#define OMAYEXEC_ENFORCE_MOUNT	(1 << 0)
+#define OMAYEXEC_ENFORCE_FILE	(1 << 1)

Please use BIT(0), BIT(1)...

quoted

+#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.

quoted

+
+/*
+ * 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

quoted

+
+/**
+ * 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?

quoted

+
 /**
  * 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.

quoted

 
 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

quoted

+	},
+#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.