On 08/09/2020 18:44, Mimi Zohar wrote:

On Tue, 2020-09-08 at 17:44 +0200, Mickaël Salaün wrote:

quoted

On 08/09/2020 17:24, Mimi Zohar wrote:

quoted

On Tue, 2020-09-08 at 14:43 +0200, Mickaël Salaün wrote:

quoted

On 08/09/2020 14:28, Mimi Zohar wrote:

quoted

Hi Mickael,

On Tue, 2020-09-08 at 09:59 +0200, Mickaël Salaün wrote:

quoted

diff --git a/fs/open.c b/fs/open.c
index 9af548fb841b..879bdfbdc6fa 100644
--- a/fs/open.c
+++ b/fs/open.c

@@ -405,9 +405,13 @@ static long do_faccessat(int dfd, const char __user *filename, int mode, int fla
 	if (mode & ~S_IRWXO)	/* where's F_OK, X_OK, W_OK, R_OK? */
 		return -EINVAL;
 
-	if (flags & ~(AT_EACCESS | AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH))
+	if (flags & ~(AT_EACCESS | AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH |
+				AT_INTERPRETED))
 		return -EINVAL;
 
+	/* Only allows X_OK with AT_INTERPRETED for now. */
+	if ((flags & AT_INTERPRETED) && !(mode & S_IXOTH))
+		return -EINVAL;
 	if (flags & AT_SYMLINK_NOFOLLOW)
 		lookup_flags &= ~LOOKUP_FOLLOW;
 	if (flags & AT_EMPTY_PATH)

@@ -426,7 +430,30 @@ static long do_faccessat(int dfd, const char __user *filename, int mode, int fla
 
 	inode = d_backing_inode(path.dentry);
 
-	if ((mode & MAY_EXEC) && S_ISREG(inode->i_mode)) {
+	if ((flags & AT_INTERPRETED)) {
+		/*
+		 * For compatibility reasons, without a defined security policy
+		 * (via sysctl or LSM), using AT_INTERPRETED must map the
+		 * execute permission to the read permission.  Indeed, from
+		 * user space point of view, being able to execute data (e.g.
+		 * scripts) implies to be able to read this data.
+		 *
+		 * The MAY_INTERPRETED_EXEC bit is set to enable LSMs to add
+		 * custom checks, while being compatible with current policies.
+		 */
+		if ((mode & MAY_EXEC)) {

Why is the ISREG() test being dropped?   Without dropping it, there
would be no reason for making the existing test an "else" clause.

The ISREG() is not dropped, it is just moved below with the rest of the
original code. The corresponding code (with the path_noexec call) for
AT_INTERPRETED is added with the next commit, and it relies on the
sysctl configuration for compatibility reasons.

Dropping the S_ISREG() check here without an explanation is wrong and
probably unsafe, as it is only re-added in the subsequent patch and
only for the "sysctl_interpreted_access" case.  Adding this new test
after the existing test is probably safer.  If the original test fails,
it returns the same value as this test -EACCES.

The original S_ISREG() is ANDed with a MAY_EXEC check and with
path_noexec(). The goal of this patch is indeed to have a different
behavior than the original faccessat2(2) thanks to the AT_INTERPRETED
flag. This can't work if we add the sysctl check after the current
path_noexec() check. Moreover, in this patch an exec check is translated
to a read check. This new behavior is harmless because using
AT_INTERPRETED with the current faccessat2(2) would return -EINVAL. The
current vanilla behavior is then unchanged.

Don't get me wrong.  I'm very interested in having this support and
appreciate all the work you're doing on getting it upstreamed.  With
the change in this patch, I see the MAY_EXEC being changed to MAY_READ,
but I don't see -EINVAL being returned.  It sounds like this change is
dependent on the faccessat2 version for -EINVAL to be returned.

No worries, unfortunately the patch format doesn't ease this review. :)
access(2) and faccessat(2) have a flag value of 0. Only faccessat2(2)
takes a flag from userspace. The -EINVAL is currently returned (by
faccessat2) if there is an unknown flag provided by userspace. With this
patch, only a mode equal to X_OK is allowed for the AT_INTERPRETED flag
(cf. second hunk in this patch). As described in the cover letter, we
could handle the other modes in the future though.

quoted

The whole point of this patch series is to have a policy which do not
break current systems and is easy to configure by the sysadmin through
sysctl. This patch series also enable LSMs to take advantage of it
without the current faccess* limitations. For instance, it is then
possible for an LSM to implement more complex policies which may allow
execution of data from pipes or sockets, while verifying the source of
this data. Enforcing S_ISREG() in this patch would forbid such policies
to be implemented. In the case of IMA, you may want to add the same
S_ISREG() check.

quoted

quoted

quoted

quoted

quoted

+			mode |= MAY_INTERPRETED_EXEC;
+			/*
+			 * For compatibility reasons, if the system-wide policy
+			 * doesn't enforce file permission checks, then
+			 * replaces the execute permission request with a read
+			 * permission request.
+			 */
+			mode &= ~MAY_EXEC;
+			/* To be executed *by* user space, files must be readable. */
+			mode |= MAY_READ;