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

[Eric Biggers]

On Sun, Jan 09, 2022 at 11:45:37PM +0300, Vitaly Chikunov wrote:
> 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.
> 

Only one algorithm needs to be broken.  For example, SM3 has the same hash
length as SHA-256.  If SM3 support were to be added to fs-verity, and if someone
were to find a way to find an input that has a specific SM3 digest, then they
could also make it match a specific SHA-256 digest.  Someone might intend to
sign a SHA-256 digest, but if they are only signing the raw 32 bytes of the
digest, then they would also be signing the corresponding SM3 digest.  That's
why the digest that is signed *must* also include the algorithm used in the
digest (not the algorithm(s) used in the signature, which is different).

- Eric