Re: [RFC] [PATCH -mm] file caps: make on-disk capabilities future-proof

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting KaiGai Kohei (kaigai@kaigai.gr.jp):
> Hi, Serge.
> 
> Thanks for the information.
> I'll update the userspace utilities next weekend.

Ok - so this change does make sense to you?

Upping _LINUX_CAPABILITY_VERSION seems drastic, but anyone who's already
been using the current patch would end up unable to run old cap-enhanced
binaries.

Now that I think about it, I suppose my patch should handle the older
_LINUX_CAPABILITY_VERSION binaries if it runs by them.  I'll send an
updated patch, though maybe not today.

> Please wait for a while.

Thanks :)

-serge

> 
> Serge E. Hallyn wrote:
> >Stephen Smalley has pointed out that the current file capabilities
> >will eventually pose a problem.
> >
> >As the capability set changes and distributions start tagging
> >binaries with capabilities, we would like for running an older
> >kernel to not necessarily make those binaries unusable.  The
> >following patch tries to address that.  Kaigai, if we went with
> >this patch, your userspace tools would need to be updated to
> >(a) insert a size parameter, and (b) update the
> >_LINUX_CAPABILITY_VERSION.
> >
> >It would be nice to solve this before file caps hit mainline.
> >
> >thanks,
> >-serge
> >
> >From: Serge E. Hallyn <serue@us.ibm.com>
> >Subject: [PATCH -mm] file caps: make on-disk capabilities future-proof
> >
> >Stephen Smalley has pointed out that the current file capabilities
> >will eventually pose a problem.
> >
> >As the capability set changes and distributions start tagging
> >binaries with capabilities, we would like for running an older
> >kernel to not necessarily make those binaries unusable.  To
> >that end,
> >
> >	1. If capabilities are specified which we don't know
> >	   about, just ignore them, do not return -EPERM as we
> >	   were doing before.
> >	2. Specify a size with the on-disk capability implementation.
> >	   In this implementation the size is the number of __u32's
> >	   used for each of (eff,perm,inh).  For now, sz is 1.
> >	   When we move to 64-bit capabilities, it becomes 2.
> >
> >Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
> >
> >---
> >
> > include/linux/capability.h |   18 ++++++--
> > security/commoncap.c       |  100 
> > +++++++++++++++++++++++++-------------------
> > 2 files changed, 70 insertions(+), 48 deletions(-)
> >
> >f4beca776d303bbb6348dc08e4d02c3bd37f3a83
> >diff --git a/include/linux/capability.h b/include/linux/capability.h
> >index 2776886..1de7a85 100644
> >--- a/include/linux/capability.h
> >+++ b/include/linux/capability.h
> >@@ -27,7 +27,7 @@
> >    library since the draft standard requires the use of malloc/free
> >    etc.. */
> >  
> >-#define _LINUX_CAPABILITY_VERSION  0x19980330
> >+#define _LINUX_CAPABILITY_VERSION  0x20070215
> > 
> > typedef struct __user_cap_header_struct {
> > 	__u32 version;
> >@@ -44,11 +44,21 @@ typedef struct __user_cap_data_struct {
> > 
> > #define XATTR_CAPS_SUFFIX "capability"
> > #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX
> >+#define XATTR_CAPS_SZ (5*sizeof(__le32))
> >+/*
> >+ * sz is the # of __le32's in each set, 1 for now.
> >+ * data[] is organized as:
> >+ *   effective[0..sz-1]
> >+ *   permitted[0..sz-1]
> >+ *   inheritable[0..sz-1]
> >+ *   ...
> >+ * this way we can just read as much of the on-disk capability as
> >+ * we know should exist and know we'll get the data we'll need.
> >+ */
> > struct vfs_cap_data_disk {
> > 	__le32 version;
> >-	__le32 effective;
> >-	__le32 permitted;
> >-	__le32 inheritable;
> >+	__le32 sz;
> >+	__le32 data[];  /* effective[sz], permitted[sz], inheritable[sz] */
> > };
> > 
> > #ifdef __KERNEL__
> >diff --git a/security/commoncap.c b/security/commoncap.c
> >index be86acb..dc8bf4f 100644
> >--- a/security/commoncap.c
> >+++ b/security/commoncap.c
> >@@ -111,35 +111,32 @@ void cap_capset_set (struct task_struct 
> > }
> > 
> > #ifdef CONFIG_SECURITY_FS_CAPABILITIES
> >-static inline void cap_from_disk(struct vfs_cap_data_disk *dcap,
> >-					struct vfs_cap_data *cap)
> >+static inline int cap_from_disk(struct vfs_cap_data_disk *dcap,
> >+					struct vfs_cap_data *cap, int size)
> > {
> >+	__u32 sz;
> >+
> > 	cap->version = le32_to_cpu(dcap->version);
> >-	cap->effective = le32_to_cpu(dcap->effective);
> >-	cap->permitted = le32_to_cpu(dcap->permitted);
> >-	cap->inheritable = le32_to_cpu(dcap->inheritable);
> >+	sz = le32_to_cpu(dcap->sz);
> >+
> >+	if ((sz*3+2)*sizeof(__u32) != size) {
> >+		printk(KERN_NOTICE "%s: sz is %d, size is %d, should be 
> >%d\n", __FUNCTION__,
> >+			sz, size, (sz*3+2)*sizeof(__u32));
> >+		return -EINVAL;
> >+	}
> >+
> >+	cap->effective = le32_to_cpu(dcap->data[0]);
> >+	cap->permitted = le32_to_cpu(dcap->data[sz]);
> >+	cap->inheritable = le32_to_cpu(dcap->data[sz*2]);
> >+
> >+	return 0;
> > }
> > 
> > static int check_cap_sanity(struct vfs_cap_data *cap)
> > {
> >-	int i;
> >-
> > 	if (cap->version != _LINUX_CAPABILITY_VERSION)
> > 		return -EPERM;
> > 
> >-	for (i = CAP_NUMCAPS; i < 8*sizeof(cap->effective); i++) {
> >-		if (cap->effective & CAP_TO_MASK(i))
> >-			return -EPERM;
> >-	}
> >-	for (i = CAP_NUMCAPS; i < 8*sizeof(cap->permitted); i++) {
> >-		if (cap->permitted & CAP_TO_MASK(i))
> >-			return -EPERM;
> >-	}
> >-	for (i = CAP_NUMCAPS; i < 8*sizeof(cap->inheritable); i++) {
> >-		if (cap->inheritable & CAP_TO_MASK(i))
> >-			return -EPERM;
> >-	}
> >-
> > 	return 0;
> > }
> > 
> >@@ -148,50 +145,65 @@ static int set_file_caps(struct linux_bi
> > {
> > 	struct dentry *dentry;
> > 	ssize_t rc;
> >-	struct vfs_cap_data_disk dcaps;
> >+	struct vfs_cap_data_disk *dcaps;
> > 	struct vfs_cap_data caps;
> > 	struct inode *inode;
> >-	int err;
> > 
> > 	if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)
> > 		return 0;
> > 
> > 	dentry = dget(bprm->file->f_dentry);
> > 	inode = dentry->d_inode;
> >-	if (!inode->i_op || !inode->i_op->getxattr) {
> >-		dput(dentry);
> >-		return 0;
> >+	rc = 0;
> >+	if (!inode->i_op || !inode->i_op->getxattr)
> >+		goto out;
> >+
> >+	rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
> >+	if (rc == -ENODATA) {
> >+		rc = 0;
> >+		goto out;
> >+	}
> >+	if (rc < 0)
> >+		goto out;
> >+	if (rc < sizeof(struct vfs_cap_data_disk)) {
> >+		rc = -EINVAL;
> >+		goto out;
> >+	}
> >+
> >+	dcaps = kmalloc(rc, GFP_KERNEL);
> >+	if (!dcaps) {
> >+		rc = -ENOMEM;
> >+		goto out;
> >+	}
> >+	rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> >+						XATTR_CAPS_SZ);
> >+	if (rc == -ENODATA) {
> >+		rc = 0;
> >+		goto out_free;
> > 	}
> > 
> >-	rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, &dcaps,
> >-						sizeof(dcaps));
> >-	dput(dentry);
> >-
> >-	if (rc == -ENODATA)
> >-		return 0;
> >-
> > 	if (rc < 0) {
> > 		printk(KERN_NOTICE "%s: Error (%zd) getting xattr\n",
> > 				__FUNCTION__, rc);
> >-		return rc;
> >+		goto out_free;
> > 	}
> > 
> >-	if (rc != sizeof(dcaps)) {
> >-		printk(KERN_NOTICE "%s: got wrong size for getxattr (%zd)\n",
> >-					__FUNCTION__, rc);
> >-		return -EPERM;
> >-	}
> >-
> >-	cap_from_disk(&dcaps, &caps);
> >-	err = check_cap_sanity(&caps);
> >-	if (err)
> >-		return err;
> >+	rc = cap_from_disk(dcaps, &caps, rc);
> >+	if (rc)
> >+		goto out_free;
> >+	rc = check_cap_sanity(&caps);
> >+	if (rc)
> >+		goto out_free;
> > 
> > 	bprm->cap_effective = caps.effective;
> > 	bprm->cap_permitted = caps.permitted;
> > 	bprm->cap_inheritable = caps.inheritable;
> > 
> >-	return 0;
> >+out_free:
> >+	kfree(dcaps);
> >+out:
> >+	dput(dentry);
> >+	return rc;
> > }
> > #else
> > static inline int set_file_caps(struct linux_binprm *bprm)
> 
> 
> -- 
> KaiGai Kohei <kaigai@kaigai.gr.jp>