On 14/05/2020 18:10, Stephen Smalley wrote:

On Thu, May 14, 2020 at 11:45 AM Kees Cook [off-list ref] wrote:

quoted

On Thu, May 14, 2020 at 08:22:01AM -0400, Stephen Smalley wrote:

quoted

On Wed, May 13, 2020 at 11:05 PM Kees Cook [off-list ref] wrote:

quoted

On Wed, May 13, 2020 at 04:27:39PM -0700, Kees Cook wrote:

quoted

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.