vboot2: Split keyblock checking and signature validation

This is necessary for the next change, which adds keyblock hash checking.

Also clean up some other assorted comments, and move the diagnostic
check of root key to see if it's the checked-in one earlier in
firmware preamble validation so it's closer to where the root key is
loaded.

No functional or higher-level API changes; just shuffling around code
under the covers.

BUG=chromium:487699
BRANCH=none
TEST=make -j runtests

Change-Id: Ibc3960a4d882dc2ad8684e235db4b9d066eac080
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/272223
Reviewed-by: Bill Richardson <wfrichar@chromium.org>

7 files changed, 176 insertions, 71 deletions

diff --git a/Makefile b/Makefile
index 0e96c5c9..17881b78 100644
--- a/Makefile
+++ b/Makefile
@@ -1444,7 +1444,7 @@ coverage_html:
 .PHONY: coverage
 ifeq (${COV},)
 coverage:
-	$(error Build coverage like this: make clean && COV=1 make)
+	$(error Build coverage like this: make clean && COV=1 make coverage)
 else
 coverage: coverage_init runtests coverage_html
 endif
diff --git a/firmware/lib20/api.c b/firmware/lib20/api.c
index 55c59ead..bee93285 100644
--- a/firmware/lib20/api.c
+++ b/firmware/lib20/api.c
@@ -77,6 +77,14 @@ int vb2api_init_hash(struct vb2_context *ctx, uint32_t tag, uint32_t *size)
 	}
 
 	/*
+	 * Work buffer now contains:
+	 *   - vb2_shared_data
+	 *   - packed firmware data key
+	 *   - firmware preamble
+	 *   - hash data
+	 */
+
+	/*
 	 * Unpack the firmware data key to see which hashing algorithm we
 	 * should use.
 	 *
diff --git a/firmware/lib20/common.c b/firmware/lib20/common.c
index cee6cb42..91221147 100644
--- a/firmware/lib20/common.c
+++ b/firmware/lib20/common.c
@@ -130,35 +130,30 @@ int vb2_verify_data(const uint8_t *data,
 	return vb2_verify_digest(key, sig, digest, &wblocal);
 }
 
-int vb2_verify_keyblock(struct vb2_keyblock *block,
-			uint32_t size,
-			const struct vb2_public_key *key,
-			const struct vb2_workbuf *wb)
+int vb2_check_keyblock(const struct vb2_keyblock *block,
+		       uint32_t size,
+		       const struct vb2_signature *sig)
 {
-	struct vb2_signature *sig;
-	int rv;
-
-	/* Sanity checks before attempting signature of data */
 	if(size < sizeof(*block)) {
 		VB2_DEBUG("Not enough space for key block header.\n");
 		return VB2_ERROR_KEYBLOCK_TOO_SMALL_FOR_HEADER;
 	}
+
 	if (memcmp(block->magic, KEY_BLOCK_MAGIC, KEY_BLOCK_MAGIC_SIZE)) {
 		VB2_DEBUG("Not a valid verified boot key block.\n");
 		return VB2_ERROR_KEYBLOCK_MAGIC;
 	}
+
 	if (block->header_version_major != KEY_BLOCK_HEADER_VERSION_MAJOR) {
 		VB2_DEBUG("Incompatible key block header version.\n");
 		return VB2_ERROR_KEYBLOCK_HEADER_VERSION;
 	}
+
 	if (size < block->keyblock_size) {
 		VB2_DEBUG("Not enough data for key block.\n");
 		return VB2_ERROR_KEYBLOCK_SIZE;
 	}
 
