Re: [PATCH V4] evm: Allow non-SHA1 digital signatures

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

On Tue, 2018-05-15 at 10:53 -0700, Matthew Garrett wrote:
> SHA1 is reasonable in HMAC constructs, but it's desirable to be able to
> use stronger hashes in digital signatures. Modify the EVM crypto code so
> the hash type is imported from the digital signature and passed down to
> the hash calculation code, and return the digest size to higher layers
> for validation.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>

> ---
>  security/integrity/evm/evm.h        | 10 ++++-
>  security/integrity/evm/evm_crypto.c | 59 +++++++++++++++--------------
>  security/integrity/evm/evm_main.c   | 19 ++++++----
>  3 files changed, 51 insertions(+), 37 deletions(-)
> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 45c4a89c02ff..1ed117f360af 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -42,6 +42,11 @@ extern struct crypto_shash *hash_tfm;
>  /* List of EVM protected security xattrs */
>  extern char *evm_config_xattrnames[];
> 
> +struct evm_digest {
> +	struct ima_digest_data hdr;
> +	char digest[IMA_MAX_DIGEST_SIZE];
> +} __packed__;


security/integrity/evm/evm_crypto.o:(.bss+0x0): multiple definition of
`__packed__'
security/integrity/evm/evm_main.o:(.bss+0x20): first defined here
security/integrity/evm/evm_secfs.o:(.bss+0x0): multiple definition of
`__packed__'
security/integrity/evm/evm_main.o:/home/mimi/src/kernel/linux-
integrity/security/integrity/evm/evm_main.c:236: first defined here
Makefile:1036: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

After removing it, the security.evm hmac's are not properly
validating, preventing the EVM/IMA keys from loading.  You can imagine
how far my system boots without the public keys.

Mimi


