[fsverity-utils RFC PATCH] Add libfsverity_enable() API

[fsverity-utils RFC PATCH] Add libfsverity_enable() API

[luca.boccassi]

From: Luca Boccassi <luca.boccassi@microsoft.com>

Factor out the 'fsverity enable' implementation in the library, to
give users a shortcut for reading signatures and enabling a file
with the default parameters.

Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
---
Marked as RFC to get guidance on how to deal with helper functions
duplication, that right now are part of the "programs" utility objects
and not usable from the "library" objects.
There's dozens of different ways to handle this, all equally valid, so
it's down to the preference of the maintainer (eg: new common helpers,
include helpers at build time, further splits of sources, etc).
Please provide a preference and I'll follow up.

 common/common_defs.h  |   9 ++
 include/libfsverity.h |  21 +++++
 lib/enable.c          | 191 ++++++++++++++++++++++++++++++++++++++++++
 programs/cmd_enable.c |  66 ++-------------
 programs/fsverity.h   |   9 --
 5 files changed, 227 insertions(+), 69 deletions(-)
 create mode 100644 lib/enable.c

diff --git a/common/common_defs.h b/common/common_defs.h
index 279385a..871db2c 100644
--- a/common/common_defs.h
+++ b/common/common_defs.h
@@ -90,4 +90,13 @@ static inline int ilog2(unsigned long n)
 #  define le64_to_cpu(v)	(__builtin_bswap64((__force u64)(v)))
 #endif
 
+/* The hash algorithm that 'fsverity' assumes when none is specified */
+#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
+
+/*
+ * Largest digest size among all hash algorithms supported by fs-verity.
+ * This can be increased if needed.
+ */
+#define FS_VERITY_MAX_DIGEST_SIZE	64
+
 #endif /* COMMON_COMMON_DEFS_H */
