Re: [PATCH 0/7] Split fsverity-utils into a shared library

[Eric Biggers <ebiggers@kernel.org>]

Hi Jes,

On Tue, Feb 11, 2020 at 06:35:45PM -0500, Jes Sorensen wrote:
> On 2/11/20 6:14 PM, Eric Biggers wrote:
> > On Tue, Feb 11, 2020 at 05:09:22PM -0500, Jes Sorensen wrote:
> >> On 2/11/20 2:22 PM, Eric Biggers wrote:
> >>> Hi Jes,
> >> So I basically want to be able to carry verity signatures in RPM as RPM
> >> internal data, similar to how it supports IMA signatures. I want to be
> >> able to install those without relying on post-install scripts and
> >> signature files being distributed as actual files that gets installed,
> >> just to have to remove them. This is how IMA support is integrated into
> >> RPM as well.
> >>
> >> Right now the RPM approach for signatures involves two steps, a build
> >> digest phase, and a sign the digest phase.
> >>
> >> The reason I included enable and measure was for completeness. I don't
> >> care wildly about those.
> > 
> > So the signing happens when the RPM is built, not when it's installed?  Are you
> > sure you actually need a library and not just 'fsverity sign' called from a
> > build script?
> 
> So the way RPM is handling these is to calculate the digest in one
> place, and sign it in another. Basically the signing is a second step,
> post build, using the rpmsign command. Shelling out is not a good fit
> for this model.
> 
> >>> Separately, before you start building something around fs-verity's builtin
> >>> signature verification support, have you also considered adding support for
> >>> fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
> >>> signature mechanism.  The IMA maintainer has been expressed interested in that.
> >>> If rpm already supports IMA signatures, maybe that way would be a better fit?
> >>
> >> I looked at IMA and it is overly complex. It is not obvious to me how
> >> you would get around that without the full complexity of IMA? The beauty
> >> of fsverity's approach is the simplicity of relying on just the kernel
> >> keyring for validation of the signature. If you have explicit
> >> suggestions, I am certainly happy to look at it.
> > 
> > fs-verity's builtin signature verification feature is simple, but does it
> > actually do what you need?  Note that unlike IMA, it doesn't provide an
> > in-kernel policy about which files have to have signatures and which don't.
> > I.e., to get any authenticity guarantee, before using any files that are
> > supposed to be protected by fs-verity, userspace has to manually check whether
> > the fs-verity bit is actually set.  Is that part of your design?
> 
> Totally aware of this, and it fits the model I am looking at.
> 

Well, this might be a legitimate use case then.  We need to define the library
interface as simply as possible, though, so that we can maintain this code in
the future without breaking users.  I suggest starting with something along the
lines of:

#ifndef _LIBFSVERITY_H
#define _LIBFSVERITY_H

#include <stddef.h>
#include <stdint.h>

#define FS_VERITY_HASH_ALG_SHA256       1
#define FS_VERITY_HASH_ALG_SHA512       2

struct libfsverity_merkle_tree_params {
	uint32_t version;
	uint32_t hash_algorithm;
	uint32_t block_size;
	uint32_t salt_size;
	const uint8_t *salt;
	size_t reserved[11];
};

struct libfsverity_digest {
	uint16_t digest_algorithm;
	uint16_t digest_size;
	uint8_t digest[];
};

struct libfsverity_signature_params {
	const char *keyfile;
	const char *certfile;
	size_t reserved[11];
};

int libfsverity_compute_digest(int fd,
			       const struct libfsverity_merkle_tree_params *params,
			       struct libfsverity_digest **digest_ret);

int libfsverity_sign_digest(const struct libfsverity_digest *digest,
			    const struct libfsverity_signature_params *sig_params,
			    void **sig_ret, size_t *sig_size_ret);

#endif /* _LIBFSVERITY_H */



I.e.:

- The stuff in util.h and hash_algs.h isn't exposed to library users.
- Then names of all library functions and structs are appropriately prefixed
  and avoid collisions with the kernel header.
- Only signing functionality is included.
- There are reserved fields, so we can add more parameters later.

Before committing to any stable API, it would also be helpful to see the RPM
patches to see what it actually does.

We'd also need to follow shared library best practices like compiling with
-fvisibility=hidden and marking the API functions explicitly with
__attribute__((visibility("default"))), and setting the 'soname' like
-Wl,-soname=libfsverity.so.0.

Also, is the GPLv2+ license okay for the use case?

- Eric