Re: [PATCH v6 15/17] ima: Use mac_admin_ns_capable() to check corresponding capability

On Sat, 2021-12-11 at 09:29 -0600, Serge E. Hallyn wrote:

On Fri, Dec 10, 2021 at 02:47:34PM -0500, Stefan Berger wrote:

quoted

Use mac_admin_ns_capable() to check corresponding capability to
allow
read/write IMA policy without CAP_SYS_ADMIN but with CAP_MAC_ADMIN.

Signed-off-by: Denis Semakin <redacted>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 include/linux/capability.h      | 6 ++++++
 security/integrity/ima/ima_fs.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/capability.h

b/include/linux/capability.h
index 65efb74c3585..991579178f32 100644

--- a/include/linux/capability.h
+++ b/include/linux/capability.h

@@ -270,6 +270,12 @@ static inline bool

checkpoint_restore_ns_capable(struct user_namespace *ns)
 		ns_capable(ns, CAP_SYS_ADMIN);
 }
 
+static inline bool mac_admin_ns_capable(struct user_namespace *ns)
+{
+	return ns_capable(ns, CAP_MAC_ADMIN) ||
+		ns_capable(ns, CAP_SYS_ADMIN);
+}
+
 /* audit system wants to get cap info from files as well */
 int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
 			   const struct dentry *dentry,

diff --git a/security/integrity/ima/ima_fs.c

b/security/integrity/ima/ima_fs.c
index a136d14f29ec..090ee85bfa3a 100644

--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c

@@ -440,7 +440,7 @@ static int ima_open_policy(struct inode *inode,

struct file *filp)
 #else
 		if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
 			return -EACCES;
-		if (!capable(CAP_SYS_ADMIN))
+		if (!mac_admin_ns_capable(user_ns))

Sorry if I'm missing something.  But I'm looking at your tree's
version of ima_update_policy() and failing to see where it adds
extra capability checks.  Note that any unprivileged user can
unshare a user namespace, map its hostuid to nsuid 0, and pass
ns_capable(CAP_MAC_ADMIN).

Likewise, a host uid 0 process which does not have CAP_MAC_ADMIN
can create a new user namespace, map hostuid 0 to nsuid 0, and
have CAP_MAC_ADMIN against the new userns.

Somewhere you need to be checking for privilege against either
the parent ns or the init_user_ns.  I'm not seeing where that's
being done.  Can you point me to it?

I think you're thinking of my patches, which used a single ima log for
everything.  With Stefan's patches, each namespace gets its own log and
starts out with and empty "do nothing" policy.  However, whatever
policy was active in the parent namespace is still run for activities
in  the child, so any activity the parent requires to be
measured/audited/appraised etc still happens, but all entries are made
in the parent.

James