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
