linux-fscrypt.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/9]  Split fsverity-utils into a shared library
@ 2020-03-12 21:47 Jes Sorensen
  2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
                   ` (9 more replies)
  0 siblings, 10 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

Hi,

This is an updated version of my patches to split fsverity-utils into
a shared library. This version addresses most of the comments I
received in the last version:

1) Document the API
2) Verified ran xfstest against the build
3) Make struct fsverity_descriptor private
4) Reviewed (and documented) error codes
5) Improved validation of input parameters, and return error if any
   reserved field is not zero.

I left struct fsverity_hash_alg in the public API, because it adds
useful information to the user, in particular providing the digest
size, and allows the caller to walk the list to obtain the supported
algorithms. The alternative is to introduce a
libverity_get_digest_size() call.

I still need to add some self-tests to the build and deal with the
soname stuff.

Next up is rpm support.

Cheers,
Jes


Jes Sorensen (9):
  Build basic shared library framework
  Change compute_file_measurement() to take a file descriptor as
    argument
  Move fsverity_descriptor definition to libfsverity.h
  Move hash algorithm code to shared library
  Create libfsverity_compute_digest() and adapt cmd_sign to use it
  Introduce libfsverity_sign_digest()
  Validate input arguments to libfsverity_compute_digest()
  Validate input parameters for libfsverity_sign_digest()
  Document API of libfsverity

 Makefile              |  18 +-
 cmd_enable.c          |  11 +-
 cmd_measure.c         |   4 +-
 cmd_sign.c            | 526 +++------------------------------------
 fsverity.c            |  16 +-
 hash_algs.c           |  26 +-
 hash_algs.h           |  27 --
 libfsverity.h         | 127 ++++++++++
 libfsverity_private.h |  33 +++
 libverity.c           | 559 ++++++++++++++++++++++++++++++++++++++++++
 util.h                |   2 +
 11 files changed, 801 insertions(+), 548 deletions(-)
 create mode 100644 libfsverity.h
 create mode 100644 libfsverity_private.h
 create mode 100644 libverity.c

-- 
2.24.1


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/9] Build basic shared library framework
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  5:23   ` Eric Biggers
  2020-03-22  5:33   ` Eric Biggers
  2020-03-12 21:47 ` [PATCH 2/9] Change compute_file_measurement() to take a file descriptor as argument Jes Sorensen
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

This introduces a dummy shared library to start moving things into.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 Makefile    | 18 +++++++++++++++---
 libverity.c | 10 ++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 libverity.c

diff --git a/Makefile b/Makefile
index b9c09b9..bb85896 100644
--- a/Makefile
+++ b/Makefile
@@ -1,20 +1,32 @@
 EXE := fsverity
+LIB := libfsverity.so
 CFLAGS := -O2 -Wall
 CPPFLAGS := -D_FILE_OFFSET_BITS=64
 LDLIBS := -lcrypto
 DESTDIR := /usr/local
+LIBDIR := /usr/lib64
 SRC := $(wildcard *.c)
-OBJ := $(SRC:.c=.o)
+OBJ := fsverity.o hash_algs.o cmd_enable.o cmd_measure.o cmd_sign.o util.o
+SSRC := libverity.c
+SOBJ := libverity.so
 HDRS := $(wildcard *.h)
 
 all:$(EXE)
 
-$(EXE):$(OBJ)
+$(EXE):$(OBJ) $(LIB)
+	$(CC) -o $@ $(OBJ) $(LDLIBS) -L . -l fsverity
 
 $(OBJ): %.o: %.c $(HDRS)
+	$(CC) -c $(CFLAGS) $(CPPFLAGS) $< -o $@
+
+$(SOBJ): %.so: %.c $(HDRS)
+	$(CC) -c -fPIC $(CFLAGS) $(CPPFLAGS) $< -o $@
+
+libfsverity.so: $(SOBJ)
+	$(CC) $(LDLIBS) -shared -o libfsverity.so $(SOBJ)
 
 clean:
-	rm -f $(EXE) $(OBJ)
+	rm -f $(EXE) $(OBJ) $(SOBJ) $(LIB)
 
 install:all
 	install -Dm755 -t $(DESTDIR)/bin $(EXE)
diff --git a/libverity.c b/libverity.c
new file mode 100644
index 0000000..6821aa2
--- /dev/null
+++ b/libverity.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The 'fsverity library'
+ *
+ * Copyright (C) 2018 Google LLC
+ * Copyright (C) 2020 Facebook
+ *
+ * Written by Eric Biggers and Jes Sorensen.
+ */
+
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/9] Change compute_file_measurement() to take a file descriptor as argument
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
  2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-12 21:47 ` [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h Jes Sorensen
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

This preps the code for splitting the signing into the shared library

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c    | 48 +++++++++++++++++++++++++++++++++++++-----------
 fsverity.c    |  1 +
 libfsverity.h | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 11 deletions(-)
 create mode 100644 libfsverity.h

diff --git a/cmd_sign.c b/cmd_sign.c
index dcb37ce..dcc44f8 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -16,6 +16,8 @@
 #include <openssl/pkcs7.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
 
 #include "commands.h"
 #include "fsverity_uapi.h"
@@ -382,11 +384,30 @@ static bool hash_one_block(struct hash_ctx *hash, struct block_buffer *cur,
 	return next->filled + hash->alg->digest_size > block_size;
 }
 
+static int full_read_fd(int fd, void *buf, size_t count)
+{
+	while (count) {
+		int n = read(fd, buf, min(count, INT_MAX));
+
+		if (n < 0) {
+			error_msg_errno("reading from file");
+			return n;
+		}
+		if (n == 0) {
+			error_msg("unexpected end-of-file");
+			return -ENODATA;
+		}
+		buf += n;
+		count -= n;
+	}
+	return 0;
+}
+
 /*
  * Compute the file's Merkle tree root hash using the given hash algorithm,
  * block size, and salt.
  */
-static bool compute_root_hash(struct filedes *file, u64 file_size,
+static bool compute_root_hash(int fd, u64 file_size,
 			      struct hash_ctx *hash, u32 block_size,
 			      const u8 *salt, u32 salt_size, u8 *root_hash)
 {
@@ -424,7 +445,7 @@ static bool compute_root_hash(struct filedes *file, u64 file_size,
 	for (offset = 0; offset < file_size; offset += block_size) {
 		buffers[-1].filled = min(block_size, file_size - offset);
 
-		if (!full_read(file, buffers[-1].data, buffers[-1].filled))
+		if (full_read_fd(fd, buffers[-1].data, buffers[-1].filled))
 			goto out;
 
 		level = -1;
@@ -457,22 +478,22 @@ out:
  * The fs-verity measurement is the hash of the fsverity_descriptor, which
  * contains the Merkle tree properties including the root hash.
  */
-static bool compute_file_measurement(const char *filename,
+static bool compute_file_measurement(int fd,
 				     const struct fsverity_hash_alg *hash_alg,
 				     u32 block_size, const u8 *salt,
 				     u32 salt_size, u8 *measurement)
 {
-	struct filedes file = { .fd = -1 };
 	struct hash_ctx *hash = hash_create(hash_alg);
 	u64 file_size;
 	struct fsverity_descriptor desc;
+	struct stat stbuf;
 	bool ok = false;
 
-	if (!open_file(&file, filename, O_RDONLY, 0))
-		goto out;
-
-	if (!get_file_size(&file, &file_size))
+	if (fstat(fd, &stbuf) != 0) {
+		error_msg_errno("can't stat input file");
 		goto out;
+	}
+	file_size = stbuf.st_size;
 
 	memset(&desc, 0, sizeof(desc));
 	desc.version = 1;
@@ -495,14 +516,13 @@ static bool compute_file_measurement(const char *filename,
 
 	/* Root hash of empty file is all 0's */
 	if (file_size != 0 &&
-	    !compute_root_hash(&file, file_size, hash, block_size, salt,
+	    !compute_root_hash(fd, file_size, hash, block_size, salt,
 			       salt_size, desc.root_hash))
 		goto out;
 
 	hash_full(hash, &desc, sizeof(desc), measurement);
 	ok = true;
 out:
-	filedes_close(&file);
 	hash_free(hash);
 	return ok;
 }
@@ -529,6 +549,7 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 		      int argc, char *argv[])
 {
 	const struct fsverity_hash_alg *hash_alg = NULL;
+	struct filedes file = { .fd = -1 };
 	u32 block_size = 0;
 	u8 *salt = NULL;
 	u32 salt_size = 0;
@@ -603,10 +624,15 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	digest->digest_algorithm = cpu_to_le16(hash_alg - fsverity_hash_algs);
 	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
 
-	if (!compute_file_measurement(argv[0], hash_alg, block_size,
+	if (!open_file(&file, argv[0], O_RDONLY, 0))
+		goto out_err;
+
+	if (!compute_file_measurement(file.fd, hash_alg, block_size,
 				      salt, salt_size, digest->digest))
 		goto out_err;
 
+	filedes_close(&file);
+
 	if (!sign_data(digest, sizeof(*digest) + hash_alg->digest_size,
 		       keyfile, certfile, hash_alg, &sig, &sig_size))
 		goto out_err;
diff --git a/fsverity.c b/fsverity.c
index 9a44df1..c8fa1b5 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -14,6 +14,7 @@
 
 #include "commands.h"
 #include "hash_algs.h"
+#include "libfsverity.h"
 
 static const struct fsverity_command {
 	const char *name;
diff --git a/libfsverity.h b/libfsverity.h
new file mode 100644
index 0000000..ceebae1
--- /dev/null
+++ b/libfsverity.h
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * libfsverity API
+ *
+ * Copyright (C) 2018 Google LLC
+ * Copyright (C) 2020 Facebook
+ *
+ * Written by Eric Biggers and modified by Jes Sorensen.
+ */
+
+#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 {
+	uint16_t version;
+	uint16_t hash_algorithm;
+	uint32_t block_size;
+	uint32_t salt_size;
+	const uint8_t *salt;
+	uint64_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;
+	uint64_t reserved[11];
+};
+
+#endif
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
  2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
  2020-03-12 21:47 ` [PATCH 2/9] Change compute_file_measurement() to take a file descriptor as argument Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  4:57   ` Eric Biggers
  2020-03-12 21:47 ` [PATCH 4/9] Move hash algorithm code to shared library Jes Sorensen
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c    | 19 +------------------
 libfsverity.h | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/cmd_sign.c b/cmd_sign.c
index dcc44f8..1792084 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -20,26 +20,9 @@
 #include <unistd.h>
 
 #include "commands.h"
-#include "fsverity_uapi.h"
+#include "libfsverity.h"
 #include "hash_algs.h"
 
-/*
- * Merkle tree properties.  The file measurement is the hash of this structure
- * excluding the signature and with the sig_size field set to 0.
- */
-struct fsverity_descriptor {
-	__u8 version;		/* must be 1 */
-	__u8 hash_algorithm;	/* Merkle tree hash algorithm */
-	__u8 log_blocksize;	/* log2 of size of data and tree blocks */
-	__u8 salt_size;		/* size of salt in bytes; 0 if none */
-	__le32 sig_size;	/* size of signature in bytes; 0 if none */
-	__le64 data_size;	/* size of file the Merkle tree is built over */
-	__u8 root_hash[64];	/* Merkle tree root hash */
-	__u8 salt[32];		/* salt prepended to each hashed block */
-	__u8 __reserved[144];	/* must be 0's */
-	__u8 signature[];	/* optional PKCS#7 signature */
-};
-
 /*
  * Format in which verity file measurements are signed.  This is the same as
  * 'struct fsverity_digest', except here some magic bytes are prepended to
diff --git a/libfsverity.h b/libfsverity.h
index ceebae1..396a6ee 100644
--- a/libfsverity.h
+++ b/libfsverity.h
@@ -13,13 +13,14 @@
 
 #include <stddef.h>
 #include <stdint.h>
+#include <linux/types.h>
 
 #define FS_VERITY_HASH_ALG_SHA256       1
 #define FS_VERITY_HASH_ALG_SHA512       2
 
 struct libfsverity_merkle_tree_params {
 	uint16_t version;
-	uint16_t hash_algorithm;
+	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
 	uint32_t block_size;
 	uint32_t salt_size;
 	const uint8_t *salt;
@@ -27,6 +28,7 @@ struct libfsverity_merkle_tree_params {
 };
 
 struct libfsverity_digest {
+	char magic[8];			/* must be "FSVerity" */
 	uint16_t digest_algorithm;
 	uint16_t digest_size;
 	uint8_t digest[];
@@ -38,4 +40,26 @@ struct libfsverity_signature_params {
 	uint64_t reserved[11];
 };
 
