[PATCH v5 0/6] Add support for O_MAYEXEC

[PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

Hi,

This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
configure the security policy at kernel build time.  As requested by
Mimi Zohar, I completed the series with one of her patches for IMA.

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.

A simple system-wide security policy can be enforced by the system
administrator through a sysctl configuration consistent with the mount
points or the file access rights.  The documentation patch explains the
prerequisites.

Furthermore, the security policy can also be delegated to an LSM, either
a MAC system or an integrity system.  For instance, the new kernel
MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
integrity gap by bringing the ability to check the use of scripts [1].
Other uses are expected, such as for openat2(2) [2], SGX integration
[3], bpffs [4] or IPE [5].

Userspace needs to adapt to take advantage of this new feature.  For
example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
extended with policy enforcement points related to code interpretation,
which can be used to align with the PowerShell audit features.
Additional Python security improvements (e.g. a limited interpreter
withou -c, stdin piping of code) are on their way.

The initial idea come from CLIP OS 4 and the original implementation has
been used for more than 12 years:
https://github.com/clipos-archive/clipos4_doc

An introduction to O_MAYEXEC was given at the Linux Security Summit
Europe 2018 - Linux Kernel Security Contributions by ANSSI:
https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
The "write xor execute" principle was explained at Kernel Recipes 2018 -
CLIP OS: a defense-in-depth OS:
https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s

This patch series can be applied on top of v5.7-rc4.  This can be tested
with CONFIG_SYSCTL.  I would really appreciate constructive comments on
this patch series.

Previous version:
https://lore.kernel.org/lkml/20200428175129.634352-1-mic@digikod.net/


[1] https://lore.kernel.org/lkml/1544647356.4028.105.camel@linux.ibm.com/
[2] https://lore.kernel.org/lkml/20190904201933.10736-6-cyphar@cyphar.com/
[3] https://lore.kernel.org/lkml/CALCETrVovr8XNZSroey7pHF46O=kj_c5D9K8h=z2T_cNrpvMig@mail.gmail.com/
[4] https://lore.kernel.org/lkml/CALCETrVeZ0eufFXwfhtaG_j+AdvbzEWE0M3wjXMWVEO7pj+xkw@mail.gmail.com/
[5] https://lore.kernel.org/lkml/20200406221439.1469862-12-deven.desai@linux.microsoft.com/
[6] https://www.python.org/dev/peps/pep-0578/

Regards,

Mickaël Salaün (5):
  fs: Add support for an O_MAYEXEC flag on openat2(2)
  fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property
  fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC
  selftest/openat2: Add tests for O_MAYEXEC enforcing
  doc: Add documentation for the fs.open_mayexec_enforce sysctl

Mimi Zohar (1):
  ima: add policy support for the new file open MAY_OPENEXEC flag

 Documentation/ABI/testing/ima_policy          |   2 +-
 Documentation/admin-guide/sysctl/fs.rst       |  44 +++
 fs/fcntl.c                                    |   2 +-
 fs/namei.c                                    |  89 ++++-
 fs/open.c                                     |   8 +
 include/linux/fcntl.h                         |   2 +-
 include/linux/fs.h                            |   9 +
 include/uapi/asm-generic/fcntl.h              |   7 +
 kernel/sysctl.c                               |   9 +
 security/Kconfig                              |  26 ++
 security/integrity/ima/ima_main.c             |   3 +-
 security/integrity/ima/ima_policy.c           |  15 +-
 tools/testing/selftests/kselftest_harness.h   |   3 +
 tools/testing/selftests/openat2/Makefile      |   3 +-
 tools/testing/selftests/openat2/config        |   1 +
 tools/testing/selftests/openat2/helpers.h     |   1 +
 .../testing/selftests/openat2/omayexec_test.c | 330 ++++++++++++++++++
 17 files changed, 544 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/openat2/config
 create mode 100644 tools/testing/selftests/openat2/omayexec_test.c

-- 
2.26.2

[PATCH v5 1/6] fs: Add support for an O_MAYEXEC flag on openat2(2)

[Mickaël Salaün]

When the O_MAYEXEC flag is passed, openat2(2) may be subject to
additional restrictions depending on a security policy managed by the
kernel through a sysctl or implemented by an LSM thanks to the
inode_permission hook.  This new flag is ignored by open(2) and
openat(2).

The underlying idea is to be able to restrict scripts interpretation
according to a policy defined by the system administrator.  For this to
be possible, script interpreters must use the O_MAYEXEC flag
appropriately.  To be fully effective, these interpreters also need to
handle the other ways to execute code: command line parameters (e.g.,
option -e for Perl), module loading (e.g., option -m for Python), stdin,
file sourcing, environment variables, configuration files, etc.
According to the threat model, it may be acceptable to allow some script
interpreters (e.g. Bash) to interpret commands from stdin, may it be a
TTY or a pipe, because it may not be enough to (directly) perform
syscalls.  Further documentation can be found in a following patch.

A simple security policy implementation, configured through a dedicated
sysctl, is available in a following patch.

This is an updated subset of the patch initially written by Vincent
Strubel for CLIP OS 4:
https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
This patch has been used for more than 11 years with customized script
interpreters.  Some examples (with the original name O_MAYEXEC) can be
found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
Reviewed-by: Deven Bowers <deven.desai@linux.microsoft.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
---

Changes since v3:
* Switch back to O_MAYEXEC, but only handle it with openat2(2) which
  checks unknown flags (suggested by Aleksa Sarai). Cf.
  https://lore.kernel.org/lkml/20200430015429.wuob7m5ofdewubui@yavin.dot.cyphar.com/

Changes since v2:
* Replace O_MAYEXEC with RESOLVE_MAYEXEC from openat2(2).  This change
  enables to not break existing application using bogus O_* flags that
  may be ignored by current kernels by using a new dedicated flag, only
  usable through openat2(2) (suggested by Jeff Layton).  Using this flag
  will results in an error if the running kernel does not support it.
  User space needs to manage this case, as with other RESOLVE_* flags.
  The best effort approach to security (for most common distros) will
  simply consists of ignoring such an error and retry without
  RESOLVE_MAYEXEC.  However, a fully controlled system may which to
  error out if such an inconsistency is detected.

Changes since v1:
* Set __FMODE_EXEC when using O_MAYEXEC to make this information
  available through the new fanotify/FAN_OPEN_EXEC event (suggested by
  Jan Kara and Matthew Bobrowski).
---
 fs/fcntl.c                       | 2 +-
 fs/open.c                        | 8 ++++++++
 include/linux/fcntl.h            | 2 +-
 include/linux/fs.h               | 2 ++
 include/uapi/asm-generic/fcntl.h | 7 +++++++
 5 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 2e4c0fa2074b..0357ad667563 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1033,7 +1033,7 @@ static int __init fcntl_init(void)
 	 * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
 	 * is defined as O_NONBLOCK on some platforms and not on others.
 	 */
-	BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+	BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
 		HWEIGHT32(
 			(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
 			__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/open.c b/fs/open.c
index 719b320ede52..f3f08a36d1d2 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -962,6 +962,8 @@ inline struct open_how build_open_how(int flags, umode_t mode)
 		.mode = mode & S_IALLUGO,
 	};
 
+	/* O_MAYEXEC is ignored by syscalls relying on build_open_how(). */
+	how.flags &= ~O_MAYEXEC;
 	/* O_PATH beats everything else. */
 	if (how.flags & O_PATH)
 		how.flags &= O_PATH_FLAGS;
@@ -1029,6 +1031,12 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
 	if (flags & __O_SYNC)
 		flags |= O_DSYNC;
 
+	/* Checks execution permissions on open. */
+	if (flags & O_MAYEXEC) {
+		acc_mode |= MAY_OPENEXEC;
+		flags |= __FMODE_EXEC;
+	}
+
 	op->open_flag = flags;
 
 	/* O_TRUNC implies we need access checks for write permissions */
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 7bcdcf4f6ab2..e188a360fa5f 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
 	(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
 	 O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
 	 FASYNC	| O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
-	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+	 O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_MAYEXEC)
 
 /* List of all valid flags for the how->upgrade_mask argument: */
 #define VALID_UPGRADE_FLAGS \
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f6f59b4f22a..313c934de9ee 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -101,6 +101,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_CHDIR		0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK		0x00000080
+/* the inode is opened with O_MAYEXEC */
+#define MAY_OPENEXEC		0x00000100
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..bca90620119f 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,13 @@
 #define O_NDELAY	O_NONBLOCK
 #endif
 
+/*
+ * Code execution from file is intended, checks such permission.  A simple
+ * policy can be enforced system-wide as explained in
+ * Documentation/admin-guide/sysctl/fs.rst .
+ */
+#define O_MAYEXEC	040000000
+
 #define F_DUPFD		0	/* dup */
 #define F_GETFD		1	/* get close_on_exec */
 #define F_SETFD		2	/* set/clear close_on_exec */
-- 
2.26.2

[PATCH v5 2/6] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property

[Mickaël Salaün]

This new MAY_EXECMOUNT flag enables to check if the underlying mount
point of an inode is marked as executable.  This is useful to implement
a security policy taking advantage of the noexec mount option.

This flag is set according to path_noexec(), which checks if a mount
point is mounted with MNT_NOEXEC or if the underlying superblock is
SB_I_NOEXEC.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
---
 fs/namei.c         | 2 ++
 include/linux/fs.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index a320371899cf..33b6d372e74a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2849,6 +2849,8 @@ static int may_open(const struct path *path, int acc_mode, int flag)
 		break;
 	}
 
+	/* Pass the mount point executability. */
+	acc_mode |= path_noexec(path) ? 0 : MAY_EXECMOUNT;
 	error = inode_permission(inode, MAY_OPEN | acc_mode);
 	if (error)
 		return error;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 313c934de9ee..79435fca6c3e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_NOT_BLOCK		0x00000080
 /* the inode is opened with O_MAYEXEC */
 #define MAY_OPENEXEC		0x00000100
+/* the mount point is marked as executable */
+#define MAY_EXECMOUNT		0x00000200
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
-- 
2.26.2

[PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Mickaël Salaün]

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 <mic@digikod.net>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
---

Changes since v4:
* Add kernel configuration options to enforce O_MAYEXEC at build time,
  and disable the sysctl in such case (requested by James Morris).
* Reword commit message.

Changes since v3:
* Update comment with O_MAYEXEC.

Changes since v2:
* Cosmetic changes.

Changes since v1:
* Move code from Yama to the FS subsystem (suggested by Kees Cook).
* Make omayexec_inode_permission() static (suggested by Jann Horn).
* Use mode 0600 for the sysctl.
* Only match regular files (not directories nor other types), which
  follows the same semantic as commit 73601ea5b7b1 ("fs/open.c: allow
  opening only regular files during execve()").
---
 fs/namei.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/fs.h |  5 +++
 kernel/sysctl.c    |  9 +++++
 security/Kconfig   | 26 ++++++++++++++
 4 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 33b6d372e74a..70f179f6bc6c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -39,6 +39,7 @@
 #include <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
+#define OMAYEXEC_ENFORCE_MOUNT	(1 << 0)
+#define OMAYEXEC_ENFORCE_FILE	(1 << 1)
+#define _OMAYEXEC_LAST		OMAYEXEC_ENFORCE_FILE
+#define _OMAYEXEC_MASK		((_OMAYEXEC_LAST << 1) - 1)
+
+#ifdef CONFIG_OMAYEXEC_STATIC
+const int sysctl_omayexec_enforce =
+#ifdef CONFIG_OMAYEXEC_ENFORCE_MOUNT
+	OMAYEXEC_ENFORCE_MOUNT |
+#endif
+#ifdef CONFIG_OMAYEXEC_ENFORCE_FILE
+	OMAYEXEC_ENFORCE_FILE |
+#endif
+	OMAYEXEC_ENFORCE_NONE;
+#else /* CONFIG_OMAYEXEC_STATIC */
+int sysctl_omayexec_enforce __read_mostly = OMAYEXEC_ENFORCE_NONE;
+#endif /* CONFIG_OMAYEXEC_STATIC */
+
+/*
+ * Handle open_mayexec_enforce sysctl
+ */
+#if defined(CONFIG_SYSCTL) && !defined(CONFIG_OMAYEXEC_STATIC)
+int proc_omayexec(struct ctl_table *table, int write, void __user *buffer,
+		size_t *lenp, loff_t *ppos)
+{
+	int error;
+
+	if (write) {
+		struct ctl_table table_copy;
+		int tmp_mayexec_enforce;
+
+		if (!capable(CAP_MAC_ADMIN))
+			return -EPERM;
+
+		tmp_mayexec_enforce = *((int *)table->data);
+		table_copy = *table;
+		/* Do not erase sysctl_omayexec_enforce. */
+		table_copy.data = &tmp_mayexec_enforce;
+		error = proc_dointvec(&table_copy, write, buffer, lenp, ppos);
+		if (error)
+			return error;
+
+		if ((tmp_mayexec_enforce | _OMAYEXEC_MASK) != _OMAYEXEC_MASK)
+			return -EINVAL;
+
+		*((int *)table->data) = tmp_mayexec_enforce;
+	} else {
+		error = proc_dointvec(table, write, buffer, lenp, ppos);
+		if (error)
+			return error;
+	}
+	return 0;
+}
+#endif
+
+/**
+ * omayexec_inode_permission - Check O_MAYEXEC before accessing an inode
+ *
+ * @inode: Inode to check permission on
+ * @mask: Right to check for (%MAY_OPENEXEC, %MAY_EXECMOUNT, %MAY_EXEC)
+ *
+ * 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;
+}
+
 /**
  * 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
 
 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,
+	},
+#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

[PATCH v5 4/6] selftest/openat2: Add tests for O_MAYEXEC enforcing

[Mickaël Salaün]

Test propagation of noexec mount points or file executability through
files open with or without O_MAYEXEC, thanks to the
fs.open_mayexec_enforce sysctl.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
---

Changes since v3:
* Replace RESOLVE_MAYEXEC with O_MAYEXEC.
* Add tests to check that O_MAYEXEC is ignored by open(2) and openat(2).

Changes since v2:
* Move tests from exec/ to openat2/ .
* Replace O_MAYEXEC with RESOLVE_MAYEXEC from openat2(2).
* Cleanup tests.

Changes since v1:
* Move tests from yama/ to exec/ .
* Fix _GNU_SOURCE in kselftest_harness.h .
* Add a new test sysctl_access_write to check if CAP_MAC_ADMIN is taken
  into account.
* Test directory execution which is always forbidden since commit
  73601ea5b7b1 ("fs/open.c: allow opening only regular files during
  execve()"), and also check that even the root user can not bypass file
  execution checks.
* Make sure delete_workspace() always as enough right to succeed.
* Cosmetic cleanup.
---
 tools/testing/selftests/kselftest_harness.h   |   3 +
 tools/testing/selftests/openat2/Makefile      |   3 +-
 tools/testing/selftests/openat2/config        |   1 +
 tools/testing/selftests/openat2/helpers.h     |   1 +
 .../testing/selftests/openat2/omayexec_test.c | 330 ++++++++++++++++++
 5 files changed, 337 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/openat2/config
 create mode 100644 tools/testing/selftests/openat2/omayexec_test.c

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 2bb8c81fc0b4..f6e056ba4a13 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -50,7 +50,10 @@
 #ifndef __KSELFTEST_HARNESS_H
 #define __KSELFTEST_HARNESS_H
 
+#ifndef _GNU_SOURCE
 #define _GNU_SOURCE
+#endif
+
 #include <asm/types.h>
 #include <errno.h>
 #include <stdbool.h>
diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 4b93b1417b86..cb98bdb4d5b1 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -1,7 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
-TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
+LDLIBS += -lcap
+TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test omayexec_test
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/openat2/config b/tools/testing/selftests/openat2/config
new file mode 100644
index 000000000000..dd53c266bf52
--- /dev/null
+++ b/tools/testing/selftests/openat2/config
@@ -0,0 +1 @@
+CONFIG_SYSCTL=y
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index a6ea27344db2..1dcd3e1e2f38 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -9,6 +9,7 @@
 
 #define _GNU_SOURCE
 #include <stdint.h>
+#include <stdbool.h>
 #include <errno.h>
 #include <linux/types.h>
 #include "../kselftest.h"
diff --git a/tools/testing/selftests/openat2/omayexec_test.c b/tools/testing/selftests/openat2/omayexec_test.c
new file mode 100644
index 000000000000..7052c852daf8
--- /dev/null
+++ b/tools/testing/selftests/openat2/omayexec_test.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test O_MAYEXEC
+ *
+ * Copyright © 2018-2020 ANSSI
+ *
+ * Author: Mickaël Salaün <mic@digikod.net>
+ */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/capability.h>
+#include <sys/mount.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "helpers.h"
+#include "../kselftest_harness.h"
+
+#ifndef O_MAYEXEC
+#define O_MAYEXEC		040000000
+#endif
+
+#define SYSCTL_MAYEXEC	"/proc/sys/fs/open_mayexec_enforce"
+
+#define BIN_DIR		"./test-mount"
+#define BIN_PATH	BIN_DIR "/file"
+#define DIR_PATH	BIN_DIR "/directory"
+
+#define ALLOWED		1
+#define DENIED		0
+
+static void ignore_dac(struct __test_metadata *_metadata, int override)
+{
+	cap_t caps;
+	const cap_value_t cap_val[2] = {
+		CAP_DAC_OVERRIDE,
+		CAP_DAC_READ_SEARCH,
+	};
+
+	caps = cap_get_proc();
+	ASSERT_NE(NULL, caps);
+	ASSERT_EQ(0, cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_val,
+				override ? CAP_SET : CAP_CLEAR));
+	ASSERT_EQ(0, cap_set_proc(caps));
+	EXPECT_EQ(0, cap_free(caps));
+}
+
+static void ignore_mac(struct __test_metadata *_metadata, int override)
+{
+	cap_t caps;
+	const cap_value_t cap_val[1] = {
+		CAP_MAC_ADMIN,
+	};
+
+	caps = cap_get_proc();
+	ASSERT_NE(NULL, caps);
+	ASSERT_EQ(0, cap_set_flag(caps, CAP_EFFECTIVE, 1, cap_val,
+				override ? CAP_SET : CAP_CLEAR));
+	ASSERT_EQ(0, cap_set_proc(caps));
+	EXPECT_EQ(0, cap_free(caps));
+}
+
+static void test_omx(struct __test_metadata *_metadata,
+		const char *const path, const int exec_allowed)
+{
+	struct open_how how = {
+		.flags = O_RDONLY | O_CLOEXEC,
+	};
+	int fd;
+
+	/* Opens without O_MAYEXEC. */
+	fd = sys_openat2(AT_FDCWD, path, &how);
+	ASSERT_LE(0, fd);
+	EXPECT_EQ(0, close(fd));
+
+	how.flags |= O_MAYEXEC;
+
+	/* Checks that O_MAYEXEC is ignored with open(2). */
+	fd = open(path, how.flags);
+	ASSERT_LE(0, fd);
+	EXPECT_EQ(0, close(fd));
+
+	/* Checks that O_MAYEXEC is ignored with openat(2). */
+	fd = openat(AT_FDCWD, path, how.flags);
+	ASSERT_LE(0, fd);
+	EXPECT_EQ(0, close(fd));
+
+	/* Opens with O_MAYEXEC. */
+	fd = sys_openat2(AT_FDCWD, path, &how);
+	if (exec_allowed) {
+		ASSERT_LE(0, fd);
+		EXPECT_EQ(0, close(fd));
+	} else {
+		ASSERT_EQ(-EACCES, fd);
+	}
+}
+
+static void test_omx_dir_file(struct __test_metadata *_metadata,
+		const char *const dir_path, const char *const file_path,
+		const int exec_allowed)
+{
+	/*
+	 * Directory execution is always denied since commit 73601ea5b7b1
+	 * ("fs/open.c: allow opening only regular files during execve()").
+	 */
+	test_omx(_metadata, dir_path, DENIED);
+	test_omx(_metadata, file_path, exec_allowed);
+}
+
+static void test_dir_file(struct __test_metadata *_metadata,
+		const char *const dir_path, const char *const file_path,
+		const int exec_allowed)
+{
+	/* Tests as root. */
+	ignore_dac(_metadata, 1);
+	test_omx_dir_file(_metadata, dir_path, file_path, exec_allowed);
+
+	/* Tests without bypass. */
+	ignore_dac(_metadata, 0);
+	test_omx_dir_file(_metadata, dir_path, file_path, exec_allowed);
+}
+
+static void sysctl_write(struct __test_metadata *_metadata,
+		const char *path, const char *value)
+{
+	int fd;
+	size_t len_value;
+	ssize_t len_wrote;
+
+	fd = open(path, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+	len_value = strlen(value);
+	len_wrote = write(fd, value, len_value);
+	ASSERT_EQ(len_wrote, len_value);
+	EXPECT_EQ(0, close(fd));
+}
+
+static void create_workspace(struct __test_metadata *_metadata,
+		int mount_exec, int file_exec)
+{
+	int fd;
+
+	/*
+	 * Cleans previous workspace if any error previously happened (don't
+	 * check errors).
+	 */
+	umount(BIN_DIR);
+	rmdir(BIN_DIR);
+
+	/* Creates a clean mount point. */
+	ASSERT_EQ(0, mkdir(BIN_DIR, 00700));
+	ASSERT_EQ(0, mount("test", BIN_DIR, "tmpfs",
+				MS_MGC_VAL | (mount_exec ? 0 : MS_NOEXEC),
+				"mode=0700,size=4k"));
+
+	/* Creates a test file. */
+	fd = open(BIN_PATH, O_CREAT | O_RDONLY | O_CLOEXEC,
+			file_exec ? 00500 : 00400);
+	ASSERT_LE(0, fd);
+	EXPECT_EQ(0, close(fd));
+
+	/* Creates a test directory. */
+	ASSERT_EQ(0, mkdir(DIR_PATH, file_exec ? 00500 : 00400));
+}
+
+static void delete_workspace(struct __test_metadata *_metadata)
+{
+	ignore_mac(_metadata, 1);
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "0");
+
+	/* There is no need to unlink BIN_PATH nor DIR_PATH. */
+	ASSERT_EQ(0, umount(BIN_DIR));
+	ASSERT_EQ(0, rmdir(BIN_DIR));
+}
+
+FIXTURE_DATA(mount_exec_file_exec) { };
+
+FIXTURE_SETUP(mount_exec_file_exec)
+{
+	create_workspace(_metadata, 1, 1);
+}
+
+FIXTURE_TEARDOWN(mount_exec_file_exec)
+{
+	delete_workspace(_metadata);
+}
+
+TEST_F(mount_exec_file_exec, mount)
+{
+	/* Enforces mount exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED);
+}
+
+TEST_F(mount_exec_file_exec, file)
+{
+	/* Enforces file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED);
+}
+
+TEST_F(mount_exec_file_exec, mount_file)
+{
+	/* Enforces mount and file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED);
+}
+
+FIXTURE_DATA(mount_exec_file_noexec) { };
+
+FIXTURE_SETUP(mount_exec_file_noexec)
+{
+	create_workspace(_metadata, 1, 0);
+}
+
+FIXTURE_TEARDOWN(mount_exec_file_noexec)
+{
+	delete_workspace(_metadata);
+}
+
+TEST_F(mount_exec_file_noexec, mount)
+{
+	/* Enforces mount exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED);
+}
+
+TEST_F(mount_exec_file_noexec, file)
+{
+	/* Enforces file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+TEST_F(mount_exec_file_noexec, mount_file)
+{
+	/* Enforces mount and file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+FIXTURE_DATA(mount_noexec_file_exec) { };
+
+FIXTURE_SETUP(mount_noexec_file_exec)
+{
+	create_workspace(_metadata, 0, 1);
+}
+
+FIXTURE_TEARDOWN(mount_noexec_file_exec)
+{
+	delete_workspace(_metadata);
+}
+
+TEST_F(mount_noexec_file_exec, mount)
+{
+	/* Enforces mount exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+TEST_F(mount_noexec_file_exec, file)
+{
+	/* Enforces file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, ALLOWED);
+}
+
+TEST_F(mount_noexec_file_exec, mount_file)
+{
+	/* Enforces mount and file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+FIXTURE_DATA(mount_noexec_file_noexec) { };
+
+FIXTURE_SETUP(mount_noexec_file_noexec)
+{
+	create_workspace(_metadata, 0, 0);
+}
+
+FIXTURE_TEARDOWN(mount_noexec_file_noexec)
+{
+	delete_workspace(_metadata);
+}
+
+TEST_F(mount_noexec_file_noexec, mount)
+{
+	/* Enforces mount exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "1");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+TEST_F(mount_noexec_file_noexec, file)
+{
+	/* Enforces file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "2");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+TEST_F(mount_noexec_file_noexec, mount_file)
+{
+	/* Enforces mount and file exec check. */
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "3");
+	test_dir_file(_metadata, DIR_PATH, BIN_PATH, DENIED);
+}
+
+TEST(sysctl_access_write)
+{
+	int fd;
+	ssize_t len_wrote;
+
+	ignore_mac(_metadata, 1);
+	sysctl_write(_metadata, SYSCTL_MAYEXEC, "0");
+
+	ignore_mac(_metadata, 0);
+	fd = open(SYSCTL_MAYEXEC, O_WRONLY | O_CLOEXEC);
+	ASSERT_LE(0, fd);
+	len_wrote = write(fd, "0", 1);
+	ASSERT_EQ(len_wrote, -1);
+	EXPECT_EQ(0, close(fd));
+
+	ignore_mac(_metadata, 1);
+}
+
+TEST_HARNESS_MAIN
-- 
2.26.2