diff --git a/include/libfsverity.h b/include/libfsverity.h
index 8f78a13..8d1f93b 100644
--- a/include/libfsverity.h
+++ b/include/libfsverity.h
@@ -112,6 +112,27 @@ 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
+ *          An fsverity_digest (also called a "file measurement") is the root of
+ *          a file's Merkle tree.  Not to be confused with a traditional file
+ *          digest computed over the entire file.
+ * @file: path to the file to enable
+ * @signature: (optional) path to signature for @file
+ * @params: struct libfsverity_merkle_tree_params specifying the fs-verity
+ *	    version, the hash algorithm, the block size, and
+ *	    optionally a salt.  Reserved fields must be zero.
+ *      All fields bar the version are optional, and defaults will be used
+ *      if set to zero.
+ *
+ * Returns:
+ * * 0 for success, -EINVAL for invalid input arguments, or a generic error
+ *   if the FS_IOC_ENABLE_VERITY ioctl fails.
+ */
+int
+libfsverity_enable(const char *file, const char *signature,
+			struct libfsverity_merkle_tree_params *params);
+
 /**
  * libfsverity_find_hash_alg_by_name() - Find hash algorithm by name
  * @name: Pointer to name of hash algorithm
diff --git a/lib/enable.c b/lib/enable.c
new file mode 100644
index 0000000..ad86cc5
--- /dev/null
+++ b/lib/enable.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Implementation of libfsverity_enable().
+ *
+ * Copyright 2020 Microsoft
+ *
+ * Use of this source code is governed by an MIT-style
+ * license that can be found in the LICENSE file or at
+ * https://opensource.org/licenses/MIT.
+ */
+
+#include "lib_private.h"
+
+#include <fcntl.h>
+#include <getopt.h>
+#include <limits.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <string.h>
+
+
+struct filedes {
+	int fd;
+	char *name;		/* filename, for logging or error messages */
+};
+
+static u32 get_default_block_size(void)
+{
+	long n = sysconf(_SC_PAGESIZE);
+
+	if (n <= 0 || n >= INT_MAX || !is_power_of_2(n)) {
+		libfsverity_error_msg("Warning: invalid _SC_PAGESIZE (%ld).  Assuming 4K blocks.\n",
+			n);
+		return 4096;
+	}
+	return n;
+}
+
+static bool open_file(struct filedes *file, const char *filename, int flags, int mode)
+{
+	file->fd = open(filename, flags, mode);
+	if (file->fd < 0) {
+		libfsverity_error_msg("can't open '%s' for %s", filename,
+				(flags & O_ACCMODE) == O_RDONLY ? "reading" :
+				(flags & O_ACCMODE) == O_WRONLY ? "writing" :
+				"reading and writing");
+		return false;
+	}
+	file->name = strdup(filename);
+	return true;
+}
+
+static bool get_file_size(struct filedes *file, u64 *size_ret)
+{
+	struct stat stbuf;
+
+	if (fstat(file->fd, &stbuf) != 0) {
+		libfsverity_error_msg("can't stat file '%s'", file->name);
+		return false;
+	}
+	*size_ret = stbuf.st_size;
+	return true;
+}
+
+static bool full_read(struct filedes *file, void *buf, size_t count)
+{
+	while (count) {
+		int n = read(file->fd, buf, min(count, INT_MAX));
+
+		if (n < 0) {
+			libfsverity_error_msg("reading from '%s'", file->name);
+			return false;
+		}
+		if (n == 0) {
+			libfsverity_error_msg("unexpected end-of-file on '%s'", file->name);
+			return false;
+		}
+		buf += n;
+		count -= n;
+	}
+	return true;
+}
+
+static bool filedes_close(struct filedes *file)
+{
+	int res;
+
+	if (file->fd < 0)
+		return true;
+	res = close(file->fd);
+	if (res != 0)
+		libfsverity_error_msg("closing '%s'", file->name);
+	file->fd = -1;
+	free(file->name);
+	file->name = NULL;
+	return res == 0;
+}
+
+static bool read_signature(const char *filename, u8 **sig_ret,
+			   u32 *sig_size_ret)
+{
+	struct filedes file = { .fd = -1 };
+	u64 file_size;
+	u8 *sig = NULL;
+	bool ok = false;
+
+	if (!open_file(&file, filename, O_RDONLY, 0))
+		goto out;
+	if (!get_file_size(&file, &file_size))
+		goto out;
+	if (file_size <= 0) {
+		libfsverity_error_msg("signature file '%s' is empty", filename);
+		goto out;
+	}
+	if (file_size > 1000000) {
+		libfsverity_error_msg("signature file '%s' is too large", filename);
+		goto out;
+	}
+	sig = libfsverity_zalloc(file_size);
+	if (!full_read(&file, sig, file_size))
+		goto out;
+	*sig_ret = sig;
+	*sig_size_ret = file_size;
+	sig = NULL;
+	ok = true;
+out:
+	filedes_close(&file);
+	free(sig);
+	return ok;
+}
+
+LIBEXPORT int
+libfsverity_enable(const char *file, const char *signature,
+			struct libfsverity_merkle_tree_params *params)
+{
+	struct fsverity_enable_arg arg;
+	u8 *sig = NULL;
+	struct filedes f;
+	int status;
+
+	if (!file || !params)  {
+		libfsverity_error_msg("missing required parameters for enable");
+		return -EINVAL;
+	}
+
+	arg = (struct fsverity_enable_arg) {
+			.version = params->version,
+			.hash_algorithm = params->hash_algorithm,
+			.block_size = params->block_size,
+			.salt_size = params->salt_size,
+			.salt_ptr = (uintptr_t)params->salt,
+	};
+
+	if (signature && !arg.sig_ptr) {
+		if (!read_signature(signature, &sig, &arg.sig_size))
+			return -EINVAL;
+		arg.sig_ptr = (uintptr_t)sig;
+	}
+
+	if (arg.hash_algorithm == 0)
+		arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
+
+	if (arg.block_size == 0)
+		arg.block_size = get_default_block_size();
+
+	if (!open_file(&f, file, O_RDONLY, 0)) {
+		status = -EINVAL;
+		goto out;
+	}
+	if (ioctl(f.fd, FS_IOC_ENABLE_VERITY, &arg) != 0) {
+		libfsverity_error_msg("FS_IOC_ENABLE_VERITY failed on '%s'",
+				f.name);
+		filedes_close(&f);
+		goto out_err;
+	}
+	if (!filedes_close(&f))
+		goto out_err;
+
+	status = 0;
+out:
+	free(sig);
+	return status;
+
+out_err:
+	status = 1;
+	goto out;
+}
diff --git a/programs/cmd_enable.c b/programs/cmd_enable.c
index d90d208..085b6a3 100644
--- a/programs/cmd_enable.c
+++ b/programs/cmd_enable.c
@@ -11,43 +11,7 @@
 
 #include "fsverity.h"
 
