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

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Jan 20, 2022 at 11:39:56AM -0500, Mimi Zohar wrote:
> 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?

Yes, I was working on some patches at
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/log/?h=wip-keys-fixes
but got distracted by other things, as well as finding the problems with SM2
which will take some time to decide what to do with.  I'll try to get some
patches ready, but there are a lot of things to fix.

> 
> 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
> };

You could certainly declare a fixed-length struct, but only sign/verify the used
portion of it.  The fixed-length struct would just be an implementation detail.

Alternatively, you could always sign/verify a fixed-length struct, with any
shorter hashes zero-padded to HASH_MAX_DIGESTSIZE (64 bytes).  This would be
fine if you're confident that hashes longer than 64 bytes will never be used.
(They don't make sense cryptographically, but who knows.)

For future extensibility, you might want to call the 'is_fsverity' field
something like 'hash_type', so it would be like an enum rather than a bool.  I
just used 'is_fsverity' as a minimal example.

- Eric