[PATCH v5 5/6] doc: Add documentation for the fs.open_mayexec_enforce sysctl

[Mickaël Salaün]

This sysctl enables to propagate executable permission to userspace
thanks to the O_MAYEXEC flag.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Kees Cook <keescook@chromium.org>
---

Changes since v3:
* Switch back to O_MAYEXEC and highlight that it is only taken into
  account by openat2(2).

Changes since v2:
* Update documentation with the new RESOLVE_MAYEXEC.
* Improve explanations, including concerns about LD_PRELOAD.

Changes since v1:
* Move from LSM/Yama to sysctl/fs .
---
 Documentation/admin-guide/sysctl/fs.rst | 44 +++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/admin-guide/sysctl/fs.rst b/Documentation/admin-guide/sysctl/fs.rst
index 2a45119e3331..d55615c36772 100644
--- a/Documentation/admin-guide/sysctl/fs.rst
+++ b/Documentation/admin-guide/sysctl/fs.rst
@@ -37,6 +37,7 @@ Currently, these files are in /proc/sys/fs:
 - inode-nr
 - inode-state
 - nr_open
+- open_mayexec_enforce
 - overflowuid
 - overflowgid
 - pipe-user-pages-hard
@@ -165,6 +166,49 @@ system needs to prune the inode list instead of allocating
 more.
 
 
+open_mayexec_enforce
+--------------------
+
+While being ignored by :manpage:`open(2)` and :manpage:`openat(2)`, the
+``O_MAYEXEC`` flag can be passed to :manpage:`openat2(2)` to only open regular
+files that are expected to be executable.  If the file is not identified as
+executable, then the syscall returns -EACCES.  This may allow a script
+interpreter to check executable permission before reading commands from a file,
+or a dynamic linker to only load executable shared objects.  One interesting
+use case is to enforce a "write xor execute" policy through interpreters.
+
+The ability to restrict code execution must be thought as a system-wide policy,
+which first starts by restricting mount points with the ``noexec`` option.
+This option is also automatically applied to special filesystems such as /proc
+.  This prevents files on such mount points to be directly executed by the
+kernel or mapped as executable memory (e.g. libraries).  With script
+interpreters using the ``O_MAYEXEC`` flag, the executable permission can then
+be checked before reading commands from files. This makes it possible to
+enforce the ``noexec`` at the interpreter level, and thus propagates this
+security policy to scripts.  To be fully effective, these interpreters also
+need to handle the other ways to execute code: command line parameters (e.g.,
+option ``-e`` for Perl), module loading (e.g., option ``-m`` for Python),
+stdin, file sourcing, environment variables, configuration files, etc.
+According to the threat model, it may be acceptable to allow some script
+interpreters (e.g. Bash) to interpret commands from stdin, may it be a TTY or a
+pipe, because it may not be enough to (directly) perform syscalls.
+
+There are two complementary security policies: enforce the ``noexec`` mount
+option, and enforce executable file permission.  These policies are handled by
+the ``fs.open_mayexec_enforce`` sysctl (writable only with ``CAP_MAC_ADMIN``)
+as a bitmask:
+
+1 - Mount restriction: checks that the mount options for the underlying VFS
+    mount do not prevent execution.
+
+2 - File permission restriction: checks that the to-be-opened file is marked as
+    executable for the current process (e.g., POSIX permissions).
+
+Code samples can be found in tools/testing/selftests/openat2/omayexec_test.c
+and at
+https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC .
+
+
 overflowgid & overflowuid
 -------------------------
 
