[PATCH V3 1/2] EVM: turn evm_config_xattrnames into a list

[PATCH V3 1/2] EVM: turn evm_config_xattrnames into a list

[Matthew Garrett]

Use a list of xattrs rather than an array - this makes it easier to
extend the list at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 security/integrity/evm/evm.h        |  7 ++-
 security/integrity/evm/evm_crypto.c | 10 ++--
 security/integrity/evm/evm_main.c   | 72 +++++++++++++++++++----------
 3 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index 45c4a89c02ff..1257c3c24723 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -30,6 +30,11 @@
 #define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP_COMPLETE | \
 		       EVM_ALLOW_METADATA_WRITES)
 
+struct xattr_list {
+	struct list_head list;
+	char *name;
+};
+
 extern int evm_initialized;
 
 #define EVM_ATTR_FSUUID		0x0001
@@ -40,7 +45,7 @@ extern struct crypto_shash *hmac_tfm;
 extern struct crypto_shash *hash_tfm;
 
 /* List of EVM protected security xattrs */
-extern char *evm_config_xattrnames[];
+extern struct list_head evm_config_xattrnames;
 
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a46fba322340..caeea20670cc 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -192,8 +192,8 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				char type, char *digest)
 {
 	struct inode *inode = d_backing_inode(dentry);
+	struct xattr_list *xattr;
 	struct shash_desc *desc;
-	char **xattrname;
 	size_t xattr_size = 0;
 	char *xattr_value = NULL;
 	int error;
@@ -208,14 +208,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		return PTR_ERR(desc);
 
 	error = -ENODATA;
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
 		bool is_ima = false;
 
-		if (strcmp(*xattrname, XATTR_NAME_IMA) == 0)
+		if (strcmp(xattr->name, XATTR_NAME_IMA) == 0)
 			is_ima = true;
 
 		if ((req_xattr_name && req_xattr_value)
-		    && !strcmp(*xattrname, req_xattr_name)) {
+		    && !strcmp(xattr->name, req_xattr_name)) {
 			error = 0;
 			crypto_shash_update(desc, (const u8 *)req_xattr_value,
 					     req_xattr_value_len);
@@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				ima_present = true;
 			continue;
 		}
-		size = vfs_getxattr_alloc(dentry, *xattrname,
+		size = vfs_getxattr_alloc(dentry, xattr->name,
 					  &xattr_value, xattr_size, GFP_NOFS);
 		if (size == -ENOMEM) {
 			error = -ENOMEM;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9ea9c19a545c..dd2415c55982 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -35,28 +35,29 @@ static const char * const integrity_status_msg[] = {
 };
 int evm_hmac_attrs;
 
-char *evm_config_xattrnames[] = {
+static struct xattr_list evm_config_default_xattrnames[] __ro_after_init = {
 #ifdef CONFIG_SECURITY_SELINUX
-	XATTR_NAME_SELINUX,
+	{.name = XATTR_NAME_SELINUX},
 #endif
 #ifdef CONFIG_SECURITY_SMACK
-	XATTR_NAME_SMACK,
+	{.name = XATTR_NAME_SMACK},
 #ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS
-	XATTR_NAME_SMACKEXEC,
-	XATTR_NAME_SMACKTRANSMUTE,
-	XATTR_NAME_SMACKMMAP,
+	{.name = XATTR_NAME_SMACKEXEC},
+	{.name = XATTR_NAME_SMACKTRANSMUTE},
+	{.name = XATTR_NAME_SMACKMMAP},
 #endif
 #endif
 #ifdef CONFIG_SECURITY_APPARMOR
-	XATTR_NAME_APPARMOR,
+	{.name = XATTR_NAME_APPARMOR},
 #endif
 #ifdef CONFIG_IMA_APPRAISE
-	XATTR_NAME_IMA,
+	{.name = XATTR_NAME_IMA},
 #endif
-	XATTR_NAME_CAPS,
-	NULL
+	{.name = XATTR_NAME_CAPS},
 };
 
+LIST_HEAD(evm_config_xattrnames);
+
 static int evm_fixmode;
 static int __init evm_set_fixmode(char *str)
 {
@@ -68,6 +69,14 @@ __setup("evm=", evm_set_fixmode);
 
 static void __init evm_init_config(void)
 {
+	int i, xattrs;
+
+	xattrs = ARRAY_SIZE(evm_config_default_xattrnames);
+
+	for (i = 0; i < xattrs; i++)
+		list_add_tail(&evm_config_default_xattrnames[i].list,
+			      &evm_config_xattrnames);
+
 #ifdef CONFIG_EVM_ATTR_FSUUID
 	evm_hmac_attrs |= EVM_ATTR_FSUUID;
 #endif
@@ -82,15 +91,15 @@ static bool evm_key_loaded(void)
 static int evm_find_protected_xattrs(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	char **xattr;
+	struct xattr_list *xattr;
 	int error;
 	int count = 0;
 
 	if (!(inode->i_opflags & IOP_XATTR))
 		return -EOPNOTSUPP;
 
-	for (xattr = evm_config_xattrnames; *xattr != NULL; xattr++) {
-		error = __vfs_getxattr(dentry, inode, *xattr, NULL, 0);
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+		error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0);
 		if (error < 0) {
 			if (error == -ENODATA)
 				continue;
@@ -211,24 +220,25 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 
 static int evm_protected_xattr(const char *req_xattr_name)
 {
-	char **xattrname;
 	int namelen;
 	int found = 0;
+	struct xattr_list *xattr;
 
 	namelen = strlen(req_xattr_name);
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
-		if ((strlen(*xattrname) == namelen)
-		    && (strncmp(req_xattr_name, *xattrname, namelen) == 0)) {
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+		if ((strlen(xattr->name) == namelen)
+		    && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) {
 			found = 1;
 			break;
 		}
 		if (strncmp(req_xattr_name,
-			    *xattrname + XATTR_SECURITY_PREFIX_LEN,
+			    xattr->name + XATTR_SECURITY_PREFIX_LEN,
 			    strlen(req_xattr_name)) == 0) {
 			found = 1;
 			break;
 		}
 	}
+
 	return found;
 }
 
@@ -544,20 +554,33 @@ void __init evm_load_x509(void)
 static int __init init_evm(void)
 {
 	int error;
+	struct list_head *pos, *q;
+	struct xattr_list *xattr;
 
 	evm_init_config();
 
 	error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
 	if (error)
-		return error;
+		goto error;
 
 	error = evm_init_secfs();
 	if (error < 0) {
 		pr_info("Error registering secfs\n");
-		return error;
+		goto error;
 	}
 
-	return 0;
+error:
+	if (error != 0) {
+		if (!list_empty(&evm_config_xattrnames)) {
+			list_for_each_safe(pos, q, &evm_config_xattrnames) {
+				xattr = list_entry(pos, struct xattr_list,
+						   list);
+				list_del(pos);
+			}
+		}
+	}
+
+	return error;
 }
 
 /*
@@ -565,10 +588,11 @@ static int __init init_evm(void)
  */
 static int __init evm_display_config(void)
 {
-	char **xattrname;
+	struct xattr_list *xattr;
+
+	list_for_each_entry(xattr, &evm_config_xattrnames, list)
+		pr_info("%s\n", xattr->name);
 
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++)
-		pr_info("%s\n", *xattrname);
 	return 0;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Matthew Garrett]

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.
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;
+
+	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';
+
+	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;
+	}
+
+	return 0;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Mimi Zohar]

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;
>  }

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

[Matthew Garrett]

On Wed, May 2, 2018 at 8:17 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-05-01 at 10:51 -0700, Matthew Garrett wrote:
> > +             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 ...

Yup.

> > +     if (*ppos != 0)
> > +             return -EINVAL;

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

There is - I'll add a check.

> > +     /* 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?

As far as I can tell the VFS doesn't do that, and I wouldn't put it past
someone to use UTF-8 at some point...

  > +     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?

I think a config option would make sense here. Locking doesn't seem
unreasonable, but I'm not sure what the threat model would really be -
adding new xattrs would only result in additional signatures validating if
they were signed with a trusted key in the first place.

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

[Mimi Zohar]

On Tue, 2018-05-08 at 21:30 +0000, Matthew Garrett wrote:
> On Wed, May 2, 2018 at 8:17 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2018-05-01 at 10:51 -0700, Matthew Garrett wrote:
> > > +             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 ...
> 
> Yup.
> 
> > > +     if (*ppos != 0)
> > > +             return -EINVAL;
> 
> > Is there a maximum xattr name size?  Should there be?
> 
> There is - I'll add a check.
> 
> > > +     /* 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?
> 
> As far as I can tell the VFS doesn't do that, and I wouldn't put it past
> someone to use UTF-8 at some point...

The audit subsystem uses audit_log_untrustedstrings() to check for
control characters.  I'm not sure what should be included, but not
checking userspace strings doesn't sound right.

> 
>   > +     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?
> 
> I think a config option would make sense here. Locking doesn't seem
> unreasonable, but I'm not sure what the threat model would really be -
> adding new xattrs would only result in additional signatures validating if
> they were signed with a trusted key in the first place.

Remember adding additional EVM xattrs isn't limited to just EVM
signatures, but for the original EVM HMAC as well.  Did you want to
limit it to the EVM portable & immutable signatures?

Mimi

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

[Matthew Garrett]

On Tue, May 8, 2018 at 2:43 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> On Tue, 2018-05-08 at 21:30 +0000, Matthew Garrett wrote:
> > As far as I can tell the VFS doesn't do that, and I wouldn't put it past
> > someone to use UTF-8 at some point...

> The audit subsystem uses audit_log_untrustedstrings() to check for
> control characters.  I'm not sure what should be included, but not
> checking userspace strings doesn't sound right.

As far as I can tell it's legitimate for xattr names to include control
characters. I can't think of /why/ someone would do that, but...

> > I think a config option would make sense here. Locking doesn't seem
> > unreasonable, but I'm not sure what the threat model would really be -
> > adding new xattrs would only result in additional signatures validating
if
> > they were signed with a trusted key in the first place.

> Remember adding additional EVM xattrs isn't limited to just EVM
> signatures, but for the original EVM HMAC as well.  Did you want to
> limit it to the EVM portable & immutable signatures?

If you add entries and then signatures are created you'll end up with
signatures that won't validate on next boot until the same attributes are
added. That feels at worst like root being able to trigger a DoS, which
they'd be able to do by tampering with the signatures via the raw device
node anyway.

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

[Mimi Zohar]

On Tue, 2018-05-08 at 22:51 +0000, Matthew Garrett wrote:
> On Tue, May 8, 2018 at 2:43 PM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > On Tue, 2018-05-08 at 21:30 +0000, Matthew Garrett wrote:
> > > As far as I can tell the VFS doesn't do that, and I wouldn't put it past
> > > someone to use UTF-8 at some point...
> 
> > The audit subsystem uses audit_log_untrustedstrings() to check for
> > control characters.  I'm not sure what should be included, but not
> > checking userspace strings doesn't sound right.
> 
> As far as I can tell it's legitimate for xattr names to include control
> characters. I can't think of /why/ someone would do that, but...
> 
> > > I think a config option would make sense here. Locking doesn't seem
> > > unreasonable, but I'm not sure what the threat model would really be -
> > > adding new xattrs would only result in additional signatures validating
> if
> > > they were signed with a trusted key in the first place.
> 
> > Remember adding additional EVM xattrs isn't limited to just EVM
> > signatures, but for the original EVM HMAC as well.  Did you want to
> > limit it to the EVM portable & immutable signatures?
> 
> If you add entries and then signatures are created you'll end up with
> signatures that won't validate on next boot until the same attributes are
> added. That feels at worst like root being able to trigger a DoS, which
> they'd be able to do by tampering with the signatures via the raw device
> node anyway.

With a system with EVM HMAC enabled and an "ima_policy=appraise_tcb"
policy, which appraises all executables and files owned by root, any
new xattrs would cause the EVM HMAC to be recalculated immediately.
 On boot, if any of these files are accessed prior to the additional
xattrs are added, it could cause the system not to boot properly.

True in both cases it is a DoS, but there is a difference between
accessing the raw device and normal usage.

Adding support for additional xattr names is fine.  Making it a
Kconfig option is required.  Being able to lock adding additional
 xattr names should also be required.

Mimi

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

[Matthew Garrett]

On Wed, May 9, 2018 at 8:30 AM Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Adding support for additional xattr names is fine.  Making it a
> Kconfig option is required.  Being able to lock adding additional
>   xattr names should also be required.

Ok! I'll add that.

Re: [PATCH V3 1/2] EVM: turn evm_config_xattrnames into a list

[Mimi Zohar]

On Tue, 2018-05-01 at 10:51 -0700, Matthew Garrett wrote:
> Use a list of xattrs rather than an array - this makes it easier to
> extend the list at runtime.

Thank you for making this a separate patch.

Once the list is initialized, additional xattrs aren't being added in
this patch. So by itself this patch doesn't require locking, but the
next patch extends the list.  Wouldn't the list then require some form
of locking (eg. RCU)?

Mimi


> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  security/integrity/evm/evm.h        |  7 ++-
>  security/integrity/evm/evm_crypto.c | 10 ++--
>  security/integrity/evm/evm_main.c   | 72 +++++++++++++++++++----------
>  3 files changed, 59 insertions(+), 30 deletions(-)
> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 45c4a89c02ff..1257c3c24723 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -30,6 +30,11 @@
>  #define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP_COMPLETE | \
>  		       EVM_ALLOW_METADATA_WRITES)
> 
> +struct xattr_list {
> +	struct list_head list;
> +	char *name;
> +};
> +
>  extern int evm_initialized;
> 
>  #define EVM_ATTR_FSUUID		0x0001
> @@ -40,7 +45,7 @@ extern struct crypto_shash *hmac_tfm;
>  extern struct crypto_shash *hash_tfm;
> 
>  /* List of EVM protected security xattrs */
> -extern char *evm_config_xattrnames[];
> +extern struct list_head evm_config_xattrnames;
> 
>  int evm_init_key(void);
>  int evm_update_evmxattr(struct dentry *dentry,
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a46fba322340..caeea20670cc 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -192,8 +192,8 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  				char type, char *digest)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> +	struct xattr_list *xattr;
>  	struct shash_desc *desc;
> -	char **xattrname;
>  	size_t xattr_size = 0;
>  	char *xattr_value = NULL;
>  	int error;
> @@ -208,14 +208,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  		return PTR_ERR(desc);
> 
>  	error = -ENODATA;
> -	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
>  		bool is_ima = false;
> 
> -		if (strcmp(*xattrname, XATTR_NAME_IMA) == 0)
> +		if (strcmp(xattr->name, XATTR_NAME_IMA) == 0)
>  			is_ima = true;
> 
>  		if ((req_xattr_name && req_xattr_value)
> -		    && !strcmp(*xattrname, req_xattr_name)) {
> +		    && !strcmp(xattr->name, req_xattr_name)) {
>  			error = 0;
>  			crypto_shash_update(desc, (const u8 *)req_xattr_value,
>  					     req_xattr_value_len);
> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  				ima_present = true;
>  			continue;
>  		}
> -		size = vfs_getxattr_alloc(dentry, *xattrname,
> +		size = vfs_getxattr_alloc(dentry, xattr->name,
>  					  &xattr_value, xattr_size, GFP_NOFS);
>  		if (size == -ENOMEM) {
>  			error = -ENOMEM;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9ea9c19a545c..dd2415c55982 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -35,28 +35,29 @@ static const char * const integrity_status_msg[] = {
>  };
>  int evm_hmac_attrs;
> 
> -char *evm_config_xattrnames[] = {
> +static struct xattr_list evm_config_default_xattrnames[] __ro_after_init = {
>  #ifdef CONFIG_SECURITY_SELINUX
> -	XATTR_NAME_SELINUX,
> +	{.name = XATTR_NAME_SELINUX},
>  #endif
>  #ifdef CONFIG_SECURITY_SMACK
> -	XATTR_NAME_SMACK,
> +	{.name = XATTR_NAME_SMACK},
>  #ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS
> -	XATTR_NAME_SMACKEXEC,
> -	XATTR_NAME_SMACKTRANSMUTE,
> -	XATTR_NAME_SMACKMMAP,
> +	{.name = XATTR_NAME_SMACKEXEC},
> +	{.name = XATTR_NAME_SMACKTRANSMUTE},
> +	{.name = XATTR_NAME_SMACKMMAP},
>  #endif
>  #endif
>  #ifdef CONFIG_SECURITY_APPARMOR
> -	XATTR_NAME_APPARMOR,
> +	{.name = XATTR_NAME_APPARMOR},
>  #endif
>  #ifdef CONFIG_IMA_APPRAISE
> -	XATTR_NAME_IMA,
> +	{.name = XATTR_NAME_IMA},
>  #endif
> -	XATTR_NAME_CAPS,
> -	NULL
> +	{.name = XATTR_NAME_CAPS},
>  };
> 
> +LIST_HEAD(evm_config_xattrnames);
> +
>  static int evm_fixmode;
>  static int __init evm_set_fixmode(char *str)
>  {
> @@ -68,6 +69,14 @@ __setup("evm=", evm_set_fixmode);
> 
>  static void __init evm_init_config(void)
>  {
> +	int i, xattrs;
> +
> +	xattrs = ARRAY_SIZE(evm_config_default_xattrnames);
> +
> +	for (i = 0; i < xattrs; i++)
> +		list_add_tail(&evm_config_default_xattrnames[i].list,
> +			      &evm_config_xattrnames);
> +
>  #ifdef CONFIG_EVM_ATTR_FSUUID
>  	evm_hmac_attrs |= EVM_ATTR_FSUUID;
>  #endif
> @@ -82,15 +91,15 @@ static bool evm_key_loaded(void)
>  static int evm_find_protected_xattrs(struct dentry *dentry)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> -	char **xattr;
> +	struct xattr_list *xattr;
>  	int error;
>  	int count = 0;
> 
>  	if (!(inode->i_opflags & IOP_XATTR))
>  		return -EOPNOTSUPP;
> 
> -	for (xattr = evm_config_xattrnames; *xattr != NULL; xattr++) {
> -		error = __vfs_getxattr(dentry, inode, *xattr, NULL, 0);
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
> +		error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0);
>  		if (error < 0) {
>  			if (error == -ENODATA)
>  				continue;
> @@ -211,24 +220,25 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> 
>  static int evm_protected_xattr(const char *req_xattr_name)
>  {
> -	char **xattrname;
>  	int namelen;
>  	int found = 0;
> +	struct xattr_list *xattr;
> 
>  	namelen = strlen(req_xattr_name);
> -	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
> -		if ((strlen(*xattrname) == namelen)
> -		    && (strncmp(req_xattr_name, *xattrname, namelen) == 0)) {
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
> +		if ((strlen(xattr->name) == namelen)
> +		    && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) {
>  			found = 1;
>  			break;
>  		}
>  		if (strncmp(req_xattr_name,
> -			    *xattrname + XATTR_SECURITY_PREFIX_LEN,
> +			    xattr->name + XATTR_SECURITY_PREFIX_LEN,
>  			    strlen(req_xattr_name)) == 0) {
>  			found = 1;
>  			break;
>  		}
>  	}
> +
>  	return found;
>  }
> 
> @@ -544,20 +554,33 @@ void __init evm_load_x509(void)
>  static int __init init_evm(void)
>  {
>  	int error;
> +	struct list_head *pos, *q;
> +	struct xattr_list *xattr;
> 
>  	evm_init_config();
> 
>  	error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
>  	if (error)
> -		return error;
> +		goto error;
> 
>  	error = evm_init_secfs();
>  	if (error < 0) {
>  		pr_info("Error registering secfs\n");
> -		return error;
> +		goto error;
>  	}
> 
> -	return 0;
> +error:
> +	if (error != 0) {
> +		if (!list_empty(&evm_config_xattrnames)) {
> +			list_for_each_safe(pos, q, &evm_config_xattrnames) {
> +				xattr = list_entry(pos, struct xattr_list,
> +						   list);
> +				list_del(pos);
> +			}
> +		}
> +	}
> +
> +	return error;
>  }
> 
>  /*
> @@ -565,10 +588,11 @@ static int __init init_evm(void)
>   */
>  static int __init evm_display_config(void)
>  {
> -	char **xattrname;
> +	struct xattr_list *xattr;
> +
> +	list_for_each_entry(xattr, &evm_config_xattrnames, list)
> +		pr_info("%s\n", xattr->name);
> 
> -	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++)
> -		pr_info("%s\n", *xattrname);
>  	return 0;
>  }
> 

[PATCH V3 1/2] EVM: turn evm_config_xattrnames into a list

[Matthew Garrett]

Use a list of xattrs rather than an array - this makes it easier to
extend the list at runtime.

Signed-off-by: Matthew Garrett <mjg59@google.com>
---
 security/integrity/evm/evm.h        |  7 ++-
 security/integrity/evm/evm_crypto.c | 10 ++--
 security/integrity/evm/evm_main.c   | 72 +++++++++++++++++++----------
 3 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index 45c4a89c02ff..1257c3c24723 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -30,6 +30,11 @@
 #define EVM_INIT_MASK (EVM_INIT_HMAC | EVM_INIT_X509 | EVM_SETUP_COMPLETE | \
 		       EVM_ALLOW_METADATA_WRITES)
 
+struct xattr_list {
+	struct list_head list;
+	char *name;
+};
+
 extern int evm_initialized;
 
 #define EVM_ATTR_FSUUID		0x0001
@@ -40,7 +45,7 @@ extern struct crypto_shash *hmac_tfm;
 extern struct crypto_shash *hash_tfm;
 
 /* List of EVM protected security xattrs */
-extern char *evm_config_xattrnames[];
+extern struct list_head evm_config_xattrnames;
 
 int evm_init_key(void);
 int evm_update_evmxattr(struct dentry *dentry,
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a46fba322340..caeea20670cc 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -192,8 +192,8 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				char type, char *digest)
 {
 	struct inode *inode = d_backing_inode(dentry);
+	struct xattr_list *xattr;
 	struct shash_desc *desc;
-	char **xattrname;
 	size_t xattr_size = 0;
 	char *xattr_value = NULL;
 	int error;
@@ -208,14 +208,14 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		return PTR_ERR(desc);
 
 	error = -ENODATA;
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
 		bool is_ima = false;
 
-		if (strcmp(*xattrname, XATTR_NAME_IMA) == 0)
+		if (strcmp(xattr->name, XATTR_NAME_IMA) == 0)
 			is_ima = true;
 
 		if ((req_xattr_name && req_xattr_value)
-		    && !strcmp(*xattrname, req_xattr_name)) {
+		    && !strcmp(xattr->name, req_xattr_name)) {
 			error = 0;
 			crypto_shash_update(desc, (const u8 *)req_xattr_value,
 					     req_xattr_value_len);
@@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				ima_present = true;
 			continue;
 		}
-		size = vfs_getxattr_alloc(dentry, *xattrname,
+		size = vfs_getxattr_alloc(dentry, xattr->name,
 					  &xattr_value, xattr_size, GFP_NOFS);
 		if (size == -ENOMEM) {
 			error = -ENOMEM;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 9ea9c19a545c..dd2415c55982 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -35,28 +35,29 @@ static const char * const integrity_status_msg[] = {
 };
 int evm_hmac_attrs;
 
-char *evm_config_xattrnames[] = {
+static struct xattr_list evm_config_default_xattrnames[] __ro_after_init = {
 #ifdef CONFIG_SECURITY_SELINUX
-	XATTR_NAME_SELINUX,
+	{.name = XATTR_NAME_SELINUX},
 #endif
 #ifdef CONFIG_SECURITY_SMACK
-	XATTR_NAME_SMACK,
+	{.name = XATTR_NAME_SMACK},
 #ifdef CONFIG_EVM_EXTRA_SMACK_XATTRS
-	XATTR_NAME_SMACKEXEC,
-	XATTR_NAME_SMACKTRANSMUTE,
-	XATTR_NAME_SMACKMMAP,
+	{.name = XATTR_NAME_SMACKEXEC},
+	{.name = XATTR_NAME_SMACKTRANSMUTE},
+	{.name = XATTR_NAME_SMACKMMAP},
 #endif
 #endif
 #ifdef CONFIG_SECURITY_APPARMOR
-	XATTR_NAME_APPARMOR,
+	{.name = XATTR_NAME_APPARMOR},
 #endif
 #ifdef CONFIG_IMA_APPRAISE
-	XATTR_NAME_IMA,
+	{.name = XATTR_NAME_IMA},
 #endif
-	XATTR_NAME_CAPS,
-	NULL
+	{.name = XATTR_NAME_CAPS},
 };
 
+LIST_HEAD(evm_config_xattrnames);
+
 static int evm_fixmode;
 static int __init evm_set_fixmode(char *str)
 {
@@ -68,6 +69,14 @@ __setup("evm=", evm_set_fixmode);
 
 static void __init evm_init_config(void)
 {
+	int i, xattrs;
+
+	xattrs = ARRAY_SIZE(evm_config_default_xattrnames);
+
+	for (i = 0; i < xattrs; i++)
+		list_add_tail(&evm_config_default_xattrnames[i].list,
+			      &evm_config_xattrnames);
+
 #ifdef CONFIG_EVM_ATTR_FSUUID
 	evm_hmac_attrs |= EVM_ATTR_FSUUID;
 #endif
@@ -82,15 +91,15 @@ static bool evm_key_loaded(void)
 static int evm_find_protected_xattrs(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	char **xattr;
+	struct xattr_list *xattr;
 	int error;
 	int count = 0;
 
 	if (!(inode->i_opflags & IOP_XATTR))
 		return -EOPNOTSUPP;
 
-	for (xattr = evm_config_xattrnames; *xattr != NULL; xattr++) {
-		error = __vfs_getxattr(dentry, inode, *xattr, NULL, 0);
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+		error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0);
 		if (error < 0) {
 			if (error == -ENODATA)
 				continue;
@@ -211,24 +220,25 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 
 static int evm_protected_xattr(const char *req_xattr_name)
 {
-	char **xattrname;
 	int namelen;
 	int found = 0;
+	struct xattr_list *xattr;
 
 	namelen = strlen(req_xattr_name);
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
-		if ((strlen(*xattrname) == namelen)
-		    && (strncmp(req_xattr_name, *xattrname, namelen) == 0)) {
+	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+		if ((strlen(xattr->name) == namelen)
+		    && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) {
 			found = 1;
 			break;
 		}
 		if (strncmp(req_xattr_name,
-			    *xattrname + XATTR_SECURITY_PREFIX_LEN,
+			    xattr->name + XATTR_SECURITY_PREFIX_LEN,
 			    strlen(req_xattr_name)) == 0) {
 			found = 1;
 			break;
 		}
 	}
+
 	return found;
 }
 
@@ -544,20 +554,33 @@ void __init evm_load_x509(void)
 static int __init init_evm(void)
 {
 	int error;
+	struct list_head *pos, *q;
+	struct xattr_list *xattr;
 
 	evm_init_config();
 
 	error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
 	if (error)
-		return error;
+		goto error;
 
 	error = evm_init_secfs();
 	if (error < 0) {
 		pr_info("Error registering secfs\n");
-		return error;
+		goto error;
 	}
 
-	return 0;
+error:
+	if (error != 0) {
+		if (!list_empty(&evm_config_xattrnames)) {
+			list_for_each_safe(pos, q, &evm_config_xattrnames) {
+				xattr = list_entry(pos, struct xattr_list,
+						   list);
+				list_del(pos);
+			}
+		}
+	}
+
+	return error;
 }
 
 /*
@@ -565,10 +588,11 @@ static int __init init_evm(void)
  */
 static int __init evm_display_config(void)
 {
-	char **xattrname;
+	struct xattr_list *xattr;
+
+	list_for_each_entry(xattr, &evm_config_xattrnames, list)
+		pr_info("%s\n", xattr->name);
 
-	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++)
-		pr_info("%s\n", *xattrname);
 	return 0;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

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

[Matthew Garrett]

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       |  11 +++
 security/integrity/evm/Kconfig      |  11 +++
 security/integrity/evm/evm_crypto.c |   2 +-
 security/integrity/evm/evm_main.c   |   6 +-
 security/integrity/evm/evm_secfs.c  | 133 ++++++++++++++++++++++++++--
 5 files changed, 153 insertions(+), 10 deletions(-)

diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
index d12cb2eae9ee..7dfa5f13f7c6 100644
--- a/Documentation/ABI/testing/evm
+++ b/Documentation/ABI/testing/evm
@@ -57,3 +57,14 @@ 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. Any signatures generated after
+		additional attributes are added (and on files posessing those
+		additional attributes) will only be valid if the same
+		additional attributes are configured on system boot.
diff --git a/security/integrity/evm/Kconfig b/security/integrity/evm/Kconfig
index e825e0ae78e7..54adb3f9ad1d 100644
--- a/security/integrity/evm/Kconfig
+++ b/security/integrity/evm/Kconfig
@@ -42,6 +42,17 @@ config EVM_EXTRA_SMACK_XATTRS
 	  additional info to the calculation, requires existing EVM
 	  labeled file systems to be relabeled.
 
+config EVM_ADD_XATTRS
+	bool "Add additional EVM extended attributes at runtime"
+	depends on EVM
+	default n
+	help
+	  Allow userland to provide additional xattrs for HMAC calculation.
+
+	  When this option is enabled, root can add additional xattrs to the
+	  list used by EVM by writing them into
+	  /sys/kernel/security/evm_xattrs.
+
 config EVM_LOAD_X509
 	bool "Load an X509 certificate onto the '.evm' trusted keyring"
 	depends on EVM && INTEGRITY_TRUSTED_KEYRING
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index caeea20670cc..494da5fcc092 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -208,7 +208,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 		return PTR_ERR(desc);
 
 	error = -ENODATA;
-	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+	list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) {
 		bool is_ima = false;
 
 		if (strcmp(xattr->name, XATTR_NAME_IMA) == 0)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index dd2415c55982..f049af2cc037 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -35,7 +35,7 @@ static const char * const integrity_status_msg[] = {
 };
 int evm_hmac_attrs;
 
-static struct xattr_list evm_config_default_xattrnames[] __ro_after_init = {
+static struct xattr_list evm_config_default_xattrnames[] = {
 #ifdef CONFIG_SECURITY_SELINUX
 	{.name = XATTR_NAME_SELINUX},
 #endif
@@ -98,7 +98,7 @@ static int evm_find_protected_xattrs(struct dentry *dentry)
 	if (!(inode->i_opflags & IOP_XATTR))
 		return -EOPNOTSUPP;
 
-	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+	list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) {
 		error = __vfs_getxattr(dentry, inode, xattr->name, NULL, 0);
 		if (error < 0) {
 			if (error == -ENODATA)
@@ -225,7 +225,7 @@ static int evm_protected_xattr(const char *req_xattr_name)
 	struct xattr_list *xattr;
 
 	namelen = strlen(req_xattr_name);
-	list_for_each_entry(xattr, &evm_config_xattrnames, list) {
+	list_for_each_entry_rcu(xattr, &evm_config_xattrnames, list) {
 		if ((strlen(xattr->name) == namelen)
 		    && (strncmp(req_xattr_name, xattr->name, namelen) == 0)) {
 			found = 1;
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index feba03bbedae..bfce986563e1 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -17,10 +17,16 @@
 
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include "evm.h"
 
 static struct dentry *evm_init_tpm;
 
+#ifdef CONFIG_EVM_ADD_XATTRS
+static struct dentry *evm_xattrs;
+static DEFINE_MUTEX(xattr_list_mutex);
+#endif
+
 /**
  * evm_read_key - read() for <securityfs>/evm
  *
@@ -107,13 +113,128 @@ static const struct file_operations evm_key_ops = {
 	.write		= evm_write_key,
 };
 
-int __init evm_init_secfs(void)
+#ifdef CONFIG_EVM_ADD_XATTRS
+/**
+ * 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;
+
+	rc = mutex_lock_interruptible(&xattr_list_mutex);
+	if (rc)
+		return -ERESTARTSYS;
 
-	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;
+	}
+
+	mutex_unlock(&xattr_list_mutex);
+	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, err;
+	struct xattr_list *xattr;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (*ppos != 0)
+		return -EINVAL;
+
+	if (count > XATTR_NAME_MAX)
+		return -E2BIG;
+
+	xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
+	if (!xattr)
+		return -ENOMEM;
+
+	xattr->name = memdup_user_nul(buf, count);
+	if (IS_ERR(xattr->name)) {
+		err = PTR_ERR(xattr->name);
+		kfree(xattr);
+		return err;
+	}
+
+	/* Remove any trailing newline */
+	len = strlen(xattr->name);
+	if (xattr->name[len-1] == '\n')
+		xattr->name[len-1] = '\0';
+
+	/* Guard against races in evm_read_xattrs */
+	mutex_lock(&xattr_list_mutex);
+	list_add_tail_rcu(&xattr->list, &evm_config_xattrnames);
+	mutex_unlock(&xattr_list_mutex);
+
+	return count;
+}
+
+static const struct file_operations evm_xattr_ops = {
+	.read		= evm_read_xattrs,
+	.write		= evm_write_xattrs,
+};
+
+static int evm_init_xattrs(void) {
+	evm_xattrs = securityfs_create_file("evm_xattrs", 0440, NULL, NULL,
+					    &evm_xattr_ops);
+	if (!evm_xattrs || IS_ERR(evm_xattrs))
+		return -EFAULT;
+
+	return 0;
+}
+#else
+static int evm_init_xattrs(void) {
+	return 0;
+}
+#endif
+
+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;
+
+	if (evm_init_xattrs()) {
+		securityfs_remove(evm_init_tpm);
+		return -EFAULT;
+	}
+
+	return 0;
 }
-- 
2.17.0.441.gb46fe60e1d-goog