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@xxxxxxxxxx>
> ---
> 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);
>
