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>
diff --git a/Makefile b/Makefile
index 0e96c5c..17881b7 100644
--- a/Makefile
+++ b/Makefile
@@ -1444,7 +1444,7 @@
 .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 55c59ea..bee9328 100644
--- a/firmware/lib20/api.c
+++ b/firmware/lib20/api.c
@@ -77,6 +77,14 @@
 	}
 
 	/*
+	 * 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 cee6cb4..9122114 100644
--- a/firmware/lib20/common.c
+++ b/firmware/lib20/common.c
@@ -130,35 +130,30 @@
 	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 @@
 		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 @@
 		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 a71cfe6..be891ca 100644
--- a/firmware/lib20/include/vb2_common.h
+++ b/firmware/lib20/include/vb2_common.h
@@ -124,7 +124,23 @@
 		    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 d3e2f40..1e39061 100644
--- a/firmware/lib20/include/vb2_struct.h
+++ b/firmware/lib20/include/vb2_struct.h
@@ -97,12 +97,12 @@
 	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 83232b0..13004c6 100644
--- a/firmware/lib20/misc.c
+++ b/firmware/lib20/misc.c
@@ -25,6 +25,13 @@
 	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 @@
 	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 @@
 		return rv;
 	}
 
-	vb2_report_dev_firmware(&root_key);
 	sd->fw_version = kb->data_key.key_version << 16;
 
 	/*
@@ -169,7 +178,13 @@
 	/* 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 @@
 	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 b41300c..0e135bd 100644
--- a/tests/vb20_common3_tests.c
+++ b/tests/vb20_common3_tests.c
@@ -29,6 +29,120 @@
 	free(sig);
 }
 
+static void test_check_keyblock(const VbPublicKey *public_key,
+				 const VbPrivateKey *private_key,
+				 const VbPublicKey *data_key)
+{
+	struct vb2_public_key key;
+	struct vb2_keyblock *hdr;
+	struct vb2_keyblock *h;
+	struct vb2_signature *sig;
+	uint32_t hsize;
+
+	/* 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);
+	sig = &h->keyblock_signature;
+
+	Memcpy(h, hdr, hsize);
+	TEST_SUCC(vb2_check_keyblock(h, hsize, sig),
+		  "vb2_check_keyblock() ok");
+
+	Memcpy(h, hdr, hsize);
+	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_check_keyblock(h, hsize + 1, sig),
+		  "vb2_check_keyblock() size++");
+
+	Memcpy(h, hdr, hsize);
+	h->magic[0] &= 0x12;
+	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_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
+		"vb2_check_keyblock() major++");
+
+	Memcpy(h, hdr, hsize);
+	h->header_version_major--;
+	resign_keyblock(h, private_key);
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
+		"vb2_check_keyblock() major--");
+
+	Memcpy(h, hdr, hsize);
+	h->header_version_minor++;
+	resign_keyblock(h, private_key);
+	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_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_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_SIG_OUTSIDE,
+		"vb2_check_keyblock() sig off end");
+
+	Memcpy(h, hdr, hsize);
+	h->keyblock_signature.data_size = h->keyblock_size + 1;
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_SIGNED_TOO_MUCH,
+		"vb2_check_keyblock() sig data past end of block");
+
+	/* Check that we signed header and data key */
+	Memcpy(h, hdr, hsize);
+	h->keyblock_signature.data_size = 4;
+	h->data_key.key_offset = 0;
+	h->data_key.key_size = 0;
+	resign_keyblock(h, private_key);
+	TEST_EQ(vb2_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_SIGNED_TOO_LITTLE,
+		"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_check_keyblock(h, hsize, sig),
+		VB2_ERROR_KEYBLOCK_DATA_KEY_OUTSIDE,
+		"vb2_check_keyblock() data key off end");
+
+	/* Corner cases for error checking */
+	TEST_EQ(vb2_check_keyblock(NULL, 4, sig),
+		VB2_ERROR_KEYBLOCK_TOO_SMALL_FOR_HEADER,
+		"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)
@@ -60,56 +174,13 @@
 	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() 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++");
-
-	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");
-
-	/* 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),
-		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
-		"vb2_verify_keyblock() major++");
-
-	Memcpy(h, hdr, hsize);
-	h->header_version_major--;
-	resign_keyblock(h, private_key);
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
-		VB2_ERROR_KEYBLOCK_HEADER_VERSION,
-		"vb2_verify_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++");
-
-	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--");
+		VB2_ERROR_KEYBLOCK_SIZE, "vb2_verify_keyblock() check");
 
 	/* 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),
-		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),
@@ -122,34 +193,6 @@
 		VB2_ERROR_KEYBLOCK_SIG_INVALID,
 		"vb2_verify_keyblock() sig mismatch");
 
-	Memcpy(h, hdr, hsize);
-	h->keyblock_signature.data_size = h->keyblock_size + 1;
-	TEST_EQ(vb2_verify_keyblock(h, hsize, &key, &wb),
-		VB2_ERROR_KEYBLOCK_SIGNED_TOO_MUCH,
-		"vb2_verify_keyblock() sig data past end of block");
-
-	/* Check that we signed header and data key */
-	Memcpy(h, hdr, hsize);
-	h->keyblock_signature.data_size = 4;
-	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),
-		VB2_ERROR_KEYBLOCK_SIGNED_TOO_LITTLE,
-		"vb2_verify_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),
-		VB2_ERROR_KEYBLOCK_DATA_KEY_OUTSIDE,
-		"vb2_verify_keyblock() data key off end");
-
-	/* Corner cases for error checking */
-	TEST_EQ(vb2_verify_keyblock(NULL, 4, &key, &wb),
-		VB2_ERROR_KEYBLOCK_TOO_SMALL_FOR_HEADER,
-		"vb2_verify_keyblock size too small");
-
 	/*
 	 * TODO: verify parser can support a bigger header (i.e., one where
 	 * data_key.key_offset is bigger than expected).
@@ -524,6 +567,8 @@
 		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,