Re: [fsverity-utils PATCH 1/2] lib: add libfsverity_enable() and libfsverity_enable_with_sig()

[Luca Boccassi <Luca.Boccassi@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 6675 bytes --]

On Mon, 2020-11-16 at 09:41 -0800, Eric Biggers wrote:
> On Mon, Nov 16, 2020 at 11:52:57AM +0000, Luca Boccassi wrote:
> > On Fri, 2020-11-13 at 16:15 -0800, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > Add convenience functions that wrap FS_IOC_ENABLE_VERITY but take a
> > > 'struct libfsverity_merkle_tree_params' instead of
> > > 'struct fsverity_enable_arg'.  This is useful because it allows
> > > libfsverity users to deal with one common struct.
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > >  include/libfsverity.h | 36 ++++++++++++++++++++++++++++++++++
> > >  lib/enable.c          | 45 +++++++++++++++++++++++++++++++++++++++++++
> > >  programs/cmd_enable.c | 28 +++++++++++++++------------
> > >  3 files changed, 97 insertions(+), 12 deletions(-)
> > >  create mode 100644 lib/enable.c
> > > 
> > > diff --git a/include/libfsverity.h b/include/libfsverity.h
> > > index 8f78a13..a8aecaf 100644
> > > --- a/include/libfsverity.h
> > > +++ b/include/libfsverity.h
> > > @@ -112,6 +112,42 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
> > >  			const struct libfsverity_signature_params *sig_params,
> > >  			uint8_t **sig_ret, size_t *sig_size_ret);
> > >  
> > > +/**
> > > + * libfsverity_enable() - Enable fs-verity on a file
> > > + * @fd: read-only file descriptor to the file
> > > + * @params: pointer to the Merkle tree parameters
> > > + *
> > > + * This is a simple wrapper around the FS_IOC_ENABLE_VERITY ioctl.
> > > + *
> > > + * Return: 0 on success, -EINVAL for invalid arguments, or a negative errno
> > > + *	   value from the FS_IOC_ENABLE_VERITY ioctl.  See
> > > + *	   Documentation/filesystems/fsverity.rst in the kernel source tree for
> > > + *	   the possible error codes from FS_IOC_ENABLE_VERITY.
> > > + */
> > > +int
> > > +libfsverity_enable(int fd, const struct libfsverity_merkle_tree_params *params);
> > > +
> > > +/**
> > > + * libfsverity_enable_with_sig() - Enable fs-verity on a file, with a signature
> > > + * @fd: read-only file descriptor to the file
> > > + * @params: pointer to the Merkle tree parameters
> > > + * @sig: pointer to the file's signature
> > > + * @sig_size: size of the file's signature in bytes
> > > + *
> > > + * Like libfsverity_enable(), but allows specifying a built-in signature (i.e. a
> > > + * singature created with libfsverity_sign_digest()) to associate with the file.
> > > + * This is only needed if the in-kernel signature verification support is being
> > > + * used; it is not needed if signatures are being verified in userspace.
> > > + *
> > > + * If @sig is NULL and @sig_size is 0, this is the same as libfsverity_enable().
> > > + *
> > > + * Return: See libfsverity_enable().
> > > + */
> > > +int
> > > +libfsverity_enable_with_sig(int fd,
> > > +			    const struct libfsverity_merkle_tree_params *params,
> > > +			    const uint8_t *sig, size_t sig_size);
> > > +
> > 
> > I don't have a strong preference either way, but any specific reason
> > for a separate function rather than treating sig == NULL and sig_size
> > == 0 as a signature-less enable? For clients deploying files, it would
> > appear easier to me to just use empty parameters to choose between
> > signed/not signed, without having to also change which API to call. But
> > maybe there's some use case I'm missing where it's better to be
> > explicit.
> 
> libfsverity_enable_with_sig() works since that; it allows sig == NULL and
> sig_size == 0.
> 
> The reason I don't want the regular libfsverity_enable() to take the signature
> parameters is that I'd like to encourage people to do userspace signature
> verification instead.  I want to avoid implying that the in-kernel signature
> verification is something that everyone should use.  Same reason I didn't want
> 'fsverity digest' to output fsverity_formatted_digest by default.

Ok, I understand - makes sense to me, thanks.

Maybe it's worth documenting in the the header description of the API
that empty/null values are accepted and will result in enabling without
signature check?

> > > +LIBEXPORT int
> > > +libfsverity_enable_with_sig(int fd,
> > > +			    const struct libfsverity_merkle_tree_params *params,
> > > +			    const uint8_t *sig, size_t sig_size)
> > > +{
> > > +	struct fsverity_enable_arg arg = {};
> > > +
> > > +	if (!params) {
> > > +		libfsverity_error_msg("missing required parameters for enable");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	arg.version = 1;
> > > +	arg.hash_algorithm = params->hash_algorithm;
> > > +	arg.block_size = params->block_size;
> > > +	arg.salt_size = params->salt_size;
> > > +	arg.salt_ptr = (uintptr_t)params->salt;
> > > +	arg.sig_size = sig_size;
> > > +	arg.sig_ptr = (uintptr_t)sig;
> > > +
> > > +	if (ioctl(fd, FS_IOC_ENABLE_VERITY, &arg) != 0)
> > > +		return -errno;
> > > +	return 0;
> > > +}
> > 
> > I'm ok with leaving file handling to clients - after all, depending on
> > infrastructure/bindings/helper libs/whatnot, file handling might vary
> > wildly.
> > 
> > But could we at least provide a default for block size and hash algo,
> > if none are specified?
> > 
> > While file handling is a generic concept and expected to be a solved
> > problem for most programs, figuring out what the default block size
> > should be or what hash algorithm to use is, are fs-verity specific
> > concepts that most clients (at least those that I deal with) wouldn't
> > care about in any way outside of this use, and would want it to "just
> > work".
> > 
> > Apart from these 2 comments, I ran a quick test of the 2 new series,
> > and everything works as expected. Thanks!
> 
> First, providing defaults in libfsverity_enable() doesn't make sense unless the
> same defaults are provided in libfsverity_compute_digest() too.
> 
> Second, PAGE_SIZE is a bad default block size.  It really should be a fixed
> value, like 4096, so that e.g. if you sign files on both x86 and PowerPC, or
> sign on x86 and enable on PowerPC, you get the same results.
> 
> So at least we shouldn't provide defaults unless it's done right.
> 
> I suppose it's probably not too late to change the default for the fsverity
> program, though.  I doubt anyone is using it with a non-4K PAGE_SIZE yet.
> 
> Would it work for you if both libfsverity_compute_digest() and
> libfsverity_enable() defaulted to SHA-256 and 4096?
> 
> - Eric

Yes, using those defaults in the functions would work perfectly fine
for me, thank you!

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]