Re: [PATCH 2/2] file caps: accomodate future 64-bit caps

[Serge E. Hallyn]

(Clearly the noop version of check_cap_sanity() needs a semicolon.
If there are no complaints about this approach in general I will send
an updated patch.  And hopefully I can find a kbd without
broken ; and .)

-serge

Quoting Serge E. Hallyn (serue@xxxxxxxxxx):
> Here is another attempt.  This format is compatible with
> KaiGai's current tools.
> 
> Tested on s390 with 32 and 64-bit caps stored in the xattrs.
> 
> -serge
> 
> 
> From: "Serge E. Hallyn" <serue@xxxxxxxxxx>
> Subject: [PATCH 2/2] file caps: accomodate future 64-bit caps
> 
> 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.
> 
> 	(1. Rename CONFIG_SECURITY_FS_CAPABILITIES to
> 	   CONFIG_SECURITY_FILE_CAPABILITIES)
> 	2. Introduce CONFIG_SECURITY_FILE_CAPABILITIES_STRICTXATTR
> 	   which, when set, prevents loading binaries with capabilities
> 	   set which the kernel doesn't know about.  When not set,
> 	   such capabilities run, ignoring the unknown caps.
> 	3. To accomodate 64-bit caps, specify that capabilities are
> 	   stored as
> 	   	u32 version; u32 eff0; u32 perm0; u32 inh0;
> 		u32 eff1; u32 perm1; u32 inh1; (etc)
> 
> Signed-off-by: Serge E. Hallyn <serue@xxxxxxxxxx>
> 
> ---
> 
>  include/linux/capability.h |   23 ++++++
>  security/Kconfig           |   12 +++
>  security/commoncap.c       |  157 ++++++++++++++++++++++++++++----------------
>  3 files changed, 131 insertions(+), 61 deletions(-)
> 
> 987fe7fcd60aaea6aaa86e6eb24a35f8bf2bdc68
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 2776886..4dbfef3 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -44,11 +44,28 @@ typedef struct __user_cap_data_struct {
>  
>  #define XATTR_CAPS_SUFFIX "capability"
>  #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX
> +
> +/* size of caps that we work with */
> +#define XATTR_CAPS_SZ (4*sizeof(__le32))
> +
> +/*
> + * data[] is organized as:
> + *   effective[0]
> + *   permitted[0]
> + *   inheritable[0]
> + *   effective[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 data[];  /* eff[0], perm[0], inh[0], eff[1], ... */
> +};
> +
> +struct vfs_cap_data_disk_v1 {
> +	__le32 version;
> +	__le32 data[3];  /* eff[0], perm[0], inh[0] */
>  };
>  
>  #ifdef __KERNEL__
> diff --git a/security/Kconfig b/security/Kconfig
> index bc5b1be..3d5de26 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -88,7 +88,7 @@ config SECURITY_CAPABILITIES
>  	  This enables the "default" Linux capabilities functionality.
>  	  If you are unsure how to answer this question, answer Y.
>  
> -config SECURITY_FS_CAPABILITIES
> +config SECURITY_FILE_CAPABILITIES
>  	bool "File POSIX Capabilities"
>  	depends on SECURITY=n || SECURITY_CAPABILITIES!=n
>  	default n
> @@ -98,6 +98,16 @@ config SECURITY_FS_CAPABILITIES
>  
>  	  If in doubt, answer N.
>  
> +config SECURITY_FILE_CAPABILITIES_STRICTXATTR
> +	bool "Refuse to run files with unknown caps"
> +	depends on SECURITY_FILE_CAPABILITIES
> +	default y
> +	help
> +	  Refuse to run files which have unknown capabilities set
> +	  in the security.capability xattr.  This could prevent
> +	  running important binaries from an updated distribution
> +	  on an older kernel.
> +
>  config SECURITY_ROOTPLUG
>  	tristate "Root Plug Support"
>  	depends on USB && SECURITY
> diff --git a/security/commoncap.c b/security/commoncap.c
> index be86acb..86894be 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -110,36 +110,73 @@ void cap_capset_set (struct task_struct 
>  	target->cap_permitted = *permitted;
>  }
>  
> -#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> -static inline void cap_from_disk(struct vfs_cap_data_disk *dcap,
> -					struct vfs_cap_data *cap)
> +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES
> +
> +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES_STRICTXATTR
> +static int check_cap_sanity(struct vfs_cap_data_disk *dcap, int size)
>  {
> -	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);
> +	int word, bit;
> +	u32 eff, inh, perm;
> +	int sz = (size-1)/3;
> +
> +	word = CAP_NUMCAPS / 32;
> +	bit = CAP_NUMCAPS % 32;
> +
> +	eff  = le32_to_cpu(dcap->data[3*word]);
> +	perm = le32_to_cpu(dcap->data[3*word+1]);
> +	inh  = le32_to_cpu(dcap->data[3*word+2]);
> +
> +	while (word < sz) {
> +		if (bit == 32) {
> +			bit = 0;
> +			word++;
> +			if (word >= sz)
> +				break;
> +			eff  = le32_to_cpu(dcap->data[3*word]);
> +			perm = le32_to_cpu(dcap->data[3*word+1]);
> +			inh  = le32_to_cpu(dcap->data[3*word+2]);
> +			continue;
> +		}
> +		if (eff & CAP_TO_MASK(bit))
> +			return -EINVAL;
> +		if (inh & CAP_TO_MASK(bit))
> +			return -EINVAL;
> +		if (perm & CAP_TO_MASK(bit))
> +			return -EINVAL;
> +		bit++;
> +	}
> +
> +	return 0;
>  }
> +#else
> +static int check_cap_sanity(struct vfs_cap_data_disk *dcap, int sz)
> +{ return 0 }
> +#endif
>  
> -static int check_cap_sanity(struct vfs_cap_data *cap)
> +static inline int cap_from_disk(struct vfs_cap_data_disk *dcap,
> +					struct linux_binprm *bprm, int size)
>  {
> -	int i;
> +	int rc, version;
>  
> -	if (cap->version != _LINUX_CAPABILITY_VERSION)
> -		return -EPERM;
> +	version = le32_to_cpu(dcap->version);
> +	if (version != _LINUX_CAPABILITY_VERSION)
> +		return -EINVAL;
>  
> -	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;
> +	size /= sizeof(u32);
> +	if ((size-1)%3) {
> +		printk(KERN_WARNING "%s: size is an invalid size (%d)\n",
> +						__FUNCTION__, size);
> +		return -EINVAL;
>  	}
>  
> +	rc = check_cap_sanity(dcap, size);
> +	if (rc)
> +		return rc;
> +
> +	bprm->cap_effective = le32_to_cpu(dcap->data[0]);
> +	bprm->cap_permitted = le32_to_cpu(dcap->data[1]);
> +	bprm->cap_inheritable = le32_to_cpu(dcap->data[2]);
> +
>  	return 0;
>  }
>  
> @@ -147,52 +184,58 @@ static int check_cap_sanity(struct vfs_c
>  static int set_file_caps(struct linux_binprm *bprm)
>  {
>  	struct dentry *dentry;
> -	ssize_t rc;
> -	struct vfs_cap_data_disk dcaps;
> -	struct vfs_cap_data caps;
> +	int rc;
> +	struct vfs_cap_data_disk_v1 v1caps;
> +	struct vfs_cap_data_disk *dcaps;
>  	struct inode *inode;
> -	int err;
>  
> +	dcaps = (struct vfs_cap_data_disk *)&v1caps;
>  	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 = 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;
> +	rc = 0;
> +	if (!inode->i_op || !inode->i_op->getxattr)
> +		goto out;
> +
> +	rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> +							XATTR_CAPS_SZ);
> +	if (rc == -ENODATA || rc == -EOPNOTSUPP) {
> +		rc = 0;
> +		goto out;
> +	}
> +	if (rc == -ERANGE) {
> +		int size;
> +		size = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
> +		if (size <= 0) {  /* shouldn't ever happen */
> +			rc = -EINVAL;
> +			goto out;
> +		}
> +		dcaps = kmalloc(size, GFP_KERNEL);
> +		if (!dcaps) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
> +		rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> +							size);
>  	}
> -
> -	if (rc != sizeof(dcaps)) {
> -		printk(KERN_NOTICE "%s: got wrong size for getxattr (%zd)\n",
> -					__FUNCTION__, rc);
> -		return -EPERM;
> +	if (rc < 0)
> +		goto out;
> +	if (rc < sizeof(struct vfs_cap_data_disk_v1)) {
> +		rc = -EINVAL;
> +		goto out;
>  	}
>  
> -	cap_from_disk(&dcaps, &caps);
> -	err = check_cap_sanity(&caps);
> -	if (err)
> -		return err;
> -
> -	bprm->cap_effective = caps.effective;
> -	bprm->cap_permitted = caps.permitted;
> -	bprm->cap_inheritable = caps.inheritable;
> +	rc = cap_from_disk(dcaps, bprm, rc);
>  
> -	return 0;
> +out:
> +	dput(dentry);
> +	if ((void *)dcaps != (void *)&v1caps)
> +		kfree(dcaps);
> +	return rc;
>  }
> +
>  #else
>  static inline int set_file_caps(struct linux_binprm *bprm)
>  {
> @@ -399,7 +442,7 @@ int cap_task_post_setuid (uid_t old_ruid
>  	return 0;
>  }
>  
> -#ifdef CONFIG_SECURITY_FS_CAPABILITIES
> +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES
>  /*
>   * Rationale: code calling task_setscheduler, task_setioprio, and
>   * task_setnice, assumes that
> -- 
> 1.1.6
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/