RE: [PATCH 3/3] ima: Add digest parameter to the functions to measure a buffer

From: Roberto Sassu
Date: Wed Jun 30 2021 - 11:09:42 EST


> From: Lakshmi Ramasubramanian [mailto:nramas@xxxxxxxxxxxxxxxxxxx]
> Sent: Wednesday, June 30, 2021 4:56 PM
> On 6/30/2021 7:16 AM, Roberto Sassu wrote:
>
> Hi Roberto,
>
> > This patch adds the 'digest' parameter to ima_measure_critical_data() and
> > process_buffer_measurement(), so that callers can get the digest of the
> > passed buffer.
> >
> > These functions calculate the digest even if there is no suitable rule in
> > the IMA policy and, in this case, they simply return 1 before generating a
> > new measurement entry.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > ---
> > include/linux/ima.h | 4 +--
> > security/integrity/ima/ima.h | 2 +-
> > security/integrity/ima/ima_appraise.c | 2 +-
> > security/integrity/ima/ima_asymmetric_keys.c | 2 +-
> > security/integrity/ima/ima_init.c | 2 +-
> > security/integrity/ima/ima_main.c | 31 +++++++++++++-------
> > security/integrity/ima/ima_queue_keys.c | 2 +-
> > security/selinux/ima.c | 4 +--
> > 8 files changed, 30 insertions(+), 19 deletions(-)
> >
>
> >
> > + if (digest)
> > + memcpy(digest, iint.ima_hash->digest,
> > + hash_digest_size[ima_hash_algo]);
>
> I think the caller should also pass the size of the buffer allocated to
> receive the calculated digest. And, here copy only up to that many bytes
> so we don't accidentally cause buffer overrun.

Hi Lakshmi

yes, I agree. I will add it in the next version of the patch set.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> -lakshmi
>
> > +
> > + if (!ima_policy_flag || (func && !(action & IMA_MEASURE)))
> > + return 1;
> > +
> > ret = ima_alloc_init_template(&event_data, &entry, template);
> > if (ret < 0) {
> > audit_cause = "alloc_entry";
> > @@ -966,7 +975,7 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > ret = process_buffer_measurement(file_mnt_user_ns(f.file),
> > file_inode(f.file), buf, size,
> > "kexec-cmdline", KEXEC_CMDLINE, 0,
> > - NULL, false);
> > + NULL, false, NULL);
> > fdput(f);
> > }
> >
> > @@ -977,26 +986,28 @@ void ima_kexec_cmdline(int kernel_fd, const void
> *buf, int size)
> > * @buf: pointer to buffer data
> > * @buf_len: length of buffer data (in bytes)
> > * @hash: measure buffer data hash
> > + * @digest: buffer digest will be written to
> > *
> > * Measure data critical to the integrity of the kernel into the IMA log
> > * and extend the pcr. Examples of critical data could be various data
> > * structures, policies, and states stored in kernel memory that can
> > * impact the integrity of the system.
> > *
> > - * Returns 0 if the buffer has been successfully measured, a negative value
> > - * otherwise.
> > + * Returns 0 if the buffer has been successfully measured, 1 if the digest
> > + * has been written to the passed location but not added to a measurement
> entry,
> > + * a negative value otherwise.
> > */
> > int ima_measure_critical_data(const char *event_label,
> > const char *event_name,
> > const void *buf, size_t buf_len,
> > - bool hash)
> > + bool hash, u8 *digest)
> > {
> > if (!event_name || !event_label || !buf || !buf_len)
> > return -ENOPARAM;
> >
> > return process_buffer_measurement(&init_user_ns, NULL, buf,
> buf_len,
> > event_name, CRITICAL_DATA, 0,
> > - event_label, hash);
> > + event_label, hash, digest);
> > }
> >
> > static int __init init_ima(void)
> > diff --git a/security/integrity/ima/ima_queue_keys.c
> b/security/integrity/ima/ima_queue_keys.c
> > index e3047ce64f39..ac00a4778a91 100644
> > --- a/security/integrity/ima/ima_queue_keys.c
> > +++ b/security/integrity/ima/ima_queue_keys.c
> > @@ -166,7 +166,7 @@ void ima_process_queued_keys(void)
> > entry-
> >keyring_name,
> > KEY_CHECK, 0,
> > entry-
> >keyring_name,
> > - false);
> > + false, NULL);
> > list_del(&entry->list);
> > ima_free_key_entry(entry);
> > }
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > index 4db9fa211638..96bd7ead8081 100644
> > --- a/security/selinux/ima.c
> > +++ b/security/selinux/ima.c
> > @@ -88,7 +88,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> >
> > measure_rc = ima_measure_critical_data("selinux", "selinux-state",
> > state_str, strlen(state_str),
> > - false);
> > + false, NULL);
> >
> > kfree(state_str);
> >
> > @@ -105,7 +105,7 @@ void selinux_ima_measure_state_locked(struct
> selinux_state *state)
> > }
> >
> > measure_rc = ima_measure_critical_data("selinux", "selinux-policy-
> hash",
> > - policy, policy_len, true);
> > + policy, policy_len, true, NULL);
> >
> > vfree(policy);
> > }
> >