> +
>  int evm_init_key(void);
>  int evm_update_evmxattr(struct dentry *dentry,
>  			const char *req_xattr_name,
> @@ -49,10 +54,11 @@ int evm_update_evmxattr(struct dentry *dentry,
>  			size_t req_xattr_value_len);
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char *digest);
> +		  size_t req_xattr_value_len, struct evm_digest *data);
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value,
> -		  size_t req_xattr_value_len, char type, char *digest);
> +		  size_t req_xattr_value_len, char type,
> +		  struct evm_digest *data);
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
>  int evm_init_secfs(void);
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index a46fba322340..e266a3f0ec33 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -21,6 +21,7 @@
>  #include <linux/evm.h>
>  #include <keys/encrypted-type.h>
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include "evm.h"
> 
>  #define EVMKEY "evm-key"
> @@ -28,8 +29,7 @@
>  static unsigned char evmkey[MAX_KEY_SIZE];
>  static int evmkey_len = MAX_KEY_SIZE;
> 
> -struct crypto_shash *hmac_tfm;
> -struct crypto_shash *hash_tfm;
> +static struct crypto_shash *evm_tfm[HASH_ALGO__LAST];
> 
>  static DEFINE_MUTEX(mutex);
> 
> @@ -37,9 +37,6 @@ static DEFINE_MUTEX(mutex);
> 
>  static unsigned long evm_set_key_flags;
> 
> -static char * const evm_hmac = "hmac(sha1)";
> -static char * const evm_hash = "sha1";
> -
>  /**
>   * evm_set_key() - set EVM HMAC key from the kernel
>   * @key: pointer to a buffer with the key data
> @@ -74,10 +71,10 @@ int evm_set_key(void *key, size_t keylen)
>  }
>  EXPORT_SYMBOL_GPL(evm_set_key);
> 
> -static struct shash_desc *init_desc(char type)
> +static struct shash_desc *init_desc(char type, uint8_t hash_algo)
>  {
>  	long rc;
> -	char *algo;
> +	const char *algo;
>  	struct crypto_shash **tfm;
>  	struct shash_desc *desc;
> 
> @@ -86,13 +83,11 @@ static struct shash_desc *init_desc(char type)
>  			pr_err_once("HMAC key is not set\n");
>  			return ERR_PTR(-ENOKEY);
>  		}
> -		tfm = &hmac_tfm;
> -		algo = evm_hmac;
> -	} else {
> -		tfm = &hash_tfm;
> -		algo = evm_hash;
>  	}
> 
> +	tfm = &evm_tfm[hash_algo];
> +	algo = hash_algo_name[hash_algo];
> +
>  	if (*tfm == NULL) {
>  		mutex_lock(&mutex);
>  		if (*tfm)
> @@ -186,10 +181,10 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
>   * each xattr, but attempt to re-use the previously allocated memory.
>   */
>  static int evm_calc_hmac_or_hash(struct dentry *dentry,
> -				const char *req_xattr_name,
> -				const char *req_xattr_value,
> -				size_t req_xattr_value_len,
> -				char type, char *digest)
> +				 const char *req_xattr_name,
> +				 const char *req_xattr_value,
> +				 size_t req_xattr_value_len,
> +				 uint8_t type, struct evm_digest *data)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
>  	struct shash_desc *desc;
> @@ -203,10 +198,17 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  	if (!(inode->i_opflags & IOP_XATTR))
>  		return -EOPNOTSUPP;
> 
> -	desc = init_desc(type);
> +	desc = init_desc(type, data->hdr.algo);
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
> 
> +	/*
> +	 * HMACs are always SHA1, but signatures may be of varying sizes -
> +	 * make sure the caller knows how long the returned data is
> +	 */
> +	if (type != EVM_XATTR_HMAC)
> +		data->hdr.length = crypto_shash_digestsize(desc->tfm);
> +
>  	error = -ENODATA;
>  	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
>  		bool is_ima = false;
> @@ -238,7 +240,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>  		if (is_ima)
>  			ima_present = true;
>  	}
> -	hmac_add_misc(desc, inode, type, digest);
> +	hmac_add_misc(desc, inode, type, data->digest);
> 
>  	/* Portable EVM signatures must include an IMA hash */
>  	if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
> @@ -251,18 +253,18 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> 
>  int evm_calc_hmac(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char *digest)
> +		  struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -			       req_xattr_value_len, EVM_XATTR_HMAC, digest);
> +		    req_xattr_value_len, EVM_XATTR_HMAC, data);
>  }
> 
>  int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  const char *req_xattr_value, size_t req_xattr_value_len,
> -		  char type, char *digest)
> +		  char type, struct evm_digest *data)
>  {
>  	return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value,
> -				     req_xattr_value_len, type, digest);
> +		      req_xattr_value_len, type, data);
>  }
> 
>  static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> @@ -302,7 +304,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  			const char *xattr_value, size_t xattr_value_len)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> -	struct evm_ima_xattr_data xattr_data;
> +	struct evm_digest data;
>  	int rc = 0;
> 
>  	/*
> @@ -315,13 +317,14 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  	if (rc)
>  		return -EPERM;
> 
> +	data.hdr.algo = HASH_ALGO_SHA1;
>  	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -			   xattr_value_len, xattr_data.digest);
> +			   xattr_value_len, &data);
>  	if (rc == 0) {
> -		xattr_data.type = EVM_XATTR_HMAC;
> +		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>  		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> -					   &xattr_data,
> -					   sizeof(xattr_data), 0);
> +					   &data.hdr.xattr.data[1],
> +					   SHA1_DIGEST_SIZE + 1, 0);
>  	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>  		rc = __vfs_removexattr(dentry, XATTR_NAME_EVM);
>  	}
> @@ -333,7 +336,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>  {
>  	struct shash_desc *desc;
> 
> -	desc = init_desc(EVM_XATTR_HMAC);
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
>  	if (IS_ERR(desc)) {
>  		pr_info("init_desc failed\n");
>  		return PTR_ERR(desc);
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 9ea9c19a545c..88ea9c1962d6 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -25,6 +25,7 @@
>  #include <linux/magic.h>
> 
>  #include <crypto/hash.h>
> +#include <crypto/hash_info.h>
>  #include <crypto/algapi.h>
>  #include "evm.h"
> 
> @@ -122,8 +123,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  					     struct integrity_iint_cache *iint)
>  {
>  	struct evm_ima_xattr_data *xattr_data = NULL;
> -	struct evm_ima_xattr_data calc;
> +	struct signature_v2_hdr *hdr;
>  	enum integrity_status evm_status = INTEGRITY_PASS;
> +	struct evm_digest digest;
>  	struct inode *inode;
>  	int rc, xattr_len;
> 
> @@ -159,25 +161,28 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  			evm_status = INTEGRITY_FAIL;
>  			goto out;
>  		}
> +
> +		digest.hdr.algo = HASH_ALGO_SHA1;
>  		rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, calc.digest);
> +				   xattr_value_len, &digest);
>  		if (rc)
>  			break;
> -		rc = crypto_memneq(xattr_data->digest, calc.digest,
> -			    sizeof(calc.digest));
> +		rc = crypto_memneq(xattr_data->digest, digest.digest,
> +				   SHA1_DIGEST_SIZE);
>  		if (rc)
>  			rc = -EINVAL;
>  		break;
>  	case EVM_IMA_XATTR_DIGSIG:
>  	case EVM_XATTR_PORTABLE_DIGSIG:
> +		hdr = (struct signature_v2_hdr *)xattr_data;
> +		digest.hdr.algo = hdr->hash_algo;
>  		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
> -				   xattr_value_len, xattr_data->type,
> -				   calc.digest);
> +				   xattr_value_len, xattr_data->type, &digest);
>  		if (rc)
>  			break;
>  		rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM,
>  					(const char *)xattr_data, xattr_len,
> -					calc.digest, sizeof(calc.digest));
> +					digest.digest, digest.hdr.length);
>  		if (!rc) {
>  			inode = d_backing_inode(dentry);
>