-	/* Check signature */
-	sig = &block->keyblock_signature;
-
 	if (vb2_verify_signature_inside(block, block->keyblock_size, sig)) {
 		VB2_DEBUG("Key block signature off end of block\n");
 		return VB2_ERROR_KEYBLOCK_SIG_OUTSIDE;
@@ -170,13 +165,6 @@ int vb2_verify_keyblock(struct vb2_keyblock *block,
 		return VB2_ERROR_KEYBLOCK_SIGNED_TOO_MUCH;
 	}
 
-	VB2_DEBUG("Checking key block signature...\n");
-	rv = vb2_verify_data((const uint8_t *)block, size, sig, key, wb);
-	if (rv) {
-		VB2_DEBUG("Invalid key block signature.\n");
-		return VB2_ERROR_KEYBLOCK_SIG_INVALID;
-	}
-
 	/* Verify we signed enough data */
 	if (sig->data_size < sizeof(struct vb2_keyblock)) {
 		VB2_DEBUG("Didn't sign enough data\n");
@@ -195,6 +183,29 @@ int vb2_verify_keyblock(struct vb2_keyblock *block,
 		return VB2_ERROR_KEYBLOCK_DATA_KEY_UNSIGNED;
 	}
 
+	return VB2_SUCCESS;
+}
+
+int vb2_verify_keyblock(struct vb2_keyblock *block,
+			uint32_t size,
+			const struct vb2_public_key *key,
+			const struct vb2_workbuf *wb)
+{
+	struct vb2_signature *sig = &block->keyblock_signature;
+	int rv;
+
+	/* Sanity check keyblock before attempting signature check of data */
+	rv = vb2_check_keyblock(block, size, sig);
+	if (rv)
+		return rv;
+
+	VB2_DEBUG("Checking key block signature...\n");
+	rv = vb2_verify_data((const uint8_t *)block, size, sig, key, wb);
+	if (rv) {
+		VB2_DEBUG("Invalid key block signature.\n");
+		return VB2_ERROR_KEYBLOCK_SIG_INVALID;
+	}
+
 	/* Success */
 	return VB2_SUCCESS;
 }
diff --git a/firmware/lib20/include/vb2_common.h b/firmware/lib20/include/vb2_common.h
index a71cfe6c..be891ca9 100644
--- a/firmware/lib20/include/vb2_common.h
+++ b/firmware/lib20/include/vb2_common.h
@@ -124,7 +124,23 @@ int vb2_verify_data(const uint8_t *data,
 		    const struct vb2_workbuf *wb);
 
 /**
- * Check the sanity of a key block using a public key.
+ * Check the sanity of a key block structure.
+ *
+ * Verifies all the header fields.  Does not verify key index or key block
+ * flags.  Should be called before verifying the key block data itself using
+ * the key.  (This function does not itself verify the signature - just that
+ * the right amount of data is claimed to be signed.)
+ *
+ * @param block		Key block to verify
+ * @param size		Size of key block buffer
+ * @param sig		Which signature inside the keyblock to use
+ */
+int vb2_check_keyblock(const struct vb2_keyblock *block,
+		       uint32_t size,
+		       const struct vb2_signature *sig);
+
+/**
+ * Verify a key block using a public key.
  *
  * Header fields are also checked for sanity.  Does not verify key index or key
  * block flags.  Signature inside block is destroyed during check.
diff --git a/firmware/lib20/include/vb2_struct.h b/firmware/lib20/include/vb2_struct.h
index d3e2f400..1e39061f 100644
--- a/firmware/lib20/include/vb2_struct.h
+++ b/firmware/lib20/include/vb2_struct.h
@@ -97,12 +97,12 @@ struct vb2_keyblock {
 	struct vb2_signature keyblock_signature;
 
 	/*
-	 * SHA-512 checksum for this key block (header + data pointed to by
+	 * SHA-512 hash for this key block (header + data pointed to by
 	 * data_key) For use with unsigned data keys.
 	 *
-	 * Note that the vb2 lib currently only supports signed blocks.
+	 * Only supported for kernel keyblocks, not firmware keyblocks.
 	 */
-	struct vb2_signature keyblock_checksum_unused;
+	struct vb2_signature keyblock_hash;
 
 	/* Flags for key (VB2_KEY_BLOCK_FLAG_*) */
 	uint32_t keyblock_flags;
diff --git a/firmware/lib20/misc.c b/firmware/lib20/misc.c
index 83232b07..13004c69 100644
--- a/firmware/lib20/misc.c
+++ b/firmware/lib20/misc.c
@@ -25,6 +25,13 @@ static const uint8_t dev_key_digest[] = {
 	0xcf, 0x04, 0x1f, 0x10,
 };
 
+/**
+ * Determine if the root key is the developer key checked into the
+ * vboot_reference repository.  Has no effect on boot; just logs this to the
+ * debug console.
+ *
+ * @param root		Root key
+ */
 static void vb2_report_dev_firmware(struct vb2_public_key *root)
 {
 	struct vb2_digest_context dc;
@@ -91,6 +98,9 @@ int vb2_load_fw_keyblock(struct vb2_context *ctx)
 	if (rv)
 		return rv;
 
+	/* If that's the checked-in root key, this is dev-signed firmware */
+	vb2_report_dev_firmware(&root_key);
+
 	/* Load the firmware keyblock header after the root key */
 	kb = vb2_workbuf_alloc(&wb, sizeof(*kb));
 	if (!kb)
@@ -137,7 +147,6 @@ int vb2_load_fw_keyblock(struct vb2_context *ctx)
 		return rv;
 	}
 
-	vb2_report_dev_firmware(&root_key);
 	sd->fw_version = kb->data_key.key_version << 16;
 
 	/*
@@ -169,7 +178,13 @@ int vb2_load_fw_keyblock(struct vb2_context *ctx)
 	/* Preamble follows the keyblock in the vblock */
 	sd->vblock_preamble_offset = kb->keyblock_size;
 
-	/* Data key will persist in the workbuf after we return */
+	/*
+	 * Data key will persist in the workbuf after we return.
+	 *
+	 * Work buffer now contains:
+	 *   - vb2_shared_data
+	 *   - packed firmware data key
+	 */
 	ctx->workbuf_used = sd->workbuf_data_key_offset +
 		sd->workbuf_data_key_size;
 
@@ -272,7 +287,17 @@ int vb2_load_fw_preamble(struct vb2_context *ctx)
 	sd->workbuf_preamble_offset = vb2_offset_of(ctx->workbuf, pre);
 	sd->workbuf_preamble_size = pre_size;
 
-	/* Preamble will persist in work buffer after we return */
+	/*
+	 * Preamble will persist in work buffer after we return.
+	 *
+	 * Work buffer now contains:
+	 *   - vb2_shared_data
+	 *   - packed firmware data key
+	 *   - firmware preamble
+	 *
+	 * TODO: we could move the preamble down over the firmware data key
+	 * since we don't need it anymore.
+	 */
 	ctx->workbuf_used = sd->workbuf_preamble_offset + pre_size;
 
 	return VB2_SUCCESS;
diff --git a/tests/vb20_common3_tests.c b/tests/vb20_common3_tests.c
index b41300c7..0e135bda 100644
--- a/tests/vb20_common3_tests.c
+++ b/tests/vb20_common3_tests.c
@@ -29,20 +29,16 @@ static void resign_keyblock(struct vb2_keyblock *h, const VbPrivateKey *key)
 	free(sig);
 }
 