-- 
2.26.2

[PATCH v5 6/6] ima: add policy support for the new file open MAY_OPENEXEC flag

[Mickaël Salaün]

From: Mimi Zohar <zohar@linux.ibm.com>

The kernel has no way of differentiating between a file containing data
or code being opened by an interpreter.  The proposed O_MAYEXEC
openat2(2) flag bridges this gap by defining and enabling the
MAY_OPENEXEC flag.

This patch adds IMA policy support for the new MAY_OPENEXEC flag.

Example:
measure func=FILE_CHECK mask=^MAY_OPENEXEC
appraise func=FILE_CHECK appraise_type=imasig mask=^MAY_OPENEXEC

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Link: https://lore.kernel.org/r/1588167523-7866-3-git-send-email-zohar@linux.ibm.com
---
 Documentation/ABI/testing/ima_policy |  2 +-
 security/integrity/ima/ima_main.c    |  3 ++-
 security/integrity/ima/ima_policy.c  | 15 +++++++++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..caca46125fe0 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -31,7 +31,7 @@ Description:
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE] [KEY_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
-			       [[^]MAY_EXEC]
+			       [[^]MAY_EXEC] [[^]MAY_OPENEXEC]
 			fsmagic:= hex value
 			fsuuid:= file system UUID (e.g 8bcbe394-4f13-4144-be8e-5aa9ea2ce2f6)
 			uid:= decimal value
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9d0abedeae77..c80cdaf13626 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -438,7 +438,8 @@ int ima_file_check(struct file *file, int mask)
 
 	security_task_getsecid(current, &secid);
 	return process_measurement(file, current_cred(), secid, NULL, 0,
-				   mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
+				   mask & (MAY_READ | MAY_WRITE |
+					   MAY_EXEC | MAY_OPENEXEC |
 					   MAY_APPEND), FILE_CHECK);
 }
 EXPORT_SYMBOL_GPL(ima_file_check);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c334e0dc6083..f54cbc55498d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -406,7 +406,8 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
  * @cred: a pointer to a credentials structure for user validation
  * @secid: the secid of the task to be validated
  * @func: LIM hook identifier
- * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
+ * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC |
+ *			    MAY_OPENEXEC)
  * @keyring: keyring name to check in policy for KEY_CHECK func
  *
  * Returns true on rule match, false on failure.
@@ -527,7 +528,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  *        being made
  * @secid: LSM secid of the task to be validated
  * @func: IMA hook identifier
- * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
+ * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC |
+ *			    MAY_OPENEXEC)
  * @pcr: set the pcr to extend
  * @template_desc: the template that should be used for this rule
  * @keyring: the keyring name, if given, to be used to check in the policy.
@@ -1089,6 +1091,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->mask = MAY_READ;
 			else if (strcmp(from, "MAY_APPEND") == 0)
 				entry->mask = MAY_APPEND;
+			else if (strcmp(from, "MAY_OPENEXEC") == 0)
+				entry->mask = MAY_OPENEXEC;
 			else
 				result = -EINVAL;
 			if (!result)
@@ -1420,14 +1424,15 @@ const char *const func_tokens[] = {
 
 #ifdef	CONFIG_IMA_READ_POLICY
 enum {
-	mask_exec = 0, mask_write, mask_read, mask_append
+	mask_exec = 0, mask_write, mask_read, mask_append, mask_openexec
 };
 
 static const char *const mask_tokens[] = {
 	"^MAY_EXEC",
 	"^MAY_WRITE",
 	"^MAY_READ",
-	"^MAY_APPEND"
+	"^MAY_APPEND",
+	"^MAY_OPENEXEC"
 };
 
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
@@ -1516,6 +1521,8 @@ int ima_policy_show(struct seq_file *m, void *v)
 			seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
 		if (entry->mask & MAY_APPEND)
 			seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
+		if (entry->mask & MAY_OPENEXEC)
+			seq_printf(m, pt(Opt_mask), mt(mask_openexec) + offset);
 		seq_puts(m, " ");
 	}
 
-- 
2.26.2

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

On 05/05/2020 17:31, Mickaël Salaün wrote:
> Hi,
> 
> This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
> OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
> configure the security policy at kernel build time.  As requested by
> Mimi Zohar, I completed the series with one of her patches for IMA.
> 
> 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.
> 
> A simple system-wide security policy can be enforced by the system
> administrator through a sysctl configuration consistent with the mount
> points or the file access rights.  The documentation patch explains the
> prerequisites.
> 
> Furthermore, the security policy can also be delegated to an LSM, either
> a MAC system or an integrity system.  For instance, the new kernel
> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
> integrity gap by bringing the ability to check the use of scripts [1].
> Other uses are expected, such as for openat2(2) [2], SGX integration
> [3], bpffs [4] or IPE [5].
> 
> Userspace needs to adapt to take advantage of this new feature.  For
> example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
> extended with policy enforcement points related to code interpretation,
> which can be used to align with the PowerShell audit features.
> Additional Python security improvements (e.g. a limited interpreter
> withou -c, stdin piping of code) are on their way.
> 
> The initial idea come from CLIP OS 4 and the original implementation has
> been used for more than 12 years:
> https://github.com/clipos-archive/clipos4_doc
> 
> An introduction to O_MAYEXEC was given at the Linux Security Summit
> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
> The "write xor execute" principle was explained at Kernel Recipes 2018 -
> CLIP OS: a defense-in-depth OS:
> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
> 
> This patch series can be applied on top of v5.7-rc4.  This can be tested
> with CONFIG_SYSCTL.  I would really appreciate constructive comments on
> this patch series.
> 
> Previous version:
> https://lore.kernel.org/lkml/20200428175129.634352-1-mic@digikod.net/

The previous version (v4) is
https://lore.kernel.org/lkml/20200430132320.699508-1-mic@digikod.net/

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Randy Dunlap]

On 5/5/20 8:31 AM, Mickaël Salaün wrote:
> diff --git a/security/Kconfig b/security/Kconfig
> index cd3cc7da3a55..d8fac9240d14 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH
>  	  If you wish for all usermode helper programs to be disabled,
>  	  specify an empty string here (i.e. "").
>  
> +menuconfig OMAYEXEC_STATIC
> +	tristate "Configure O_MAYEXEC behavior at build time"
> +	---help---
> +	  Enable to enforce O_MAYEXEC at build time, and disable the dedicated
> +	  fs.open_mayexec_enforce sysctl.

That help message is a bit confusing IMO.  Does setting/enabling OMAYEXEC_STATIC
both enforce O_MAYEXEC at build time and also disable the dedicated sysctl?

Or are these meant to be alternatives, one for what Enabling this kconfig symbol
does and the other for what Disabling this symbol does?  If so, it doesn't
say that.

> +
> +	  See Documentation/admin-guide/sysctl/fs.rst for more details.
> +
> +if OMAYEXEC_STATIC
> +
> +config OMAYEXEC_ENFORCE_MOUNT
> +	bool "Mount restriction"
> +	default y
> +	---help---
> +	  Forbid opening files with the O_MAYEXEC option if their underlying VFS is
> +	  mounted with the noexec option or if their superblock forbids execution
> +	  of its content (e.g., /proc).
> +
> +config OMAYEXEC_ENFORCE_FILE
> +	bool "File permission restriction"
> +	---help---
> +	  Forbid opening files with the O_MAYEXEC option if they are not marked as
> +	  executable for the current process (e.g., POSIX permissions).
> +
> +endif # OMAYEXEC_STATIC
> +
>  source "security/selinux/Kconfig"
>  source "security/smack/Kconfig"
>  source "security/tomoyo/Kconfig"


-- 
~Randy
Reported-by: Randy Dunlap <rdunlap@infradead.org>

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Mickaël Salaün]

On 05/05/2020 17:44, Randy Dunlap wrote:
> On 5/5/20 8:31 AM, Mickaël Salaün wrote:
>> diff --git a/security/Kconfig b/security/Kconfig
>> index cd3cc7da3a55..d8fac9240d14 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH
>>  	  If you wish for all usermode helper programs to be disabled,
>>  	  specify an empty string here (i.e. "").
>>  
>> +menuconfig OMAYEXEC_STATIC
>> +	tristate "Configure O_MAYEXEC behavior at build time"
>> +	---help---
>> +	  Enable to enforce O_MAYEXEC at build time, and disable the dedicated
>> +	  fs.open_mayexec_enforce sysctl.
> 
> That help message is a bit confusing IMO.  Does setting/enabling OMAYEXEC_STATIC
> both enforce O_MAYEXEC at build time and also disable the dedicated sysctl?

Yes. What about this?
"Define the O_MAYEXEC policy at build time only. As a side effect, this
also disables the fs.open_mayexec_enforce sysctl."

> 
> Or are these meant to be alternatives, one for what Enabling this kconfig symbol
> does and the other for what Disabling this symbol does?  If so, it doesn't
> say that.
> 
>> +
>> +	  See Documentation/admin-guide/sysctl/fs.rst for more details.
>> +
>> +if OMAYEXEC_STATIC
>> +
>> +config OMAYEXEC_ENFORCE_MOUNT
>> +	bool "Mount restriction"
>> +	default y
>> +	---help---
>> +	  Forbid opening files with the O_MAYEXEC option if their underlying VFS is
>> +	  mounted with the noexec option or if their superblock forbids execution
>> +	  of its content (e.g., /proc).
>> +
>> +config OMAYEXEC_ENFORCE_FILE
>> +	bool "File permission restriction"
>> +	---help---
>> +	  Forbid opening files with the O_MAYEXEC option if they are not marked as
>> +	  executable for the current process (e.g., POSIX permissions).
>> +
>> +endif # OMAYEXEC_STATIC
>> +
>>  source "security/selinux/Kconfig"
>>  source "security/smack/Kconfig"
>>  source "security/tomoyo/Kconfig"
> 
> 

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Randy Dunlap]

On 5/5/20 9:55 AM, Mickaël Salaün wrote:
> 
> 
> On 05/05/2020 17:44, Randy Dunlap wrote:
>> On 5/5/20 8:31 AM, Mickaël Salaün wrote:
>>> diff --git a/security/Kconfig b/security/Kconfig
>>> index cd3cc7da3a55..d8fac9240d14 100644
>>> --- a/security/Kconfig
>>> +++ b/security/Kconfig
>>> @@ -230,6 +230,32 @@ config STATIC_USERMODEHELPER_PATH
>>>  	  If you wish for all usermode helper programs to be disabled,
>>>  	  specify an empty string here (i.e. "").
>>>  
>>> +menuconfig OMAYEXEC_STATIC
>>> +	tristate "Configure O_MAYEXEC behavior at build time"
>>> +	---help---
>>> +	  Enable to enforce O_MAYEXEC at build time, and disable the dedicated
>>> +	  fs.open_mayexec_enforce sysctl.
>>
>> That help message is a bit confusing IMO.  Does setting/enabling OMAYEXEC_STATIC
>> both enforce O_MAYEXEC at build time and also disable the dedicated sysctl?
> 
> Yes. What about this?
> "Define the O_MAYEXEC policy at build time only. As a side effect, this
> also disables the fs.open_mayexec_enforce sysctl."
> 

Yes, much better. Thanks.

>>
>> Or are these meant to be alternatives, one for what Enabling this kconfig symbol
>> does and the other for what Disabling this symbol does?  If so, it doesn't
>> say that.
>>
>>> +
>>> +	  See Documentation/admin-guide/sysctl/fs.rst for more details.
>>> +
>>> +if OMAYEXEC_STATIC
>>> +
>>> +config OMAYEXEC_ENFORCE_MOUNT
>>> +	bool "Mount restriction"
>>> +	default y
>>> +	---help---
>>> +	  Forbid opening files with the O_MAYEXEC option if their underlying VFS is
>>> +	  mounted with the noexec option or if their superblock forbids execution
>>> +	  of its content (e.g., /proc).
>>> +
>>> +config OMAYEXEC_ENFORCE_FILE
>>> +	bool "File permission restriction"
>>> +	---help---
>>> +	  Forbid opening files with the O_MAYEXEC option if they are not marked as
>>> +	  executable for the current process (e.g., POSIX permissions).
>>> +
>>> +endif # OMAYEXEC_STATIC
>>> +
>>>  source "security/selinux/Kconfig"
>>>  source "security/smack/Kconfig"
>>>  source "security/tomoyo/Kconfig"
>>
>>


-- 
~Randy

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Lev R. Oshvang .]

On Tue, May 5, 2020 at 6:36 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 05/05/2020 17:31, Mickaël Salaün wrote:
> > Hi,
> >
> > This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
> > OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
> > configure the security policy at kernel build time.  As requested by
> > Mimi Zohar, I completed the series with one of her patches for IMA.
> >
> > 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.
> >
> > A simple system-wide security policy can be enforced by the system
> > administrator through a sysctl configuration consistent with the mount
> > points or the file access rights.  The documentation patch explains the
> > prerequisites.
> >
> > Furthermore, the security policy can also be delegated to an LSM, either
> > a MAC system or an integrity system.  For instance, the new kernel
> > MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
> > integrity gap by bringing the ability to check the use of scripts [1].
> > Other uses are expected, such as for openat2(2) [2], SGX integration
> > [3], bpffs [4] or IPE [5].
> >
> > Userspace needs to adapt to take advantage of this new feature.  For
> > example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
> > extended with policy enforcement points related to code interpretation,
> > which can be used to align with the PowerShell audit features.
> > Additional Python security improvements (e.g. a limited interpreter
> > withou -c, stdin piping of code) are on their way.
> >
> > The initial idea come from CLIP OS 4 and the original implementation has
> > been used for more than 12 years:
> > https://github.com/clipos-archive/clipos4_doc
> >
> > An introduction to O_MAYEXEC was given at the Linux Security Summit
> > Europe 2018 - Linux Kernel Security Contributions by ANSSI:
> > https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
> > The "write xor execute" principle was explained at Kernel Recipes 2018 -
> > CLIP OS: a defense-in-depth OS:
> > https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
> >
> > This patch series can be applied on top of v5.7-rc4.  This can be tested
> > with CONFIG_SYSCTL.  I would really appreciate constructive comments on
> > this patch series.
> >
> > Previous version:
> > https://lore.kernel.org/lkml/20200428175129.634352-1-mic@digikod.net/
>
> The previous version (v4) is
> https://lore.kernel.org/lkml/20200430132320.699508-1-mic@digikod.net/