-#include <fcntl.h>
 #include <getopt.h>
-#include <limits.h>
-#include <sys/ioctl.h>
-
-static bool read_signature(const char *filename, u8 **sig_ret,
-			   u32 *sig_size_ret)
-{
-	struct filedes file = { .fd = -1 };
-	u64 file_size;
-	u8 *sig = NULL;
-	bool ok = false;
-
-	if (!open_file(&file, filename, O_RDONLY, 0))
-		goto out;
-	if (!get_file_size(&file, &file_size))
-		goto out;
-	if (file_size <= 0) {
-		error_msg("signature file '%s' is empty", filename);
-		goto out;
-	}
-	if (file_size > 1000000) {
-		error_msg("signature file '%s' is too large", filename);
-		goto out;
-	}
-	sig = xmalloc(file_size);
-	if (!full_read(&file, sig, file_size))
-		goto out;
-	*sig_ret = sig;
-	*sig_size_ret = file_size;
-	sig = NULL;
-	ok = true;
-out:
-	filedes_close(&file);
-	free(sig);
-	return ok;
-}
 
 enum {
 	OPT_HASH_ALG,
@@ -68,10 +32,8 @@ static const struct option longopts[] = {
 int fsverity_cmd_enable(const struct fsverity_command *cmd,
 			int argc, char *argv[])
 {
-	struct fsverity_enable_arg arg = { .version = 1 };
-	u8 *salt = NULL;
-	u8 *sig = NULL;
-	struct filedes file;
+	struct libfsverity_merkle_tree_params arg = { .version = 1 };
+	char *sig = NULL;
 	int status;
 	int c;
 
@@ -86,18 +48,17 @@ int fsverity_cmd_enable(const struct fsverity_command *cmd,
 				goto out_usage;
 			break;
 		case OPT_SALT:
-			if (!parse_salt_option(optarg, &salt, &arg.salt_size))
+			if (!parse_salt_option(optarg, (uint8_t **)&arg.salt, &arg.salt_size))
 				goto out_usage;
-			arg.salt_ptr = (uintptr_t)salt;
 			break;
 		case OPT_SIGNATURE:
 			if (sig != NULL) {
 				error_msg("--signature can only be specified once");
 				goto out_usage;
 			}
-			if (!read_signature(optarg, &sig, &arg.sig_size))
+			sig = strdup(optarg);
+			if (!sig)
 				goto out_err;
-			arg.sig_ptr = (uintptr_t)sig;
 			break;
 		default:
 			goto out_usage;
@@ -110,26 +71,11 @@ int fsverity_cmd_enable(const struct fsverity_command *cmd,
 	if (argc != 1)
 		goto out_usage;
 
-	if (arg.hash_algorithm == 0)
-		arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
-
-	if (arg.block_size == 0)
-		arg.block_size = get_default_block_size();
-
-	if (!open_file(&file, argv[0], O_RDONLY, 0))
-		goto out_err;
-	if (ioctl(file.fd, FS_IOC_ENABLE_VERITY, &arg) != 0) {
-		error_msg_errno("FS_IOC_ENABLE_VERITY failed on '%s'",
-				file.name);
-		filedes_close(&file);
-		goto out_err;
-	}
-	if (!filedes_close(&file))
+	if (libfsverity_enable(argv[0], sig, &arg))
 		goto out_err;
 
 	status = 0;
 out:
-	free(salt);
 	free(sig);
 	return status;
 
diff --git a/programs/fsverity.h b/programs/fsverity.h
index 669fef2..5e33eee 100644
--- a/programs/fsverity.h
+++ b/programs/fsverity.h
@@ -14,15 +14,6 @@
 #include "utils.h"
 #include "../common/fsverity_uapi.h"
 
-/* The hash algorithm that 'fsverity' assumes when none is specified */
-#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
-
-/*
- * Largest digest size among all hash algorithms supported by fs-verity.
- * This can be increased if needed.
- */
-#define FS_VERITY_MAX_DIGEST_SIZE	64
-
 struct fsverity_command;
 
 /* cmd_digest.c */
-- 
2.27.0

Re: [fsverity-utils RFC PATCH] Add libfsverity_enable() API

[Marcus Hüwe]

On 2020-11-13 14:35:27 +0000, luca.boccassi@gmail.com wrote:
> diff --git a/lib/enable.c b/lib/enable.c
> new file mode 100644
> index 0000000..ad86cc5
> --- /dev/null
> +++ b/lib/enable.c

...

> +static bool open_file(struct filedes *file, const char *filename, int flags, int mode)
> +{
> +	file->fd = open(filename, flags, mode);
> +	if (file->fd < 0) {
> +		libfsverity_error_msg("can't open '%s' for %s", filename,
> +				(flags & O_ACCMODE) == O_RDONLY ? "reading" :
> +				(flags & O_ACCMODE) == O_WRONLY ? "writing" :
> +				"reading and writing");
> +		return false;
> +	}
> +	file->name = strdup(filename);

Hmm we should probably check for NULL.

> +	return true;
> +}

(Otherwise, I cannot really comment on the patch...)


Marcus

Re: [fsverity-utils RFC PATCH] Add libfsverity_enable() API

[Eric Biggers]

On Fri, Nov 13, 2020 at 02:35:27PM +0000, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <luca.boccassi@microsoft.com>
> 
> Factor out the 'fsverity enable' implementation in the library, to
> give users a shortcut for reading signatures and enabling a file
> with the default parameters.
> 
> Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> ---
> Marked as RFC to get guidance on how to deal with helper functions
> duplication, that right now are part of the "programs" utility objects
> and not usable from the "library" objects.
> There's dozens of different ways to handle this, all equally valid, so
> it's down to the preference of the maintainer (eg: new common helpers,
> include helpers at build time, further splits of sources, etc).
> Please provide a preference and I'll follow up.
> 
>  common/common_defs.h  |   9 ++
>  include/libfsverity.h |  21 +++++
>  lib/enable.c          | 191 ++++++++++++++++++++++++++++++++++++++++++
>  programs/cmd_enable.c |  66 ++-------------
>  programs/fsverity.h   |   9 --
>  5 files changed, 227 insertions(+), 69 deletions(-)
>  create mode 100644 lib/enable.c
> 
> diff --git a/common/common_defs.h b/common/common_defs.h
> index 279385a..871db2c 100644
> --- a/common/common_defs.h
> +++ b/common/common_defs.h
> @@ -90,4 +90,13 @@ static inline int ilog2(unsigned long n)
>  #  define le64_to_cpu(v)	(__builtin_bswap64((__force u64)(v)))
>  #endif
>  
> +/* The hash algorithm that 'fsverity' assumes when none is specified */
> +#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
> +
> +/*
> + * Largest digest size among all hash algorithms supported by fs-verity.
> + * This can be increased if needed.
> + */
> +#define FS_VERITY_MAX_DIGEST_SIZE	64
> +
>  #endif /* COMMON_COMMON_DEFS_H */
> diff --git a/include/libfsverity.h b/include/libfsverity.h
> index 8f78a13..8d1f93b 100644
> --- a/include/libfsverity.h
> +++ b/include/libfsverity.h
> @@ -112,6 +112,27 @@ 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
> + *          An fsverity_digest (also called a "file measurement") is the root of
> + *          a file's Merkle tree.  Not to be confused with a traditional file
> + *          digest computed over the entire file.
> + * @file: path to the file to enable
> + * @signature: (optional) path to signature for @file
> + * @params: struct libfsverity_merkle_tree_params specifying the fs-verity
> + *	    version, the hash algorithm, the block size, and
> + *	    optionally a salt.  Reserved fields must be zero.
> + *      All fields bar the version are optional, and defaults will be used
> + *      if set to zero.
> + *
> + * Returns:
> + * * 0 for success, -EINVAL for invalid input arguments, or a generic error
> + *   if the FS_IOC_ENABLE_VERITY ioctl fails.
> + */
> +int
> +libfsverity_enable(const char *file, const char *signature,
> +			struct libfsverity_merkle_tree_params *params);
> +
>  /**
>   * libfsverity_find_hash_alg_by_name() - Find hash algorithm by name
>   * @name: Pointer to name of hash algorithm

Hi Luca, can you consider
https://lkml.kernel.org/linux-fscrypt/20201114001529.185751-1-ebiggers@kernel.org/T/#u
instead?

It's somewhat useful to have a wrapper around FS_IOC_ENABLE_VERITY that takes
'struct libfsverity_merkle_tree_params', so that library users can deal with one
common struct.  (And I took advantage of that to simplify the code that parses
the parameters.)

But I think we should keep it as a thin wrapper, and not have file path
parameters or set defaults in the libfsverity_merkle_tree_params.  The library
user is better suited to deal with those, like they already do for
libfsverity_compute_digest().

- Eric

Re: [fsverity-utils RFC PATCH] Add libfsverity_enable() API

[Luca Boccassi]

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

On Fri, 2020-11-13 at 16:21 -0800, Eric Biggers wrote:
> On Fri, Nov 13, 2020 at 02:35:27PM +0000, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <luca.boccassi@microsoft.com>
> > 
> > Factor out the 'fsverity enable' implementation in the library, to
> > give users a shortcut for reading signatures and enabling a file
> > with the default parameters.
> > 
> > Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> > ---
> > Marked as RFC to get guidance on how to deal with helper functions
> > duplication, that right now are part of the "programs" utility objects
> > and not usable from the "library" objects.
> > There's dozens of different ways to handle this, all equally valid, so
> > it's down to the preference of the maintainer (eg: new common helpers,
> > include helpers at build time, further splits of sources, etc).
> > Please provide a preference and I'll follow up.
> > 
> >  common/common_defs.h  |   9 ++
> >  include/libfsverity.h |  21 +++++
> >  lib/enable.c          | 191 ++++++++++++++++++++++++++++++++++++++++++
> >  programs/cmd_enable.c |  66 ++-------------
> >  programs/fsverity.h   |   9 --
> >  5 files changed, 227 insertions(+), 69 deletions(-)
> >  create mode 100644 lib/enable.c
> > 
> > diff --git a/common/common_defs.h b/common/common_defs.h
> > index 279385a..871db2c 100644
> > --- a/common/common_defs.h
> > +++ b/common/common_defs.h
> > @@ -90,4 +90,13 @@ static inline int ilog2(unsigned long n)
> >  #  define le64_to_cpu(v)	(__builtin_bswap64((__force u64)(v)))
> >  #endif
> >  
> > +/* The hash algorithm that 'fsverity' assumes when none is specified */
> > +#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
> > +
> > +/*
> > + * Largest digest size among all hash algorithms supported by fs-verity.
> > + * This can be increased if needed.
> > + */
> > +#define FS_VERITY_MAX_DIGEST_SIZE	64
> > +
> >  #endif /* COMMON_COMMON_DEFS_H */
> > diff --git a/include/libfsverity.h b/include/libfsverity.h
> > index 8f78a13..8d1f93b 100644
> > --- a/include/libfsverity.h
> > +++ b/include/libfsverity.h
> > @@ -112,6 +112,27 @@ 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
> > + *          An fsverity_digest (also called a "file measurement") is the root of
> > + *          a file's Merkle tree.  Not to be confused with a traditional file
> > + *          digest computed over the entire file.
> > + * @file: path to the file to enable
> > + * @signature: (optional) path to signature for @file
> > + * @params: struct libfsverity_merkle_tree_params specifying the fs-verity
> > + *	    version, the hash algorithm, the block size, and
> > + *	    optionally a salt.  Reserved fields must be zero.
> > + *      All fields bar the version are optional, and defaults will be used
> > + *      if set to zero.
> > + *
> > + * Returns:
> > + * * 0 for success, -EINVAL for invalid input arguments, or a generic error
> > + *   if the FS_IOC_ENABLE_VERITY ioctl fails.
> > + */
> > +int
> > +libfsverity_enable(const char *file, const char *signature,
> > +			struct libfsverity_merkle_tree_params *params);
> > +
> >  /**
> >   * libfsverity_find_hash_alg_by_name() - Find hash algorithm by name
> >   * @name: Pointer to name of hash algorithm
> 
> Hi Luca, can you consider
> https://lkml.kernel.org/linux-fscrypt/20201114001529.185751-1-ebiggers@kernel.org/T/#u
> instead?
> 
> It's somewhat useful to have a wrapper around FS_IOC_ENABLE_VERITY that takes
> 'struct libfsverity_merkle_tree_params', so that library users can deal with one
> common struct.  (And I took advantage of that to simplify the code that parses
> the parameters.)
> 
> But I think we should keep it as a thin wrapper, and not have file path
> parameters or set defaults in the libfsverity_merkle_tree_params.  The library
> user is better suited to deal with those, like they already do for
> libfsverity_compute_digest().
> 
> - Eric

Hi,

Sure, no problem, we can drop this, thanks for sending those series.
Left a comment in the other thread.

-- 
Kind regards,
Luca Boccassi

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