-static void test_verify_keyblock(const VbPublicKey *public_key,
+static void test_check_keyblock(const VbPublicKey *public_key,
 				 const VbPrivateKey *private_key,
 				 const VbPublicKey *data_key)
 {
-	uint8_t workbuf[VB2_KEY_BLOCK_VERIFY_WORKBUF_BYTES]
-		__attribute__ ((aligned (VB2_WORKBUF_ALIGN)));
-	struct vb2_workbuf wb;
 	struct vb2_public_key key;
 	struct vb2_keyblock *hdr;
 	struct vb2_keyblock *h;
+	struct vb2_signature *sig;
 	uint32_t hsize;
 
-	vb2_workbuf_init(&wb, workbuf, sizeof(workbuf));
-
 	/* Unpack public key */
 	TEST_SUCC(vb2_unpack_key(&key, (uint8_t *)public_key,
 				 public_key->key_offset + public_key->key_size),
@@ -55,78 +51,66 @@ static void test_verify_keyblock(const VbPublicKey *public_key,
 		return;
 	hsize = hdr->keyblock_size;
 	h = (struct vb2_keyblock *)malloc(hsize + 2048);
+	sig = &h->keyblock_signature;
 
 	Memcpy(h, hdr, hsize);
-	TEST_SUCC(vb2_verify_keyblock(h, hsize, &key, &wb),
-		  "vb2_verify_keyblock() ok using key");
+	TEST_SUCC(vb2_check_keyblock(h, hsize, sig),
+		  "vb2_check_keyblock() ok");
 
 	Memcpy(h, hdr, hsize);
-	TEST_EQ(vb2_verify_keyblock(h, hsize - 1, &key, &wb),
-		VB2_ERROR_KEYBLOCK_SIZE, "vb2_verify_keyblock() size--");
+	TEST_EQ(vb2_check_keyblock(h, hsize - 1, sig),
+		VB2_ERROR_KEYBLOCK_SIZE, "vb2_check_keyblock() size--");
 
 	/* Buffer is allowed to be bigger than keyblock */
 	Memcpy(h, hdr, hsize);
-	TEST_SUCC(vb2_verify_keyblock(h, hsize + 1, &key, &wb),
-		  "vb2_verify_keyblock() size++");
+	TEST_SUCC(vb2_check_keyblock(h, hsize + 1, sig),
+		  "vb2_check_keyblock() size++");
 
 	Memcpy(h, hdr, hsize);
 	h->magic[0] &= 0x12;
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
-		VB2_ERROR_KEYBLOCK_MAGIC, "vb2_verify_keyblock() magic");
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_MAGIC, "vb2_check_keyblock() magic");
 
 	/* Care about major version but not minor */
 	Memcpy(h, hdr, hsize);
 	h->header_version_major++;
 	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
-		"vb2_verify_keyblock() major++");
+		"vb2_check_keyblock() major++");
 
 	Memcpy(h, hdr, hsize);
 	h->header_version_major--;
 	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