Hi Michael

I have couple of question
1. Why you did not add O_MAYEXEC to open()?
Some time ago (around v4.14) open() did not return EINVAL when
VALID_OPEN_FLAGS check failed.
Now it does, so I do not see a problem that interpreter will use
simple open(),  ( Although that path might be manipulated, but file
contents will be verified by IMA)
2. When you apply a new flag to mount, it means that IMA will check
all files under this mount and it does not matter whether the file in
question is a script or not.
IMHO it is too hard overhead for performance reasons.

Regards,
LEv

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 3950 bytes --]

On 2020-05-06, Lev R. Oshvang . <levonshe@gmail.com> wrote:
> On Tue, May 5, 2020 at 6:36 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> >
> > On 05/05/2020 17:31, Mickaël Salaün wrote:
> > > Hi,
> > >
> > > This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
> > > OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
> > > configure the security policy at kernel build time.  As requested by
> > > Mimi Zohar, I completed the series with one of her patches for IMA.
> > >
> > > 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.
> > >
> > > A simple system-wide security policy can be enforced by the system
> > > administrator through a sysctl configuration consistent with the mount
> > > points or the file access rights.  The documentation patch explains the
> > > prerequisites.
> > >
> > > Furthermore, the security policy can also be delegated to an LSM, either
> > > a MAC system or an integrity system.  For instance, the new kernel
> > > MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
> > > integrity gap by bringing the ability to check the use of scripts [1].
> > > Other uses are expected, such as for openat2(2) [2], SGX integration
> > > [3], bpffs [4] or IPE [5].
> > >
> > > Userspace needs to adapt to take advantage of this new feature.  For
> > > example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
> > > extended with policy enforcement points related to code interpretation,
> > > which can be used to align with the PowerShell audit features.
> > > Additional Python security improvements (e.g. a limited interpreter
> > > withou -c, stdin piping of code) are on their way.
> > >
> > > The initial idea come from CLIP OS 4 and the original implementation has
> > > been used for more than 12 years:
> > > https://github.com/clipos-archive/clipos4_doc
> > >
> > > An introduction to O_MAYEXEC was given at the Linux Security Summit
> > > Europe 2018 - Linux Kernel Security Contributions by ANSSI:
> > > https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
> > > The "write xor execute" principle was explained at Kernel Recipes 2018 -
> > > CLIP OS: a defense-in-depth OS:
> > > https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
> > >
> > > This patch series can be applied on top of v5.7-rc4.  This can be tested
> > > with CONFIG_SYSCTL.  I would really appreciate constructive comments on
> > > this patch series.
> > >
> > > Previous version:
> > > https://lore.kernel.org/lkml/20200428175129.634352-1-mic@digikod.net/
> >
> > The previous version (v4) is
> > https://lore.kernel.org/lkml/20200430132320.699508-1-mic@digikod.net/
> 
> 
> Hi Michael
> 
> I have couple of question
> 1. Why you did not add O_MAYEXEC to open()?
> Some time ago (around v4.14) open() did not return EINVAL when
> VALID_OPEN_FLAGS check failed.
> Now it does, so I do not see a problem that interpreter will use
> simple open(),  ( Although that path might be manipulated, but file
> contents will be verified by IMA)

You don't get -EINVAL from open() in the case of unknown flags, that's
something only openat2() does in the open*() family. Hence why it's only
introduced for openat2().

> 2. When you apply a new flag to mount, it means that IMA will check
> all files under this mount and it does not matter whether the file in
> question is a script or not.
> IMHO it is too hard overhead for performance reasons.
> 
> Regards,
> LEv


-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v5 0/6] Add support for O_MAYEXEC

[David Laight]

From: Mickaël Salaün
> Sent: 05 May 2020 16:32
> 
> This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
> OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
> configure the security policy at kernel build time.  As requested by
> Mimi Zohar, I completed the series with one of her patches for IMA.
> 
> 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.
> 
> A simple system-wide security policy can be enforced by the system
> administrator through a sysctl configuration consistent with the mount
> points or the file access rights.  The documentation patch explains the
> prerequisites.
> 
> Furthermore, the security policy can also be delegated to an LSM, either
> a MAC system or an integrity system.  For instance, the new kernel
> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
> integrity gap by bringing the ability to check the use of scripts [1].
> Other uses are expected, such as for openat2(2) [2], SGX integration
> [3], bpffs [4] or IPE [5].
> 
> Userspace needs to adapt to take advantage of this new feature.  For
> example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
> extended with policy enforcement points related to code interpretation,
> which can be used to align with the PowerShell audit features.
> Additional Python security improvements (e.g. a limited interpreter
> withou -c, stdin piping of code) are on their way.
> 
> The initial idea come from CLIP OS 4 and the original implementation has
> been used for more than 12 years:
> https://github.com/clipos-archive/clipos4_doc
> 
> An introduction to O_MAYEXEC was given at the Linux Security Summit
> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
> The "write xor execute" principle was explained at Kernel Recipes 2018 -
> CLIP OS: a defense-in-depth OS:
> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
> 
> This patch series can be applied on top of v5.7-rc4.  This can be tested
> with CONFIG_SYSCTL.  I would really appreciate constructive comments on
> this patch series.

None of that description actually says what the patch actually does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

On 06/05/2020 15:58, Lev R. Oshvang . wrote:
> On Tue, May 5, 2020 at 6:36 PM Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 05/05/2020 17:31, Mickaël Salaün wrote:
>>> Hi,
>>>
>>> This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
>>> OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
>>> configure the security policy at kernel build time.  As requested by
>>> Mimi Zohar, I completed the series with one of her patches for IMA.
>>>
>>> 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.
>>>
>>> A simple system-wide security policy can be enforced by the system
>>> administrator through a sysctl configuration consistent with the mount
>>> points or the file access rights.  The documentation patch explains the
>>> prerequisites.
>>>
>>> Furthermore, the security policy can also be delegated to an LSM, either
>>> a MAC system or an integrity system.  For instance, the new kernel
>>> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
>>> integrity gap by bringing the ability to check the use of scripts [1].
>>> Other uses are expected, such as for openat2(2) [2], SGX integration
>>> [3], bpffs [4] or IPE [5].
>>>
>>> Userspace needs to adapt to take advantage of this new feature.  For
>>> example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
>>> extended with policy enforcement points related to code interpretation,
>>> which can be used to align with the PowerShell audit features.
>>> Additional Python security improvements (e.g. a limited interpreter
>>> withou -c, stdin piping of code) are on their way.
>>>
>>> The initial idea come from CLIP OS 4 and the original implementation has
>>> been used for more than 12 years:
>>> https://github.com/clipos-archive/clipos4_doc
>>>
>>> An introduction to O_MAYEXEC was given at the Linux Security Summit
>>> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
>>> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
>>> The "write xor execute" principle was explained at Kernel Recipes 2018 -
>>> CLIP OS: a defense-in-depth OS:
>>> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
>>>
>>> This patch series can be applied on top of v5.7-rc4.  This can be tested
>>> with CONFIG_SYSCTL.  I would really appreciate constructive comments on
>>> this patch series.
>>>
>>> Previous version:
>>> https://lore.kernel.org/lkml/20200428175129.634352-1-mic@digikod.net/
>>
>> The previous version (v4) is
>> https://lore.kernel.org/lkml/20200430132320.699508-1-mic@digikod.net/
> 
> 
> Hi Michael
> 
> I have couple of question
> 1. Why you did not add O_MAYEXEC to open()?
> Some time ago (around v4.14) open() did not return EINVAL when
> VALID_OPEN_FLAGS check failed.
> Now it does, so I do not see a problem that interpreter will use
> simple open(),  ( Although that path might be manipulated, but file
> contents will be verified by IMA)

Aleksa replied to this.

> 2. When you apply a new flag to mount, it means that IMA will check
> all files under this mount and it does not matter whether the file in
> question is a script or not.
> IMHO it is too hard overhead for performance reasons.

This patch series doesn't change the way IMA handles mount points.

> 
> Regards,
> LEv
> 

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

On 07/05/2020 10:05, David Laight wrote:
> From: Mickaël Salaün
>> Sent: 05 May 2020 16:32
>>
>> This fifth patch series add new kernel configurations (OMAYEXEC_STATIC,
>> OMAYEXEC_ENFORCE_MOUNT, and OMAYEXEC_ENFORCE_FILE) to enable to
>> configure the security policy at kernel build time.  As requested by
>> Mimi Zohar, I completed the series with one of her patches for IMA.
>>
>> 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.
>>
>> A simple system-wide security policy can be enforced by the system
>> administrator through a sysctl configuration consistent with the mount
>> points or the file access rights.  The documentation patch explains the
>> prerequisites.
>>
>> Furthermore, the security policy can also be delegated to an LSM, either
>> a MAC system or an integrity system.  For instance, the new kernel
>> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
>> integrity gap by bringing the ability to check the use of scripts [1].
>> Other uses are expected, such as for openat2(2) [2], SGX integration
>> [3], bpffs [4] or IPE [5].
>>
>> Userspace needs to adapt to take advantage of this new feature.  For
>> example, the PEP 578 [6] (Runtime Audit Hooks) enables Python 3.8 to be
>> extended with policy enforcement points related to code interpretation,
>> which can be used to align with the PowerShell audit features.
>> Additional Python security improvements (e.g. a limited interpreter
>> withou -c, stdin piping of code) are on their way.
>>
>> The initial idea come from CLIP OS 4 and the original implementation has
>> been used for more than 12 years:
>> https://github.com/clipos-archive/clipos4_doc
>>
>> An introduction to O_MAYEXEC was given at the Linux Security Summit
>> Europe 2018 - Linux Kernel Security Contributions by ANSSI:
>> https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
>> The "write xor execute" principle was explained at Kernel Recipes 2018 -
>> CLIP OS: a defense-in-depth OS:
>> https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
>>
>> This patch series can be applied on top of v5.7-rc4.  This can be tested
>> with CONFIG_SYSCTL.  I would really appreciate constructive comments on
>> this patch series.
> 
> None of that description actually says what the patch actually does.

"Add support for O_MAYEXEC" "to enable to control script execution".
What is not clear here? This seems well understood by other commenters.
The documentation patch and the talks can also help.

RE: [PATCH v5 0/6] Add support for O_MAYEXEC

[David Laight]

From: Mickaël Salaün
> Sent: 07 May 2020 09:37
...
> > None of that description actually says what the patch actually does.
> 
> "Add support for O_MAYEXEC" "to enable to control script execution".
> What is not clear here? This seems well understood by other commenters.
> The documentation patch and the talks can also help.

I'm guessing that passing O_MAYEXEC to open() requests the kernel
check for execute 'x' permissions (as well as read).

Then kernel policy determines whether 'read' access is actually enough,
or whether 'x' access (possibly masked by mount permissions) is needed.

If that is true, two lines say what is does.

Have you ever set a shell script permissions to --x--s--x ?
Ends up being executable by everyone except the owner!
Having the kernel pass all '#!' files to their interpreters
through an open fd might help security.
In that case the user doesn't need read access to the file
in order to get an interpreter to process it.
(You'd need to stop strace showing the contents to actually
hide them.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

On 07/05/2020 11:00, David Laight wrote:
> From: Mickaël Salaün
>> Sent: 07 May 2020 09:37
> ...
>>> None of that description actually says what the patch actually does.
>>
>> "Add support for O_MAYEXEC" "to enable to control script execution".
>> What is not clear here? This seems well understood by other commenters.
>> The documentation patch and the talks can also help.
> 
> I'm guessing that passing O_MAYEXEC to open() requests the kernel
> check for execute 'x' permissions (as well as read).

Yes, but only with openat2().

> 
> Then kernel policy determines whether 'read' access is actually enough,
> or whether 'x' access (possibly masked by mount permissions) is needed.
> 
> If that is true, two lines say what is does.

The "A simple system-wide security policy" paragraph introduce that, but
I'll highlight it in the next cover letter. The most important point is
to understand why it is required, before getting to how it will be
implemented.

> 
> Have you ever set a shell script permissions to --x--s--x ?
> Ends up being executable by everyone except the owner!

In this case the script is indeed executable but it can't be executed
because the interpreter (i.e. the user) needs to be able to read the
file. Of course, if the user has CAP_DAC_OVERRIDE (like the root user),
read is still allowed.

> Having the kernel pass all '#!' files to their interpreters
> through an open fd might help security.

This is interesting but it doesn't address the current issue: being able
to have a consistent (script) executability system policy. Maybe its
this point of view which wasn't clear enough?

> In that case the user doesn't need read access to the file
> in order to get an interpreter to process it.

Yes, but this brings security issues, because the interpreter (i.e. the
user) would then be able to read files without read permission.

> (You'd need to stop strace showing the contents to actually
> hide them.)

It doesn't matter if the process is traced or not, the kernel handles
impersonation scopes thanks to ptrace_may_access().

RE: [PATCH v5 0/6] Add support for O_MAYEXEC

[David Laight]

From: Mickaël Salaün <mic@digikod.net>
> Sent: 07 May 2020 10:30
> On 07/05/2020 11:00, David Laight wrote:
> > From: Mickaël Salaün
> >> Sent: 07 May 2020 09:37
> > ...
> >>> None of that description actually says what the patch actually does.
> >>
> >> "Add support for O_MAYEXEC" "to enable to control script execution".
> >> What is not clear here? This seems well understood by other commenters.
> >> The documentation patch and the talks can also help.
> >
> > I'm guessing that passing O_MAYEXEC to open() requests the kernel
> > check for execute 'x' permissions (as well as read).
> 
> Yes, but only with openat2().

It can't matter if the flag is ignored.
It just means the kernel isn't enforcing the policy.
If openat2() fail because the flag is unsupported then
the application will need to retry without the flag.

So if the user has any ability create executable files this
is all pointless (from a security point of view).
The user can either copy the file or copy in an interpreter
that doesn't request O_MAYEXEC.

It might stop accidental issues, but nothing malicious.

> > Then kernel policy determines whether 'read' access is actually enough,
> > or whether 'x' access (possibly masked by mount permissions) is needed.
> >
> > If that is true, two lines say what is does.
> 
> The "A simple system-wide security policy" paragraph introduce that, but
> I'll highlight it in the next cover letter.

No it doesn't.
It just says there is some kind of policy that some flags change.
It doesn't say what is being checked for.

> The most important point is
> to understand why it is required, before getting to how it will be
> implemented.

But you don't say what is required.
Just a load of buzzword ramblings.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mickaël Salaün]

