> -----Original Message----- > From: Mimi Zohar [mailto:zohar@xxxxxxxxxxxxx] > Sent: Friday, January 31, 2020 3:22 PM > To: Roberto Sassu <roberto.sassu@xxxxxxxxxx>; > jarkko.sakkinen@xxxxxxxxxxxxxxx; > james.bottomley@xxxxxxxxxxxxxxxxxxxxx; linux-integrity@xxxxxxxxxxxxxxx > Cc: linux-security-module@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > Silviu Vlasceanu <Silviu.Vlasceanu@xxxxxxxxxx> > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection in > the measurement list > > On Fri, 2020-01-31 at 14:02 +0000, Roberto Sassu wrote: > > > -----Original Message----- > > > From: linux-integrity-owner@xxxxxxxxxxxxxxx [mailto:linux-integrity- > > > owner@xxxxxxxxxxxxxxx] On Behalf Of Mimi Zohar > > > Sent: Thursday, January 30, 2020 11:26 PM > > > To: Roberto Sassu <roberto.sassu@xxxxxxxxxx>; > > > jarkko.sakkinen@xxxxxxxxxxxxxxx; > > > james.bottomley@xxxxxxxxxxxxxxxxxxxxx; linux- > integrity@xxxxxxxxxxxxxxx > > > Cc: linux-security-module@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; > > > Silviu Vlasceanu <Silviu.Vlasceanu@xxxxxxxxxx> > > > Subject: Re: [PATCH 7/8] ima: use ima_hash_algo for collision detection > in > > > the measurement list > > > > > > On Mon, 2020-01-27 at 18:04 +0100, Roberto Sassu wrote: > > > > Before calculating a digest for each PCR bank, collisions were detected > > > > with a SHA1 digest. This patch includes ima_hash_algo among the > > > algorithms > > > > used to calculate the template digest and checks collisions on that > digest. > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > > > > > Definitely needed to protect against a sha1 collision attack. > > > > > > <snip> > > > > > > > > > > > diff --git a/security/integrity/ima/ima_api.c > > > b/security/integrity/ima/ima_api.c > > > > index ebaf0056735c..a9bb45de6db9 100644 > > > > --- a/security/integrity/ima/ima_api.c > > > > +++ b/security/integrity/ima/ima_api.c > > > > @@ -51,7 +51,7 @@ int ima_alloc_init_template(struct > ima_event_data > > > *event_data, > > > > if (!*entry) > > > > return -ENOMEM; > > > > > > > > - (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 1, > > > > + (*entry)->digests = kcalloc(ima_tpm_chip->nr_allocated_banks + 2, > > > > sizeof(*(*entry)->digests), GFP_NOFS); > > > > if (!(*entry)->digests) { > > > > result = -ENOMEM; > > > > > > I would prefer not having to allocate and use "nr_allocated_banks + 1" > > > everywhere, but I understand the need for it. I'm not sure this patch > > > warrants allocating +2. Perhaps, if a TPM bank doesn't exist for the > > > IMA default hash algorithm, use a different algorithm or, worst case, > > > continue using the ima_sha1_idx. > > > > We could introduce a new option called ima_hash_algo_tpm to specify > > the algorithm of an allocated bank. We can use this for boot_aggregate > > and hash collision detection. > > I don't think that would work in the case where the IMA default hash > is set to sha256, but the system has a TPM 1.2 chip. We would be left > using SHA1 for the file hash collision detection. I thought that using a stronger algorithm for hash collision detection but doing remote attestation with the weaker would not bring additional value. If there is a hash collision on SHA1, an attacker can still replace the data of one of the two entries in the measurement list with the data of the other without being detected (without additional countermeasures). If the verifier additionally checks for duplicate template digests, he could detect the attack (IMA would not add a new measurement entry with the same template digest of previous entries). Ok, I will use ima_hash_algo for hash collision detection. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
