| Message ID | 20180420203408.50769-1-mjg59@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 20, 2018 at 1:34 PM Matthew Garrett <mjg59@google.com> 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. Hi! Any feedback on this?
[Cc'ing Thiago Bauermann] On Fri, 2018-04-20 at 13:34 -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. Currently the code passes just the digest field of the evm_ima_xattr_data structure to evm_calc_hmac() and evm_calc_hash(). Instead of passing three fields - hash algorithm, digest size, and digest - pass a structure. Consider using the existing ima_digest_data structure. My only concern is that Thiago is working in this area of the code. Thiago, any comments? Mimi > > Signed-off-by: Matthew Garrett <mjg59@google.com> > --- > security/integrity/evm/evm.h | 5 ++-- > security/integrity/evm/evm_crypto.c | 40 ++++++++++++++++++++--------- > security/integrity/evm/evm_main.c | 24 ++++++++++++----- > 3 files changed, 48 insertions(+), 21 deletions(-) > > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h > index 45c4a89c02ff..ae1674d86e4c 100644 > --- a/security/integrity/evm/evm.h > +++ b/security/integrity/evm/evm.h > @@ -49,10 +49,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, char **digest); > 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, uint8_t hash_algo, > + char **digest, int *digestlen); > 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..a3e30e34bcf5 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" > @@ -74,10 +75,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; > > @@ -90,7 +91,7 @@ static struct shash_desc *init_desc(char type) > algo = evm_hmac; > } else { > tfm = &hash_tfm; > - algo = evm_hash; > + algo = hash_algo_name[hash_algo]; > } > > if (*tfm == NULL) { > @@ -189,7 +190,8 @@ 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) > + uint8_t type, uint8_t hash_algo, > + char **digest, int *digestlen) > { > struct inode *inode = d_backing_inode(dentry); > struct shash_desc *desc; > @@ -203,10 +205,24 @@ 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, hash_algo); > if (IS_ERR(desc)) > return PTR_ERR(desc); > > + /* > + * HMACs are always SHA1, but signatures may be of varying sizes, so > + * allocate a buffer for the non-HMAC case and return the size to > + * the caller > + */ > + if (type != EVM_XATTR_HMAC) { > + *digestlen = crypto_shash_digestsize(desc->tfm); > + *digest = kmalloc(*digestlen, GFP_KERNEL); > + if (!*digest) { > + error = -ENOMEM; > + goto out; > + } > + } > + > error = -ENODATA; > for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) { > bool is_ima = false; > @@ -238,7 +254,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, *digest); > > /* Portable EVM signatures must include an IMA hash */ > if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present) > @@ -251,18 +267,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) > + char **digest) > { > 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, 0x02, digest, NULL); > } > > 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, uint8_t hash_algo, char **digest, int *digestlen) > { > return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value, > - req_xattr_value_len, type, digest); > + req_xattr_value_len, type, hash_algo, digest, digestlen); > } > > static int evm_is_immutable(struct dentry *dentry, struct inode *inode) > @@ -316,7 +332,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, > return -EPERM; > > rc = evm_calc_hmac(dentry, xattr_name, xattr_value, > - xattr_value_len, xattr_data.digest); > + xattr_value_len, (char **)&xattr_data.digest); > if (rc == 0) { > xattr_data.type = EVM_XATTR_HMAC; > rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, > @@ -333,7 +349,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, 0x02); > 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..3d952e69852f 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -28,6 +28,8 @@ > #include <crypto/algapi.h> > #include "evm.h" > > +#define HMAC_LEN 20 > + > int evm_initialized; > > static const char * const integrity_status_msg[] = { > @@ -122,10 +124,11 @@ 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 inode *inode; > - int rc, xattr_len; > + int rc, xattr_len, digest_len; > + char *digest = NULL; > > if (iint && (iint->evm_status == INTEGRITY_PASS || > iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) > @@ -155,29 +158,35 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, > /* check value type */ > switch (xattr_data->type) { > case EVM_XATTR_HMAC: > + digest = kmalloc(HMAC_LEN, GFP_KERNEL); > + if (!digest) { > + rc = -ENOMEM; > + break; > + } > if (xattr_len != sizeof(struct evm_ima_xattr_data)) { > evm_status = INTEGRITY_FAIL; > goto out; > } > 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, HMAC_LEN); > if (rc) > rc = -EINVAL; > break; > case EVM_IMA_XATTR_DIGSIG: > case EVM_XATTR_PORTABLE_DIGSIG: > + hdr = (struct signature_v2_hdr *)xattr_value; > + > rc = evm_calc_hash(dentry, xattr_name, xattr_value, > xattr_value_len, xattr_data->type, > - calc.digest); > + hdr->hash_algo, &digest, &digest_len); > if (rc) > break; > rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, > (const char *)xattr_data, xattr_len, > - calc.digest, sizeof(calc.digest)); > + digest, digest_len); > if (!rc) { > inode = d_backing_inode(dentry); > > @@ -203,6 +212,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, > evm_status = (rc == -ENODATA) ? > INTEGRITY_NOXATTRS : INTEGRITY_FAIL; > out: > + kfree(digest); > if (iint) > iint->evm_status = evm_status; > kfree(xattr_data);
Mimi Zohar <zohar@linux.vnet.ibm.com> writes: > [Cc'ing Thiago Bauermann] > > On Fri, 2018-04-20 at 13:34 -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. > > Currently the code passes just the digest field of the > evm_ima_xattr_data structure to evm_calc_hmac() and evm_calc_hash(). > Instead of passing three fields - hash algorithm, digest size, and > digest - pass a structure. Consider using the existing > ima_digest_data structure. > > My only concern is that Thiago is working in this area of the code. > Thiago, any comments? Thanks for your concern. This is not a problem for me. It will be simple to rebase on top of this patch when I post my patches.
Hello Matthew, I have just a few minor nits. Feel free to ignore if you don't think it's worth addressing them. Matthew Garrett <mjg59@google.com> writes: > @@ -203,10 +205,24 @@ 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, hash_algo); > if (IS_ERR(desc)) > return PTR_ERR(desc); > > + /* > + * HMACs are always SHA1, but signatures may be of varying sizes, so > + * allocate a buffer for the non-HMAC case and return the size to > + * the caller > + */ > + if (type != EVM_XATTR_HMAC) { > + *digestlen = crypto_shash_digestsize(desc->tfm); > + *digest = kmalloc(*digestlen, GFP_KERNEL); > + if (!*digest) { > + error = -ENOMEM; > + goto out; > + } > + } > + > error = -ENODATA; > for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) { > bool is_ima = false; I think it's worth mentioning either here or in the comment at the beginning of this function that when type == EVM_XATTR_HMAC *digest is expected to point to an existing buffer. > @@ -251,18 +267,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) > + char **digest) IMHO it's a slightly better interface if evm_calc_hmac() keeps taking a char *, since it won't need to change where its argument points to. > { > 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, 0x02, digest, NULL); It's clearer to use HASH_ALGO_SHA1 instead of 0x02 here. > } > @@ -333,7 +349,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, 0x02); Same here. > 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..3d952e69852f 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -28,6 +28,8 @@ > #include <crypto/algapi.h> > #include "evm.h" > > +#define HMAC_LEN 20 It's clearer to use SHA1_DIGEST_SIZE instead of 20 here. > + > int evm_initialized; > > static const char * const integrity_status_msg[] = { -- Thiago Jung Bauermann IBM Linux Technology Center
diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index 45c4a89c02ff..ae1674d86e4c 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -49,10 +49,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, char **digest); 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, uint8_t hash_algo, + char **digest, int *digestlen); 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..a3e30e34bcf5 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" @@ -74,10 +75,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; @@ -90,7 +91,7 @@ static struct shash_desc *init_desc(char type) algo = evm_hmac; } else { tfm = &hash_tfm; - algo = evm_hash; + algo = hash_algo_name[hash_algo]; } if (*tfm == NULL) { @@ -189,7 +190,8 @@ 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) + uint8_t type, uint8_t hash_algo, + char **digest, int *digestlen) { struct inode *inode = d_backing_inode(dentry); struct shash_desc *desc; @@ -203,10 +205,24 @@ 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, hash_algo); if (IS_ERR(desc)) return PTR_ERR(desc); + /* + * HMACs are always SHA1, but signatures may be of varying sizes, so + * allocate a buffer for the non-HMAC case and return the size to + * the caller + */ + if (type != EVM_XATTR_HMAC) { + *digestlen = crypto_shash_digestsize(desc->tfm); + *digest = kmalloc(*digestlen, GFP_KERNEL); + if (!*digest) { + error = -ENOMEM; + goto out; + } + } + error = -ENODATA; for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) { bool is_ima = false; @@ -238,7 +254,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, *digest); /* Portable EVM signatures must include an IMA hash */ if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present) @@ -251,18 +267,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) + char **digest) { 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, 0x02, digest, NULL); } 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, uint8_t hash_algo, char **digest, int *digestlen) { return evm_calc_hmac_or_hash(dentry, req_xattr_name, req_xattr_value, - req_xattr_value_len, type, digest); + req_xattr_value_len, type, hash_algo, digest, digestlen); } static int evm_is_immutable(struct dentry *dentry, struct inode *inode) @@ -316,7 +332,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, return -EPERM; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, - xattr_value_len, xattr_data.digest); + xattr_value_len, (char **)&xattr_data.digest); if (rc == 0) { xattr_data.type = EVM_XATTR_HMAC; rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, @@ -333,7 +349,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, 0x02); 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..3d952e69852f 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -28,6 +28,8 @@ #include <crypto/algapi.h> #include "evm.h" +#define HMAC_LEN 20 + int evm_initialized; static const char * const integrity_status_msg[] = { @@ -122,10 +124,11 @@ 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 inode *inode; - int rc, xattr_len; + int rc, xattr_len, digest_len; + char *digest = NULL; if (iint && (iint->evm_status == INTEGRITY_PASS || iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) @@ -155,29 +158,35 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, /* check value type */ switch (xattr_data->type) { case EVM_XATTR_HMAC: + digest = kmalloc(HMAC_LEN, GFP_KERNEL); + if (!digest) { + rc = -ENOMEM; + break; + } if (xattr_len != sizeof(struct evm_ima_xattr_data)) { evm_status = INTEGRITY_FAIL; goto out; } 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, HMAC_LEN); if (rc) rc = -EINVAL; break; case EVM_IMA_XATTR_DIGSIG: case EVM_XATTR_PORTABLE_DIGSIG: + hdr = (struct signature_v2_hdr *)xattr_value; + rc = evm_calc_hash(dentry, xattr_name, xattr_value, xattr_value_len, xattr_data->type, - calc.digest); + hdr->hash_algo, &digest, &digest_len); if (rc) break; rc = integrity_digsig_verify(INTEGRITY_KEYRING_EVM, (const char *)xattr_data, xattr_len, - calc.digest, sizeof(calc.digest)); + digest, digest_len); if (!rc) { inode = d_backing_inode(dentry); @@ -203,6 +212,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, evm_status = (rc == -ENODATA) ? INTEGRITY_NOXATTRS : INTEGRITY_FAIL; out: + kfree(digest); if (iint) iint->evm_status = evm_status; kfree(xattr_data);
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 | 5 ++-- security/integrity/evm/evm_crypto.c | 40 ++++++++++++++++++++--------- security/integrity/evm/evm_main.c | 24 ++++++++++++----- 3 files changed, 48 insertions(+), 21 deletions(-)