lib20/misc: Small robustness improvements to vb2_load_fw_keyblock()

This patch fixes an issue discovered while fuzzing
vb2_load_fw_keyblock(): the data key contained in the keyblock is not
sanity-checked before moving it around on the work buffer, resulting in
a potential overflow if it's key_size flows over the end of the
keyblock. This is not exploitable since the keyblock was already
verified, so only signed (=trusted) keyblocks can get to this stage, but
there's nothing wrong with double-checking anyway.

This patch also rewrites the data_key moving code a bit to just move the
whole key rather than individually copying the header elements and then
just memmove()ing the data (and keeping the previous key_offset from the
root key rather than the one from the data key). None of these issues
affect correctness but it seems simpler and cleaner to me this way.

Finally, remove an instance where the keyblock was accessed after the
memmove(). This would be bad if the data key was so much larger than the
keyblock that memmove()ing it overwrites the keyblock header. Like an
existing comment points out, that doesn't happen with the key sizes we
choose in practice, but it's still better to not rely on that.

BRANCH=none
BUG=chromium:1017793
TEST=make runtests and reran failing fuzz testcase

Change-Id: I78ded43ad999e0883a69cbb2ea7e876888a9fa22
Signed-off-by: Julius Werner <jwerner@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1880015
Reviewed-by: Joel Kitching <kitching@chromium.org>
(cherry picked from commit 2cc38ec1b74cf7db7357fc177d2be813cca9fe06)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/1918003
Reviewed-by: Shelley Chen <shchen@chromium.org>
Commit-Queue: Shelley Chen <shchen@chromium.org>
Tested-by: Shelley Chen <shchen@chromium.org>

1 files changed, 17 insertions, 19 deletions

diff --git a/firmware/lib20/misc.c b/firmware/lib20/misc.c
index 0fa80e1d..c81c3fed 100644
--- a/firmware/lib20/misc.c
+++ b/firmware/lib20/misc.c
@@ -73,7 +73,6 @@ vb2_error_t vb2_load_fw_keyblock(struct vb2_context *ctx)
 
 	uint8_t *key_data;
 	uint32_t key_size;
-	struct vb2_packed_key *packed_key;
 	struct vb2_public_key root_key;
 
 	struct vb2_keyblock *kb;
@@ -150,34 +149,33 @@ vb2_error_t vb2_load_fw_keyblock(struct vb2_context *ctx)
 
 	sd->fw_version = kb->data_key.key_version << 16;
 
+	/* Preamble follows the keyblock in the vblock. */
+	sd->vblock_preamble_offset = kb->keyblock_size;
+
 	/*
-	 * Save the data key in the work buffer.  This overwrites the root key
+	 * Save the data key in the work buffer.  We'll overwrite the root key
 	 * we read above.  That's ok, because now that we have the data key we
-	 * no longer need the root key.
+	 * no longer need the root key.  First, let's double-check that it is
+	 * well-formed though (although the keyblock was signed anyway).
 	 */
-	packed_key = (struct vb2_packed_key *)key_data;
+	rv = vb2_verify_packed_key_inside(kb, block_size, &kb->data_key);
+	if (rv)
+		return rv;
 
-	packed_key->algorithm = kb->data_key.algorithm;
-	packed_key->key_version = kb->data_key.key_version;
-	packed_key->key_size = kb->data_key.key_size;
+	/* Save the future offset and size while kb->data_key is still valid.
+	   The check above made sure that key_offset and key_size are sane. */
+	sd->data_key_offset = vb2_offset_of(sd, key_data);
+	sd->data_key_size = kb->data_key.key_offset + kb->data_key.key_size;
 
 	/*
 	 * Use memmove() instead of memcpy().  In theory, the destination will
 	 * never overlap because with the source because the root key is likely
 	 * to be at least as large as the data key, but there's no harm here in
-	 * being paranoid.
+	 * being paranoid.  Make sure we immediately invalidate 'kb' after the
+	 * move to guarantee we won't try to access it anymore.
 	 */
-	memmove(key_data + packed_key->key_offset,
-		(uint8_t*)&kb->data_key + kb->data_key.key_offset,
-		packed_key->key_size);
-
-	/* Save the packed key offset and size */
-	sd->data_key_offset = vb2_offset_of(sd, key_data);
-	sd->data_key_size =
-		packed_key->key_offset + packed_key->key_size;
-
-	/* Preamble follows the keyblock in the vblock */
-	sd->vblock_preamble_offset = kb->keyblock_size;
+	memmove(key_data, &kb->data_key, sd->data_key_size);
+	kb = NULL;
 
 	/*
 	 * Data key will persist in the workbuf after we return.