| Message ID | 20220109185517.312280-6-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ima: support fs-verity digests and signatures | expand |
Mimi, On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote: > Instead of calculating a file hash and verifying the signature stored > in the security.ima xattr against the calculated file hash, verify the > signature based on a hash of fs-verity's file digest and the digest's > metadata. > > To differentiate between a regular file hash and an fs-verity file digest > based signature stored as security.ima xattr, define a new signature type > named IMA_VERITY_DIGSIG. > > The hash format of fs-verity's file digest and the digest's metadata to > be signed is defined as a structure named "ima_tbs_hash". > > Update the 'ima-sig' template field to display the new fs-verity signature > type as well. > > For example: > appraise func=BPRM_CHECK digest_type=hash|verity > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > Documentation/ABI/testing/ima_policy | 10 +++ > Documentation/security/IMA-templates.rst | 4 +- > include/uapi/linux/ima.h | 26 ++++++++ > security/integrity/ima/ima_appraise.c | 81 +++++++++++++++++++++++ > security/integrity/ima/ima_template_lib.c | 3 +- > security/integrity/integrity.h | 1 + > 6 files changed, 122 insertions(+), 3 deletions(-) > create mode 100644 include/uapi/linux/ima.h > > diff --git a/include/uapi/linux/ima.h b/include/uapi/linux/ima.h > new file mode 100644 > index 000000000000..6a2a68fc0fad > --- /dev/null > +++ b/include/uapi/linux/ima.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* > + * IMA user API > + * > + */ > +#ifndef _UAPI_LINUX_IMA_H > +#define _UAPI_LINUX_IMA_H > + > +#include <linux/types.h> > + > +/* > + * The hash format of fs-verity's file digest and other file metadata > + * to be signed. The resulting signature is stored as a security.ima > + * xattr. > + * > + * "type" is defined as IMA_VERITY_DIGSIG > + * "algo" is the hash_algo enum of fs-verity's file digest > + * (e.g. HASH_ALGO_SHA256, HASH_ALGO_SHA512). > + */ > +struct ima_tbs_hash { > + __u8 type; /* xattr type [enum evm_ima_xattr_type] */ > + __u8 algo; /* Digest algorithm [enum hash_algo] */ > + __u8 digest[]; /* fs-verity digest */ Maximum digest size is known to be FS_VERITY_MAX_DIGEST_SIZE. If it's allocated here then ima_tbs_hash could be allocated temporarily on stack instead of kalloc. > +}; > + > +#endif /* _UAPI_LINUX_IMA_H */ > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index dbba51583e7c..4e092c189ed0 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -13,7 +13,10 @@ > #include <linux/magic.h> > #include <linux/ima.h> > #include <linux/evm.h> > +#include <linux/fsverity.h> > #include <keys/system_keyring.h> > +#include <uapi/linux/fsverity.h> > +#include <uapi/linux/ima.h> > > #include "ima.h" > > @@ -183,6 +186,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, > return ima_hash_algo; > > switch (xattr_value->type) { > + case IMA_VERITY_DIGSIG: > + fallthrough; I think fallthrough is not needed there, since it's just multiple case's and no code. > case EVM_IMA_XATTR_DIGSIG: > sig = (typeof(sig))xattr_value; > if (sig->version != 2 || xattr_len <= sizeof(*sig) > @@ -225,6 +230,47 @@ int ima_read_xattr(struct dentry *dentry, > return ret; > } > > +/* > + * calc_tbs_hash - calculate hash of a digest and digest metadata > + * @type: signature type [IMA_VERITY_DIGSIG] Parameter seems renamed, but why it's ever need if it's called once and ever with IMA_VERITY_DIGSIG? If it's deleted then its value no need to be checked below. > + * @algo: hash algorithm [enum hash_algo] > + * @digest: pointer to the digest to be hashed > + * @hash: (out) pointer to the hash > + * > + * The IMA signature is a signature over the hash of fs-verity's file digest > + * with other digest metadata, not just fs-verity's file digest. Calculate > + * the to be signed hash. > + * > + * Return 0 on success, error code otherwise. > + */ > +static int calc_tbs_hash(enum evm_ima_xattr_type xattr_type, > + enum hash_algo algo, const u8 *digest, > + struct ima_digest_data *hash) > +{ > + struct ima_tbs_hash *tbs_h; > + int rc = 0; > + > + if (xattr_type != IMA_VERITY_DIGSIG) > + return -EINVAL; > + > + tbs_h = kzalloc(sizeof(*tbs_h) + hash_digest_size[algo], GFP_KERNEL); > + if (!tbs_h) > + return -ENOMEM; > + > + tbs_h->type = xattr_type; > + tbs_h->algo = algo; > + memcpy(tbs_h->digest, digest, hash_digest_size[algo]); > + > + hash->algo = algo; As I understood all of this - hash algo used in fs-verity and algo used to hash it here are the same. Ultimate source of which is algo id from xattr - if fs-verity digest algo differs from xattr's then fs-verity digest is ignored. Thanks, > + hash->length = hash_digest_size[algo]; > + > + rc = ima_calc_buffer_hash(tbs_h, > + sizeof(*tbs_h) + hash_digest_size[algo], > + hash); > + kfree(tbs_h); > + return rc; > +} > + > /* > * xattr_verify - verify xattr digest or signature > * > @@ -236,7 +282,9 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > struct evm_ima_xattr_data *xattr_value, int xattr_len, > enum integrity_status *status, const char **cause) > { > + struct ima_digest_data *hash = NULL; > int rc = -EINVAL, hash_start = 0; > + u8 algo; /* Digest algorithm [enum hash_algo] */ > > switch (xattr_value->type) { > case IMA_XATTR_DIGEST_NG: > @@ -271,6 +319,38 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > break; > } > *status = INTEGRITY_PASS; > + break; > + case IMA_VERITY_DIGSIG: > + set_bit(IMA_DIGSIG, &iint->atomic_flags); > + > + algo = iint->ima_hash->algo; > + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], > + GFP_KERNEL); > + if (!hash) { > + *cause = "verity-hashing-error"; > + *status = INTEGRITY_FAIL; > + break; > + } > + > + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, > + iint->ima_hash->digest, hash); > + if (rc) { > + *cause = "verity-hashing-error"; > + *status = INTEGRITY_FAIL; > + break; > + } > + > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > + (const char *)xattr_value, > + xattr_len, hash->digest, > + hash->length); > + if (rc) { > + *cause = "invalid-verity-signature"; > + *status = INTEGRITY_FAIL; > + } else { > + *status = INTEGRITY_PASS; > + } > + > break; > case EVM_IMA_XATTR_DIGSIG: > set_bit(IMA_DIGSIG, &iint->atomic_flags); > @@ -303,6 +383,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > break; > } > > + kfree(hash); > return rc; > } > > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c > index 1c0cea2b805f..31a14943e459 100644 > --- a/security/integrity/ima/ima_template_lib.c > +++ b/security/integrity/ima/ima_template_lib.c > @@ -498,7 +498,8 @@ int ima_eventsig_init(struct ima_event_data *event_data, > { > struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; > > - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) > + if ((!xattr_value) || > + !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG))) > return ima_eventevmsig_init(event_data, field_data); > > return ima_write_template_field_data(xattr_value, event_data->xattr_len, > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index e7ac1086d1d9..51124708c072 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -79,6 +79,7 @@ enum evm_ima_xattr_type { > EVM_IMA_XATTR_DIGSIG, > IMA_XATTR_DIGEST_NG, > EVM_XATTR_PORTABLE_DIGSIG, > + IMA_VERITY_DIGSIG, > IMA_XATTR_LAST > }; > > -- > 2.27.0
On Mon, 2022-01-10 at 04:24 +0300, Vitaly Chikunov wrote: > Mimi, > > On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote: > > Instead of calculating a file hash and verifying the signature stored > > in the security.ima xattr against the calculated file hash, verify the > > signature based on a hash of fs-verity's file digest and the digest's > > metadata. > > > > To differentiate between a regular file hash and an fs-verity file digest > > based signature stored as security.ima xattr, define a new signature type > > named IMA_VERITY_DIGSIG. > > > > The hash format of fs-verity's file digest and the digest's metadata to > > be signed is defined as a structure named "ima_tbs_hash". > > > > Update the 'ima-sig' template field to display the new fs-verity signature > > type as well. > > > > For example: > > appraise func=BPRM_CHECK digest_type=hash|verity > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > Documentation/ABI/testing/ima_policy | 10 +++ > > Documentation/security/IMA-templates.rst | 4 +- > > include/uapi/linux/ima.h | 26 ++++++++ > > security/integrity/ima/ima_appraise.c | 81 +++++++++++++++++++++++ > > security/integrity/ima/ima_template_lib.c | 3 +- > > security/integrity/integrity.h | 1 + > > 6 files changed, 122 insertions(+), 3 deletions(-) > > create mode 100644 include/uapi/linux/ima.h > > > > diff --git a/include/uapi/linux/ima.h b/include/uapi/linux/ima.h > > new file mode 100644 > > index 000000000000..6a2a68fc0fad > > --- /dev/null > > +++ b/include/uapi/linux/ima.h > > @@ -0,0 +1,26 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > > +/* > > + * IMA user API > > + * > > + */ > > +#ifndef _UAPI_LINUX_IMA_H > > +#define _UAPI_LINUX_IMA_H > > + > > +#include <linux/types.h> > > + > > +/* > > + * The hash format of fs-verity's file digest and other file metadata > > + * to be signed. The resulting signature is stored as a security.ima > > + * xattr. > > + * > > + * "type" is defined as IMA_VERITY_DIGSIG > > + * "algo" is the hash_algo enum of fs-verity's file digest > > + * (e.g. HASH_ALGO_SHA256, HASH_ALGO_SHA512). > > + */ > > +struct ima_tbs_hash { > > + __u8 type; /* xattr type [enum evm_ima_xattr_type] */ > > + __u8 algo; /* Digest algorithm [enum hash_algo] */ > > + __u8 digest[]; /* fs-verity digest */ > > Maximum digest size is known to be FS_VERITY_MAX_DIGEST_SIZE. If it's > allocated here then ima_tbs_hash could be allocated temporarily on stack > instead of kalloc. The buffer size to be hashed is currently straight forward "sizeof(*tbs_h) + hash_digest_size[algo]". With this change, we would need to calculate the unused part of FS_VERITY_MAX_DIGEST_SIZE and subtract it from the struct size. Perhaps something like: "sizeof(*tbs_h) - (FS_VERITY_MAX_DIGEST_SIZE - hash_digest_size[algo])" thanks, Mimi > > > +}; > > + > > +#endif /* _UAPI_LINUX_IMA_H */ > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > index dbba51583e7c..4e092c189ed0 100644 > > --- a/security/integrity/ima/ima_appraise.c > > +++ b/security/integrity/ima/ima_appraise.c > > @@ -13,7 +13,10 @@ > > #include <linux/magic.h> > > #include <linux/ima.h> > > #include <linux/evm.h> > > +#include <linux/fsverity.h> > > #include <keys/system_keyring.h> > > +#include <uapi/linux/fsverity.h> > > +#include <uapi/linux/ima.h> > > > > #include "ima.h" > > > > @@ -183,6 +186,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, > > return ima_hash_algo; > > > > switch (xattr_value->type) { > > + case IMA_VERITY_DIGSIG: > > + fallthrough; > > I think fallthrough is not needed there, since it's just multiple case's > and no code. Thanks. > > > case EVM_IMA_XATTR_DIGSIG: > > sig = (typeof(sig))xattr_value; > > if (sig->version != 2 || xattr_len <= sizeof(*sig) > > @@ -225,6 +230,47 @@ int ima_read_xattr(struct dentry *dentry, > > return ret; > > } > > > > +/* > > + * calc_tbs_hash - calculate hash of a digest and digest metadata > > + * @type: signature type [IMA_VERITY_DIGSIG] > > Parameter seems renamed, but why it's ever need if it's called once and > ever with IMA_VERITY_DIGSIG? If it's deleted then its value no need to be > checked below. Thanks for catching the comment and function parameter mismatch. At the moment calc_tbs_hash() is limited to fs-verity. True the parameter isn't needed now, but at some point we probably should add similar support for regular file hashes. That will require incrementing the signature version number for backwards compatability. > > > + * @algo: hash algorithm [enum hash_algo] > > + * @digest: pointer to the digest to be hashed > > + * @hash: (out) pointer to the hash > > + * > > + * The IMA signature is a signature over the hash of fs-verity's file digest > > + * with other digest metadata, not just fs-verity's file digest. Calculate > > + * the to be signed hash. > > + * > > + * Return 0 on success, error code otherwise. > > + */ > > +static int calc_tbs_hash(enum evm_ima_xattr_type xattr_type, > > + enum hash_algo algo, const u8 *digest, > > + struct ima_digest_data *hash) > > +{ > > + struct ima_tbs_hash *tbs_h; > > + int rc = 0; > > + > > + if (xattr_type != IMA_VERITY_DIGSIG) > > + return -EINVAL; > > + > > + tbs_h = kzalloc(sizeof(*tbs_h) + hash_digest_size[algo], GFP_KERNEL); > > + if (!tbs_h) > > + return -ENOMEM; > > + > > + tbs_h->type = xattr_type; > > + tbs_h->algo = algo; > > + memcpy(tbs_h->digest, digest, hash_digest_size[algo]); > > + > > + hash->algo = algo; > > As I understood all of this - hash algo used in fs-verity and algo used > to hash it here are the same. Ultimate source of which is algo id from > xattr - if fs-verity digest algo differs from xattr's then fs-verity > digest is ignored. Right, the assumption is that the fs-verity digest, the tbs hash, and signature all use the same hash algorithm. > > > + hash->length = hash_digest_size[algo]; > > + > > + rc = ima_calc_buffer_hash(tbs_h, > > + sizeof(*tbs_h) + hash_digest_size[algo], > > + hash); > > + kfree(tbs_h); > > + return rc; > > +} > > + > > /* > > * xattr_verify - verify xattr digest or signature > > * > > @@ -236,7 +282,9 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > struct evm_ima_xattr_data *xattr_value, int xattr_len, > > enum integrity_status *status, const char **cause) > > { > > + struct ima_digest_data *hash = NULL; > > int rc = -EINVAL, hash_start = 0; > > + u8 algo; /* Digest algorithm [enum hash_algo] */ > > > > switch (xattr_value->type) { > > case IMA_XATTR_DIGEST_NG: > > @@ -271,6 +319,38 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > break; > > } > > *status = INTEGRITY_PASS; > > + break; > > + case IMA_VERITY_DIGSIG: > > + set_bit(IMA_DIGSIG, &iint->atomic_flags); > > + > > + algo = iint->ima_hash->algo; > > + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], > > + GFP_KERNEL); > > + if (!hash) { > > + *cause = "verity-hashing-error"; > > + *status = INTEGRITY_FAIL; > > + break; > > + } > > + > > + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, > > + iint->ima_hash->digest, hash); > > + if (rc) { > > + *cause = "verity-hashing-error"; > > + *status = INTEGRITY_FAIL; > > + break; > > + } > > + > > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > + (const char *)xattr_value, > > + xattr_len, hash->digest, > > + hash->length); > > + if (rc) { > > + *cause = "invalid-verity-signature"; > > + *status = INTEGRITY_FAIL; > > + } else { > > + *status = INTEGRITY_PASS; > > + } > > + > > break; > > case EVM_IMA_XATTR_DIGSIG: > > set_bit(IMA_DIGSIG, &iint->atomic_flags); > > @@ -303,6 +383,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, > > break; > > } > > > > + kfree(hash); > > return rc; > > } > > > > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c > > index 1c0cea2b805f..31a14943e459 100644 > > --- a/security/integrity/ima/ima_template_lib.c > > +++ b/security/integrity/ima/ima_template_lib.c > > @@ -498,7 +498,8 @@ int ima_eventsig_init(struct ima_event_data *event_data, > > { > > struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; > > > > - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) > > + if ((!xattr_value) || > > + !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG))) > > return ima_eventevmsig_init(event_data, field_data); > > > > return ima_write_template_field_data(xattr_value, event_data->xattr_len, > > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > > index e7ac1086d1d9..51124708c072 100644 > > --- a/security/integrity/integrity.h > > +++ b/security/integrity/integrity.h > > @@ -79,6 +79,7 @@ enum evm_ima_xattr_type { > > EVM_IMA_XATTR_DIGSIG, > > IMA_XATTR_DIGEST_NG, > > EVM_XATTR_PORTABLE_DIGSIG, > > + IMA_VERITY_DIGSIG, > > IMA_XATTR_LAST > > }; > > > > -- > > 2.27.0
On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote: > + case IMA_VERITY_DIGSIG: > + set_bit(IMA_DIGSIG, &iint->atomic_flags); > + > + algo = iint->ima_hash->algo; > + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], > + GFP_KERNEL); > + if (!hash) { > + *cause = "verity-hashing-error"; > + *status = INTEGRITY_FAIL; > + break; > + } > + > + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, > + iint->ima_hash->digest, hash); > + if (rc) { > + *cause = "verity-hashing-error"; > + *status = INTEGRITY_FAIL; > + break; > + } > + > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > + (const char *)xattr_value, > + xattr_len, hash->digest, > + hash->length); This is still verifying a raw hash value, which is wrong as I've explained several times. Yes, you are now hashing the hash algorithm ID together with the original hash value, but at the end the thing being signed/verified is still a raw hash value, which is ambigious. I think I see where the confusion is. If rsa-pkcs1pad is used, then the asymmetric algorithm is parameterized by a hash algorithm, and this hash algorithm's identifier is automatically built-in to the data which is signed/verified. And the data being signed/verified is assumed to be a hash value of the same type. So in this case, the caller doesn't need to handle disambiguating raw hashes. However, asymmetric_verify() also supports ecdsa and ecrdsa signatures. As far as I can tell, those do *not* have the hash algorithm identifier built-in to the data which is signed/verified; they just sign/verify the data given. That creates an ambiguity if the hash algorithm identifier is not included. For example, someone might have intended to sign the SHA-256 hash 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b. However, the Streebog or SM3 hash 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b would also pass the signature check too. That's wrong; to have a valid cryptosystem, you mustn't let the adversary choose the crypto algorithms for you. I'm not sure how this can be reconciled, given the differences between rsa-pkcs1pad and ecdsa and ecrdsa. Could you just use the lowest common denominator and prepend the hash algorithm ID to the hash value, or would that cause issues with rsa-pkcs1pad? In any case, to move forward you're going to need to solve this problem. - Eric
On 1/10/22 17:45, Eric Biggers wrote: > On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote: >> + case IMA_VERITY_DIGSIG: >> + set_bit(IMA_DIGSIG, &iint->atomic_flags); >> + >> + algo = iint->ima_hash->algo; >> + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], >> + GFP_KERNEL); >> + if (!hash) { >> + *cause = "verity-hashing-error"; >> + *status = INTEGRITY_FAIL; >> + break; >> + } >> + >> + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, >> + iint->ima_hash->digest, hash); >> + if (rc) { >> + *cause = "verity-hashing-error"; >> + *status = INTEGRITY_FAIL; >> + break; >> + } >> + >> + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, >> + (const char *)xattr_value, >> + xattr_len, hash->digest, >> + hash->length); > This is still verifying a raw hash value, which is wrong as I've explained > several times. Yes, you are now hashing the hash algorithm ID together with the > original hash value, but at the end the thing being signed/verified is still a > raw hash value, which is ambigious. > > I think I see where the confusion is. If rsa-pkcs1pad is used, then the > asymmetric algorithm is parameterized by a hash algorithm, and this hash > algorithm's identifier is automatically built-in to the data which is > signed/verified. And the data being signed/verified is assumed to be a hash > value of the same type. So in this case, the caller doesn't need to handle > disambiguating raw hashes. > > However, asymmetric_verify() also supports ecdsa and ecrdsa signatures. As far > as I can tell, those do *not* have the hash algorithm identifier built-in to the > data which is signed/verified; they just sign/verify the data given. That The signatures are generated by evmctl by this code here, which works for RSA and ECDSA and likely also ECRDSA: https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/src/libimaevm.c#l1036 if (!EVP_PKEY_sign_init(ctx)) goto err; st = "EVP_get_digestbyname"; if (!(md = EVP_get_digestbyname(algo))) goto err; st = "EVP_PKEY_CTX_set_signature_md"; if (!EVP_PKEY_CTX_set_signature_md(ctx, md)) goto err; st = "EVP_PKEY_sign"; sigsize = MAX_SIGNATURE_SIZE - sizeof(struct signature_v2_hdr) - 1; if (!EVP_PKEY_sign(ctx, hdr->sig, &sigsize, hash, size)) goto err; len = (int)sigsize; As far as I know, these are not raw signatures but generate the OIDs for RSA + shaXYZ or ECDSA + shaXYZ (1.2.840.10045.4.1 et al) and prepend them to the hash and then sign that. > creates an ambiguity if the hash algorithm identifier is not included. For > example, someone might have intended to sign the SHA-256 hash > 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b. However, the > Streebog or SM3 hash > 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b would also pass > the signature check too. That's wrong; to have a valid cryptosystem, you > mustn't let the adversary choose the crypto algorithms for you. There's a hash algorithm identifier in the xattr in the header, which is prepended to the bytes of the actual signature. This hash algo identifer tells IMA which hash to use on the file data so that subsequent signature verification with the same hash works. That same hash identifier is then again embedded in the signature using the OID and thus has to match on the signature verification level. The effectively double hashed data via calc_tbs_hash() above is not good. calc_tbs_hash() is unnecessary. On the evmctl level the signature should be created from the digest retrieved via ioctl() [or similar I suppose] from fsverity on the file and fsverity presumably then also says what type of hash this is. So, fsverity ioctl response of hash + size of hash and hash_algo become input to the evmctl snippet above. On the kernel level the data from fsverity_get_digest() should be all it takes to verify against an xattr created by evmctl as described. > > I'm not sure how this can be reconciled, given the differences between > rsa-pkcs1pad and ecdsa and ecrdsa. Could you just use the lowest common > denominator and prepend the hash algorithm ID to the hash value, or would that > cause issues with rsa-pkcs1pad? In any case, to move forward you're going to > need to solve this problem. > > - Eric
On Mon, Jan 10, 2022 at 10:26:23PM -0500, Stefan Berger wrote: > > On 1/10/22 17:45, Eric Biggers wrote: > > On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote: > > > + case IMA_VERITY_DIGSIG: > > > + set_bit(IMA_DIGSIG, &iint->atomic_flags); > > > + > > > + algo = iint->ima_hash->algo; > > > + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], > > > + GFP_KERNEL); > > > + if (!hash) { > > > + *cause = "verity-hashing-error"; > > > + *status = INTEGRITY_FAIL; > > > + break; > > > + } > > > + > > > + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, > > > + iint->ima_hash->digest, hash); > > > + if (rc) { > > > + *cause = "verity-hashing-error"; > > > + *status = INTEGRITY_FAIL; > > > + break; > > > + } > > > + > > > + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, > > > + (const char *)xattr_value, > > > + xattr_len, hash->digest, > > > + hash->length); > > This is still verifying a raw hash value, which is wrong as I've explained > > several times. Yes, you are now hashing the hash algorithm ID together with the > > original hash value, but at the end the thing being signed/verified is still a > > raw hash value, which is ambigious. > > > > I think I see where the confusion is. If rsa-pkcs1pad is used, then the > > asymmetric algorithm is parameterized by a hash algorithm, and this hash > > algorithm's identifier is automatically built-in to the data which is > > signed/verified. And the data being signed/verified is assumed to be a hash > > value of the same type. So in this case, the caller doesn't need to handle > > disambiguating raw hashes. > > > > However, asymmetric_verify() also supports ecdsa and ecrdsa signatures. As far > > as I can tell, those do *not* have the hash algorithm identifier built-in to the > > data which is signed/verified; they just sign/verify the data given. That > > > The signatures are generated by evmctl by this code here, which works for > RSA and ECDSA and likely also ECRDSA: > > https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/src/libimaevm.c#l1036 > > if (!EVP_PKEY_sign_init(ctx)) > goto err; > st = "EVP_get_digestbyname"; > if (!(md = EVP_get_digestbyname(algo))) > goto err; > st = "EVP_PKEY_CTX_set_signature_md"; > if (!EVP_PKEY_CTX_set_signature_md(ctx, md)) > goto err; > st = "EVP_PKEY_sign"; > sigsize = MAX_SIGNATURE_SIZE - sizeof(struct signature_v2_hdr) - 1; > if (!EVP_PKEY_sign(ctx, hdr->sig, &sigsize, hash, size)) > goto err; > len = (int)sigsize; > > As far as I know, these are not raw signatures but generate the OIDs for RSA > + shaXYZ or ECDSA + shaXYZ (1.2.840.10045.4.1 et al) and prepend them to the > hash and then sign that. As I said, this appears to be true for RSA but not for ECDSA or ECRDSA. - Eric
diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy index 444bb7ccbe03..fadf90dde289 100644 --- a/Documentation/ABI/testing/ima_policy +++ b/Documentation/ABI/testing/ima_policy @@ -151,6 +151,16 @@ Description: appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512 + Example of measure and appraise rules allowing fs-verity + signed digests on a particular filesystem identified by + it's fsuuid: + + measure func=BPRM_CHECK digest_type=hash|verity \ + fsuuid=... template=ima-sig + appraise func=BPRM_CHECK digest_type=hash|verity \ + fsuuid=... + + Example of measure rule allowing fs-verity's digests on a particular filesystem with indication of type of digest. diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst index 5e31513e8ec4..390936810ebc 100644 --- a/Documentation/security/IMA-templates.rst +++ b/Documentation/security/IMA-templates.rst @@ -71,8 +71,8 @@ descriptors by adding their identifier to the format string - 'd-modsig': the digest of the event without the appended modsig; - 'd-type': the type of file digest (e.g. hash, verity[1]); - 'n-ng': the name of the event, without size limitations; - - 'sig': the file signature, or the EVM portable signature if the file - signature is not found; + - 'sig': the file signature, based on either the file's/fsverity's digest[1], + or the EVM portable signature if the file signature is not found; - 'modsig' the appended file signature; - 'buf': the buffer data that was used to generate the hash without size limitations; - 'evmsig': the EVM portable signature; diff --git a/include/uapi/linux/ima.h b/include/uapi/linux/ima.h new file mode 100644 index 000000000000..6a2a68fc0fad --- /dev/null +++ b/include/uapi/linux/ima.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ +/* + * IMA user API + * + */ +#ifndef _UAPI_LINUX_IMA_H +#define _UAPI_LINUX_IMA_H + +#include <linux/types.h> + +/* + * The hash format of fs-verity's file digest and other file metadata + * to be signed. The resulting signature is stored as a security.ima + * xattr. + * + * "type" is defined as IMA_VERITY_DIGSIG + * "algo" is the hash_algo enum of fs-verity's file digest + * (e.g. HASH_ALGO_SHA256, HASH_ALGO_SHA512). + */ +struct ima_tbs_hash { + __u8 type; /* xattr type [enum evm_ima_xattr_type] */ + __u8 algo; /* Digest algorithm [enum hash_algo] */ + __u8 digest[]; /* fs-verity digest */ +}; + +#endif /* _UAPI_LINUX_IMA_H */ diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index dbba51583e7c..4e092c189ed0 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -13,7 +13,10 @@ #include <linux/magic.h> #include <linux/ima.h> #include <linux/evm.h> +#include <linux/fsverity.h> #include <keys/system_keyring.h> +#include <uapi/linux/fsverity.h> +#include <uapi/linux/ima.h> #include "ima.h" @@ -183,6 +186,8 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, return ima_hash_algo; switch (xattr_value->type) { + case IMA_VERITY_DIGSIG: + fallthrough; case EVM_IMA_XATTR_DIGSIG: sig = (typeof(sig))xattr_value; if (sig->version != 2 || xattr_len <= sizeof(*sig) @@ -225,6 +230,47 @@ int ima_read_xattr(struct dentry *dentry, return ret; } +/* + * calc_tbs_hash - calculate hash of a digest and digest metadata + * @type: signature type [IMA_VERITY_DIGSIG] + * @algo: hash algorithm [enum hash_algo] + * @digest: pointer to the digest to be hashed + * @hash: (out) pointer to the hash + * + * The IMA signature is a signature over the hash of fs-verity's file digest + * with other digest metadata, not just fs-verity's file digest. Calculate + * the to be signed hash. + * + * Return 0 on success, error code otherwise. + */ +static int calc_tbs_hash(enum evm_ima_xattr_type xattr_type, + enum hash_algo algo, const u8 *digest, + struct ima_digest_data *hash) +{ + struct ima_tbs_hash *tbs_h; + int rc = 0; + + if (xattr_type != IMA_VERITY_DIGSIG) + return -EINVAL; + + tbs_h = kzalloc(sizeof(*tbs_h) + hash_digest_size[algo], GFP_KERNEL); + if (!tbs_h) + return -ENOMEM; + + tbs_h->type = xattr_type; + tbs_h->algo = algo; + memcpy(tbs_h->digest, digest, hash_digest_size[algo]); + + hash->algo = algo; + hash->length = hash_digest_size[algo]; + + rc = ima_calc_buffer_hash(tbs_h, + sizeof(*tbs_h) + hash_digest_size[algo], + hash); + kfree(tbs_h); + return rc; +} + /* * xattr_verify - verify xattr digest or signature * @@ -236,7 +282,9 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, struct evm_ima_xattr_data *xattr_value, int xattr_len, enum integrity_status *status, const char **cause) { + struct ima_digest_data *hash = NULL; int rc = -EINVAL, hash_start = 0; + u8 algo; /* Digest algorithm [enum hash_algo] */ switch (xattr_value->type) { case IMA_XATTR_DIGEST_NG: @@ -271,6 +319,38 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, break; } *status = INTEGRITY_PASS; + break; + case IMA_VERITY_DIGSIG: + set_bit(IMA_DIGSIG, &iint->atomic_flags); + + algo = iint->ima_hash->algo; + hash = kzalloc(sizeof(*hash) + hash_digest_size[algo], + GFP_KERNEL); + if (!hash) { + *cause = "verity-hashing-error"; + *status = INTEGRITY_FAIL; + break; + } + + rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo, + iint->ima_hash->digest, hash); + if (rc) { + *cause = "verity-hashing-error"; + *status = INTEGRITY_FAIL; + break; + } + + rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA, + (const char *)xattr_value, + xattr_len, hash->digest, + hash->length); + if (rc) { + *cause = "invalid-verity-signature"; + *status = INTEGRITY_FAIL; + } else { + *status = INTEGRITY_PASS; + } + break; case EVM_IMA_XATTR_DIGSIG: set_bit(IMA_DIGSIG, &iint->atomic_flags); @@ -303,6 +383,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint, break; } + kfree(hash); return rc; } diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 1c0cea2b805f..31a14943e459 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -498,7 +498,8 @@ int ima_eventsig_init(struct ima_event_data *event_data, { struct evm_ima_xattr_data *xattr_value = event_data->xattr_value; - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG)) + if ((!xattr_value) || + !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG))) return ima_eventevmsig_init(event_data, field_data); return ima_write_template_field_data(xattr_value, event_data->xattr_len, diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index e7ac1086d1d9..51124708c072 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -79,6 +79,7 @@ enum evm_ima_xattr_type { EVM_IMA_XATTR_DIGSIG, IMA_XATTR_DIGEST_NG, EVM_XATTR_PORTABLE_DIGSIG, + IMA_VERITY_DIGSIG, IMA_XATTR_LAST };
Instead of calculating a file hash and verifying the signature stored in the security.ima xattr against the calculated file hash, verify the signature based on a hash of fs-verity's file digest and the digest's metadata. To differentiate between a regular file hash and an fs-verity file digest based signature stored as security.ima xattr, define a new signature type named IMA_VERITY_DIGSIG. The hash format of fs-verity's file digest and the digest's metadata to be signed is defined as a structure named "ima_tbs_hash". Update the 'ima-sig' template field to display the new fs-verity signature type as well. For example: appraise func=BPRM_CHECK digest_type=hash|verity Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- Documentation/ABI/testing/ima_policy | 10 +++ Documentation/security/IMA-templates.rst | 4 +- include/uapi/linux/ima.h | 26 ++++++++ security/integrity/ima/ima_appraise.c | 81 +++++++++++++++++++++++ security/integrity/ima/ima_template_lib.c | 3 +- security/integrity/integrity.h | 1 + 6 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 include/uapi/linux/ima.h