On Mon, Jul 27, 2020 at 10:15 AM Mimi Zohar <zohar@xxxxxxxxxx> 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.