+/*
+ * Merkle tree properties.  The file measurement is the hash of this structure
+ * excluding the signature and with the sig_size field set to 0.
+ */
+struct fsverity_descriptor {
+	uint8_t version;	/* must be 1 */
+	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
+	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
+	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
+	__le32 sig_size;	/* size of signature in bytes; 0 if none */
+	__le64 data_size;	/* size of file the Merkle tree is built over */
+	uint8_t root_hash[64];	/* Merkle tree root hash */
+	uint8_t salt[32];	/* salt prepended to each hashed block */
+	uint8_t __reserved[144];/* must be 0's */
+	uint8_t signature[];	/* optional PKCS#7 signature */
+};
+
+int
+libfsverity_compute_digest(int fd,
+			   const struct libfsverity_merkle_tree_params *params,
+			   struct libfsverity_digest **digest_ret);
+
 #endif
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 4/9] Move hash algorithm code to shared library
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (2 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  5:38   ` Eric Biggers
  2020-03-12 21:47 ` [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it Jes Sorensen
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

Reimplement show_all_hash_algs() to not rely on direct access to the list,
and add the algorithm number to the struct, so the user can find it easily.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 Makefile      |  6 +++---
 cmd_enable.c  | 11 ++++++++---
 cmd_measure.c |  4 ++--
 cmd_sign.c    | 18 ++++++++++++------
 fsverity.c    | 15 ++++++++++++++-
 hash_algs.c   | 26 +++++++-------------------
 hash_algs.h   | 27 ---------------------------
 libfsverity.h | 22 ++++++++++++++++++++++
 util.h        |  2 ++
 9 files changed, 70 insertions(+), 61 deletions(-)

diff --git a/Makefile b/Makefile
index bb85896..966afa0 100644
--- a/Makefile
+++ b/Makefile
@@ -6,9 +6,9 @@ LDLIBS := -lcrypto
 DESTDIR := /usr/local
 LIBDIR := /usr/lib64
 SRC := $(wildcard *.c)
-OBJ := fsverity.o hash_algs.o cmd_enable.o cmd_measure.o cmd_sign.o util.o
-SSRC := libverity.c
-SOBJ := libverity.so
+OBJ := fsverity.o cmd_enable.o cmd_measure.o cmd_sign.o util.o
+SSRC := libverity.c hash_algs.c
+SOBJ := libverity.so hash_algs.so
 HDRS := $(wildcard *.h)
 
 all:$(EXE)
diff --git a/cmd_enable.c b/cmd_enable.c
index 1646299..1bed3ef 100644
--- a/cmd_enable.c
+++ b/cmd_enable.c
@@ -16,7 +16,7 @@
 
 #include "commands.h"
 #include "fsverity_uapi.h"
-#include "hash_algs.h"
+#include "libfsverity.h"
 
 static bool parse_hash_alg_option(const char *arg, u32 *alg_ptr)
 {
@@ -36,11 +36,16 @@ static bool parse_hash_alg_option(const char *arg, u32 *alg_ptr)
 	}
 
 	/* Specified by name? */
-	alg = find_hash_alg_by_name(arg);
+	alg = libfsverity_find_hash_alg_by_name(arg);
 	if (alg != NULL) {
-		*alg_ptr = alg - fsverity_hash_algs;
+		*alg_ptr = alg->hash_num;
 		return true;
 	}
+	error_msg("unknown hash algorithm: '%s'", arg);
+	fputs("Available hash algorithms: ", stderr);
+	show_all_hash_algs(stderr);
+	putc('\n', stderr);
+
 	return false;
 }
 
diff --git a/cmd_measure.c b/cmd_measure.c
index 574e3ca..4c0777f 100644
--- a/cmd_measure.c
+++ b/cmd_measure.c
@@ -13,7 +13,7 @@
 
 #include "commands.h"
 #include "fsverity_uapi.h"
-#include "hash_algs.h"
+#include "libfsverity.h"
 
 /* Display the measurement of the given verity file(s). */
 int fsverity_cmd_measure(const struct fsverity_command *cmd,
@@ -48,7 +48,7 @@ int fsverity_cmd_measure(const struct fsverity_command *cmd,
 
 		ASSERT(d->digest_size <= FS_VERITY_MAX_DIGEST_SIZE);
 		bin2hex(d->digest, d->digest_size, digest_hex);
-		hash_alg = find_hash_alg_by_num(d->digest_algorithm);
+		hash_alg = libfsverity_find_hash_alg_by_num(d->digest_algorithm);
 		if (hash_alg) {
 			hash_alg_name = hash_alg->name;
 		} else {
diff --git a/cmd_sign.c b/cmd_sign.c
index 1792084..5ad4eda 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -466,7 +466,7 @@ static bool compute_file_measurement(int fd,
 				     u32 block_size, const u8 *salt,
 				     u32 salt_size, u8 *measurement)
 {
-	struct hash_ctx *hash = hash_create(hash_alg);
+	struct hash_ctx *hash = hash_alg->create_ctx(hash_alg);
 	u64 file_size;
 	struct fsverity_descriptor desc;
 	struct stat stbuf;
@@ -480,7 +480,7 @@ static bool compute_file_measurement(int fd,
 
 	memset(&desc, 0, sizeof(desc));
 	desc.version = 1;
-	desc.hash_algorithm = hash_alg - fsverity_hash_algs;
+	desc.hash_algorithm = hash_alg->hash_num;
 
 	ASSERT(is_power_of_2(block_size));
 	desc.log_blocksize = ilog2(block_size);
@@ -552,9 +552,15 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 				error_msg("--hash-alg can only be specified once");
 				goto out_usage;
 			}
-			hash_alg = find_hash_alg_by_name(optarg);
-			if (hash_alg == NULL)
+			hash_alg = libfsverity_find_hash_alg_by_name(optarg);
+			if (hash_alg == NULL) {
+				error_msg("unknown hash algorithm: '%s'",
+					  optarg);
+				fputs("Available hash algorithms: ", stderr);
+				show_all_hash_algs(stderr);
+				putc('\n', stderr);
 				goto out_usage;
+			}
 			break;
 		case OPT_BLOCK_SIZE:
 			if (!parse_block_size_option(optarg, &block_size))
@@ -590,7 +596,7 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 		goto out_usage;
 
 	if (hash_alg == NULL)
-		hash_alg = &fsverity_hash_algs[FS_VERITY_HASH_ALG_DEFAULT];
+		hash_alg = libfsverity_find_hash_alg_by_num(FS_VERITY_HASH_ALG_DEFAULT);
 
 	if (block_size == 0)
 		block_size = get_default_block_size();
@@ -604,7 +610,7 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 
 	digest = xzalloc(sizeof(*digest) + hash_alg->digest_size);
 	memcpy(digest->magic, "FSVerity", 8);
-	digest->digest_algorithm = cpu_to_le16(hash_alg - fsverity_hash_algs);
+	digest->digest_algorithm = cpu_to_le16(hash_alg->hash_num);
 	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
 
 	if (!open_file(&file, argv[0], O_RDONLY, 0))
diff --git a/fsverity.c b/fsverity.c
index c8fa1b5..f9df72e 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -13,7 +13,6 @@
 #include <unistd.h>
 
 #include "commands.h"
-#include "hash_algs.h"
 #include "libfsverity.h"
 
 static const struct fsverity_command {
@@ -48,6 +47,20 @@ static const struct fsverity_command {
 	}
 };
 
+void show_all_hash_algs(FILE *fp)
+{
+	int i = 1;
+	const char *sep = "";
+	const struct fsverity_hash_alg *alg;
+
+	while ((alg = libfsverity_find_hash_alg_by_num(i++))) {
+		if (alg && alg->name) {
+			fprintf(fp, "%s%s", sep, alg->name);
+			sep = ", ";
+		}
+	}
+}
+
 static void usage_all(FILE *fp)
 {
 	int i;
diff --git a/hash_algs.c b/hash_algs.c
index 7251bf2..d9f70b4 100644
--- a/hash_algs.c
+++ b/hash_algs.c
@@ -12,6 +12,7 @@
 #include <string.h>
 
 #include "fsverity_uapi.h"
+#include "libfsverity.h"
 #include "hash_algs.h"
 
 /* ========== libcrypto (OpenSSL) wrappers ========== */
@@ -106,17 +107,20 @@ const struct fsverity_hash_alg fsverity_hash_algs[] = {
 		.name = "sha256",
 		.digest_size = 32,
 		.block_size = 64,
+		.hash_num = FS_VERITY_HASH_ALG_SHA256,
 		.create_ctx = create_sha256_ctx,
 	},
 	[FS_VERITY_HASH_ALG_SHA512] = {
 		.name = "sha512",
 		.digest_size = 64,
 		.block_size = 128,
+		.hash_num = FS_VERITY_HASH_ALG_SHA512,
 		.create_ctx = create_sha512_ctx,
 	},
 };
 
-const struct fsverity_hash_alg *find_hash_alg_by_name(const char *name)
+const struct fsverity_hash_alg *
+libfsverity_find_hash_alg_by_name(const char *name)
 {
 	int i;
 
@@ -125,14 +129,11 @@ const struct fsverity_hash_alg *find_hash_alg_by_name(const char *name)
 		    !strcmp(name, fsverity_hash_algs[i].name))
 			return &fsverity_hash_algs[i];
 	}
-	error_msg("unknown hash algorithm: '%s'", name);
-	fputs("Available hash algorithms: ", stderr);
-	show_all_hash_algs(stderr);
-	putc('\n', stderr);
 	return NULL;
 }
 
-const struct fsverity_hash_alg *find_hash_alg_by_num(unsigned int num)
+const struct fsverity_hash_alg *
+libfsverity_find_hash_alg_by_num(unsigned int num)
 {
 	if (num < ARRAY_SIZE(fsverity_hash_algs) &&
 	    fsverity_hash_algs[num].name)
@@ -141,19 +142,6 @@ const struct fsverity_hash_alg *find_hash_alg_by_num(unsigned int num)
 	return NULL;
 }
 
-void show_all_hash_algs(FILE *fp)
-{
-	int i;
-	const char *sep = "";
-
-	for (i = 0; i < ARRAY_SIZE(fsverity_hash_algs); i++) {
-		if (fsverity_hash_algs[i].name) {
-			fprintf(fp, "%s%s", sep, fsverity_hash_algs[i].name);
-			sep = ", ";
-		}
-	}
-}
-
 /* ->init(), ->update(), and ->final() all in one step */
 void hash_full(struct hash_ctx *ctx, const void *data, size_t size, u8 *digest)
 {
diff --git a/hash_algs.h b/hash_algs.h
index 3e90f49..2c7269a 100644
--- a/hash_algs.h
+++ b/hash_algs.h
@@ -6,15 +6,6 @@
 
 #include "util.h"
 
-struct fsverity_hash_alg {
-	const char *name;
-	unsigned int digest_size;
-	unsigned int block_size;
-	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
-};
-
-extern const struct fsverity_hash_alg fsverity_hash_algs[];
-
 struct hash_ctx {
 	const struct fsverity_hash_alg *alg;
 	void (*init)(struct hash_ctx *ctx);
@@ -23,24 +14,6 @@ struct hash_ctx {
 	void (*free)(struct hash_ctx *ctx);
 };
 
-const struct fsverity_hash_alg *find_hash_alg_by_name(const char *name);
-const struct fsverity_hash_alg *find_hash_alg_by_num(unsigned int num);
-void show_all_hash_algs(FILE *fp);
-
-/* The hash algorithm that fsverity-utils 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
-
-static inline struct hash_ctx *hash_create(const struct fsverity_hash_alg *alg)
-{
-	return alg->create_ctx(alg);
-}
-
 static inline void hash_init(struct hash_ctx *ctx)
 {
 	ctx->init(ctx);
diff --git a/libfsverity.h b/libfsverity.h
index 396a6ee..318dcd7 100644
--- a/libfsverity.h
+++ b/libfsverity.h
@@ -18,6 +18,9 @@
 #define FS_VERITY_HASH_ALG_SHA256       1
 #define FS_VERITY_HASH_ALG_SHA512       2
 
+/* The hash algorithm that fsverity-utils assumes when none is specified */
+#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
+
 struct libfsverity_merkle_tree_params {
 	uint16_t version;
 	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
@@ -27,6 +30,12 @@ struct libfsverity_merkle_tree_params {
 	uint64_t reserved[11];
 };
 
+/*
+ * 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 libfsverity_digest {
 	char magic[8];			/* must be "FSVerity" */
 	uint16_t digest_algorithm;
@@ -57,9 +66,22 @@ struct fsverity_descriptor {
 	uint8_t signature[];	/* optional PKCS#7 signature */
 };
 
+struct fsverity_hash_alg {
+	const char *name;
+	unsigned int digest_size;
+	unsigned int block_size;
+	uint16_t hash_num;
+	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
+};
+
 int
 libfsverity_compute_digest(int fd,
 			   const struct libfsverity_merkle_tree_params *params,
 			   struct libfsverity_digest **digest_ret);
 
+const struct fsverity_hash_alg *
+libfsverity_find_hash_alg_by_name(const char *name);
+const struct fsverity_hash_alg *
+libfsverity_find_hash_alg_by_num(unsigned int num);
+
 #endif
diff --git a/util.h b/util.h
index dfa10f2..dd9b803 100644
--- a/util.h
+++ b/util.h
@@ -122,4 +122,6 @@ bool filedes_close(struct filedes *file);
 bool hex2bin(const char *hex, u8 *bin, size_t bin_len);
 void bin2hex(const u8 *bin, size_t bin_len, char *hex);
 
+void show_all_hash_algs();
+
 #endif /* UTIL_H */
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (3 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 4/9] Move hash algorithm code to shared library Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  5:40   ` Eric Biggers
  2020-03-12 21:47 ` [PATCH 6/9] Introduce libfsverity_sign_digest() Jes Sorensen
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

This reorganizes the digest signing code and moves it to the shared library.
In addition libfsverity_private.h is created for library internal data
structures, in particular struct fsverity_decriptor.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c            | 194 ++-------------------------------------
 libfsverity.h         |  17 ----
 libfsverity_private.h |  33 +++++++
 libverity.c           | 207 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 250 insertions(+), 201 deletions(-)
 create mode 100644 libfsverity_private.h

diff --git a/cmd_sign.c b/cmd_sign.c
index 5ad4eda..6a5d185 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -16,12 +16,9 @@
 #include <openssl/pkcs7.h>
 #include <stdlib.h>
 #include <string.h>
-#include <sys/stat.h>
-#include <unistd.h>
 
 #include "commands.h"
 #include "libfsverity.h"
-#include "hash_algs.h"
 
 /*
  * Format in which verity file measurements are signed.  This is the same as
@@ -337,179 +334,6 @@ static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
 	return ok;
 }
 
-#define FS_VERITY_MAX_LEVELS	64
-
-struct block_buffer {
-	u32 filled;
-	u8 *data;
-};
-
-/*
- * Hash a block, writing the result to the next level's pending block buffer.
- * Returns true if the next level's block became full, else false.
- */
-static bool hash_one_block(struct hash_ctx *hash, struct block_buffer *cur,
-			   u32 block_size, const u8 *salt, u32 salt_size)
-{
-	struct block_buffer *next = cur + 1;
-
-	/* Zero-pad the block if it's shorter than block_size. */
-	memset(&cur->data[cur->filled], 0, block_size - cur->filled);
-
-	hash_init(hash);
-	hash_update(hash, salt, salt_size);
-	hash_update(hash, cur->data, block_size);
-	hash_final(hash, &next->data[next->filled]);
-
-	next->filled += hash->alg->digest_size;
-	cur->filled = 0;
-
-	return next->filled + hash->alg->digest_size > block_size;
-}
-
-static int full_read_fd(int fd, void *buf, size_t count)
-{
-	while (count) {
-		int n = read(fd, buf, min(count, INT_MAX));
-
-		if (n < 0) {
-			error_msg_errno("reading from file");
-			return n;
-		}
-		if (n == 0) {
-			error_msg("unexpected end-of-file");
-			return -ENODATA;
-		}
-		buf += n;
-		count -= n;
-	}
-	return 0;
-}
-
-/*
- * Compute the file's Merkle tree root hash using the given hash algorithm,
- * block size, and salt.
- */
-static bool compute_root_hash(int fd, u64 file_size,
-			      struct hash_ctx *hash, u32 block_size,
-			      const u8 *salt, u32 salt_size, u8 *root_hash)
-{
-	const u32 hashes_per_block = block_size / hash->alg->digest_size;
-	const u32 padded_salt_size = roundup(salt_size, hash->alg->block_size);
-	u8 *padded_salt = xzalloc(padded_salt_size);
-	u64 blocks;
-	int num_levels = 0;
-	int level;
-	struct block_buffer _buffers[1 + FS_VERITY_MAX_LEVELS + 1] = {};
-	struct block_buffer *buffers = &_buffers[1];
-	u64 offset;
-	bool ok = false;
-
-	if (salt_size != 0)
-		memcpy(padded_salt, salt, salt_size);
-
-	/* Compute number of levels */
-	for (blocks = DIV_ROUND_UP(file_size, block_size); blocks > 1;
-	     blocks = DIV_ROUND_UP(blocks, hashes_per_block)) {
-		ASSERT(num_levels < FS_VERITY_MAX_LEVELS);
-		num_levels++;
-	}
-
-	/*
-	 * Allocate the block buffers.  Buffer "-1" is for data blocks.
-	 * Buffers 0 <= level < num_levels are for the actual tree levels.
-	 * Buffer 'num_levels' is for the root hash.
-	 */
-	for (level = -1; level < num_levels; level++)
-		buffers[level].data = xmalloc(block_size);
-	buffers[num_levels].data = root_hash;
-
-	/* Hash each data block, also hashing the tree blocks as they fill up */
-	for (offset = 0; offset < file_size; offset += block_size) {
-		buffers[-1].filled = min(block_size, file_size - offset);
-
-		if (full_read_fd(fd, buffers[-1].data, buffers[-1].filled))
-			goto out;
-
-		level = -1;
-		while (hash_one_block(hash, &buffers[level], block_size,
-				      padded_salt, padded_salt_size)) {
-			level++;
-			ASSERT(level < num_levels);
-		}
-	}
-	/* Finish all nonempty pending tree blocks */
-	for (level = 0; level < num_levels; level++) {
-		if (buffers[level].filled != 0)
-			hash_one_block(hash, &buffers[level], block_size,
-				       padded_salt, padded_salt_size);
-	}
-
-	/* Root hash was filled by the last call to hash_one_block() */
-	ASSERT(buffers[num_levels].filled == hash->alg->digest_size);
-	ok = true;
-out:
-	for (level = -1; level < num_levels; level++)
-		free(buffers[level].data);
-	free(padded_salt);
-	return ok;
-}
-
-/*
- * Compute the fs-verity measurement of the given file.
- *
- * The fs-verity measurement is the hash of the fsverity_descriptor, which
- * contains the Merkle tree properties including the root hash.
- */
-static bool compute_file_measurement(int fd,
-				     const struct fsverity_hash_alg *hash_alg,
-				     u32 block_size, const u8 *salt,
-				     u32 salt_size, u8 *measurement)
-{
-	struct hash_ctx *hash = hash_alg->create_ctx(hash_alg);
-	u64 file_size;
-	struct fsverity_descriptor desc;
-	struct stat stbuf;
-	bool ok = false;
-
-	if (fstat(fd, &stbuf) != 0) {
-		error_msg_errno("can't stat input file");
-		goto out;
-	}
-	file_size = stbuf.st_size;
-
-	memset(&desc, 0, sizeof(desc));
-	desc.version = 1;
-	desc.hash_algorithm = hash_alg->hash_num;
-
-	ASSERT(is_power_of_2(block_size));
-	desc.log_blocksize = ilog2(block_size);
-
-	if (salt_size != 0) {
-		if (salt_size > sizeof(desc.salt)) {
-			error_msg("Salt too long (got %u bytes; max is %zu bytes)",
-				  salt_size, sizeof(desc.salt));
-			goto out;
-		}
-		memcpy(desc.salt, salt, salt_size);
-		desc.salt_size = salt_size;
-	}
-
-	desc.data_size = cpu_to_le64(file_size);
-
-	/* Root hash of empty file is all 0's */
-	if (file_size != 0 &&
-	    !compute_root_hash(fd, file_size, hash, block_size, salt,
-			       salt_size, desc.root_hash))
-		goto out;
-
-	hash_full(hash, &desc, sizeof(desc), measurement);
-	ok = true;
-out:
-	hash_free(hash);
-	return ok;
-}
-
 enum {
 	OPT_HASH_ALG,
 	OPT_BLOCK_SIZE,
@@ -538,7 +362,8 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	u32 salt_size = 0;
 	const char *keyfile = NULL;
 	const char *certfile = NULL;
-	struct fsverity_signed_digest *digest = NULL;
+	struct libfsverity_digest *digest = NULL;
+	struct libfsverity_merkle_tree_params params;
 	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
 	u8 *sig = NULL;
 	u32 sig_size;
@@ -608,16 +433,17 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	if (certfile == NULL)
 		certfile = keyfile;
 
-	digest = xzalloc(sizeof(*digest) + hash_alg->digest_size);
-	memcpy(digest->magic, "FSVerity", 8);
-	digest->digest_algorithm = cpu_to_le16(hash_alg->hash_num);
-	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
-
 	if (!open_file(&file, argv[0], O_RDONLY, 0))
 		goto out_err;
 
-	if (!compute_file_measurement(file.fd, hash_alg, block_size,
-				      salt, salt_size, digest->digest))
+	memset(&params, 0, sizeof(struct libfsverity_merkle_tree_params));
+	params.version = 1;
+	params.hash_algorithm = hash_alg->hash_num;
+	params.block_size = block_size;
+	params.salt_size = salt_size;
+	params.salt = salt;
+
+	if (libfsverity_compute_digest(file.fd, &params, &digest))
 		goto out_err;
 
 	filedes_close(&file);
diff --git a/libfsverity.h b/libfsverity.h
index 318dcd7..cb5f5b6 100644
--- a/libfsverity.h
+++ b/libfsverity.h
@@ -49,23 +49,6 @@ struct libfsverity_signature_params {
 	uint64_t reserved[11];
 };
 
-/*
- * Merkle tree properties.  The file measurement is the hash of this structure
- * excluding the signature and with the sig_size field set to 0.
- */
-struct fsverity_descriptor {
-	uint8_t version;	/* must be 1 */
-	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
-	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
-	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
-	__le32 sig_size;	/* size of signature in bytes; 0 if none */
-	__le64 data_size;	/* size of file the Merkle tree is built over */
-	uint8_t root_hash[64];	/* Merkle tree root hash */
-	uint8_t salt[32];	/* salt prepended to each hashed block */
-	uint8_t __reserved[144];/* must be 0's */
-	uint8_t signature[];	/* optional PKCS#7 signature */
-};
-
 struct fsverity_hash_alg {
 	const char *name;
 	unsigned int digest_size;
diff --git a/libfsverity_private.h b/libfsverity_private.h
new file mode 100644
index 0000000..5f3e1b4
--- /dev/null
+++ b/libfsverity_private.h
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * libfsverity private interfaces
+ *
+ * Copyright (C) 2018 Google LLC
+ * Copyright (C) 2020 Facebook
+ *
+ * Written by Eric Biggers and modified by Jes Sorensen.
+ */
+#ifndef _LIBFSVERITY_PRIVATE_H
+#define _LIBFSVERITY_PRIVATE_H
+
+#include <stdint.h>
+#include <linux/types.h>
+
+/*
+ * Merkle tree properties.  The file measurement is the hash of this structure
+ * excluding the signature and with the sig_size field set to 0.
+ */
+struct fsverity_descriptor {
+	uint8_t version;	/* must be 1 */
+	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
+	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
+	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
+	__le32 sig_size;	/* size of signature in bytes; 0 if none */
+	__le64 data_size;	/* size of file the Merkle tree is built over */
+	uint8_t root_hash[64];	/* Merkle tree root hash */
+	uint8_t salt[32];	/* salt prepended to each hashed block */
+	uint8_t __reserved[144];/* must be 0's */
+	uint8_t signature[];	/* optional PKCS#7 signature */
+};
+
+#endif
diff --git a/libverity.c b/libverity.c
index 6821aa2..19272f7 100644
--- a/libverity.c
+++ b/libverity.c
@@ -8,3 +8,210 @@
  * Written by Eric Biggers and Jes Sorensen.
  */
 
+#include <openssl/bio.h>
+#include <openssl/err.h>
+#include <openssl/pem.h>
+#include <openssl/pkcs7.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "libfsverity.h"
+#include "libfsverity_private.h"
+#include "hash_algs.h"
+
+#define FS_VERITY_MAX_LEVELS	64
+
+struct block_buffer {
+	u32 filled;
+	u8 *data;
+};
+
+/*
+ * Hash a block, writing the result to the next level's pending block buffer.
+ * Returns true if the next level's block became full, else false.
+ */
+static bool hash_one_block(struct hash_ctx *hash, struct block_buffer *cur,
+			   u32 block_size, const u8 *salt, u32 salt_size)
+{
+	struct block_buffer *next = cur + 1;
+
+	/* Zero-pad the block if it's shorter than block_size. */
+	memset(&cur->data[cur->filled], 0, block_size - cur->filled);
+
+	hash_init(hash);
+	hash_update(hash, salt, salt_size);
+	hash_update(hash, cur->data, block_size);
+	hash_final(hash, &next->data[next->filled]);
+
+	next->filled += hash->alg->digest_size;
+	cur->filled = 0;
+
+	return next->filled + hash->alg->digest_size > block_size;
+}
+
+static int full_read_fd(int fd, void *buf, size_t count)
+{
+	while (count) {
+		int n = read(fd, buf, min(count, INT_MAX));
+
+		if (n < 0) {
+			error_msg_errno("reading from file");
+			return n;
+		}
+		if (n == 0) {
+			error_msg("unexpected end-of-file");
+			return -ENODATA;
+		}
+		buf += n;
+		count -= n;
+	}
+	return 0;
+}
+
+/*
+ * Compute the file's Merkle tree root hash using the given hash algorithm,
+ * block size, and salt.
+ */
+static bool compute_root_hash(int fd, u64 file_size,
+			      struct hash_ctx *hash, u32 block_size,
+			      const u8 *salt, u32 salt_size, u8 *root_hash)
+{
+	const u32 hashes_per_block = block_size / hash->alg->digest_size;
+	const u32 padded_salt_size = roundup(salt_size, hash->alg->block_size);
+	u8 *padded_salt = xzalloc(padded_salt_size);
+	u64 blocks;
+	int num_levels = 0;
+	int level;
+	struct block_buffer _buffers[1 + FS_VERITY_MAX_LEVELS + 1] = {};
+	struct block_buffer *buffers = &_buffers[1];
+	u64 offset;
+	bool ok = false;
+
+	if (salt_size != 0)
+		memcpy(padded_salt, salt, salt_size);
+
+	/* Compute number of levels */
+	for (blocks = DIV_ROUND_UP(file_size, block_size); blocks > 1;
+	     blocks = DIV_ROUND_UP(blocks, hashes_per_block)) {
+		ASSERT(num_levels < FS_VERITY_MAX_LEVELS);
+		num_levels++;
+	}
+
+	/*
+	 * Allocate the block buffers.  Buffer "-1" is for data blocks.
+	 * Buffers 0 <= level < num_levels are for the actual tree levels.
+	 * Buffer 'num_levels' is for the root hash.
+	 */
+	for (level = -1; level < num_levels; level++)
+		buffers[level].data = xmalloc(block_size);
+	buffers[num_levels].data = root_hash;
+
+	/* Hash each data block, also hashing the tree blocks as they fill up */
+	for (offset = 0; offset < file_size; offset += block_size) {
+		buffers[-1].filled = min(block_size, file_size - offset);
+
+		if (full_read_fd(fd, buffers[-1].data, buffers[-1].filled))
+			goto out;
+
+		level = -1;
+		while (hash_one_block(hash, &buffers[level], block_size,
+				      padded_salt, padded_salt_size)) {
+			level++;
+			ASSERT(level < num_levels);
+		}
+	}
+	/* Finish all nonempty pending tree blocks */
+	for (level = 0; level < num_levels; level++) {
+		if (buffers[level].filled != 0)
+			hash_one_block(hash, &buffers[level], block_size,
+				       padded_salt, padded_salt_size);
+	}
+
+	/* Root hash was filled by the last call to hash_one_block() */
+	ASSERT(buffers[num_levels].filled == hash->alg->digest_size);
+	ok = true;
+out:
+	for (level = -1; level < num_levels; level++)
+		free(buffers[level].data);
+	free(padded_salt);
+	return ok;
+}
+
+/*
+ * Compute the fs-verity measurement of the given file.
+ *
+ * The fs-verity measurement is the hash of the fsverity_descriptor, which
+ * contains the Merkle tree properties including the root hash.
+ */
+int
+libfsverity_compute_digest(int fd,
+			   const struct libfsverity_merkle_tree_params *params,
+			   struct libfsverity_digest **digest_ret)
+{
+	const struct fsverity_hash_alg *hash_alg;
+	struct libfsverity_digest *digest;
+	struct hash_ctx *hash;
+	struct fsverity_descriptor desc;
+	struct stat stbuf;
+	u64 file_size;
+	int retval = -EINVAL;
+
+	hash_alg = libfsverity_find_hash_alg_by_num(params->hash_algorithm);
+	hash = hash_alg->create_ctx(hash_alg);
+
+	digest = malloc(sizeof(struct libfsverity_digest) +
+			hash_alg->digest_size);
+	if (!digest_ret)
+		return -ENOMEM;
+	memcpy(digest->magic, "FSVerity", 8);
+	digest->digest_algorithm = cpu_to_le16(hash_alg->hash_num);
+	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
+	memset(digest->digest, 0, hash_alg->digest_size);
+
+	if (fstat(fd, &stbuf) != 0) {
+		error_msg_errno("can't stat input file");
+		retval = -EBADF;
+		goto error_out;
+	}
+	file_size = stbuf.st_size;
+
+	memset(&desc, 0, sizeof(desc));
+	desc.version = 1;
+	desc.hash_algorithm = params->hash_algorithm;
+
+	ASSERT(is_power_of_2(params->block_size));
+	desc.log_blocksize = ilog2(params->block_size);
+
+	if (params->salt_size != 0) {
+		if (params->salt_size > sizeof(desc.salt)) {
+			error_msg("Salt too long (got %u bytes; max is %zu bytes)",
+				  params->salt_size, sizeof(desc.salt));
+			retval = EINVAL;
+			goto error_out;
+		}
+		memcpy(desc.salt, params->salt, params->salt_size);
+		desc.salt_size = params->salt_size;
+	}
+
+	desc.data_size = cpu_to_le64(file_size);
+
+	/* Root hash of empty file is all 0's */
+	if (file_size != 0 &&
+	    !compute_root_hash(fd, file_size, hash, params->block_size,
+			       params->salt, params->salt_size,
+			       desc.root_hash)) {
+		retval = -EAGAIN;
+		goto error_out;
+	}
+
+	hash_full(hash, &desc, sizeof(desc), digest->digest);
+	hash_free(hash);
+	*digest_ret = digest;
+
+	return 0;
+
+ error_out:
+	free(digest);
+	return retval;
+}
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 6/9] Introduce libfsverity_sign_digest()
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (4 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-12 21:47 ` [PATCH 7/9] Validate input arguments to libfsverity_compute_digest() Jes Sorensen
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

This moves the signing code into libfsverity and switches cmd_sign to use it.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c    | 317 ++------------------------------------------------
 libfsverity.h |  12 ++
 libverity.c   | 309 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 329 insertions(+), 309 deletions(-)

diff --git a/cmd_sign.c b/cmd_sign.c
index 6a5d185..e48e0aa 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -10,318 +10,12 @@
 #include <fcntl.h>
 #include <getopt.h>
 #include <limits.h>
-#include <openssl/bio.h>
-#include <openssl/err.h>
-#include <openssl/pem.h>
-#include <openssl/pkcs7.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "commands.h"
 #include "libfsverity.h"
 
-/*
- * Format in which verity file measurements are signed.  This is the same as
- * 'struct fsverity_digest', except here some magic bytes are prepended to
- * provide some context about what is being signed in case the same key is used
- * for non-fsverity purposes, and here the fields have fixed endianness.
- */
-struct fsverity_signed_digest {
-	char magic[8];			/* must be "FSVerity" */
-	__le16 digest_algorithm;
-	__le16 digest_size;
-	__u8 digest[];
-};
-
-static void __printf(1, 2) __cold
-error_msg_openssl(const char *format, ...)
-{
-	va_list va;
-
-	va_start(va, format);
-	do_error_msg(format, va, 0);
-	va_end(va);
-
-	if (ERR_peek_error() == 0)
-		return;
-
-	fprintf(stderr, "OpenSSL library errors:\n");
-	ERR_print_errors_fp(stderr);
-}
-
-/* Read a PEM PKCS#8 formatted private key */
-static EVP_PKEY *read_private_key(const char *keyfile)
-{
-	BIO *bio;
-	EVP_PKEY *pkey;
-
-	bio = BIO_new_file(keyfile, "r");
-	if (!bio) {
-		error_msg_openssl("can't open '%s' for reading", keyfile);
-		return NULL;
-	}
-
-	pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL);
-	if (!pkey) {
-		error_msg_openssl("Failed to parse private key file '%s'.\n"
-				  "       Note: it must be in PEM PKCS#8 format.",
-				  keyfile);
-	}
-	BIO_free(bio);
-	return pkey;
-}
-
-/* Read a PEM X.509 formatted certificate */
-static X509 *read_certificate(const char *certfile)
-{
-	BIO *bio;
-	X509 *cert;
-
-	bio = BIO_new_file(certfile, "r");
-	if (!bio) {
-		error_msg_openssl("can't open '%s' for reading", certfile);
-		return NULL;
-	}
-	cert = PEM_read_bio_X509(bio, NULL, NULL, NULL);
-	if (!cert) {
-		error_msg_openssl("Failed to parse X.509 certificate file '%s'.\n"
-				  "       Note: it must be in PEM format.",
-				  certfile);
-	}
-	BIO_free(bio);
-	return cert;
-}
-
-#ifdef OPENSSL_IS_BORINGSSL
-
-static bool sign_pkcs7(const void *data_to_sign, size_t data_size,
-		       EVP_PKEY *pkey, X509 *cert, const EVP_MD *md,
-		       u8 **sig_ret, u32 *sig_size_ret)
-{
-	CBB out, outer_seq, wrapped_seq, seq, digest_algos_set, digest_algo,
-		null, content_info, issuer_and_serial, signer_infos,
-		signer_info, sign_algo, signature;
-	EVP_MD_CTX md_ctx;
-	u8 *name_der = NULL, *sig = NULL, *pkcs7_data = NULL;
-	size_t pkcs7_data_len, sig_len;
-	int name_der_len, sig_nid;
-	bool ok = false;
-
-	EVP_MD_CTX_init(&md_ctx);
-	BIGNUM *serial = ASN1_INTEGER_to_BN(X509_get_serialNumber(cert), NULL);
-
-	if (!CBB_init(&out, 1024)) {
-		error_msg("out of memory");
-		goto out;
-	}
-
-	name_der_len = i2d_X509_NAME(X509_get_subject_name(cert), &name_der);
-	if (name_der_len < 0) {
-		error_msg_openssl("i2d_X509_NAME failed");
-		goto out;
-	}
-
-	if (!EVP_DigestSignInit(&md_ctx, NULL, md, NULL, pkey)) {
-		error_msg_openssl("EVP_DigestSignInit failed");
-		goto out;
-	}
-
-	sig_len = EVP_PKEY_size(pkey);
-	sig = xmalloc(sig_len);
-	if (!EVP_DigestSign(&md_ctx, sig, &sig_len, data_to_sign, data_size)) {
-		error_msg_openssl("EVP_DigestSign failed");
-		goto out;
-	}
-
-	sig_nid = EVP_PKEY_id(pkey);
-	/* To mirror OpenSSL behaviour, always use |NID_rsaEncryption| with RSA
-	 * rather than the combined hash+pkey NID. */
-	if (sig_nid != NID_rsaEncryption) {
-		OBJ_find_sigid_by_algs(&sig_nid, EVP_MD_type(md),
-				       EVP_PKEY_id(pkey));
-	}
-
-	// See https://tools.ietf.org/html/rfc2315#section-7
-	if (!CBB_add_asn1(&out, &outer_seq, CBS_ASN1_SEQUENCE) ||
-	    !OBJ_nid2cbb(&outer_seq, NID_pkcs7_signed) ||
-	    !CBB_add_asn1(&outer_seq, &wrapped_seq, CBS_ASN1_CONTEXT_SPECIFIC |
-			  CBS_ASN1_CONSTRUCTED | 0) ||
-	    // See https://tools.ietf.org/html/rfc2315#section-9.1
-	    !CBB_add_asn1(&wrapped_seq, &seq, CBS_ASN1_SEQUENCE) ||
-	    !CBB_add_asn1_uint64(&seq, 1 /* version */) ||
-	    !CBB_add_asn1(&seq, &digest_algos_set, CBS_ASN1_SET) ||
-	    !CBB_add_asn1(&digest_algos_set, &digest_algo, CBS_ASN1_SEQUENCE) ||
-	    !OBJ_nid2cbb(&digest_algo, EVP_MD_type(md)) ||
-	    !CBB_add_asn1(&digest_algo, &null, CBS_ASN1_NULL) ||
-	    !CBB_add_asn1(&seq, &content_info, CBS_ASN1_SEQUENCE) ||
-	    !OBJ_nid2cbb(&content_info, NID_pkcs7_data) ||
-	    !CBB_add_asn1(&seq, &signer_infos, CBS_ASN1_SET) ||
-	    !CBB_add_asn1(&signer_infos, &signer_info, CBS_ASN1_SEQUENCE) ||
-	    !CBB_add_asn1_uint64(&signer_info, 1 /* version */) ||
-	    !CBB_add_asn1(&signer_info, &issuer_and_serial,
-			  CBS_ASN1_SEQUENCE) ||
-	    !CBB_add_bytes(&issuer_and_serial, name_der, name_der_len) ||
-	    !BN_marshal_asn1(&issuer_and_serial, serial) ||
-	    !CBB_add_asn1(&signer_info, &digest_algo, CBS_ASN1_SEQUENCE) ||
-	    !OBJ_nid2cbb(&digest_algo, EVP_MD_type(md)) ||
-	    !CBB_add_asn1(&digest_algo, &null, CBS_ASN1_NULL) ||
-	    !CBB_add_asn1(&signer_info, &sign_algo, CBS_ASN1_SEQUENCE) ||
-	    !OBJ_nid2cbb(&sign_algo, sig_nid) ||
-	    !CBB_add_asn1(&sign_algo, &null, CBS_ASN1_NULL) ||
-	    !CBB_add_asn1(&signer_info, &signature, CBS_ASN1_OCTETSTRING) ||
-	    !CBB_add_bytes(&signature, sig, sig_len) ||
-	    !CBB_finish(&out, &pkcs7_data, &pkcs7_data_len)) {
-		error_msg_openssl("failed to construct PKCS#7 data");
-		goto out;
-	}
-
-	*sig_ret = xmemdup(pkcs7_data, pkcs7_data_len);
-	*sig_size_ret = pkcs7_data_len;
-	ok = true;
-out:
-	BN_free(serial);
-	EVP_MD_CTX_cleanup(&md_ctx);
-	CBB_cleanup(&out);
-	free(sig);
-	OPENSSL_free(name_der);
-	OPENSSL_free(pkcs7_data);
-	return ok;
-}
-
-#else /* OPENSSL_IS_BORINGSSL */
-
-static BIO *new_mem_buf(const void *buf, size_t size)
-{
-	BIO *bio;
-
-	ASSERT(size <= INT_MAX);
-	/*
-	 * Prior to OpenSSL 1.1.0, BIO_new_mem_buf() took a non-const pointer,
-	 * despite still marking the resulting bio as read-only.  So cast away
-	 * the const to avoid a compiler warning with older OpenSSL versions.
-	 */
-	bio = BIO_new_mem_buf((void *)buf, size);
-	if (!bio)
-		error_msg_openssl("out of memory");
-	return bio;
-}
-
-static bool sign_pkcs7(const void *data_to_sign, size_t data_size,
-		       EVP_PKEY *pkey, X509 *cert, const EVP_MD *md,
-		       u8 **sig_ret, u32 *sig_size_ret)
-{
-	/*
-	 * PKCS#7 signing flags:
-	 *
-	 * - PKCS7_BINARY	signing binary data, so skip MIME translation
-	 *
-	 * - PKCS7_DETACHED	omit the signed data (include signature only)
-	 *
-	 * - PKCS7_NOATTR	omit extra authenticated attributes, such as
-	 *			SMIMECapabilities
-	 *
-	 * - PKCS7_NOCERTS	omit the signer's certificate
-	 *
-	 * - PKCS7_PARTIAL	PKCS7_sign() creates a handle only, then
-	 *			PKCS7_sign_add_signer() can add a signer later.
-	 *			This is necessary to change the message digest
-	 *			algorithm from the default of SHA-1.  Requires
-	 *			OpenSSL 1.0.0 or later.
-	 */
-	int pkcs7_flags = PKCS7_BINARY | PKCS7_DETACHED | PKCS7_NOATTR |
-			  PKCS7_NOCERTS | PKCS7_PARTIAL;
-	u8 *sig;
-	u32 sig_size;
-	BIO *bio = NULL;
-	PKCS7 *p7 = NULL;
-	bool ok = false;
-
-	bio = new_mem_buf(data_to_sign, data_size);
-	if (!bio)
-		goto out;
-
-	p7 = PKCS7_sign(NULL, NULL, NULL, bio, pkcs7_flags);
-	if (!p7) {
-		error_msg_openssl("failed to initialize PKCS#7 signature object");
-		goto out;
-	}
-
-	if (!PKCS7_sign_add_signer(p7, cert, pkey, md, pkcs7_flags)) {
-		error_msg_openssl("failed to add signer to PKCS#7 signature object");
-		goto out;
-	}
-
-	if (PKCS7_final(p7, bio, pkcs7_flags) != 1) {
-		error_msg_openssl("failed to finalize PKCS#7 signature");
-		goto out;
-	}
-
-	BIO_free(bio);
-	bio = BIO_new(BIO_s_mem());
-	if (!bio) {
-		error_msg_openssl("out of memory");
-		goto out;
-	}
-
-	if (i2d_PKCS7_bio(bio, p7) != 1) {
-		error_msg_openssl("failed to DER-encode PKCS#7 signature object");
-		goto out;
-	}
-
-	sig_size = BIO_get_mem_data(bio, &sig);
-	*sig_ret = xmemdup(sig, sig_size);
-	*sig_size_ret = sig_size;
-	ok = true;
-out:
-	PKCS7_free(p7);
-	BIO_free(bio);
-	return ok;
-}
-
-#endif /* !OPENSSL_IS_BORINGSSL */
-
-/*
- * Sign the specified @data_to_sign of length @data_size bytes using the private
- * key in @keyfile, the certificate in @certfile, and the hash algorithm
- * @hash_alg.  Returns the DER-formatted PKCS#7 signature in @sig_ret and
- * @sig_size_ret.
- */
-static bool sign_data(const void *data_to_sign, size_t data_size,
-		      const char *keyfile, const char *certfile,
-		      const struct fsverity_hash_alg *hash_alg,
-		      u8 **sig_ret, u32 *sig_size_ret)
-{
-	EVP_PKEY *pkey = NULL;
-	X509 *cert = NULL;
-	const EVP_MD *md;
-	bool ok = false;
-
-	pkey = read_private_key(keyfile);
-	if (!pkey)
-		goto out;
-
-	cert = read_certificate(certfile);
-	if (!cert)
-		goto out;
-
-	OpenSSL_add_all_digests();
-	md = EVP_get_digestbyname(hash_alg->name);
-	if (!md) {
-		fprintf(stderr,
-			"Warning: '%s' algorithm not found in OpenSSL library.\n"
-			"         Falling back to SHA-256 signature.\n",
-			hash_alg->name);
-		md = EVP_sha256();
-	}
-
-	ok = sign_pkcs7(data_to_sign, data_size, pkey, cert, md,
-			sig_ret, sig_size_ret);
-out:
-	EVP_PKEY_free(pkey);
-	X509_free(cert);
-	return ok;
-}
-
 static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
 {
 	struct filedes file;
@@ -364,9 +58,10 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	const char *certfile = NULL;
 	struct libfsverity_digest *digest = NULL;
 	struct libfsverity_merkle_tree_params params;
+	struct libfsverity_signature_params sig_params;
 	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
 	u8 *sig = NULL;
-	u32 sig_size;
+	size_t sig_size;
 	int status;
 	int c;
 
@@ -448,9 +143,13 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 
 	filedes_close(&file);
 
-	if (!sign_data(digest, sizeof(*digest) + hash_alg->digest_size,
-		       keyfile, certfile, hash_alg, &sig, &sig_size))
+	memset(&sig_params, 0, sizeof(struct libfsverity_signature_params));
+	sig_params.keyfile = keyfile;
+	sig_params.certfile = certfile;
+	if (libfsverity_sign_digest(digest, &sig_params, &sig, &sig_size)) {
+		error_msg("Failed to sign digest");
 		goto out_err;
+	}
 
 	if (!write_signature(argv[1], sig, sig_size))
 		goto out_err;
diff --git a/libfsverity.h b/libfsverity.h
index cb5f5b6..a2abdb3 100644
--- a/libfsverity.h
+++ b/libfsverity.h
@@ -36,6 +36,13 @@ struct libfsverity_merkle_tree_params {
  */
 #define FS_VERITY_MAX_DIGEST_SIZE	64
 
+/*
+ * Format in which verity file measurements are signed.  This is the same as
+ * 'struct fsverity_digest', except here some magic bytes are prepended to
+ * provide some context about what is being signed in case the same key is used
+ * for non-fsverity purposes, and here the fields have fixed endianness.
+ */
+
 struct libfsverity_digest {
 	char magic[8];			/* must be "FSVerity" */
 	uint16_t digest_algorithm;
@@ -62,6 +69,11 @@ 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,
+			uint8_t **sig_ret, size_t *sig_size_ret);
+
 const struct fsverity_hash_alg *
 libfsverity_find_hash_alg_by_name(const char *name);
 const struct fsverity_hash_alg *
diff --git a/libverity.c b/libverity.c
index 19272f7..183259e 100644
--- a/libverity.c
+++ b/libverity.c
@@ -215,3 +215,312 @@ libfsverity_compute_digest(int fd,
 	free(digest);
 	return retval;
 }
+
+static void __printf(1, 2) __cold
+error_msg_openssl(const char *format, ...)
+{
+	va_list va;
+
+	va_start(va, format);
+	do_error_msg(format, va, 0);
+	va_end(va);
+
+	if (ERR_peek_error() == 0)
+		return;
+
+	fprintf(stderr, "OpenSSL library errors:\n");
+	ERR_print_errors_fp(stderr);
+}
+
+/* Read a PEM PKCS#8 formatted private key */
+static EVP_PKEY *read_private_key(const char *keyfile)
+{
+	BIO *bio;
+	EVP_PKEY *pkey;
+
+	bio = BIO_new_file(keyfile, "r");
+	if (!bio) {
+		error_msg_openssl("can't open '%s' for reading", keyfile);
+		return NULL;
+	}
+
+	pkey = PEM_read_bio_PrivateKey(bio, NULL, NULL, NULL);
+	if (!pkey) {
+		error_msg_openssl("Failed to parse private key file '%s'.\n"
+				  "       Note: it must be in PEM PKCS#8 format.",
+				  keyfile);
+	}
+	BIO_free(bio);
+	return pkey;
+}
+
+/* Read a PEM X.509 formatted certificate */
+static X509 *read_certificate(const char *certfile)
+{
+	BIO *bio;
+	X509 *cert;
+
+	bio = BIO_new_file(certfile, "r");
+	if (!bio) {
+		error_msg_openssl("can't open '%s' for reading", certfile);
+		return NULL;
+	}
+	cert = PEM_read_bio_X509(bio, NULL, NULL, NULL);
+	if (!cert) {
+		error_msg_openssl("Failed to parse X.509 certificate file '%s'.\n"
+				  "       Note: it must be in PEM format.",
+				  certfile);
+	}
+	BIO_free(bio);
+	return cert;
+}
+
+#ifdef OPENSSL_IS_BORINGSSL
+
+static bool sign_pkcs7(const void *data_to_sign, size_t data_size,
+		       EVP_PKEY *pkey, X509 *cert, const EVP_MD *md,
+		       u8 **sig_ret, size_t *sig_size_ret)
+{
+	CBB out, outer_seq, wrapped_seq, seq, digest_algos_set, digest_algo,
+		null, content_info, issuer_and_serial, signer_infos,
+		signer_info, sign_algo, signature;
+	EVP_MD_CTX md_ctx;
+	u8 *name_der = NULL, *sig = NULL, *pkcs7_data = NULL;
+	size_t pkcs7_data_len, sig_len;
+	int name_der_len, sig_nid;
+	bool ok = false;
+
+	EVP_MD_CTX_init(&md_ctx);
+	BIGNUM *serial = ASN1_INTEGER_to_BN(X509_get_serialNumber(cert), NULL);
+
+	if (!CBB_init(&out, 1024)) {
+		error_msg("out of memory");
+		goto out;
+	}
+
+	name_der_len = i2d_X509_NAME(X509_get_subject_name(cert), &name_der);
+	if (name_der_len < 0) {
+		error_msg_openssl("i2d_X509_NAME failed");
+		goto out;
+	}
+
+	if (!EVP_DigestSignInit(&md_ctx, NULL, md, NULL, pkey)) {
+		error_msg_openssl("EVP_DigestSignInit failed");
+		goto out;
+	}
+
+	sig_len = EVP_PKEY_size(pkey);
+	sig = xmalloc(sig_len);
+	if (!EVP_DigestSign(&md_ctx, sig, &sig_len, data_to_sign, data_size)) {
+		error_msg_openssl("EVP_DigestSign failed");
+		goto out;
+	}
+
+	sig_nid = EVP_PKEY_id(pkey);
+	/* To mirror OpenSSL behaviour, always use |NID_rsaEncryption| with RSA
+	 * rather than the combined hash+pkey NID. */
+	if (sig_nid != NID_rsaEncryption) {
+		OBJ_find_sigid_by_algs(&sig_nid, EVP_MD_type(md),
+				       EVP_PKEY_id(pkey));
+	}
+
+	// See https://tools.ietf.org/html/rfc2315#section-7
+	if (!CBB_add_asn1(&out, &outer_seq, CBS_ASN1_SEQUENCE) ||
+	    !OBJ_nid2cbb(&outer_seq, NID_pkcs7_signed) ||
+	    !CBB_add_asn1(&outer_seq, &wrapped_seq, CBS_ASN1_CONTEXT_SPECIFIC |
+			  CBS_ASN1_CONSTRUCTED | 0) ||
+	    // See https://tools.ietf.org/html/rfc2315#section-9.1
+	    !CBB_add_asn1(&wrapped_seq, &seq, CBS_ASN1_SEQUENCE) ||
+	    !CBB_add_asn1_uint64(&seq, 1 /* version */) ||
+	    !CBB_add_asn1(&seq, &digest_algos_set, CBS_ASN1_SET) ||
+	    !CBB_add_asn1(&digest_algos_set, &digest_algo, CBS_ASN1_SEQUENCE) ||
+	    !OBJ_nid2cbb(&digest_algo, EVP_MD_type(md)) ||
+	    !CBB_add_asn1(&digest_algo, &null, CBS_ASN1_NULL) ||
+	    !CBB_add_asn1(&seq, &content_info, CBS_ASN1_SEQUENCE) ||
+	    !OBJ_nid2cbb(&content_info, NID_pkcs7_data) ||
+	    !CBB_add_asn1(&seq, &signer_infos, CBS_ASN1_SET) ||
+	    !CBB_add_asn1(&signer_infos, &signer_info, CBS_ASN1_SEQUENCE) ||
+	    !CBB_add_asn1_uint64(&signer_info, 1 /* version */) ||
+	    !CBB_add_asn1(&signer_info, &issuer_and_serial,
+			  CBS_ASN1_SEQUENCE) ||
+	    !CBB_add_bytes(&issuer_and_serial, name_der, name_der_len) ||
+	    !BN_marshal_asn1(&issuer_and_serial, serial) ||
+	    !CBB_add_asn1(&signer_info, &digest_algo, CBS_ASN1_SEQUENCE) ||
+	    !OBJ_nid2cbb(&digest_algo, EVP_MD_type(md)) ||
+	    !CBB_add_asn1(&digest_algo, &null, CBS_ASN1_NULL) ||
+	    !CBB_add_asn1(&signer_info, &sign_algo, CBS_ASN1_SEQUENCE) ||
+	    !OBJ_nid2cbb(&sign_algo, sig_nid) ||
+	    !CBB_add_asn1(&sign_algo, &null, CBS_ASN1_NULL) ||
+	    !CBB_add_asn1(&signer_info, &signature, CBS_ASN1_OCTETSTRING) ||
+	    !CBB_add_bytes(&signature, sig, sig_len) ||
+	    !CBB_finish(&out, &pkcs7_data, &pkcs7_data_len)) {
+		error_msg_openssl("failed to construct PKCS#7 data");
+		goto out;
+	}
+
+	*sig_ret = xmemdup(pkcs7_data, pkcs7_data_len);
+	*sig_size_ret = pkcs7_data_len;
+	ok = true;
+out:
+	BN_free(serial);
+	EVP_MD_CTX_cleanup(&md_ctx);
+	CBB_cleanup(&out);
+	free(sig);
+	OPENSSL_free(name_der);
+	OPENSSL_free(pkcs7_data);
+	return ok;
+}
+
+#else /* OPENSSL_IS_BORINGSSL */
+
+static BIO *new_mem_buf(const void *buf, size_t size)
+{
+	BIO *bio;
+
+	ASSERT(size <= INT_MAX);
+	/*
+	 * Prior to OpenSSL 1.1.0, BIO_new_mem_buf() took a non-const pointer,
+	 * despite still marking the resulting bio as read-only.  So cast away
+	 * the const to avoid a compiler warning with older OpenSSL versions.
+	 */
+	bio = BIO_new_mem_buf((void *)buf, size);
+	if (!bio)
+		error_msg_openssl("out of memory");
+	return bio;
+}
+
+static bool sign_pkcs7(const void *data_to_sign, size_t data_size,
+		       EVP_PKEY *pkey, X509 *cert, const EVP_MD *md,
+		       u8 **sig_ret, size_t *sig_size_ret)
+{
+	/*
+	 * PKCS#7 signing flags:
+	 *
+	 * - PKCS7_BINARY	signing binary data, so skip MIME translation
+	 *
+	 * - PKCS7_DETACHED	omit the signed data (include signature only)
+	 *
+	 * - PKCS7_NOATTR	omit extra authenticated attributes, such as
+	 *			SMIMECapabilities
+	 *
+	 * - PKCS7_NOCERTS	omit the signer's certificate
+	 *
+	 * - PKCS7_PARTIAL	PKCS7_sign() creates a handle only, then
+	 *			PKCS7_sign_add_signer() can add a signer later.
+	 *			This is necessary to change the message digest
+	 *			algorithm from the default of SHA-1.  Requires
+	 *			OpenSSL 1.0.0 or later.
+	 */
+	int pkcs7_flags = PKCS7_BINARY | PKCS7_DETACHED | PKCS7_NOATTR |
+			  PKCS7_NOCERTS | PKCS7_PARTIAL;
+	u8 *sig;
+	u32 sig_size;
+	BIO *bio = NULL;
+	PKCS7 *p7 = NULL;
+	bool ok = false;
+
+	bio = new_mem_buf(data_to_sign, data_size);
+	if (!bio)
+		goto out;
+
+	p7 = PKCS7_sign(NULL, NULL, NULL, bio, pkcs7_flags);
+	if (!p7) {
+		error_msg_openssl("failed to initialize PKCS#7 signature object");
+		goto out;
+	}
+
+	if (!PKCS7_sign_add_signer(p7, cert, pkey, md, pkcs7_flags)) {
+		error_msg_openssl("failed to add signer to PKCS#7 signature object");
+		goto out;
+	}
+
+	if (PKCS7_final(p7, bio, pkcs7_flags) != 1) {
+		error_msg_openssl("failed to finalize PKCS#7 signature");
+		goto out;
+	}
+
+	BIO_free(bio);
+	bio = BIO_new(BIO_s_mem());
+	if (!bio) {
+		error_msg_openssl("out of memory");
+		goto out;
+	}
+
+	if (i2d_PKCS7_bio(bio, p7) != 1) {
+		error_msg_openssl("failed to DER-encode PKCS#7 signature object");
+		goto out;
+	}
+
+	sig_size = BIO_get_mem_data(bio, &sig);
+	*sig_ret = xmemdup(sig, sig_size);
+	*sig_size_ret = sig_size;
+	ok = true;
+out:
+	PKCS7_free(p7);
+	BIO_free(bio);
+	return ok;
+}
+
+#endif /* !OPENSSL_IS_BORINGSSL */
+
+/*
+ * Sign the digest using the private key in @keyfile, the certificate in
+ * @certfile, and the hash algorithm specified in the digest.
+ * Return 0 on success, the DER-formatted PKCS#7 signature in @sig_ret and
+ * it's size in @sig_size_ret.
+ */
+int
+libfsverity_sign_digest(const struct libfsverity_digest *digest,
+			const struct libfsverity_signature_params *sig_params,
+			uint8_t **sig_ret, size_t *sig_size_ret)
+{
+	const struct fsverity_hash_alg *hash_alg;
+	EVP_PKEY *pkey = NULL;
+	X509 *cert = NULL;
+	const EVP_MD *md;
+	size_t data_size;
+	uint16_t alg_nr;
+	int retval = -EAGAIN;
+
+	data_size = sizeof(struct libfsverity_digest) +
+		le16_to_cpu(digest->digest_size);
+	alg_nr = le16_to_cpu(digest->digest_algorithm);
+	hash_alg = libfsverity_find_hash_alg_by_num(alg_nr);
+
+	if (!hash_alg) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	pkey = read_private_key(sig_params->keyfile);
+	if (!pkey) {
+		retval = -EAGAIN;
+		goto out;
+	}
+
+	cert = read_certificate(sig_params->certfile);
+	if (!cert) {
+		retval = -EAGAIN;
+		goto out;
+	}
+
+	OpenSSL_add_all_digests();
+
+	md = EVP_get_digestbyname(hash_alg->name);
+	if (!md) {
+		fprintf(stderr,
+			"Warning: '%s' algorithm not found in OpenSSL library.\n"
+			"         Falling back to SHA-256 signature.\n",
+			hash_alg->name);
+		md = EVP_sha256();
+	}
+
+	if (sign_pkcs7(digest, data_size, pkey, cert, md,
+		       sig_ret, sig_size_ret))
+		retval = 0;
+
+ out:
+	EVP_PKEY_free(pkey);
+	X509_free(cert);
+	return retval;
+}
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 7/9] Validate input arguments to libfsverity_compute_digest()
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (5 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 6/9] Introduce libfsverity_sign_digest() Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-12 21:47 ` [PATCH 8/9] Validate input parameters for libfsverity_sign_digest() Jes Sorensen
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

If any argument is invalid, return -EINVAL. Similarly
if any of the reserved fields in the params struct
are set, return -EINVAL;

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 libverity.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/libverity.c b/libverity.c
index 183259e..1cef544 100644
--- a/libverity.c
+++ b/libverity.c
@@ -155,9 +155,31 @@ libfsverity_compute_digest(int fd,
 	struct fsverity_descriptor desc;
 	struct stat stbuf;
 	u64 file_size;
-	int retval = -EINVAL;
+	int i, retval = -EINVAL;
+
+	if (!digest_ret)
+		return -EINVAL;
+	if (params->version != 1)
+		return -EINVAL;
+	if (!is_power_of_2(params->block_size))
+		return -EINVAL;
+	if (params->salt_size > sizeof(desc.salt)) {
+		error_msg("Salt too long (got %u bytes; max is %zu bytes)",
+			  params->salt_size, sizeof(desc.salt));
+		return -EINVAL;
+	}
+	if (params->salt_size && !params->salt)
+		return -EINVAL;
+	for (i = 0;
+	     i < sizeof(params->reserved) / sizeof(params->reserved[0]); i++) {
+		if (params->reserved[i])
+			return -EINVAL;
+	}
 
 	hash_alg = libfsverity_find_hash_alg_by_num(params->hash_algorithm);
+	if (!hash_alg)
+		return -EINVAL;
+
 	hash = hash_alg->create_ctx(hash_alg);
 
 	digest = malloc(sizeof(struct libfsverity_digest) +
@@ -180,16 +202,9 @@ libfsverity_compute_digest(int fd,
 	desc.version = 1;
 	desc.hash_algorithm = params->hash_algorithm;
 
-	ASSERT(is_power_of_2(params->block_size));
 	desc.log_blocksize = ilog2(params->block_size);
 
 	if (params->salt_size != 0) {
-		if (params->salt_size > sizeof(desc.salt)) {
-			error_msg("Salt too long (got %u bytes; max is %zu bytes)",
-				  params->salt_size, sizeof(desc.salt));
-			retval = EINVAL;
-			goto error_out;
-		}
 		memcpy(desc.salt, params->salt, params->salt_size);
 		desc.salt_size = params->salt_size;
 	}
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 8/9] Validate input parameters for libfsverity_sign_digest()
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (6 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 7/9] Validate input arguments to libfsverity_compute_digest() Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  5:27   ` Eric Biggers
  2020-03-12 21:47 ` [PATCH 9/9] Document API of libfsverity Jes Sorensen
  2020-03-22  5:05 ` [PATCH v3 0/9] Split fsverity-utils into a shared library Eric Biggers
  9 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

Return -EINVAL on any invalid input argument, as well
as if any of the reserved fields are set in
struct libfsverity_signature_digest

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 libverity.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/libverity.c b/libverity.c
index 1cef544..e16306d 100644
--- a/libverity.c
+++ b/libverity.c
@@ -494,18 +494,36 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
 	X509 *cert = NULL;
 	const EVP_MD *md;
 	size_t data_size;
-	uint16_t alg_nr;
-	int retval = -EAGAIN;
+	uint16_t alg_nr, digest_size;
+	int i, retval = -EAGAIN;
+	const char magic[8] = "FSVerity";
+
+	if (!digest || !sig_params || !sig_ret || !sig_size_ret)
+		return -EINVAL;
+
+	if (strncmp(digest->magic, magic, sizeof(magic)))
+		return -EINVAL;
+
+	if (!sig_params->keyfile || !sig_params->certfile)
+		return -EINVAL;
+
+	for (i = 0; i < sizeof(sig_params->reserved) /
+		     sizeof(sig_params->reserved[0]); i++) {
+		if (sig_params->reserved[i])
+			return -EINVAL;
+	}
+
+	digest_size = le16_to_cpu(digest->digest_size);
+	data_size = sizeof(struct libfsverity_digest) + digest_size;
 
-	data_size = sizeof(struct libfsverity_digest) +
-		le16_to_cpu(digest->digest_size);
 	alg_nr = le16_to_cpu(digest->digest_algorithm);
 	hash_alg = libfsverity_find_hash_alg_by_num(alg_nr);
 
-	if (!hash_alg) {
-		retval = -EINVAL;
-		goto out;
-	}
+	if (!hash_alg)
+		return -EINVAL;
+
+	if (digest_size != hash_alg->digest_size)
+		return -EINVAL;
 
 	pkey = read_private_key(sig_params->keyfile);
 	if (!pkey) {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 9/9] Document API of libfsverity
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (7 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 8/9] Validate input parameters for libfsverity_sign_digest() Jes Sorensen
@ 2020-03-12 21:47 ` Jes Sorensen
  2020-03-22  5:54   ` Eric Biggers
  2020-03-22  5:05 ` [PATCH v3 0/9] Split fsverity-utils into a shared library Eric Biggers
  9 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-03-12 21:47 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: kernel-team, ebiggers, Jes Sorensen

From: Jes Sorensen <jsorensen@fb.com>

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 libfsverity.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/libfsverity.h b/libfsverity.h
index a2abdb3..f6c4b13 100644
--- a/libfsverity.h
+++ b/libfsverity.h
@@ -64,18 +64,63 @@ struct fsverity_hash_alg {
 	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
 };
 
+/*
+ * libfsverity_compute_digest - Compute digest of a file
+ * @fd: open file descriptor of file to compute digest for
+ * @params: struct libfsverity_merkle_tree_params specifying hash algorithm,
+ *	    block size, version, and optional salt parameters.
+ *	    reserved parameters must be zero.
+ * @digest_ret: Pointer to pointer for computed digest
+ *
+ * Returns:
+ * * 0 for success, -EINVAL for invalid input arguments, -ENOMEM if failed
+ *   to allocate memory, -EBADF if fd is invalid, and -EAGAIN if root hash
+ *   fails to compute.
+ * * digest_ret returns a pointer to the digest on success.
+ */
 int
 libfsverity_compute_digest(int fd,
 			   const struct libfsverity_merkle_tree_params *params,
 			   struct libfsverity_digest **digest_ret);
 
+/*
+ * libfsverity_sign_digest - Sign previously computed digest of a file
+ * @digest: pointer to previously computed digest
+ * @sig_params: struct libfsverity_signature_params providing filenames of
+ *          the keyfile and certificate file. Reserved parameters must be zero.
+ * @sig_ret: Pointer to pointer for signed digest
+ * @sig_size_ret: Pointer to size of signed return digest
+ *
+ * Returns:
+ * * 0 for success, -EINVAL for invalid input arguments, -EAGAIN if key or
+ *   certificate files fail to read, or if signing the digest fails.
+ * * sig_ret returns a pointer to the signed digest on success.
+ * * sig_size_ret returns the size of the signed digest on success.
+ */
 int
 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_find_hash_alg_by_name - Find hash algorithm by name
+ * @name: Pointer to name of hash algorithm
+ *
+ * Returns:
+ * struct fsverity_hash_alg success
+ * NULL on error
+ */
 const struct fsverity_hash_alg *
 libfsverity_find_hash_alg_by_name(const char *name);
+
+/*
+ * libfsverity_find_hash_alg_by_num - Find hash algorithm by number
+ * @name: Number of hash algorithm
+ *
+ * Returns:
+ * struct fsverity_hash_alg success
+ * NULL on error
+ */
 const struct fsverity_hash_alg *
 libfsverity_find_hash_alg_by_num(unsigned int num);
 
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h
  2020-03-12 21:47 ` [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h Jes Sorensen
@ 2020-03-22  4:57   ` Eric Biggers
  2020-04-21 16:07     ` Jes Sorensen
  0 siblings, 1 reply; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  4:57 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:52PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  cmd_sign.c    | 19 +------------------
>  libfsverity.h | 26 +++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/cmd_sign.c b/cmd_sign.c
> index dcc44f8..1792084 100644
> --- a/cmd_sign.c
> +++ b/cmd_sign.c
> @@ -20,26 +20,9 @@
>  #include <unistd.h>
>  
>  #include "commands.h"
> -#include "fsverity_uapi.h"
> +#include "libfsverity.h"
>  #include "hash_algs.h"
>  
> -/*
> - * Merkle tree properties.  The file measurement is the hash of this structure
> - * excluding the signature and with the sig_size field set to 0.
> - */
> -struct fsverity_descriptor {
> -	__u8 version;		/* must be 1 */
> -	__u8 hash_algorithm;	/* Merkle tree hash algorithm */
> -	__u8 log_blocksize;	/* log2 of size of data and tree blocks */
> -	__u8 salt_size;		/* size of salt in bytes; 0 if none */
> -	__le32 sig_size;	/* size of signature in bytes; 0 if none */
> -	__le64 data_size;	/* size of file the Merkle tree is built over */
> -	__u8 root_hash[64];	/* Merkle tree root hash */
> -	__u8 salt[32];		/* salt prepended to each hashed block */
> -	__u8 __reserved[144];	/* must be 0's */
> -	__u8 signature[];	/* optional PKCS#7 signature */
> -};
> -
>  /*
>   * Format in which verity file measurements are signed.  This is the same as
>   * 'struct fsverity_digest', except here some magic bytes are prepended to
> diff --git a/libfsverity.h b/libfsverity.h
> index ceebae1..396a6ee 100644
> --- a/libfsverity.h
> +++ b/libfsverity.h
> @@ -13,13 +13,14 @@
>  
>  #include <stddef.h>
>  #include <stdint.h>
> +#include <linux/types.h>
>  
>  #define FS_VERITY_HASH_ALG_SHA256       1
>  #define FS_VERITY_HASH_ALG_SHA512       2
>  
>  struct libfsverity_merkle_tree_params {
>  	uint16_t version;
> -	uint16_t hash_algorithm;
> +	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
>  	uint32_t block_size;
>  	uint32_t salt_size;
>  	const uint8_t *salt;
> @@ -27,6 +28,7 @@ struct libfsverity_merkle_tree_params {
>  };
>  
>  struct libfsverity_digest {
> +	char magic[8];			/* must be "FSVerity" */
>  	uint16_t digest_algorithm;
>  	uint16_t digest_size;
>  	uint8_t digest[];
> @@ -38,4 +40,26 @@ struct libfsverity_signature_params {
>  	uint64_t reserved[11];
>  };
>  
> +/*
> + * Merkle tree properties.  The file measurement is the hash of this structure
> + * excluding the signature and with the sig_size field set to 0.
> + */
> +struct fsverity_descriptor {
> +	uint8_t version;	/* must be 1 */
> +	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
> +	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
> +	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
> +	__le32 sig_size;	/* size of signature in bytes; 0 if none */
> +	__le64 data_size;	/* size of file the Merkle tree is built over */
> +	uint8_t root_hash[64];	/* Merkle tree root hash */
> +	uint8_t salt[32];	/* salt prepended to each hashed block */
> +	uint8_t __reserved[144];/* must be 0's */
> +	uint8_t signature[];	/* optional PKCS#7 signature */
> +};
> +

I thought there was no need for this to be part of the library API?

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/9]  Split fsverity-utils into a shared library
  2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
                   ` (8 preceding siblings ...)
  2020-03-12 21:47 ` [PATCH 9/9] Document API of libfsverity Jes Sorensen
@ 2020-03-22  5:05 ` Eric Biggers
  9 siblings, 0 replies; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:05 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:49PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Hi,
> 
> This is an updated version of my patches to split fsverity-utils into
> a shared library. This version addresses most of the comments I
> received in the last version:
> 
> 1) Document the API
> 2) Verified ran xfstest against the build
> 3) Make struct fsverity_descriptor private
> 4) Reviewed (and documented) error codes
> 5) Improved validation of input parameters, and return error if any
>    reserved field is not zero.
> 
> I left struct fsverity_hash_alg in the public API, because it adds
> useful information to the user, in particular providing the digest
> size, and allows the caller to walk the list to obtain the supported
> algorithms. The alternative is to introduce a
> libverity_get_digest_size() call.
> 
> I still need to add some self-tests to the build and deal with the
> soname stuff.
> 
> Next up is rpm support.
> 
> Cheers,
> Jes
> 
> 
> Jes Sorensen (9):
>   Build basic shared library framework
>   Change compute_file_measurement() to take a file descriptor as
>     argument
>   Move fsverity_descriptor definition to libfsverity.h
>   Move hash algorithm code to shared library
>   Create libfsverity_compute_digest() and adapt cmd_sign to use it
>   Introduce libfsverity_sign_digest()
>   Validate input arguments to libfsverity_compute_digest()
>   Validate input parameters for libfsverity_sign_digest()
>   Document API of libfsverity
> 
>  Makefile              |  18 +-
>  cmd_enable.c          |  11 +-
>  cmd_measure.c         |   4 +-
>  cmd_sign.c            | 526 +++------------------------------------
>  fsverity.c            |  16 +-
>  hash_algs.c           |  26 +-
>  hash_algs.h           |  27 --
>  libfsverity.h         | 127 ++++++++++
>  libfsverity_private.h |  33 +++
>  libverity.c           | 559 ++++++++++++++++++++++++++++++++++++++++++
>  util.h                |   2 +
>  11 files changed, 801 insertions(+), 548 deletions(-)
>  create mode 100644 libfsverity.h
>  create mode 100644 libfsverity_private.h
>  create mode 100644 libverity.c

Have you tried using the library?  It doesn't work for me because it uses
functions from util.c which aren't compiled in:

test.c:

	#include "libfsverity.h"

	int main() { }

$ gcc test.c -L. -lfsverity

/usr/bin/ld: ./libfsverity.so: undefined reference to `do_error_msg'
/usr/bin/ld: ./libfsverity.so: undefined reference to `error_msg_errno'
/usr/bin/ld: ./libfsverity.so: undefined reference to `error_msg'
/usr/bin/ld: ./libfsverity.so: undefined reference to `fatal_error'
/usr/bin/ld: ./libfsverity.so: undefined reference to `assertion_failed'
/usr/bin/ld: ./libfsverity.so: undefined reference to `xmalloc'
/usr/bin/ld: ./libfsverity.so: undefined reference to `xzalloc'
/usr/bin/ld: ./libfsverity.so: undefined reference to `xmemdup'

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/9] Build basic shared library framework
  2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
@ 2020-03-22  5:23   ` Eric Biggers
  2020-03-22  5:33   ` Eric Biggers
  1 sibling, 0 replies; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:23 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:50PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> This introduces a dummy shared library to start moving things into.
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  Makefile    | 18 +++++++++++++++---
>  libverity.c | 10 ++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
>  create mode 100644 libverity.c
> 
> diff --git a/Makefile b/Makefile
> index b9c09b9..bb85896 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,20 +1,32 @@
>  EXE := fsverity
> +LIB := libfsverity.so
>  CFLAGS := -O2 -Wall
>  CPPFLAGS := -D_FILE_OFFSET_BITS=64
>  LDLIBS := -lcrypto
>  DESTDIR := /usr/local
> +LIBDIR := /usr/lib64
>  SRC := $(wildcard *.c)
> -OBJ := $(SRC:.c=.o)
> +OBJ := fsverity.o hash_algs.o cmd_enable.o cmd_measure.o cmd_sign.o util.o
> +SSRC := libverity.c
> +SOBJ := libverity.so
>  HDRS := $(wildcard *.h)
>  
>  all:$(EXE)
>  
> -$(EXE):$(OBJ)
> +$(EXE):$(OBJ) $(LIB)
> +	$(CC) -o $@ $(OBJ) $(LDLIBS) -L . -l fsverity
>  
>  $(OBJ): %.o: %.c $(HDRS)
> +	$(CC) -c $(CFLAGS) $(CPPFLAGS) $< -o $@
> +
> +$(SOBJ): %.so: %.c $(HDRS)
> +	$(CC) -c -fPIC $(CFLAGS) $(CPPFLAGS) $< -o $@
> +
> +libfsverity.so: $(SOBJ)
> +	$(CC) $(LDLIBS) -shared -o libfsverity.so $(SOBJ)
>  
>  clean:
> -	rm -f $(EXE) $(OBJ)
> +	rm -f $(EXE) $(OBJ) $(SOBJ) $(LIB)
>  
>  install:all
>  	install -Dm755 -t $(DESTDIR)/bin $(EXE)
> diff --git a/libverity.c b/libverity.c
> new file mode 100644
> index 0000000..6821aa2
> --- /dev/null
> +++ b/libverity.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The 'fsverity library'
> + *
> + * Copyright (C) 2018 Google LLC
> + * Copyright (C) 2020 Facebook
> + *
> + * Written by Eric Biggers and Jes Sorensen.
> + */
> +

Could you preserve the option to build the 'fsverity' program as a statically
linked binary?  Having to deal with installing libfsverity.so in some
environments can be annoying.

We maybe should use proper build system that would handle things like this --
though, for small projects like this it's nice to just have a simple Makefile.
What I've done in another project that uses just a Makefile is support building
both a static library a shared library, where the static library is created from
.o files and the shared library is built from .shlib.o files.  Then the binaries
can be linked to either one based on a 'make' variable.

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 8/9] Validate input parameters for libfsverity_sign_digest()
  2020-03-12 21:47 ` [PATCH 8/9] Validate input parameters for libfsverity_sign_digest() Jes Sorensen
@ 2020-03-22  5:27   ` Eric Biggers
  0 siblings, 0 replies; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:27 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:57PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Return -EINVAL on any invalid input argument, as well
> as if any of the reserved fields are set in
> struct libfsverity_signature_digest
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  libverity.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/libverity.c b/libverity.c
> index 1cef544..e16306d 100644
> --- a/libverity.c
> +++ b/libverity.c
> @@ -494,18 +494,36 @@ libfsverity_sign_digest(const struct libfsverity_digest *digest,
>  	X509 *cert = NULL;
>  	const EVP_MD *md;
>  	size_t data_size;
> -	uint16_t alg_nr;
> -	int retval = -EAGAIN;
> +	uint16_t alg_nr, digest_size;
> +	int i, retval = -EAGAIN;
> +	const char magic[8] = "FSVerity";
> +
> +	if (!digest || !sig_params || !sig_ret || !sig_size_ret)
> +		return -EINVAL;
> +
> +	if (strncmp(digest->magic, magic, sizeof(magic)))
> +		return -EINVAL;
> +
> +	if (!sig_params->keyfile || !sig_params->certfile)
> +		return -EINVAL;
> +
> +	for (i = 0; i < sizeof(sig_params->reserved) /
> +		     sizeof(sig_params->reserved[0]); i++) {
> +		if (sig_params->reserved[i])
> +			return -EINVAL;
> +	}

This can use ARRAY_SIZE().

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/9] Build basic shared library framework
  2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
  2020-03-22  5:23   ` Eric Biggers
@ 2020-03-22  5:33   ` Eric Biggers
  2020-04-21 21:00     ` Jes Sorensen
  1 sibling, 1 reply; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:33 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:50PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> This introduces a dummy shared library to start moving things into.
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  Makefile    | 18 +++++++++++++++---
>  libverity.c | 10 ++++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
>  create mode 100644 libverity.c
> 
> diff --git a/Makefile b/Makefile
> index b9c09b9..bb85896 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,20 +1,32 @@
>  EXE := fsverity
> +LIB := libfsverity.so
>  CFLAGS := -O2 -Wall
>  CPPFLAGS := -D_FILE_OFFSET_BITS=64
>  LDLIBS := -lcrypto
>  DESTDIR := /usr/local
> +LIBDIR := /usr/lib64

LIBDIR isn't used at all.  I assume you meant for it to be location where the
library gets installed?  The proper way to handle installation locations
(assuming we stay with a plain Makefile and not move to another build system)
would be:

PREFIX ?= /usr/local
BINDIR ?= $(PREFIX)/bin
INCDIR ?= $(PREFIX)/include
LIBDIR ?= $(PREFIX)/lib

then install binaries into $(DESTDIR)$(BINDIR), headers into
$(DESTDIR)$(INCDIR), and libraries into $(DESTDIR)$(LIBDIR).
This matches the conventions for autoconf.

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/9] Move hash algorithm code to shared library
  2020-03-12 21:47 ` [PATCH 4/9] Move hash algorithm code to shared library Jes Sorensen
@ 2020-03-22  5:38   ` Eric Biggers
  2020-04-22 17:57     ` Jes Sorensen
  0 siblings, 1 reply; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:38 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:53PM -0400, Jes Sorensen wrote:
> diff --git a/libfsverity.h b/libfsverity.h
> index 396a6ee..318dcd7 100644
> --- a/libfsverity.h
> +++ b/libfsverity.h
> @@ -18,6 +18,9 @@
>  #define FS_VERITY_HASH_ALG_SHA256       1
>  #define FS_VERITY_HASH_ALG_SHA512       2
>  
> +/* The hash algorithm that fsverity-utils assumes when none is specified */
> +#define FS_VERITY_HASH_ALG_DEFAULT	FS_VERITY_HASH_ALG_SHA256
> +
>  struct libfsverity_merkle_tree_params {
>  	uint16_t version;
>  	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
> @@ -27,6 +30,12 @@ struct libfsverity_merkle_tree_params {
>  	uint64_t reserved[11];
>  };
>  
> +/*
> + * 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 libfsverity_digest {
>  	char magic[8];			/* must be "FSVerity" */
>  	uint16_t digest_algorithm;
> @@ -57,9 +66,22 @@ struct fsverity_descriptor {
>  	uint8_t signature[];	/* optional PKCS#7 signature */
>  };
>  
> +struct fsverity_hash_alg {
> +	const char *name;
> +	unsigned int digest_size;
> +	unsigned int block_size;
> +	uint16_t hash_num;
> +	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
> +};
> +

It's still a bit weird to have struct fsverity_hash_alg as part of the library
API, since the .create_ctx() member is for internal library use only.  We at
least need to clearly comment this:

	struct fsverity_hash_alg {
		const char *name;
		unsigned int digest_size;
		unsigned int block_size;
		uint16_t hash_num;

		/* for library-internal use only */
		struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
	};

But ideally there would be nothing library-internal in the API at all.

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it
  2020-03-12 21:47 ` [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it Jes Sorensen
@ 2020-03-22  5:40   ` Eric Biggers
  0 siblings, 0 replies; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:40 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:54PM -0400, Jes Sorensen wrote:
> @@ -608,16 +433,17 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
>  	if (certfile == NULL)
>  		certfile = keyfile;
>  
> -	digest = xzalloc(sizeof(*digest) + hash_alg->digest_size);
> -	memcpy(digest->magic, "FSVerity", 8);
> -	digest->digest_algorithm = cpu_to_le16(hash_alg->hash_num);
> -	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
> -
>  	if (!open_file(&file, argv[0], O_RDONLY, 0))
>  		goto out_err;
>  
> -	if (!compute_file_measurement(file.fd, hash_alg, block_size,
> -				      salt, salt_size, digest->digest))
> +	memset(&params, 0, sizeof(struct libfsverity_merkle_tree_params));

Please use 'sizeof(params)' in cases like this.

> +	params.version = 1;
> +	params.hash_algorithm = hash_alg->hash_num;
> +	params.block_size = block_size;
> +	params.salt_size = salt_size;
> +	params.salt = salt;
> +
> +	if (libfsverity_compute_digest(file.fd, &params, &digest))
>  		goto out_err;

This doesn't close the file on error.

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 9/9] Document API of libfsverity
  2020-03-12 21:47 ` [PATCH 9/9] Document API of libfsverity Jes Sorensen
@ 2020-03-22  5:54   ` Eric Biggers
  0 siblings, 0 replies; 23+ messages in thread
From: Eric Biggers @ 2020-03-22  5:54 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On Thu, Mar 12, 2020 at 05:47:58PM -0400, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> ---
>  libfsverity.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/libfsverity.h b/libfsverity.h
> index a2abdb3..f6c4b13 100644
> --- a/libfsverity.h
> +++ b/libfsverity.h
> @@ -64,18 +64,63 @@ struct fsverity_hash_alg {
>  	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
>  };
>  
> +/*
> + * libfsverity_compute_digest - Compute digest of a file
> + * @fd: open file descriptor of file to compute digest for
> + * @params: struct libfsverity_merkle_tree_params specifying hash algorithm,
> + *	    block size, version, and optional salt parameters.
> + *	    reserved parameters must be zero.
> + * @digest_ret: Pointer to pointer for computed digest
> + *
> + * Returns:
> + * * 0 for success, -EINVAL for invalid input arguments, -ENOMEM if failed
> + *   to allocate memory, -EBADF if fd is invalid, and -EAGAIN if root hash
> + *   fails to compute.
> + * * digest_ret returns a pointer to the digest on success.
> + */
>  int
>  libfsverity_compute_digest(int fd,
>  			   const struct libfsverity_merkle_tree_params *params,
>  			   struct libfsverity_digest **digest_ret);

Can you add a brief explanation here that clarifies that the "digest of a file"
in this context means fs-verity's Merkle tree-based hash (what the kernel calls
the "file measurement"), not a standard file hash?  These could easily be
confused, especially by people not as familiar with fs-verity.

Also, it's important to also mention that the digest needs to be free()d.

>  
> +/*
> + * libfsverity_sign_digest - Sign previously computed digest of a file
> + * @digest: pointer to previously computed digest
> + * @sig_params: struct libfsverity_signature_params providing filenames of
> + *          the keyfile and certificate file. Reserved parameters must be zero.
> + * @sig_ret: Pointer to pointer for signed digest
> + * @sig_size_ret: Pointer to size of signed return digest
> + *
> + * Returns:
> + * * 0 for success, -EINVAL for invalid input arguments, -EAGAIN if key or
> + *   certificate files fail to read, or if signing the digest fails.
> + * * sig_ret returns a pointer to the signed digest on success.
> + * * sig_size_ret returns the size of the signed digest on success.
> + */
>  int
>  libfsverity_sign_digest(const struct libfsverity_digest *digest,
>  			const struct libfsverity_signature_params *sig_params,
>  			uint8_t **sig_ret, size_t *sig_size_ret);

Can you add some more details here?  What is the signature for, what type of
signature is it, what format do the key file and certificate file need to be in,
what crypto algorithms do they need to use, etc.?

(I know, I need to add a man page for the 'fsverity' program that explains this
too.  There's some explanation in the README but not enough.)

Also, it's important to mention that the signature needs to be free()d.

> +/*
> + * libfsverity_find_hash_alg_by_name - Find hash algorithm by name
> + * @name: Pointer to name of hash algorithm
> + *
> + * Returns:
> + * struct fsverity_hash_alg success
> + * NULL on error
> + */

We should be more specific and write "NULL if not found", since "not found" is
the only type of error that makes sense for this.

>  const struct fsverity_hash_alg *
>  libfsverity_find_hash_alg_by_name(const char *name);
> +
> +/*
> + * libfsverity_find_hash_alg_by_num - Find hash algorithm by number
> + * @name: Number of hash algorithm
> + *
> + * Returns:
> + * struct fsverity_hash_alg success
> + * NULL on error
> + */
>  const struct fsverity_hash_alg *
>  libfsverity_find_hash_alg_by_num(unsigned int num);

Likewise.

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h
  2020-03-22  4:57   ` Eric Biggers
@ 2020-04-21 16:07     ` Jes Sorensen
  2020-04-21 16:16       ` Eric Biggers
  0 siblings, 1 reply; 23+ messages in thread
From: Jes Sorensen @ 2020-04-21 16:07 UTC (permalink / raw)
  To: Eric Biggers, Jes Sorensen; +Cc: linux-fscrypt, kernel-team

On 3/22/20 12:57 AM, Eric Biggers wrote:
> On Thu, Mar 12, 2020 at 05:47:52PM -0400, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>>
>> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>> ---
>>  cmd_sign.c    | 19 +------------------
>>  libfsverity.h | 26 +++++++++++++++++++++++++-
>>  2 files changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/cmd_sign.c b/cmd_sign.c
>> index dcc44f8..1792084 100644
>> --- a/cmd_sign.c
>> +++ b/cmd_sign.c
>> @@ -20,26 +20,9 @@
>>  #include <unistd.h>
>>  
>>  #include "commands.h"
>> -#include "fsverity_uapi.h"
>> +#include "libfsverity.h"
>>  #include "hash_algs.h"
>>  
>> -/*
>> - * Merkle tree properties.  The file measurement is the hash of this structure
>> - * excluding the signature and with the sig_size field set to 0.
>> - */
>> -struct fsverity_descriptor {
>> -	__u8 version;		/* must be 1 */
>> -	__u8 hash_algorithm;	/* Merkle tree hash algorithm */
>> -	__u8 log_blocksize;	/* log2 of size of data and tree blocks */
>> -	__u8 salt_size;		/* size of salt in bytes; 0 if none */
>> -	__le32 sig_size;	/* size of signature in bytes; 0 if none */
>> -	__le64 data_size;	/* size of file the Merkle tree is built over */
>> -	__u8 root_hash[64];	/* Merkle tree root hash */
>> -	__u8 salt[32];		/* salt prepended to each hashed block */
>> -	__u8 __reserved[144];	/* must be 0's */
>> -	__u8 signature[];	/* optional PKCS#7 signature */
>> -};
>> -
>>  /*
>>   * Format in which verity file measurements are signed.  This is the same as
>>   * 'struct fsverity_digest', except here some magic bytes are prepended to
>> diff --git a/libfsverity.h b/libfsverity.h
>> index ceebae1..396a6ee 100644
>> --- a/libfsverity.h
>> +++ b/libfsverity.h
>> @@ -13,13 +13,14 @@
>>  
>>  #include <stddef.h>
>>  #include <stdint.h>
>> +#include <linux/types.h>
>>  
>>  #define FS_VERITY_HASH_ALG_SHA256       1
>>  #define FS_VERITY_HASH_ALG_SHA512       2
>>  
>>  struct libfsverity_merkle_tree_params {
>>  	uint16_t version;
>> -	uint16_t hash_algorithm;
>> +	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
>>  	uint32_t block_size;
>>  	uint32_t salt_size;
>>  	const uint8_t *salt;
>> @@ -27,6 +28,7 @@ struct libfsverity_merkle_tree_params {
>>  };
>>  
>>  struct libfsverity_digest {
>> +	char magic[8];			/* must be "FSVerity" */
>>  	uint16_t digest_algorithm;
>>  	uint16_t digest_size;
>>  	uint8_t digest[];
>> @@ -38,4 +40,26 @@ struct libfsverity_signature_params {
>>  	uint64_t reserved[11];
>>  };
>>  
>> +/*
>> + * Merkle tree properties.  The file measurement is the hash of this structure
>> + * excluding the signature and with the sig_size field set to 0.
>> + */
>> +struct fsverity_descriptor {
>> +	uint8_t version;	/* must be 1 */
>> +	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
>> +	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
>> +	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
>> +	__le32 sig_size;	/* size of signature in bytes; 0 if none */
>> +	__le64 data_size;	/* size of file the Merkle tree is built over */
>> +	uint8_t root_hash[64];	/* Merkle tree root hash */
>> +	uint8_t salt[32];	/* salt prepended to each hashed block */
>> +	uint8_t __reserved[144];/* must be 0's */
>> +	uint8_t signature[];	/* optional PKCS#7 signature */
>> +};
>> +
> 
> I thought there was no need for this to be part of the library API?

Hi Eric,

Been busy working on RPM support, but looking at this again now. Given
that the fsverity signature is a hash of the descriptor, I don't see how
we can avoid this?

Cheers,
Jes




^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h
  2020-04-21 16:07     ` Jes Sorensen
@ 2020-04-21 16:16       ` Eric Biggers
  2020-04-21 16:20         ` Jes Sorensen
  0 siblings, 1 reply; 23+ messages in thread
From: Eric Biggers @ 2020-04-21 16:16 UTC (permalink / raw)
  To: Jes Sorensen; +Cc: Jes Sorensen, linux-fscrypt, kernel-team

On Tue, Apr 21, 2020 at 12:07:07PM -0400, Jes Sorensen wrote:
> On 3/22/20 12:57 AM, Eric Biggers wrote:
> > On Thu, Mar 12, 2020 at 05:47:52PM -0400, Jes Sorensen wrote:
> >> From: Jes Sorensen <jsorensen@fb.com>
> >>
> >> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
> >> ---
> >>  cmd_sign.c    | 19 +------------------
> >>  libfsverity.h | 26 +++++++++++++++++++++++++-
> >>  2 files changed, 26 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/cmd_sign.c b/cmd_sign.c
> >> index dcc44f8..1792084 100644
> >> --- a/cmd_sign.c
> >> +++ b/cmd_sign.c
> >> @@ -20,26 +20,9 @@
> >>  #include <unistd.h>
> >>  
> >>  #include "commands.h"
> >> -#include "fsverity_uapi.h"
> >> +#include "libfsverity.h"
> >>  #include "hash_algs.h"
> >>  
> >> -/*
> >> - * Merkle tree properties.  The file measurement is the hash of this structure
> >> - * excluding the signature and with the sig_size field set to 0.
> >> - */
> >> -struct fsverity_descriptor {
> >> -	__u8 version;		/* must be 1 */
> >> -	__u8 hash_algorithm;	/* Merkle tree hash algorithm */
> >> -	__u8 log_blocksize;	/* log2 of size of data and tree blocks */
> >> -	__u8 salt_size;		/* size of salt in bytes; 0 if none */
> >> -	__le32 sig_size;	/* size of signature in bytes; 0 if none */
> >> -	__le64 data_size;	/* size of file the Merkle tree is built over */
> >> -	__u8 root_hash[64];	/* Merkle tree root hash */
> >> -	__u8 salt[32];		/* salt prepended to each hashed block */
> >> -	__u8 __reserved[144];	/* must be 0's */
> >> -	__u8 signature[];	/* optional PKCS#7 signature */
> >> -};
> >> -
> >>  /*
> >>   * Format in which verity file measurements are signed.  This is the same as
> >>   * 'struct fsverity_digest', except here some magic bytes are prepended to
> >> diff --git a/libfsverity.h b/libfsverity.h
> >> index ceebae1..396a6ee 100644
> >> --- a/libfsverity.h
> >> +++ b/libfsverity.h
> >> @@ -13,13 +13,14 @@
> >>  
> >>  #include <stddef.h>
> >>  #include <stdint.h>
> >> +#include <linux/types.h>
> >>  
> >>  #define FS_VERITY_HASH_ALG_SHA256       1
> >>  #define FS_VERITY_HASH_ALG_SHA512       2
> >>  
> >>  struct libfsverity_merkle_tree_params {
> >>  	uint16_t version;
> >> -	uint16_t hash_algorithm;
> >> +	uint16_t hash_algorithm;	/* Matches the digest_algorithm type */
> >>  	uint32_t block_size;
> >>  	uint32_t salt_size;
> >>  	const uint8_t *salt;
> >> @@ -27,6 +28,7 @@ struct libfsverity_merkle_tree_params {
> >>  };
> >>  
> >>  struct libfsverity_digest {
> >> +	char magic[8];			/* must be "FSVerity" */
> >>  	uint16_t digest_algorithm;
> >>  	uint16_t digest_size;
> >>  	uint8_t digest[];
> >> @@ -38,4 +40,26 @@ struct libfsverity_signature_params {
> >>  	uint64_t reserved[11];
> >>  };
> >>  
> >> +/*
> >> + * Merkle tree properties.  The file measurement is the hash of this structure
> >> + * excluding the signature and with the sig_size field set to 0.
> >> + */
> >> +struct fsverity_descriptor {
> >> +	uint8_t version;	/* must be 1 */
> >> +	uint8_t hash_algorithm;	/* Merkle tree hash algorithm */
> >> +	uint8_t log_blocksize;	/* log2 of size of data and tree blocks */
> >> +	uint8_t salt_size;	/* size of salt in bytes; 0 if none */
> >> +	__le32 sig_size;	/* size of signature in bytes; 0 if none */
> >> +	__le64 data_size;	/* size of file the Merkle tree is built over */
> >> +	uint8_t root_hash[64];	/* Merkle tree root hash */
> >> +	uint8_t salt[32];	/* salt prepended to each hashed block */
> >> +	uint8_t __reserved[144];/* must be 0's */
> >> +	uint8_t signature[];	/* optional PKCS#7 signature */
> >> +};
> >> +
> > 
> > I thought there was no need for this to be part of the library API?
> 
> Hi Eric,
> 
> Been busy working on RPM support, but looking at this again now. Given
> that the fsverity signature is a hash of the descriptor, I don't see how
> we can avoid this?
> 

struct fsverity_descriptor isn't signed directly; it's hashed as an intermediate
step in libfsverity_compute_digest().  So why would the library user need the
definition of 'struct fsverity_descriptor'?

- Eric

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h
  2020-04-21 16:16       ` Eric Biggers
@ 2020-04-21 16:20         ` Jes Sorensen
  0 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-04-21 16:20 UTC (permalink / raw)
  To: Eric Biggers, Jes Sorensen; +Cc: linux-fscrypt, kernel-team

On 4/21/20 12:16 PM, Eric Biggers wrote:
> On Tue, Apr 21, 2020 at 12:07:07PM -0400, Jes Sorensen wrote:
>> On 3/22/20 12:57 AM, Eric Biggers wrote:
>>> I thought there was no need for this to be part of the library API?
>>
>> Hi Eric,
>>
>> Been busy working on RPM support, but looking at this again now. Given
>> that the fsverity signature is a hash of the descriptor, I don't see how
>> we can avoid this?
>>
> 
> struct fsverity_descriptor isn't signed directly; it's hashed as an intermediate
> step in libfsverity_compute_digest().  So why would the library user need the
> definition of 'struct fsverity_descriptor'?

Hi Eric,

You're right, I actually moved it to libfsverity_private.h already, but
it's in the new patches I am working on.

I pushed it all to git.kernel.org, but I still need to address some of
the issues you responded about. I'll post an update to this when I have
worked through your list of comments. Most noticeable is that I had to
rework the read API to make it work with RPM, but you can find my
current tree here (libfsverity branch):
https://git.kernel.org/pub/scm/linux/kernel/git/jes/fsverity-utils.git/

Current RPM work is here:
https://github.com/jessorensen/rpm/tree/rpm-fsverity

Cheers,
Jes

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/9] Build basic shared library framework
  2020-03-22  5:33   ` Eric Biggers
@ 2020-04-21 21:00     ` Jes Sorensen
  0 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-04-21 21:00 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On 3/22/20 1:33 AM, Eric Biggers wrote:
> On Thu, Mar 12, 2020 at 05:47:50PM -0400, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>>
>> This introduces a dummy shared library to start moving things into.
>>
>> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>> ---
>>  Makefile    | 18 +++++++++++++++---
>>  libverity.c | 10 ++++++++++
>>  2 files changed, 25 insertions(+), 3 deletions(-)
>>  create mode 100644 libverity.c
>>
>> diff --git a/Makefile b/Makefile
>> index b9c09b9..bb85896 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1,20 +1,32 @@
>>  EXE := fsverity
>> +LIB := libfsverity.so
>>  CFLAGS := -O2 -Wall
>>  CPPFLAGS := -D_FILE_OFFSET_BITS=64
>>  LDLIBS := -lcrypto
>>  DESTDIR := /usr/local
>> +LIBDIR := /usr/lib64
> 
> LIBDIR isn't used at all.  I assume you meant for it to be location where the
> library gets installed?  The proper way to handle installation locations
> (assuming we stay with a plain Makefile and not move to another build system)
> would be:
> 
> PREFIX ?= /usr/local
> BINDIR ?= $(PREFIX)/bin
> INCDIR ?= $(PREFIX)/include
> LIBDIR ?= $(PREFIX)/lib
> 
> then install binaries into $(DESTDIR)$(BINDIR), headers into
> $(DESTDIR)$(INCDIR), and libraries into $(DESTDIR)$(LIBDIR).
> This matches the conventions for autoconf.

I pushed a change to git which addresses this, I still need to address
the soname though.

Long term it might be nice to switch to autoconf/automake, at the same
time I love dealing with it about as much as going to the dentist for a
root canal.

Cheers,
Jes

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 4/9] Move hash algorithm code to shared library
  2020-03-22  5:38   ` Eric Biggers
@ 2020-04-22 17:57     ` Jes Sorensen
  0 siblings, 0 replies; 23+ messages in thread
From: Jes Sorensen @ 2020-04-22 17:57 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-fscrypt, kernel-team, Jes Sorensen

On 3/22/20 1:38 AM, Eric Biggers wrote:
> On Thu, Mar 12, 2020 at 05:47:53PM -0400, Jes Sorensen wrote:
>> diff --git a/libfsverity.h b/libfsverity.h
>> +struct fsverity_hash_alg {
>> +	const char *name;
>> +	unsigned int digest_size;
>> +	unsigned int block_size;
>> +	uint16_t hash_num;
>> +	struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
>> +};
>> +
> 
> It's still a bit weird to have struct fsverity_hash_alg as part of the library
> API, since the .create_ctx() member is for internal library use only.  We at
> least need to clearly comment this:
> 
> 	struct fsverity_hash_alg {
> 		const char *name;
> 		unsigned int digest_size;
> 		unsigned int block_size;
> 		uint16_t hash_num;
> 
> 		/* for library-internal use only */
> 		struct hash_ctx *(*create_ctx)(const struct fsverity_hash_alg *alg);
> 	};
> 
> But ideally there would be nothing library-internal in the API at all.

So I looked this over again, and came up with a solution which gets rid
of struct fsverity_hash_alg from the public API. To get around it I
extended the API like this:

<uint16_t> = libfsverity_find_alg_by_name(<char *name>)
<int>      = libfsverity_digest_size(<uint16_t alg_nr>)

In order to be able to print the algorithm name and list supported
algorithms, I introduced:

<char *>   = libfsverity_alg_name(<uint16_t alg_nr>)

Cheers,
Jes

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-04-22 17:57 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12 21:47 [PATCH v3 0/9] Split fsverity-utils into a shared library Jes Sorensen
2020-03-12 21:47 ` [PATCH 1/9] Build basic shared library framework Jes Sorensen
2020-03-22  5:23   ` Eric Biggers
2020-03-22  5:33   ` Eric Biggers
2020-04-21 21:00     ` Jes Sorensen
2020-03-12 21:47 ` [PATCH 2/9] Change compute_file_measurement() to take a file descriptor as argument Jes Sorensen
2020-03-12 21:47 ` [PATCH 3/9] Move fsverity_descriptor definition to libfsverity.h Jes Sorensen
2020-03-22  4:57   ` Eric Biggers
2020-04-21 16:07     ` Jes Sorensen
2020-04-21 16:16       ` Eric Biggers
2020-04-21 16:20         ` Jes Sorensen
2020-03-12 21:47 ` [PATCH 4/9] Move hash algorithm code to shared library Jes Sorensen
2020-03-22  5:38   ` Eric Biggers
2020-04-22 17:57     ` Jes Sorensen
2020-03-12 21:47 ` [PATCH 5/9] Create libfsverity_compute_digest() and adapt cmd_sign to use it Jes Sorensen
2020-03-22  5:40   ` Eric Biggers
2020-03-12 21:47 ` [PATCH 6/9] Introduce libfsverity_sign_digest() Jes Sorensen
2020-03-12 21:47 ` [PATCH 7/9] Validate input arguments to libfsverity_compute_digest() Jes Sorensen
2020-03-12 21:47 ` [PATCH 8/9] Validate input parameters for libfsverity_sign_digest() Jes Sorensen
2020-03-22  5:27   ` Eric Biggers
2020-03-12 21:47 ` [PATCH 9/9] Document API of libfsverity Jes Sorensen
2020-03-22  5:54   ` Eric Biggers
2020-03-22  5:05 ` [PATCH v3 0/9] Split fsverity-utils into a shared library Eric Biggers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).