On 07/05/2020 11:44, David Laight wrote:
> From: Mickaël Salaün <mic@digikod.net>
>> Sent: 07 May 2020 10:30
>> On 07/05/2020 11:00, David Laight wrote:
>>> From: Mickaël Salaün
>>>> Sent: 07 May 2020 09:37
>>> ...
>>>>> None of that description actually says what the patch actually does.
>>>>
>>>> "Add support for O_MAYEXEC" "to enable to control script execution".
>>>> What is not clear here? This seems well understood by other commenters.
>>>> The documentation patch and the talks can also help.
>>>
>>> I'm guessing that passing O_MAYEXEC to open() requests the kernel
>>> check for execute 'x' permissions (as well as read).
>>
>> Yes, but only with openat2().
> 
> It can't matter if the flag is ignored.
> It just means the kernel isn't enforcing the policy.
> If openat2() fail because the flag is unsupported then
> the application will need to retry without the flag.

I don't get what you want to prove. Please read carefully the cover
letter, the use case and the threat model.

> 
> So if the user has any ability create executable files this
> is all pointless (from a security point of view).
> The user can either copy the file or copy in an interpreter
> that doesn't request O_MAYEXEC.>
> It might stop accidental issues, but nothing malicious.

The execute permission (like the write permission) does not only depends
on the permission set on files, but it also depends on the
options/permission of their mount points, the MAC policy, etc. The
initial use case to enforce O_MAYEXEC is to rely on the noexec mount option.

If you want a consistent policy, you need to make one. Only dealing with
file properties may not be enough. This is explain in the cover letter
and the patches. If you allow all users to write and execute their
files, then there is no point in enforcing anything with O_MAYEXEC.

> 
>>> Then kernel policy determines whether 'read' access is actually enough,
>>> or whether 'x' access (possibly masked by mount permissions) is needed.
>>>
>>> If that is true, two lines say what is does.
>>
>> The "A simple system-wide security policy" paragraph introduce that, but
>> I'll highlight it in the next cover letter.
> 
> No it doesn't.
> It just says there is some kind of policy that some flags change.
> It doesn't say what is being checked for.

It said "the mount points or the file access rights". Please take a look
at the documentation patch.

> 
>> The most important point is
>> to understand why it is required, before getting to how it will be
>> implemented.
> 
> But you don't say what is required.

A consistent policy. Please take a look at the documentation patch which
explains the remaining prerequisites. You can also take a look at the
talks for further details.

> Just a load of buzzword ramblings.

It is a summary. Can you please suggest something better?

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Lev R. Oshvang .]

On Thu, May 7, 2020 at 4:38 PM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 07/05/2020 11:44, David Laight wrote:
> > From: Mickaël Salaün <mic@digikod.net>
> >> Sent: 07 May 2020 10:30
> >> On 07/05/2020 11:00, David Laight wrote:
> >>> From: Mickaël Salaün
> >>>> Sent: 07 May 2020 09:37
> >>> ...
> >>>>> None of that description actually says what the patch actually does.
> >>>>
> >>>> "Add support for O_MAYEXEC" "to enable to control script execution".
> >>>> What is not clear here? This seems well understood by other commenters.
> >>>> The documentation patch and the talks can also help.
> >>>
> >>> I'm guessing that passing O_MAYEXEC to open() requests the kernel
> >>> check for execute 'x' permissions (as well as read).
> >>
> >> Yes, but only with openat2().
> >
> > It can't matter if the flag is ignored.
> > It just means the kernel isn't enforcing the policy.
> > If openat2() fail because the flag is unsupported then
> > the application will need to retry without the flag.
>
> I don't get what you want to prove. Please read carefully the cover
> letter, the use case and the threat model.
>
> >
> > So if the user has any ability create executable files this
> > is all pointless (from a security point of view).
> > The user can either copy the file or copy in an interpreter
> > that doesn't request O_MAYEXEC.>
> > It might stop accidental issues, but nothing malicious.
>
> The execute permission (like the write permission) does not only depends
> on the permission set on files, but it also depends on the
> options/permission of their mount points, the MAC policy, etc. The
> initial use case to enforce O_MAYEXEC is to rely on the noexec mount option.
>
> If you want a consistent policy, you need to make one. Only dealing with
> file properties may not be enough. This is explain in the cover letter
> and the patches. If you allow all users to write and execute their
> files, then there is no point in enforcing anything with O_MAYEXEC.
>
> >
> >>> Then kernel policy determines whether 'read' access is actually enough,
> >>> or whether 'x' access (possibly masked by mount permissions) is needed.
> >>>
> >>> If that is true, two lines say what is does.
> >>
> >> The "A simple system-wide security policy" paragraph introduce that, but
> >> I'll highlight it in the next cover letter.
> >
> > No it doesn't.
> > It just says there is some kind of policy that some flags change.
> > It doesn't say what is being checked for.
>
> It said "the mount points or the file access rights". Please take a look
> at the documentation patch.
>
> >
> >> The most important point is
> >> to understand why it is required, before getting to how it will be
> >> implemented.
> >
> > But you don't say what is required.
>
> A consistent policy. Please take a look at the documentation patch which
> explains the remaining prerequisites. You can also take a look at the
> talks for further details.
>
> > Just a load of buzzword ramblings.
>
> It is a summary. Can you please suggest something better?

I can suggest something better ( I believe)
Some time ago I proposed patch to IMA -  Add suffix in IMA policy rule criteria
It allows IMA to verify scripts, configuration files and even single file.
It is very simple and does not depend on open flags.
Mimi Zohar decided not to include this patch on the reason it tries to
protect the file name.
( Why ??).

https://lore.kernel.org/linux-integrity/20200330122434.GB28214@kl/

Re: [PATCH v5 0/6] Add support for O_MAYEXEC

[Mimi Zohar]

On Fri, 2020-05-08 at 10:15 +0300, Lev R. Oshvang . wrote:

> I can suggest something better ( I believe)
> Some time ago I proposed patch to IMA -  Add suffix in IMA policy rule criteria
> It allows IMA to verify scripts, configuration files and even single file.
> It is very simple and does not depend on open flags.
> Mimi Zohar decided not to include this patch on the reason it tries to
> protect the file name.
> ( Why ??).

Your patch relies on the filename, but does nothing to protect it. 

Mimi

Re: [PATCH v5 1/6] fs: Add support for an O_MAYEXEC flag on openat2(2)

[Kees Cook]

On Tue, May 05, 2020 at 05:31:51PM +0200, Mickaël Salaün wrote:
> When the O_MAYEXEC flag is passed, openat2(2) may be subject to
> additional restrictions depending on a security policy managed by the
> kernel through a sysctl or implemented by an LSM thanks to the
> inode_permission hook.  This new flag is ignored by open(2) and
> openat(2).
> 
> The underlying idea is to be able to restrict scripts interpretation
> according to a policy defined by the system administrator.  For this to
> be possible, script interpreters must use the O_MAYEXEC flag
> appropriately.  To be fully effective, these interpreters also need to
> handle the other ways to execute code: command line parameters (e.g.,
> option -e for Perl), module loading (e.g., option -m for Python), stdin,
> file sourcing, environment variables, configuration files, etc.
> According to the threat model, it may be acceptable to allow some script
> interpreters (e.g. Bash) to interpret commands from stdin, may it be a
> TTY or a pipe, because it may not be enough to (directly) perform
> syscalls.  Further documentation can be found in a following patch.

You touch on this lightly in the cover letter, but it seems there are
plans for Python to restrict stdin parsing? Are there patches pending
anywhere for other interpreters? (e.g. does CLIP OS have such patches?)

There's always a push-back against adding features that have external
dependencies, and then those external dependencies can't happen without
the kernel first adding a feature. :) I like getting these catch-22s
broken, and I think the kernel is the right place to start, especially
since the threat model (and implementation) is already proven out in
CLIP OS, and now with IMA. So, while the interpreter side of this is
still under development, this gives them the tool they need to get it
done on the kernel side. So showing those pieces (as you've done) is
great, and I think finding a little bit more detail here would be even
better.

> A simple security policy implementation, configured through a dedicated
> sysctl, is available in a following patch.
> 
> This is an updated subset of the patch initially written by Vincent
> Strubel for CLIP OS 4:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than 11 years with customized script
> interpreters.  Some examples (with the original name O_MAYEXEC) can be
> found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>

nit: this needs to be reordered. It's expected that the final SoB
matches the sender. If you're trying to show co-authorship, please
see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Based on what I've inferred about author ordering, I think you want:

Co-developed-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
Co-developed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
Co-developed-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Mickaël Salaün <mic@digikod.net>

> Reviewed-by: Deven Bowers <deven.desai@linux.microsoft.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>

Everything else appears good to me, but Al and Aleksa know VFS internals
way better. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v5 2/6] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property

[Kees Cook]

