Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <c0676336a7992b6495c5f5dec7ca1897fb4005eb.camel@linux.ibm.com>
Subject: Re: [PATCH v1 4/5] ima: support fs-verity file digest based
 signatures
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Eric Biggers <ebiggers@kernel.org>,
        Vitaly Chikunov <vt@altlinux.org>
Cc:     linux-integrity@vger.kernel.org, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Thu, 20 Jan 2022 11:39:56 -0500
In-Reply-To: <Yedd0CKCHSq1ugFk@sol.localdomain>
References: <20211202215507.298415-1-zohar@linux.ibm.com>
         <20211202215507.298415-5-zohar@linux.ibm.com>
         <YalDvGjq0inMFKln@sol.localdomain>
         <56c53b027ae8ae6909d38904bf089e73011657d7.camel@linux.ibm.com>
         <YdYrw4eiQPryOMkZ@gmail.com> <20220109204537.oueokvvkrkyy3ipq@altlinux.org>
         <YdtOhsv/A5dqlApY@sol.localdomain>
         <20220115053101.36xoy2bc7ypozo6l@altlinux.org>
         <YeJn7hxLEfdVrUQT@sol.localdomain>
         <bc803a35d914dde65640428d2b29cc6e89d176d4.camel@linux.ibm.com>
         <Yedd0CKCHSq1ugFk@sol.localdomain>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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