-		"vb2_verify_keyblock() major--");
+		"vb2_check_keyblock() major--");
 
 	Memcpy(h, hdr, hsize);
 	h->header_version_minor++;
 	resign_keyblock(h, private_key);
-	TEST_SUCC(vb2_verify_keyblock(h, hsize, &key, &wb),
-		  "vb2_verify_keyblock() minor++");
+	TEST_SUCC(vb2_check_keyblock(h, hsize, sig),
+		  "vb2_check_keyblock() minor++");
 
 	Memcpy(h, hdr, hsize);
 	h->header_version_minor--;
 	resign_keyblock(h, private_key);
-	TEST_SUCC(vb2_verify_keyblock(h, hsize, &key, &wb),
-		  "vb2_verify_keyblock() minor--");
+	TEST_SUCC(vb2_check_keyblock(h, hsize, sig),
+		  "vb2_check_keyblock() minor--");
 
 	/* Check signature */
 	Memcpy(h, hdr, hsize);
 	h->keyblock_signature.sig_offset = hsize;
 	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_SIG_OUTSIDE,
-		"vb2_verify_keyblock() sig off end");
-
-	Memcpy(h, hdr, hsize);
-	h->keyblock_signature.sig_size--;
-	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
-		VB2_ERROR_KEYBLOCK_SIG_INVALID,
-		"vb2_verify_keyblock() sig too small");
-
-	Memcpy(h, hdr, hsize);
-	((uint8_t *)vb2_packed_key_data(&h->data_key))[0] ^= 0x34;
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
-		VB2_ERROR_KEYBLOCK_SIG_INVALID,
-		"vb2_verify_keyblock() sig mismatch");
+		"vb2_check_keyblock() sig off end");
 
 	Memcpy(h, hdr, hsize);
 	h->keyblock_signature.data_size = h->keyblock_size + 1;
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_SIGNED_TOO_MUCH,
-		"vb2_verify_keyblock() sig data past end of block");
+		"vb2_check_keyblock() sig data past end of block");
 
 	/* Check that we signed header and data key */
 	Memcpy(h, hdr, hsize);
