RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate

[Roberto Sassu <roberto.sassu@huawei.com>]

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, March 2, 2020 2:42 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com; Dmitry Kasatkin
> <dmitry.kasatkin@gmail.com>
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
> 
> On Tue, 2020-02-11 at 10:09 +0000, Roberto Sassu wrote:
> > > -----Original Message-----
> 
> Please find/use a mailer that doesn't include this junk.

I will do. I didn't have the time yet.

> > > On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > > > boot_aggregate is the first entry of IMA measurement list. Its purpose
> is
> > > > to link pre-boot measurements to IMA measurements. As IMA was
> > > designed to
> > > > work with a TPM 1.2, the SHA1 PCR bank was always selected.
> > > >
> > > > Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> > > > However, the assumption that the SHA1 PCR bank is always available is
> not
> > > > correct, as PCR banks can be selected with the PCR_Allocate() TPM
> > > command.
> > > >
> > > > This patch tries to use ima_hash_algo as hash algorithm for
> > > boot_aggregate.
> > > > If no PCR bank uses that algorithm, the patch tries to find the SHA256
> PCR
> > > > bank (which is mandatory in the TCG PC Client specification).
> > >
> > > Up to here, the patch description matches the code.
> > > > If also this
> > > > bank is not found, the patch selects the first one. If the TPM algorithm
> > > > of that bank is not mapped to a crypto ID, boot_aggregate is set to
> zero.
> > >
> > > This comment and the one inline are left over from previous version.
> >
> > Hi Mimi
> >
> > actually the code does what is described above. bank_idx is initially
> > set to zero and remains as it is if there is no PCR bank for the default
> > IMA algorithm or SHA256.
> 
> Sorry for the delay in continuing to review this patch set.  It took a
> while to write ima-evm-utils regression tests for it.
> 
> Dmitry and you were the ones that initiated ima-evm-utils, saying
> there should a single package for signing files and integrity testing.
>  The features in ima-evm-utils should reflect what is actually
> upstreamed in the kernel.  (Currently there are a few experimental
> features which were never upstreamed.  I'd like to remove them, but am
> a bit concerned that they are being used.)  I'd appreciate your help
> in keeping ima-evm-utils up to date.  It will help simplify
> upstreaming new kernel features.
> 
> My initial patch attempted to use any common TPM and kernel hash
> algorithm to calculate the boot_aggregate.  The discussion with James
> was pretty clear, which you even stated in the Changelog.  Either we
> use the IMA default hash algorithm, SHA256 for TPM 2.0 or SHA1 for TPM
> 1.2 for the boot-aggregate.

Ok, I didn't understand fully. I thought we should use the default IMA
algorithm and select SHA256 as fallback choice for TPM 2.0 if there is no
PCR bank for default algorithm. I additionally implemented the logic to
select the first PCR bank if the SHA256 PCR bank is not available but I can
remove it.

SHA256 should be the minimum requirement for boot aggregate. The
advantage of using the default IMA algorithm is that it will be possible to
select stronger algorithms when they are supported by the TPM. We might
introduce a new option to specify only the algorithm for boot aggregate,
like James suggested to support embedded systems. Let me know which
option you prefer.

Thanks

Roberto

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