Re: [PATCH v1 4/5] ima: support fs-verity file digest based signatures

[Vitaly Chikunov]

Eric,

On Wed, Jan 05, 2022 at 03:37:39PM -0800, Eric Biggers wrote:
> On Fri, Dec 31, 2021 at 10:35:00AM -0500, Mimi Zohar wrote:
> > On Thu, 2021-12-02 at 14:07 -0800, Eric Biggers wrote:
> > > On Thu, Dec 02, 2021 at 04:55:06PM -0500, Mimi Zohar wrote:
> > > >  	case IMA_VERITY_DIGSIG:
> > > > -		fallthrough;
> > > > +		set_bit(IMA_DIGSIG, &iint->atomic_flags);
> > > > +
> > > > +		/*
> > > > +		 * The IMA signature is based on a hash of IMA_VERITY_DIGSIG
> > > > +		 * and the fs-verity file digest, not directly on the
> > > > +		 * fs-verity file digest.  Both digests should probably be
> > > > +		 * included in the IMA measurement list, but for now this
> > > > +		 * digest is only used for verifying the IMA signature.
> > > > +		 */
> > > > +		verity_digest[0] = IMA_VERITY_DIGSIG;
> > > > +		memcpy(verity_digest + 1, iint->ima_hash->digest,
> > > > +		       iint->ima_hash->length);
> > > > +
> > > > +		hash.hdr.algo = iint->ima_hash->algo;
> > > > +		hash.hdr.length = iint->ima_hash->length;
> > > 
> > > This is still wrong because the bytes being signed don't include the hash
> > > algorithm.  Unless you mean for it to be implicitly always SHA-256?  fs-verity
> > > supports SHA-512 too, and it may support other hash algorithms in the future.
> > 
> > IMA assumes that the file hash algorithm and the signature algorithm
> > are the same.   If they're not the same, for whatever reason, the
> > signature verification would simply fail.
> > 
> > Based on the v2 signature header 'type' field, IMA can differentiate
> > between regular IMA file hash based signatures and fs-verity file
> > digest based signatures.  The digest field (d-ng) in the IMA
> > meausrement list prefixes the digest with the hash algorithm. I'm
> > missing the reason for needing to hash fs-verity's file digest with
> > other metadata, and sign that hash rather than fs-verity's file digest
> > directly.
> 
> Because if someone signs a raw hash, then they also implicitly sign the same
> hash value for all supported hash algorithms that produce the same length hash.

Unless there is broken hash algorithm allowing for preimage attacks this
is irrelevant. If there is two broken algorithms allowing for collisions,
colliding hashes could be prepared even if algo id is hashed too.

Thanks,

> Signing a raw hash is only appropriate when there is only 1 supported algorithm.
> 
> All the other stuff you mentioned is irrelevant.
> 
> - Eric