On Tue, May 05, 2020 at 05:31:52PM +0200, Mickaël Salaün wrote:
> This new MAY_EXECMOUNT flag enables to check if the underlying mount
> point of an inode is marked as executable.  This is useful to implement
> a security policy taking advantage of the noexec mount option.
> 
> This flag is set according to path_noexec(), which checks if a mount
> point is mounted with MNT_NOEXEC or if the underlying superblock is
> SB_I_NOEXEC.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> ---
>  fs/namei.c         | 2 ++
>  include/linux/fs.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a320371899cf..33b6d372e74a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2849,6 +2849,8 @@ static int may_open(const struct path *path, int acc_mode, int flag)
>  		break;
>  	}
>  
> +	/* Pass the mount point executability. */
> +	acc_mode |= path_noexec(path) ? 0 : MAY_EXECMOUNT;
>  	error = inode_permission(inode, MAY_OPEN | acc_mode);
>  	if (error)
>  		return error;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 313c934de9ee..79435fca6c3e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
>  #define MAY_NOT_BLOCK		0x00000080
>  /* the inode is opened with O_MAYEXEC */
>  #define MAY_OPENEXEC		0x00000100
> +/* the mount point is marked as executable */
> +#define MAY_EXECMOUNT		0x00000200
>  
>  /*
>   * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond

I find this name unintuitive, but I cannot think of anything better,
since I think my problem is that "MAY" doesn't map to the language I
want to use to describe what this flag is indicating.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v5 1/6] fs: Add support for an O_MAYEXEC flag on openat2(2)

[Christian Heimes]

On 12/05/2020 23.05, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:51PM +0200, Mickaël Salaün wrote:
>> When the O_MAYEXEC flag is passed, openat2(2) may be subject to
>> additional restrictions depending on a security policy managed by the
>> kernel through a sysctl or implemented by an LSM thanks to the
>> inode_permission hook.  This new flag is ignored by open(2) and
>> openat(2).
>>
>> The underlying idea is to be able to restrict scripts interpretation
>> according to a policy defined by the system administrator.  For this to
>> be possible, script interpreters must use the O_MAYEXEC flag
>> appropriately.  To be fully effective, these interpreters also need to
>> handle the other ways to execute code: command line parameters (e.g.,
>> option -e for Perl), module loading (e.g., option -m for Python), stdin,
>> file sourcing, environment variables, configuration files, etc.
>> According to the threat model, it may be acceptable to allow some script
>> interpreters (e.g. Bash) to interpret commands from stdin, may it be a
>> TTY or a pipe, because it may not be enough to (directly) perform
>> syscalls.  Further documentation can be found in a following patch.
> 
> You touch on this lightly in the cover letter, but it seems there are
> plans for Python to restrict stdin parsing? Are there patches pending
> anywhere for other interpreters? (e.g. does CLIP OS have such patches?)
> 
> There's always a push-back against adding features that have external
> dependencies, and then those external dependencies can't happen without
> the kernel first adding a feature. :) I like getting these catch-22s
> broken, and I think the kernel is the right place to start, especially
> since the threat model (and implementation) is already proven out in
> CLIP OS, and now with IMA. So, while the interpreter side of this is
> still under development, this gives them the tool they need to get it
> done on the kernel side. So showing those pieces (as you've done) is
> great, and I think finding a little bit more detail here would be even
> better.

Hi,

Python core dev here.

Yes, there are plans to use feature for Python in combination with
additional restrictions. For backwards compatibility reasons we cannot
change the behavior of the default Python interpreter. I have plans to
provide a restricted Python binary that prohibits piping from stdin,
disables -c "some_code()", restricts import locations, and a couple of
other things. O_MAYEXEC flag makes it easier to block imports from
noexec filesystems.

My PoC [1] for a talk [2] last year is inspired by IMA appraisal and a
previous talk by Mickaël on O_MAYEXEC.

Christian

[1] https://github.com/zooba/spython/blob/master/linux_xattr/spython.c
[2]
https://speakerdeck.com/tiran/europython-2019-auditing-hooks-and-security-transparency-for-cpython

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Kees Cook]

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

Re: [PATCH v5 4/6] selftest/openat2: Add tests for O_MAYEXEC enforcing

[Kees Cook]

On Tue, May 05, 2020 at 05:31:54PM +0200, Mickaël Salaün wrote:
> Test propagation of noexec mount points or file executability through
> files open with or without O_MAYEXEC, thanks to the
> fs.open_mayexec_enforce sysctl.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Shuah Khan <shuah@kernel.org>

Yay tests! :) Notes below...

> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 4b93b1417b86..cb98bdb4d5b1 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  
>  CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
> -TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
> +LDLIBS += -lcap
> +TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test omayexec_test

I realize the others have _test in their name, but that feels intensely
redundant to me. :)

> [...]
> diff --git a/tools/testing/selftests/openat2/omayexec_test.c b/tools/testing/selftests/openat2/omayexec_test.c
> new file mode 100644
> index 000000000000..7052c852daf8
> --- /dev/null
> +++ b/tools/testing/selftests/openat2/omayexec_test.c
> [...]
> +FIXTURE_DATA(mount_exec_file_exec) { };

For each of these, Please use "FIXTURE" not "FIXTURE_DATA". See:
1ae81d78a8b2 ("selftests/seccomp: Adjust test fixture counts")

> +FIXTURE_SETUP(mount_exec_file_exec)
> +{
> +	create_workspace(_metadata, 1, 1);

Maybe save the system's original sysctl in create_workspace() instead
of always restoring it to 0 in delete_workspace()?

Otherwise, looks good!

-- 
Kees Cook

Re: [PATCH v5 5/6] doc: Add documentation for the fs.open_mayexec_enforce sysctl

[Kees Cook]

On Tue, May 05, 2020 at 05:31:55PM +0200, Mickaël Salaün wrote:
> This sysctl enables to propagate executable permission to userspace
> thanks to the O_MAYEXEC flag.
> 
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Kees Cook <keescook@chromium.org>

I think this should be folded into the patch that adds the sysctl.

-- 
Kees Cook

Re: [PATCH v5 1/6] fs: Add support for an O_MAYEXEC flag on openat2(2)

[Kees Cook]

On Tue, May 12, 2020 at 11:40:35PM +0200, Christian Heimes wrote:
> On 12/05/2020 23.05, Kees Cook wrote:
> > On Tue, May 05, 2020 at 05:31:51PM +0200, Mickaël Salaün wrote:
> >> When the O_MAYEXEC flag is passed, openat2(2) may be subject to
> >> additional restrictions depending on a security policy managed by the
> >> kernel through a sysctl or implemented by an LSM thanks to the
> >> inode_permission hook.  This new flag is ignored by open(2) and
> >> openat(2).
> >>
> >> The underlying idea is to be able to restrict scripts interpretation
> >> according to a policy defined by the system administrator.  For this to
> >> be possible, script interpreters must use the O_MAYEXEC flag
> >> appropriately.  To be fully effective, these interpreters also need to
> >> handle the other ways to execute code: command line parameters (e.g.,
> >> option -e for Perl), module loading (e.g., option -m for Python), stdin,
> >> file sourcing, environment variables, configuration files, etc.
> >> According to the threat model, it may be acceptable to allow some script
> >> interpreters (e.g. Bash) to interpret commands from stdin, may it be a
> >> TTY or a pipe, because it may not be enough to (directly) perform
> >> syscalls.  Further documentation can be found in a following patch.
> > 
> > You touch on this lightly in the cover letter, but it seems there are
> > plans for Python to restrict stdin parsing? Are there patches pending
> > anywhere for other interpreters? (e.g. does CLIP OS have such patches?)
> > 
> > There's always a push-back against adding features that have external
> > dependencies, and then those external dependencies can't happen without
> > the kernel first adding a feature. :) I like getting these catch-22s
> > broken, and I think the kernel is the right place to start, especially
> > since the threat model (and implementation) is already proven out in
> > CLIP OS, and now with IMA. So, while the interpreter side of this is
> > still under development, this gives them the tool they need to get it
> > done on the kernel side. So showing those pieces (as you've done) is
> > great, and I think finding a little bit more detail here would be even
> > better.
> 
> Hi,
> 
> Python core dev here.
> 
> Yes, there are plans to use feature for Python in combination with
> additional restrictions. For backwards compatibility reasons we cannot
> change the behavior of the default Python interpreter. I have plans to
> provide a restricted Python binary that prohibits piping from stdin,
> disables -c "some_code()", restricts import locations, and a couple of
> other things. O_MAYEXEC flag makes it easier to block imports from
> noexec filesystems.
> 
> My PoC [1] for a talk [2] last year is inspired by IMA appraisal and a
> previous talk by Mickaël on O_MAYEXEC.
> 
> Christian
> 
> [1] https://github.com/zooba/spython/blob/master/linux_xattr/spython.c
> [2]
> https://speakerdeck.com/tiran/europython-2019-auditing-hooks-and-security-transparency-for-cpython

Ah, fantastic; thank you! Yes, this will go a long way for helping
demonstration to other folks that there are people who will be using
this feature. :)

-- 
Kees Cook

Re: [PATCH v5 1/6] fs: Add support for an O_MAYEXEC flag on openat2(2)

[Mickaël Salaün]

On 12/05/2020 23:05, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:51PM +0200, Mickaël Salaün wrote:
>> When the O_MAYEXEC flag is passed, openat2(2) may be subject to
>> additional restrictions depending on a security policy managed by the
>> kernel through a sysctl or implemented by an LSM thanks to the
>> inode_permission hook.  This new flag is ignored by open(2) and
>> openat(2).
>>
>> The underlying idea is to be able to restrict scripts interpretation
>> according to a policy defined by the system administrator.  For this to
>> be possible, script interpreters must use the O_MAYEXEC flag
>> appropriately.  To be fully effective, these interpreters also need to
>> handle the other ways to execute code: command line parameters (e.g.,
>> option -e for Perl), module loading (e.g., option -m for Python), stdin,
>> file sourcing, environment variables, configuration files, etc.
>> According to the threat model, it may be acceptable to allow some script
>> interpreters (e.g. Bash) to interpret commands from stdin, may it be a
>> TTY or a pipe, because it may not be enough to (directly) perform
>> syscalls.  Further documentation can be found in a following patch.
> 
> You touch on this lightly in the cover letter, but it seems there are
> plans for Python to restrict stdin parsing? Are there patches pending
> anywhere for other interpreters? (e.g. does CLIP OS have such patches?)

There is some example from CLIP OS 4 here :
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
If you take a look at the whole pointed patches there is more than the
O_MAYEXEC changes (which matches this search) e.g., to prevent Python
interactive execution. There is patches for Bash, Wine, Java (Icedtea),
Busybox's ash, Perl and Python. There is also some related patches which
do not directly rely on O_MAYEXEC but which restrict the use of browser
plugins and extensions, which may be seen as scripts too:
https://github.com/clipos-archive/clipos4_portage-overlay/tree/master/www-client

> 
> There's always a push-back against adding features that have external
> dependencies, and then those external dependencies can't happen without
> the kernel first adding a feature. :) I like getting these catch-22s
> broken, and I think the kernel is the right place to start, especially
> since the threat model (and implementation) is already proven out in
> CLIP OS, and now with IMA. So, while the interpreter side of this is
> still under development, this gives them the tool they need to get it
> done on the kernel side. So showing those pieces (as you've done) is
> great, and I think finding a little bit more detail here would be even
> better.

OK, I can add my previous comment in the next cover letter.

> 
>> A simple security policy implementation, configured through a dedicated
>> sysctl, is available in a following patch.
>>
>> This is an updated subset of the patch initially written by Vincent
>> Strubel for CLIP OS 4:
>> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
>> This patch has been used for more than 11 years with customized script
>> interpreters.  Some examples (with the original name O_MAYEXEC) can be
>> found here:
>> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> 
> nit: this needs to be reordered. It's expected that the final SoB
> matches the sender.

OK, I just sorted the list alphabetically.

> If you're trying to show co-authorship, please
> see:
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
> Based on what I've inferred about author ordering, I think you want:
> 
> Co-developed-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> Signed-off-by: Vincent Strubel <vincent.strubel@ssi.gouv.fr>
> Co-developed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Signed-off-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Co-developed-by: Mickaël Salaün <mic@digikod.net>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>

OK, according to the doc I'll remove myself as Co-developped-by because
I'm already in the From, though.

> 
>> Reviewed-by: Deven Bowers <deven.desai@linux.microsoft.com>
>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Kees Cook <keescook@chromium.org>
> 
> Everything else appears good to me, but Al and Aleksa know VFS internals
> way better. :)
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 

Thanks!

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Mickaël Salaün]

On 12/05/2020 23:48, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:53PM +0200, Mickaël Salaün wrote:
>> Enable to forbid access to files open with O_MAYEXEC.  Thanks to the
>> noexec option from the underlying VFS mount, or to the file execute
>> permission, userspace can enforce these execution policies.  This may
>> allow script interpreters to check execution permission before reading
>> commands from a file, or dynamic linkers to allow shared object loading.
> 
> Some language tailoring. I might change the first sentence to:
> 
> Allow for the enforcement of the O_MAYEXEC openat2(2) flag.

OK

> 
>> Add a new sysctl fs.open_mayexec_enforce to enable system administrators
>> to enforce two complementary security policies according to the
>> installed system: enforce the noexec mount option, and enforce
>> executable file permission.  Indeed, because of compatibility with
>> installed systems, only system administrators are able to check that
>> this new enforcement is in line with the system mount points and file
>> permissions.  A following patch adds documentation.
>>
>> For tailored Linux distributions, it is possible to enforce such
>> restriction at build time thanks to the CONFIG_OMAYEXEC_STATIC option.
>> The policy can then be configured with CONFIG_OMAYEXEC_ENFORCE_MOUNT and
>> CONFIG_OMAYEXEC_ENFORCE_FILE.
> 
> OMAYEXEC feels like the wrong name here. Maybe something closer to the
> sysctl name? CONFIG_OPEN_MAYEXEC?
> 
> And I think it's not needed to have 3 configs for this. That's a lot of
> mess for a corner case option. I think I would model this after other
> sysctl CONFIGs, and just call this CONFIG_OPEN_MAYEXEC_DEFAULT.
OK, I guess you mean to store the default integer value of the sysctl in
this config option.

> 
> Is _disabling_ the sysctl needed? This patch gets much smaller without
> the ..._STATIC bit. (And can we avoid "static", it means different
> things to different people. How about invert the logic and call it
> CONFIG_OPEN_MAYEXEC_SYSCTL?)

I added this in response to James's comment:
https://lore.kernel.org/lkml/alpine.LRH.2.21.2005020405210.5924@namei.org/
I'm fine to let the sysctl visible whatever the kernel config is. It
makes the code simpler. I guess tailored security distros already
protect sysctl entries anyway.

> 
> Further notes below...
> 
>> [...]
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 33b6d372e74a..70f179f6bc6c 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -39,6 +39,7 @@
>>  #include <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.
> 

Re: [PATCH v5 4/6] selftest/openat2: Add tests for O_MAYEXEC enforcing

[Mickaël Salaün]

On 12/05/2020 23:57, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:54PM +0200, Mickaël Salaün wrote:
>> Test propagation of noexec mount points or file executability through
>> files open with or without O_MAYEXEC, thanks to the
>> fs.open_mayexec_enforce sysctl.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Shuah Khan <shuah@kernel.org>
> 
> Yay tests! :) Notes below...
> 
>> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
>> index 4b93b1417b86..cb98bdb4d5b1 100644
>> --- a/tools/testing/selftests/openat2/Makefile
>> +++ b/tools/testing/selftests/openat2/Makefile
>> @@ -1,7 +1,8 @@
>>  # SPDX-License-Identifier: GPL-2.0-or-later
>>  
>>  CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>> -TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>> +LDLIBS += -lcap
>> +TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test omayexec_test
> 
> I realize the others have _test in their name, but that feels intensely
> redundant to me. :)

It is redundant in the path name but it is useful to match the generated
files e.g., in gitignore.

> 
>> [...]
>> diff --git a/tools/testing/selftests/openat2/omayexec_test.c b/tools/testing/selftests/openat2/omayexec_test.c
>> new file mode 100644
>> index 000000000000..7052c852daf8
>> --- /dev/null
>> +++ b/tools/testing/selftests/openat2/omayexec_test.c
>> [...]
>> +FIXTURE_DATA(mount_exec_file_exec) { };
> 
> For each of these, Please use "FIXTURE" not "FIXTURE_DATA". See:
> 1ae81d78a8b2 ("selftests/seccomp: Adjust test fixture counts")

Indeed.

> 
>> +FIXTURE_SETUP(mount_exec_file_exec)
>> +{
>> +	create_workspace(_metadata, 1, 1);
> 
> Maybe save the system's original sysctl in create_workspace() instead
> of always restoring it to 0 in delete_workspace()?

Right.

> 
> Otherwise, looks good!
> 

Thanks.

Re: [PATCH v5 5/6] doc: Add documentation for the fs.open_mayexec_enforce sysctl

[Mickaël Salaün]

On 13/05/2020 00:00, Kees Cook wrote:
> On Tue, May 05, 2020 at 05:31:55PM +0200, Mickaël Salaün wrote:
>> This sysctl enables to propagate executable permission to userspace
>> thanks to the O_MAYEXEC flag.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Jonathan Corbet <corbet@lwn.net>
>> Cc: Kees Cook <keescook@chromium.org>
> 
> I think this should be folded into the patch that adds the sysctl.

OK, I guess it is fine for the documentation maintainers too?

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Stephen Smalley]

On Tue, May 5, 2020 at 11:33 AM Mickaël Salaün <mic@digikod.net> 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 <mic@digikod.net>
> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> ---

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

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Kees Cook]

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 <mic@digikod.net> 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 <mic@digikod.net>
> > Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> 
> > 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

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Kees Cook]

On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> Like, couldn't just the entire thing just be:
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a320371899cf..0ab18e19f5da 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
>  		break;
>  	}
>  
> +	if (unlikely(mask & MAY_OPENEXEC)) {
> +		if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> +		    path_noexec(path))
> +			return -EACCES;
> +		if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> +			acc_mode |= MAY_EXEC;
> +	}
>  	error = inode_permission(inode, MAY_OPEN | acc_mode);
>  	if (error)
>  		return error;
> 

FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
reduced to this plus the Kconfig and sysctl changes, the self tests
pass.

I think this makes things much cleaner and correct.

-- 
Kees Cook

Re: [PATCH v5 2/6] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property

[Lev R. Oshvang .]

On Wed, May 13, 2020 at 12:09 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 05, 2020 at 05:31:52PM +0200, Mickaël Salaün wrote:
> > This new MAY_EXECMOUNT flag enables to check if the underlying mount
> > point of an inode is marked as executable.  This is useful to implement
> > a security policy taking advantage of the noexec mount option.
> >
Security policy is expressed by sysadmin by mount -noexec very clear,
I don't think there is a need
in sysctl, wish is system-wide

> > This flag is set according to path_noexec(), which checks if a mount
> > point is mounted with MNT_NOEXEC or if the underlying superblock is
> > SB_I_NOEXEC.
> >
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Reviewed-by: Philippe Trébuchet <philippe.trebuchet@ssi.gouv.fr>
> > Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> >  fs/namei.c         | 2 ++
> >  include/linux/fs.h | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index a320371899cf..33b6d372e74a 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2849,6 +2849,8 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> >               break;
> >       }
> >
> > +     /* Pass the mount point executability. */
> > +     acc_mode |= path_noexec(path) ? 0 : MAY_EXECMOUNT;
> >       error = inode_permission(inode, MAY_OPEN | acc_mode);
> >       if (error)
> >               return error;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 313c934de9ee..79435fca6c3e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -103,6 +103,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> >  #define MAY_NOT_BLOCK                0x00000080
> >  /* the inode is opened with O_MAYEXEC */
> >  #define MAY_OPENEXEC         0x00000100
> > +/* the mount point is marked as executable */
> > +#define MAY_EXECMOUNT                0x00000200
> >
> >  /*
> >   * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
>
> I find this name unintuitive, but I cannot think of anything better,
> since I think my problem is that "MAY" doesn't map to the language I
> want to use to describe what this flag is indicating.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> --
> Kees Cook


I think that the original patch was perfect, I quite it again
@@ -3167,6 +3167,14 @@ static int may_open(struct path *path, int
acc_mode, int flag)

+
+ if ((acc_mode & MAY_OPENEXEC)
+ && (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))
+ && (path->mnt && (path->mnt->mnt_flags & MNT_NOEXEC)))
+            return -EACCES;
+
+
+
error = inode_permission(inode, MAY_OPEN | acc_mode);

As I said in the inline comment above, sysadmin had already express
security policy in a very clear way,
mount -noexec !
I would only check inside inode_permission() whether the file mode is
any  ---x  permission and deny such
open when file is opened with O_MAYEXEC under MNT_NOEXEC mount point

New sysctl is indeed required to allow userspace that places scripts
or libs under noexec mounts.
fs.mnt_noexec_strict =0 (allow, e) , 1 (deny any file with --x
permission), 2 (deny when O_MAYEXEC absent), for any file with ---x
permissions)

RE: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[David Laight]

From: Kees Cook
> Sent: 14 May 2020 04:05
> On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > Like, couldn't just the entire thing just be:
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index a320371899cf..0ab18e19f5da 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> >  		break;
> >  	}
> >
> > +	if (unlikely(mask & MAY_OPENEXEC)) {
> > +		if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > +		    path_noexec(path))
> > +			return -EACCES;
> > +		if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > +			acc_mode |= MAY_EXEC;
> > +	}
> >  	error = inode_permission(inode, MAY_OPEN | acc_mode);
> >  	if (error)
> >  		return error;
> >
> 
> FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> reduced to this plus the Kconfig and sysctl changes, the self tests
> pass.
> 
> I think this makes things much cleaner and correct.

And a summary of that would be right for the 0/n patch email.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Stephen Smalley]

On Wed, May 13, 2020 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > Like, couldn't just the entire thing just be:
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index a320371899cf..0ab18e19f5da 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> >               break;
> >       }
> >
> > +     if (unlikely(mask & MAY_OPENEXEC)) {
> > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > +                 path_noexec(path))
> > +                     return -EACCES;
> > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > +                     acc_mode |= MAY_EXEC;
> > +     }
> >       error = inode_permission(inode, MAY_OPEN | acc_mode);
> >       if (error)
> >               return error;
> >
>
> FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> reduced to this plus the Kconfig and sysctl changes, the self tests
> pass.
>
> I think this makes things much cleaner and correct.

I think that covers inode-based security modules but not path-based
ones (they don't implement the inode_permission hook).  For those, I
would tentatively guess that we need to make sure FMODE_EXEC is set on
the open file and then they need to check for that in their file_open
hooks.

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Kees Cook]

On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> On Wed, May 13, 2020 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > Like, couldn't just the entire thing just be:
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index a320371899cf..0ab18e19f5da 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> > >               break;
> > >       }
> > >
> > > +     if (unlikely(mask & MAY_OPENEXEC)) {
> > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > +                 path_noexec(path))
> > > +                     return -EACCES;
> > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > +                     acc_mode |= MAY_EXEC;
> > > +     }
> > >       error = inode_permission(inode, MAY_OPEN | acc_mode);
> > >       if (error)
> > >               return error;
> > >
> >
> > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > reduced to this plus the Kconfig and sysctl changes, the self tests
> > pass.
> >
> > I think this makes things much cleaner and correct.
> 
> I think that covers inode-based security modules but not path-based
> ones (they don't implement the inode_permission hook).  For those, I
> would tentatively guess that we need to make sure FMODE_EXEC is set on
> the open file and then they need to check for that in their file_open
> hooks.

Does there need to be an FMODE_OPENEXEC, or is the presence of
FMODE_OPEN with FMODE_EXEC sufficient?

-- 
Kees Cook

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Kees Cook]

On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:
> On Wed, May 13, 2020 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > Like, couldn't just the entire thing just be:
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index a320371899cf..0ab18e19f5da 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> > >               break;
> > >       }
> > >
> > > +     if (unlikely(mask & MAY_OPENEXEC)) {
> > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > +                 path_noexec(path))
> > > +                     return -EACCES;
> > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > +                     acc_mode |= MAY_EXEC;
> > > +     }
> > >       error = inode_permission(inode, MAY_OPEN | acc_mode);
> > >       if (error)
> > >               return error;
> > >
> >
> > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > reduced to this plus the Kconfig and sysctl changes, the self tests
> > pass.
> >
> > I think this makes things much cleaner and correct.
> 
> I think that covers inode-based security modules but not path-based
> ones (they don't implement the inode_permission hook).  For those, I
> would tentatively guess that we need to make sure FMODE_EXEC is set on
> the open file and then they need to check for that in their file_open
> hooks.

I kept confusing myself about what order things happened in, so I made
these handy notes about the call graph:

openat2(dfd, char * filename, open_how)
    do_filp_open(dfd, filename, open_flags)
        path_openat(nameidata, open_flags, flags)
            do_open(nameidata, file, open_flags) 
                may_open(path, acc_mode, open_flag)
                    inode_permission(inode, MAY_OPEN | acc_mode)
                        security_inode_permission(inode, acc_mode)
                vfs_open(path, file)
                    do_dentry_open(file, path->dentry->d_inode, open)
                        if (unlikely(f->f_flags & FMODE_EXEC && !S_ISREG(inode->i_mode))) ...
                        security_file_open(f)
                        open()

So, it looks like adding FMODE_EXEC into f_flags in do_open() is needed in
addition to injecting MAY_EXEC into acc_mode in do_open()? Hmmm

-- 
Kees Cook

Re: [PATCH v5 2/6] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property

[Kees Cook]

On Thu, May 14, 2020 at 11:14:04AM +0300, Lev R. Oshvang . wrote:
> New sysctl is indeed required to allow userspace that places scripts
> or libs under noexec mounts.

But since this is a not-uncommon environment, we must have the sysctl
otherwise this change would break those systems.

> fs.mnt_noexec_strict =0 (allow, e) , 1 (deny any file with --x
> permission), 2 (deny when O_MAYEXEC absent), for any file with ---x
> permissions)

I don't think we want another mount option -- this is already fully
expressed with noexec and the system-wide sysctl.

-- 
Kees Cook

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Stephen Smalley]

On Thu, May 14, 2020 at 10:41 AM Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org> wrote:
> > >
> > > On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
> > > > Like, couldn't just the entire thing just be:
> > > >
> > > > diff --git a/fs/namei.c b/fs/namei.c
> > > > index a320371899cf..0ab18e19f5da 100644
> > > > --- a/fs/namei.c
> > > > +++ b/fs/namei.c
> > > > @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
> > > >               break;
> > > >       }
> > > >
> > > > +     if (unlikely(mask & MAY_OPENEXEC)) {
> > > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
> > > > +                 path_noexec(path))
> > > > +                     return -EACCES;
> > > > +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
> > > > +                     acc_mode |= MAY_EXEC;
> > > > +     }
> > > >       error = inode_permission(inode, MAY_OPEN | acc_mode);
> > > >       if (error)
> > > >               return error;
> > > >
> > >
> > > FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
> > > reduced to this plus the Kconfig and sysctl changes, the self tests
> > > pass.
> > >
> > > I think this makes things much cleaner and correct.
> >
> > I think that covers inode-based security modules but not path-based
> > ones (they don't implement the inode_permission hook).  For those, I
> > would tentatively guess that we need to make sure FMODE_EXEC is set on
> > the open file and then they need to check for that in their file_open
> > hooks.
>
> Does there need to be an FMODE_OPENEXEC, or is the presence of
> FMODE_OPEN with FMODE_EXEC sufficient?

I don't think we need an extra flag/mode bit.  But note that 1)
FMODE_OPENED isn't set until after security_file_open() is called so
we can't rely on it there, 2) __FMODE_EXEC aka FMODE_EXEC is set in
f_flags not f_mode, 3) FMODE_EXEC was originally introduced for
distributed filesystems so that they could return ETXTBUSY if the file
was opened for write and execute on different nodes, 4) AppArmor and
TOMOYO have special handling of execve based on current->in_execve so
I guess the only overlap would be for uselib(2).

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Stephen Smalley]

On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org> 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?

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Mickaël Salaün]

On 14/05/2020 18:10, Stephen Smalley wrote:
> On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org> wrote:
>>>>
>>>> On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:
>>>>> Like, couldn't just the entire thing just be:
>>>>>
>>>>> diff --git a/fs/namei.c b/fs/namei.c
>>>>> index a320371899cf..0ab18e19f5da 100644
>>>>> --- a/fs/namei.c
>>>>> +++ b/fs/namei.c
>>>>> @@ -2849,6 +2849,13 @@ static int may_open(const struct path *path, int acc_mode, int flag)
>>>>>               break;
>>>>>       }
>>>>>
>>>>> +     if (unlikely(mask & MAY_OPENEXEC)) {
>>>>> +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_MOUNT &&
>>>>> +                 path_noexec(path))
>>>>> +                     return -EACCES;
>>>>> +             if (sysctl_omayexec_enforce & OMAYEXEC_ENFORCE_FILE)
>>>>> +                     acc_mode |= MAY_EXEC;
>>>>> +     }
>>>>>       error = inode_permission(inode, MAY_OPEN | acc_mode);
>>>>>       if (error)
>>>>>               return error;
>>>>>
>>>>
>>>> FYI, I've confirmed this now. Effectively with patch 2 dropped, patch 3
>>>> reduced to this plus the Kconfig and sysctl changes, the self tests
>>>> pass.
>>>>
>>>> I think this makes things much cleaner and correct.
>>>
>>> I think that covers inode-based security modules but not path-based
>>> ones (they don't implement the inode_permission hook).  For those, I
>>> would tentatively guess that we need to make sure FMODE_EXEC is set on
>>> the open file and then they need to check for that in their file_open
>>> hooks.
>>
>> I kept confusing myself about what order things happened in, so I made
>> these handy notes about the call graph:
>>
>> openat2(dfd, char * filename, open_how)
>>     do_filp_open(dfd, filename, open_flags)
>>         path_openat(nameidata, open_flags, flags)
>>             do_open(nameidata, file, open_flags)
>>                 may_open(path, acc_mode, open_flag)
>>                     inode_permission(inode, MAY_OPEN | acc_mode)
>>                         security_inode_permission(inode, acc_mode)
>>                 vfs_open(path, file)
>>                     do_dentry_open(file, path->dentry->d_inode, open)
>>                         if (unlikely(f->f_flags & FMODE_EXEC && !S_ISREG(inode->i_mode))) ...
>>                         security_file_open(f)
>>                         open()
>>
>> So, it looks like adding FMODE_EXEC into f_flags in do_open() is needed in
>> addition to injecting MAY_EXEC into acc_mode in do_open()? Hmmm
> 
> Just do both in build_open_flags() and be done with it? Looks like he
> was already setting FMODE_EXEC in patch 1 so we just need to teach
> AppArmor/TOMOYO to check for it and perform file execute checking in
> that case if !current->in_execve?

I can postpone the file permission check for another series to make this
one simpler (i.e. mount noexec only). Because it depends on the sysctl
setting, it is OK to add this check later, if needed. In the meantime,
AppArmor and Tomoyo could be getting ready for this.

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Mickaël Salaün]

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 <mic@digikod.net> 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 <mic@digikod.net>
>>> Reviewed-by: Thibaut Sautereau <thibaut.sautereau@ssi.gouv.fr>
>>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> ---
>>
>>> 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().

Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC

[Tetsuo Handa]

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.

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 Thu, May 14, 2020 at 09:16:13PM +0200, Mickaël Salaün wrote:
> On 14/05/2020 18:10, Stephen Smalley wrote:
> > On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@chromium.org> wrote:
> >> So, it looks like adding FMODE_EXEC into f_flags in do_open() is needed in
> >> addition to injecting MAY_EXEC into acc_mode in do_open()? Hmmm
> > 
> > Just do both in build_open_flags() and be done with it? Looks like he
> > was already setting FMODE_EXEC in patch 1 so we just need to teach
> > AppArmor/TOMOYO to check for it and perform file execute checking in
> > that case if !current->in_execve?
> 
> I can postpone the file permission check for another series to make this
> one simpler (i.e. mount noexec only). Because it depends on the sysctl
> setting, it is OK to add this check later, if needed. In the meantime,
> AppArmor and Tomoyo could be getting ready for this.

So, after playing around with this series, investigating Stephen's
comments, digging through the existing FMODE_EXEC uses, and spending a
bit more time thinking about Lev and Aleksa's dislike of the sysctls, I've
got a much more radically simplified solution that I think could work.

Maybe I've missed some earlier discussion that ruled this out, but I
couldn't find it: let's just add O_EXEC and be done with it. It actually
makes the execve() path more like openat2() and is much cleaner after
a little refactoring. Here are the results, though I haven't emailed it
yet since I still want to do some more testing:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1

I look forward to flames! ;)

-- 
Kees Cook

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Florian Weimer]

* Kees Cook:

> Maybe I've missed some earlier discussion that ruled this out, but I
> couldn't find it: let's just add O_EXEC and be done with it. It actually
> makes the execve() path more like openat2() and is much cleaner after
> a little refactoring. Here are the results, though I haven't emailed it
> yet since I still want to do some more testing:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1

I think POSIX specifies O_EXEC in such a way that it does not confer
read permissions.  This seems incompatible with what we are trying to
achieve here.

Thanks,
Florian

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Mickaël Salaün]

On 15/05/2020 10:01, Kees Cook wrote:
> On Thu, May 14, 2020 at 09:16:13PM +0200, Mickaël Salaün wrote:
>> On 14/05/2020 18:10, Stephen Smalley wrote:
>>> On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@chromium.org> wrote:
>>>> So, it looks like adding FMODE_EXEC into f_flags in do_open() is needed in
>>>> addition to injecting MAY_EXEC into acc_mode in do_open()? Hmmm
>>>
>>> Just do both in build_open_flags() and be done with it? Looks like he
>>> was already setting FMODE_EXEC in patch 1 so we just need to teach
>>> AppArmor/TOMOYO to check for it and perform file execute checking in
>>> that case if !current->in_execve?
>>
>> I can postpone the file permission check for another series to make this
>> one simpler (i.e. mount noexec only). Because it depends on the sysctl
>> setting, it is OK to add this check later, if needed. In the meantime,
>> AppArmor and Tomoyo could be getting ready for this.
> 
> So, after playing around with this series, investigating Stephen's
> comments, digging through the existing FMODE_EXEC uses, and spending a
> bit more time thinking about Lev and Aleksa's dislike of the sysctls, I've
> got a much more radically simplified solution that I think could work.

Not having a sysctl would mean that distros will probably have to patch
script interpreters to remove the use of O_MAYEXEC. Or distros would
have to exclude newer version of script interpreters because they
implement O_MAYEXEC. Or distros would have to patch their kernel to
implement themselves the sysctl knob I'm already providing. Sysadmins
may not control the kernel build nor the user space build, they control
the system configuration (some mount point options and some file
execution permissions) but I guess that a distro update breaking a
running system is not acceptable. Either way, unfortunately, I think it
doesn't help anyone to not have a controlling sysctl. The same apply for
access-control LSMs relying on a security policy which can be defined by
sysadmins.

Your commits enforce file exec checks, which is a good thing from a
security point of view, but unfortunately that would requires distros to
update all the packages providing shared objects once the dynamic linker
uses O_MAYEXEC.

> 
> Maybe I've missed some earlier discussion that ruled this out, but I
> couldn't find it: let's just add O_EXEC and be done with it. It actually
> makes the execve() path more like openat2() and is much cleaner after
> a little refactoring. Here are the results, though I haven't emailed it
> yet since I still want to do some more testing:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1
> 
> I look forward to flames! ;)
> 

Like Florian said, O_EXEC is for execute-only (which obviously doesn't
work for scripts):
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
On the other hand, the semantic of O_MAYEXEC is complementary to other
O_* flags. It is inspired by the VM_MAYEXEC flag.

The O_EXEC flag is specified for open(2). openat2(2) is Linux-specific
and it is highly unlikely that new flags will be added to open(2) or
openat(2) because of compatibility issues.

FYI, musl implements O_EXEC on Linux with O_PATH:
https://www.openwall.com/lists/musl/2013/02/22/1
https://git.musl-libc.org/cgit/musl/commit/?id=6d05d862975188039e648273ceab350d9ab5b69e

However, the O_EXEC flag/semantic could be useful for the dynamic
linkers, i.e. to only be able to map files in an executable (and
read-only) way. If this is OK, then we may want to rename O_MAYEXEC to
something like O_INTERPRET. This way we could have two new flags for
sightly (but important) different use cases. The sysctl bitfield could
be extended to manage both of these flags.

Other than that, the other commits are interesting. I'm a bit worried
about the implication of the f_flags/f_mode change though.

From a practical point of view, I'm also wondering how you intent to
submit this series on LKML without conflicting with the current
O_MAYEXEC series (versions, changes…). I would like you to keep the
warnings from my patches about other ways to execute/interpret code and
the threat model (patch 1/6 and 5/6).

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

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

+	/* Should execution permissions be checked on open? */
+	if (flags & O_EXEC) {
+		flags |= __FMODE_EXEC;
+		acc_mode |= MAY_EXEC;
+	}