@@ -134,21 +118,80 @@ static void test_verify_keyblock(const VbPublicKey *public_key,
 	h->data_key.key_offset = 0;
 	h->data_key.key_size = 0;
 	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_SIGNED_TOO_LITTLE,
-		"vb2_verify_keyblock() didn't sign header");
+		"vb2_check_keyblock() didn't sign header");
 
 	Memcpy(h, hdr, hsize);
 	h->data_key.key_offset = hsize;
 	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
 		VB2_ERROR_KEYBLOCK_DATA_KEY_OUTSIDE,
-		"vb2_verify_keyblock() data key off end");
+		"vb2_check_keyblock() data key off end");
 
 	/* Corner cases for error checking */
-	TEST_EQ(vb2_verify_keyblock(NULL, 4, &key, &wb),
+	TEST_EQ(vb2_check_keyblock(NULL, 4, sig),
 		VB2_ERROR_KEYBLOCK_TOO_SMALL_FOR_HEADER,
-		"vb2_verify_keyblock size too small");
+		"vb2_check_keyblock size too small");
+
+	/*
+	 * TODO: verify parser can support a bigger header (i.e., one where
+	 * data_key.key_offset is bigger than expected).
+	 */
+
+	free(h);
+	free(hdr);
+}
+
+static void test_verify_keyblock(const VbPublicKey *public_key,
+				 const VbPrivateKey *private_key,
+				 const VbPublicKey *data_key)
+{
+	uint8_t workbuf[VB2_KEY_BLOCK_VERIFY_WORKBUF_BYTES]
+		__attribute__ ((aligned (VB2_WORKBUF_ALIGN)));
+	struct vb2_workbuf wb;
+	struct vb2_public_key key;
+	struct vb2_keyblock *hdr;
+	struct vb2_keyblock *h;
+	uint32_t hsize;
+
+	vb2_workbuf_init(&wb, workbuf, sizeof(workbuf));
+
+	/* Unpack public key */
+	TEST_SUCC(vb2_unpack_key(&key, (uint8_t *)public_key,
+				 public_key->key_offset + public_key->key_size),
+		  "vb2_verify_keyblock public key");
+
+	hdr = (struct vb2_keyblock *)
+		KeyBlockCreate(data_key, private_key, 0x1234);
+	TEST_NEQ((size_t)hdr, 0, "vb2_verify_keyblock() prerequisites");
+	if (!hdr)
+		return;
+	hsize = hdr->keyblock_size;
+	h = (struct vb2_keyblock *)malloc(hsize + 2048);
+
+	Memcpy(h, hdr, hsize);
+	TEST_SUCC(vb2_verify_keyblock(h, hsize, &key, &wb),
+		  "vb2_verify_keyblock() ok using key");
+
+	/* Failures in keyblock check also cause verify to fail */
+	Memcpy(h, hdr, hsize);
+	TEST_EQ(vb2_verify_keyblock(h, hsize - 1, &key, &wb),
+		VB2_ERROR_KEYBLOCK_SIZE, "vb2_verify_keyblock() check");
+
+	/* Check signature */
+	Memcpy(h, hdr, hsize);
+	h->keyblock_signature.sig_size--;
+	resign_keyblock(h, private_key);
+	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+		VB2_ERROR_KEYBLOCK_SIG_INVALID,
+		"vb2_verify_keyblock() sig too small");
+
+	Memcpy(h, hdr, hsize);
+	((uint8_t *)vb2_packed_key_data(&h->data_key))[0] ^= 0x34;
+	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
+		VB2_ERROR_KEYBLOCK_SIG_INVALID,
+		"vb2_verify_keyblock() sig mismatch");
 
 	/*
 	 * TODO: verify parser can support a bigger header (i.e., one where
@@ -524,6 +567,8 @@ int test_permutation(int signing_key_algorithm, int data_key_algorithm,
 		return 1;
 	}
 
+	test_check_keyblock(signing_public_key, signing_private_key,
+			    data_public_key);
 	test_verify_keyblock(signing_public_key, signing_private_key,
 			     data_public_key);
 	test_verify_fw_preamble(signing_public_key, signing_private_key,