Re: [PATCH v4 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files

From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Mimi Zohar <zohar@kernel.org>
Cc: linux-integrity@vger.kernel.org,
	William Roberts <bill.c.roberts@gmail.com>
Subject: Re: [PATCH v4 ima-evm-utils] extend ima_measurement --pcrs option to support per-bank pcr files
Date: Mon, 27 Jul 2020 10:34:52 -0400	[thread overview]
Message-ID: <CAEjxPJ66--SytmNQpHF-DOkoWsE3ri-Ni6nA0k3YmY1fqU-0qQ@mail.gmail.com> (raw)
In-Reply-To: <1595859338.4841.116.camel@kernel.org>

On Mon, Jul 27, 2020 at 10:15 AM Mimi Zohar <zohar@kernel.org> wrote:
>
> On Mon, 2020-07-27 at 09:21 -0400, Stephen Smalley wrote:
>
> > ---
> > v4 updates the usage in the README and usage message, reduces MAX_NPCRFILE
> > to 2 (for sha1 and sha256) and changes the buffer size to
> > MAX_DIGEST_SIZE * 2 + 8 for the lines read from the pcrs file(s).
> >
> > One thing that is unclear to me is correct/expected usage of the
> > --verify and --validate options to evmctl ima_measurement. For an
> > appraisal of a remote attestation, when would one NOT want to use
> > --verify (i.e. doesn't lack of --verify render the result insecure)
> > and when would one want to use --validate (i.e. doesn't use of --validate
> > render the result insecure)? And shouldn't the default in both cases
> > be the more secure case (i.e. verify = 1, validate = 0)?  The naming of
> > --validate is also confusing since one might expect it to mean
> > to validate/check the result as opposed to ignore violations?
>
> Yes, agreed.  Thank you for reviewing and commenting on the code.
>
> While adding support for these features, originally in LTP and the
> standalone version, they should be cleaned up.  Should "--verify" just
> be dropped?

Unless there is some reason to not always verify during
ima_measurement, I'd drop the option and just always do it.

> Without a custom policy, with just the builtin
> "ima_policy=tcb" policy, a few files are read while being opened for
> write (e.g. audit, log, print files).  Perhaps rename "validate" to
> something like "force-validate".

As long as there isn't a backward compatibility concern, that makes
more sense to me. Or "ignore-violations".

> I forgot to add "evmctl boot_aggregate" to the README.  The supplied
> pcrs could also be used to calculate the "boot_aggregate" value(s).

I guess that support is automatically picked up since nothing
restricts usage of the --pcrs option to only ima_measurement and both
call read_tpm_banks(), which includes the pcr file support.  So just a
matter of updating the usage message and README?  That can be done as
a separate patch IMHO.