-- 
Kees Cook

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Florian Weimer]

* Kees Cook:

> On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
>> * Kees Cook:
>> 
>> > Maybe I've missed some earlier discussion that ruled this out, but I
>> > couldn't find it: let's just add O_EXEC and be done with it. It actually
>> > makes the execve() path more like openat2() and is much cleaner after
>> > a little refactoring. Here are the results, though I haven't emailed it
>> > yet since I still want to do some more testing:
>> > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1
>> 
>> I think POSIX specifies O_EXEC in such a way that it does not confer
>> read permissions.  This seems incompatible with what we are trying to
>> achieve here.
>
> I was trying to retain this behavior, since we already make this
> distinction between execve() and uselib() with the MAY_* flags:
>
> execve():
>         struct open_flags open_exec_flags = {
>                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
>                 .acc_mode = MAY_EXEC,
>
> uselib():
>         static const struct open_flags uselib_flags = {
>                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
>                 .acc_mode = MAY_READ | MAY_EXEC,
>
> I tried to retain this in my proposal, in the O_EXEC does not imply
> MAY_READ:

That doesn't quite parse for me, sorry.

The point is that the script interpreter actually needs to *read* those
files in order to execute them.

Thanks,
Florian

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Kees Cook]

On Fri, May 15, 2020 at 01:04:08PM +0200, Mickaël Salaün wrote:
> 
> On 15/05/2020 10:01, Kees Cook wrote:
> > On Thu, May 14, 2020 at 09:16:13PM +0200, Mickaël Salaün wrote:
> >> On 14/05/2020 18:10, Stephen Smalley wrote:
> >>> On Thu, May 14, 2020 at 11:45 AM Kees Cook <keescook@chromium.org> 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

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 04:43:37PM +0200, Florian Weimer wrote:
> * Kees Cook:
> 
> > On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
> >> * Kees Cook:
> >> 
> >> > Maybe I've missed some earlier discussion that ruled this out, but I
> >> > couldn't find it: let's just add O_EXEC and be done with it. It actually
> >> > makes the execve() path more like openat2() and is much cleaner after
> >> > a little refactoring. Here are the results, though I haven't emailed it
> >> > yet since I still want to do some more testing:
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1
> >> 
> >> I think POSIX specifies O_EXEC in such a way that it does not confer
> >> read permissions.  This seems incompatible with what we are trying to
> >> achieve here.
> >
> > I was trying to retain this behavior, since we already make this
> > distinction between execve() and uselib() with the MAY_* flags:
> >
> > execve():
> >         struct open_flags open_exec_flags = {
> >                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> >                 .acc_mode = MAY_EXEC,
> >
> > uselib():
> >         static const struct open_flags uselib_flags = {
> >                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
> >                 .acc_mode = MAY_READ | MAY_EXEC,
> >
> > I tried to retain this in my proposal, in the O_EXEC does not imply
> > MAY_READ:
> 
> That doesn't quite parse for me, sorry.
> 
> The point is that the script interpreter actually needs to *read* those
> files in order to execute them.

I think I misunderstood what you meant (Mickaël got me sorted out
now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
then yes, this new flag can't be O_EXEC. I was reading the glibc
documentation (which treats it as a permission bit flag, not POSIX,
which treats it as a complete mode description).

-- 
Kees Cook

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Mickaël Salaün]

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 <keescook@chromium.org> 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 :)

Re: [PATCH v5 2/6] fs: Add a MAY_EXECMOUNT flag to infer the noexec mount property

[Lev R. Oshvang .]

On Thu, May 14, 2020 at 6:48 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, May 14, 2020 at 11:14:04AM +0300, Lev R. Oshvang . wrote:
> > New sysctl is indeed required to allow userspace that places scripts
> > or libs under noexec mounts.
>
> But since this is a not-uncommon environment, we must have the sysctl
> otherwise this change would break those systems.
>
 But I proposed sysctl on a line below.

> > fs.mnt_noexec_strict =1 (allow, e) , 1 (deny any file with --x
> > permission), 2 (deny when O_MAYEXEC absent), for any file with ---x
> > permissions)
>
> I don't think we want another mount option -- this is already fully
> expressed with noexec and the system-wide sysctl.
>
> --

The intended use of proposed sysctl is to ebable sysadmin to decide
whar is desired semantics  mount with NO_EXEC option.

fs.mnt_noexec_scope =0 |1|2|3
0  - means old behaviour i.e do nor run executables and scripts (default)
1 - deny any file with --x permissions, i.e executables , script and libs
2 - deny any file when O_MAYEXEC is present.

I think this is enough to handle all use cases and to not break
current sysadmin file mounts setting
I oppose the new O_MAY_EXECMOUNT flag, kernel already has MNT_NO_EXEC,
SB_NOEXEC and SB_I_NOEXEC and I frankly do not understand why so many
variants exist.
Lev

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Florian Weimer]

