Re: [PATCH V3 2/2] EVM: Allow runtime modification of the set of verified xattrs

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Tue, 2018-05-01 at 10:51 -0700, Matthew Garrett wrote:
> Sites may wish to provide additional metadata alongside files in order
> to make more fine-grained security decisions[1]. The security of this is
> enhanced if this metadata is protected, something that EVM makes
> possible. However, the kernel cannot know about the set of extended
> attributes that local admins may wish to protect, and hardcoding this
> policy in the kernel makes it difficult to change over time and less
> convenient for distributions to enable.
> 
> This patch adds a new /sys/kernel/security/evm_xattrs node, which can be
> read to obtain the current set of EVM-protected extended attributes or
> written to in order to add new entries. Extending this list will not
> change the validity of any existing signatures provided that the file
> in question does not have any of the additional extended attributes -
> missing xattrs are skipped when calculating the EVM hash.
> 
> [1] For instance, a package manager could install information about the
> package uploader in an additional extended attribute. Local LSM policy
> could then be associated with that extended attribute in order to
> restrict the privileges available to packages from less trusted
> uploaders.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  Documentation/ABI/testing/evm      |  13 ++++
>  security/integrity/evm/evm_secfs.c | 102 +++++++++++++++++++++++++++--
>  2 files changed, 109 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
> index d12cb2eae9ee..bfe529eb26b2 100644
> --- a/Documentation/ABI/testing/evm
> +++ b/Documentation/ABI/testing/evm
> @@ -57,3 +57,16 @@ Description:
>  		dracut (via 97masterkey and 98integrity) and systemd (via
>  		core/ima-setup) have support for loading keys at boot
>  		time.
> +
> +What:		security/evm_xattrs
> +Date:		April 2018
> +Contact:	Matthew Garrett <mjg59@google.com>
> +Description:
> +		Shows the set of extended attributes used to calculate or
> +		validate the EVM signature, and allows additional attributes
> +		to be added at runtime. Adding additional extended attributes
> +		will result in any existing signatures generated without the
> +		additional attributes becoming invalid, and any signatures
> +		generated after additional attributes are added will only be
> +		valid if the same additional attributes are configured on
> +		system boot.

This needs to be updated ...

> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index feba03bbedae..3b371125d439 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -20,6 +20,7 @@
>  #include "evm.h"
> 
>  static struct dentry *evm_init_tpm;
> +static struct dentry *evm_xattrs;
> 
>  /**
>   * evm_read_key - read() for <securityfs>/evm
> @@ -107,13 +108,102 @@ static const struct file_operations evm_key_ops = {
>  	.write		= evm_write_key,
>  };
> 
> -int __init evm_init_secfs(void)
> +/**
> + * evm_read_xattrs - read() for <securityfs>/evm_xattrs
> + *
> + * @filp: file pointer, not actually used
> + * @buf: where to put the result
> + * @count: maximum to send along
> + * @ppos: where to start
> + *
> + * Returns number of bytes read or error code, as appropriate
> + */
> +static ssize_t evm_read_xattrs(struct file *filp, char __user *buf,
> +			       size_t count, loff_t *ppos)
>  {
> -	int error = 0;
> +	char *temp;
> +	int offset = 0;
> +	ssize_t rc, size = 0;
> +	struct xattr_list *xattr;
> +
> +	if (*ppos != 0)
> +		return 0;
> 
> -	evm_init_tpm = securityfs_create_file("evm", S_IRUSR | S_IRGRP,
> -					      NULL, NULL, &evm_key_ops);
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list)
> +		size += strlen(xattr->name) + 1;
> +
> +	temp = kmalloc(size + 1, GFP_KERNEL);
> +	if (!temp)
> +		return -ENOMEM;
> +
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
> +		sprintf(temp + offset, "%s\n", xattr->name);
> +		offset += strlen(xattr->name) + 1;
> +	}
> +
> +	rc = simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
> +
> +	return rc;
> +}
> +
> +/**
> + * evm_write_xattrs - write() for <securityfs>/evm_xattrs
> + * @file: file pointer, not actually used
> + * @buf: where to get the data from
> + * @count: bytes sent
> + * @ppos: where to start
> + *
> + * Returns number of bytes written or error code, as appropriate
> + */
> +static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> +				size_t count, loff_t *ppos)
> +{
> +	int len;
> +	struct xattr_list *xattr;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (*ppos != 0)
> +		return -EINVAL;

Is there a maximum xattr name size?  Should there be?

> +
> +	xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
> +	if (!xattr)
> +		return -ENOMEM;
> +
> +	xattr->name = memdup_user_nul(buf, count);
> +	if (!xattr->name) {
> +		kfree(xattr);
> +		return -ENOMEM;
> +	}
> +
> +	/* Remove any trailing newline */
> +	len = strlen(xattr->name);
> +	if (xattr->name[len-1] == '\n')
> +		xattr->name[len-1] = '\0';

Shouldn't we somehow sanity check userspace provided strings, before
adding them to the list of xattrs?  Perhaps limit the string to the
upper and lower case letters?
  
> +
> +	list_add_tail(&xattr->list, &evm_config_xattrnames);
> +	return count;
> +}
> +
> +static const struct file_operations evm_xattr_ops = {
> +	.read		= evm_read_xattrs,
> +	.write		= evm_write_xattrs,
> +};
> +
> +int __init evm_init_secfs(void)
> +{
> +	evm_init_tpm = securityfs_create_file("evm", 0440, NULL, NULL,
> +					      &evm_key_ops);
>  	if (!evm_init_tpm || IS_ERR(evm_init_tpm))
> -		error = -EFAULT;
> -	return error;
> +		return -EFAULT;
> +
> +	evm_xattrs = securityfs_create_file("evm_xattrs", 0440, NULL, NULL,
> +					    &evm_xattr_ops);
> +	if (!evm_xattrs || IS_ERR(evm_xattrs)) {
> +		securityfs_remove(evm_init_tpm);
> +		return -EFAULT;
> +	}
> +

Do we really want this feature unconditionally enabled?  How often do
you expect to add xattrs?  Is there ever a point where you would want
to lock the list of xattrs from changing?

Mimi

> +	return 0;
>  }