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

[Mimi Zohar <zohar@linux.ibm.com>]

Eric, Vitaly,

On Tue, 2022-01-18 at 16:39 -0800, Eric Biggers wrote:

> > > The easiest way to do this would be sign/verify the following struct:
> > > 	struct ima_file_id {
> > > 		u8 is_fsverity;
> > > 		u8 hash_algorithm;
> > > 		u8 hash[];
> > > 	};


> > This would be the *data* that is signed/verified -- meaning that it
would be
> > > hashed again as part of the signature algorithm (whether that hash is built-in
> > > to the signature algorithm, as is the case for modern algorithms, or handled by
> > > the caller as is the case for legacy algorithms).
> > 
> > There seems to be an inconsistency, here, with what you said above,
> > "... ECDSA just signs/verifies a raw hash, and in fact it *must* be a
> > raw hash for it to be secure."
> 
> There isn't an inconsistency.  ECDSA is among the algorithms where the caller is
> expected to handle the hash.
> 
> It is confusing dealing with all these different signature algorithms.  I think
> the right way to think about this is in terms of what *data* is being
> signed/verified.  Currently the data is the full file contents.  I think it
> needs to be made into an annotated hash, e.g. the struct I gave.
> 
> public_key_verify_signature() also needs to be fixed to support both types of
> signature algorithms (caller-provided hash and internal hash) in a logical way.
> Originally it only supported caller-provided hashes, but then SM2 support was
> added and now it is super broken.

Eric, did you say you're working on fixes to address these problems?

> 
> > > Note that both traditional
> > > and fs-verity hashes would need to use this same method for it to be secure; the
> > > kernel must not accept signatures using the old method at the same time.
> > 
> > The v2 version of this patch set signed the hash of a hash just for fs-
> > verity signatures.  Adding the equivalent support for regular file
> > hashes will require the version in the IMA signature_v2_hdr to be
> > incremented.  If the version is incremented now, both signatures
> > versions should then be able to co-exist.
> 
> That seems like a good thing, unless you want users to be responsible for only
> ever signing full file hashes *or* fs-verity file hashes with each key.  That
> seems like something that users will get wrong.

Instead of using a flexible array, Vitaly suggested defining the hash
as FS_VERITY_MAX_DIGEST_SIZE, so that it could be allocated temporarily
on stack
instead of kalloc.

As the above struct is not limited to fsverity, we could use
MAX_HASH_DIGESTSIZE, if it was exported, but it isn't.  Would the
following work for you?

/*
 * IMA signature header version 3 disambiguates the data that is signed
by
 * indirectly signing the hash of this structure, containing either the
 * fsverity_descriptor struct digest or, in the future, the traditional
IMA
 * file hash.
 */
struct ima_file_id {
        __u8 is_fsverity;       /* set to 1 for IMA_VERITY_DIGSIG */
        __u8 hash_algorithm;    /* Digest algorithm [enum hash_algo] */
#ifdef __KERNEL__
        __u8 hash[HASH_MAX_DIGESTSIZE];
#else
        __u8 hash[];
#endif
};

thanks,

Mimi