* Kees Cook:

> I think I misunderstood what you meant (Mickaël got me sorted out
> now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
> then yes, this new flag can't be O_EXEC. I was reading the glibc
> documentation (which treats it as a permission bit flag, not POSIX,
> which treats it as a complete mode description).

I see.  I think this part of the manual is actually very Hurd-specific
(before the O_ACCMODE description).  I'll see if I can make this clearer
in the markup.

Thanks,
Florian

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 3741 bytes --]

On 2020-05-15, Kees Cook <keescook@chromium.org> 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/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: How about just O_EXEC? (was Re: [PATCH v5 3/6] fs: Enable to enforce noexec mounts or file exec through O_MAYEXEC)

[Mickaël Salaün]

On 19/05/2020 04:23, Aleksa Sarai wrote:
> On 2020-05-15, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, May 15, 2020 at 04:43:37PM +0200, Florian Weimer wrote:
>>> * Kees Cook:
>>>
>>>> On Fri, May 15, 2020 at 10:43:34AM +0200, Florian Weimer wrote:
>>>>> * Kees Cook:
>>>>>
>>>>>> Maybe I've missed some earlier discussion that ruled this out, but I
>>>>>> couldn't find it: let's just add O_EXEC and be done with it. It actually
>>>>>> makes the execve() path more like openat2() and is much cleaner after
>>>>>> a little refactoring. Here are the results, though I haven't emailed it
>>>>>> yet since I still want to do some more testing:
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/o_exec/v1
>>>>>
>>>>> I think POSIX specifies O_EXEC in such a way that it does not confer
>>>>> read permissions.  This seems incompatible with what we are trying to
>>>>> achieve here.
>>>>
>>>> I was trying to retain this behavior, since we already make this
>>>> distinction between execve() and uselib() with the MAY_* flags:
>>>>
>>>> execve():
>>>>         struct open_flags open_exec_flags = {
>>>>                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
>>>>                 .acc_mode = MAY_EXEC,
>>>>
>>>> uselib():
>>>>         static const struct open_flags uselib_flags = {
>>>>                 .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
>>>>                 .acc_mode = MAY_READ | MAY_EXEC,
>>>>
>>>> I tried to retain this in my proposal, in the O_EXEC does not imply
>>>> MAY_READ:
>>>
>>> That doesn't quite parse for me, sorry.
>>>
>>> The point is that the script interpreter actually needs to *read* those
>>> files in order to execute them.
>>
>> I think I misunderstood what you meant (Mickaël got me sorted out
>> now). If O_EXEC is already meant to be "EXEC and _not_ READ nor WRITE",
>> then yes, this new flag can't be O_EXEC. I was reading the glibc
>> documentation (which treats it as a permission bit flag, not POSIX,
>> which treats it as a complete mode description).
> 
> On the other hand, if we had O_EXEC (or O_EXONLY a-la O_RDONLY) then the
> interpreter could re-open the file descriptor as O_RDONLY after O_EXEC
> succeeds. Not ideal, but I don't think it's a deal-breaker.
> 
> Regarding O_MAYEXEC, I do feel a little conflicted.
> 
> I do understand that its goal is not to be what O_EXEC was supposed to
> be (which is loosely what O_PATH has effectively become), so I think
> that this is not really a huge problem -- especially since you could
> just do O_MAYEXEC|O_PATH if you wanted to disallow reading explicitly.
> It would be nice to have an O_EXONLY concept, but it's several decades
> too late to make it mandatory (and making it optional has questionable
> utility IMHO).
> 
> However, the thing I still feel mildly conflicted about is the sysctl. I
> do understand the argument for it (ultimately, whether O_MAYEXEC is
> usable on a system depends on the distribution) but it means that any
> program which uses O_MAYEXEC cannot rely on it to provide the security
> guarantees they expect. Even if the program goes and reads the sysctl
> value, it could change underneath them. If this is just meant to be a
> best-effort protection then this doesn't matter too much, but I just
> feel uneasy about these kinds of best-effort protections.

I think there is a cognitive bias here. There is a difference between
application-centric policies and system policies. For example, openat2
RESOLVE_* flags targets application developers and are self-sufficient:
the kernel provides features (applied to FDs, owned and managed by user
space) which must be known (by the application) to be supported (by the
kernel), otherwise the application may give more privileges than
expected. However, the O_MAYEXEC flag targets system administrators: it
does not make sense to enable an application to know nor enforce the
system(-wide) policy, but only to enable applications to follow this
policy (i.e. best-effort *from the application developer point of
view*). Indeed, access-control such as file executability depends on
multiple layers (e.g. file permission, mount options, ACL, SELinux
policy), most of them managed and enforced in a consistent way by
(multiple parts of) the system.

Applications should not and it does not make sense for them to expect
anything from O_MAYEXEC. This flag only enables the system to enforce a
security policy and that's all. It is really a different use case than
FD management. This feature is meant to extend the system ability thanks
to applications collaboration. Here the sysctl should not be looked at
by applications, the same way an application should not look at the
currently enforced SELinux policy nor the mount options. An application
may be launched differently according to the system-wide policy, but
this is again a system configuration. There is a difference between ABI
compatibility (i.e. does this feature is supported by the kernel?) and
system-wide security policy (what is the policy of the running system?),
in which case (common) applications should not care about system-wide
policy management but only care about policy enforcement (at their
level, if it makes sense from the system point of view). If the feature
is not provided by the system, then it is not the job of applications to
change their behavior, which means applications do their job by using
O_MAYEXEC but they do not care if it is enforce or not. It does not make
sense for an application to stop because the system does not provide a
system-centric security feature, moreover based on system introspection
(i.e. through sysctl read). It is the system role to provide and
*manage* other components executability.

More explanation can be found in a separate thread:
https://lore.kernel.org/lkml/d5df691d-bfcb-2106-08a2-cfe589b0a86c@digikod.net/

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

An O_EXEC flag could make sense for execveat(2), but O_MAYEXEC targets a
different and complementary use case. See
https://lore.kernel.org/lkml/1e2f6913-42f2-3578-28ed-567f6a4bdda1@digikod.net/
But again, see the above comment about the rational of system-wide
policy management.