ec: Update U2F_ATTEST function for new-style key handles.

The existing implementation of this function was intended for
old-style key handles, and needs to be replaced with a new
implementation for new-style key handles that incorporate
user secrets.

For bonus points, the existing implementatation is actually
broken, and performs no verification, so will blindly attest
to anything, which is Not Good.

BUG=b:124237003
BRANCH=none
TEST=test_that firmware_Cr50U2fCommands

Change-Id: I9b4a487707acf81da39e6495adb42e277f2fdb4a
Signed-off-by: Louis Collard <louiscollard@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1475102
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Marius Schilder <mschilder@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

2 files changed, 36 insertions, 24 deletions

diff --git a/common/u2f.c b/common/u2f.c
index 1fdd3e44ce..f23a8b968a 100644
--- a/common/u2f.c
+++ b/common/u2f.c
@@ -450,6 +450,25 @@ static enum vendor_cmd_rc u2f_generate(enum vendor_cmd_cc code,
 }
 DECLARE_VENDOR_COMMAND(VENDOR_CC_U2F_GENERATE, u2f_generate);
 
+static int verify_kh_owned(const uint8_t *user_secret, const uint8_t *app_id,
+			   const uint8_t *key_handle)
+{
+	/* Re-created key handle. */
+	uint8_t recreated_kh[KH_LEN];
+
+	/*
+	 * Re-create the key handle and compare against that which
+	 * was provided. This allows us to verify that the key handle
+	 * is owned by this combination of device, current user and app_id.
+	 */
+	u2f_origin_user_keyhandle(app_id,
+				  user_secret,
+				  key_handle,
+				  recreated_kh);
+
+	return safe_memcmp(recreated_kh, key_handle, KH_LEN) == 0;
+}
+
 /* U2F SIGN command */
 static enum vendor_cmd_rc u2f_sign(enum vendor_cmd_cc code,
 				   void *buf,
@@ -459,9 +478,6 @@ static enum vendor_cmd_rc u2f_sign(enum vendor_cmd_cc code,
 	const U2F_SIGN_REQ *req = buf;
 	U2F_SIGN_RESP *resp;
 
-	/* Re-created key handle. */
-	uint8_t recreated_kh[KH_LEN];
-
 	struct drbg_ctx ctx;
 
 	/* Origin private key. */
@@ -473,23 +489,13 @@ static enum vendor_cmd_rc u2f_sign(enum vendor_cmd_cc code,
 	if (input_size != sizeof(U2F_SIGN_REQ))
 		return VENDOR_RC_BOGUS_ARGS;
 
-	/*
-	 * Re-create the key handle and compare against that which
-	 * was provided. This allows us to verify that the key handle
-	 * is owned by the current user and appId.
-	 */
-	u2f_origin_user_keyhandle(req->appId,
-				  req->userSecret,
-				  req->keyHandle,
-				  recreated_kh);
-
-	if (memcmp(recreated_kh, req->keyHandle, KH_LEN) != 0)
-		return VENDOR_RC_NOT_ALLOWED;
-
 	/* Always enforce user presence, with optional consume. */
 	if (pop_check_presence(req->flags & G2F_CONSUME) != POP_TOUCH_YES)
 		return VENDOR_RC_NOT_ALLOWED;
 
+	if (!verify_kh_owned(req->userSecret, req->appId, req->keyHandle))
+		return VENDOR_RC_NOT_ALLOWED;
+
 	/* Re-create origin-specific key. */
 	if (u2f_origin_user_keypair(
 		req->keyHandle, &origin_d, NULL, NULL) != EC_SUCCESS)
@@ -529,29 +535,29 @@ struct G2F_REGISTER_MSG {
 	U2F_EC_POINT public_key;
 };
 
-static inline int u2f_attest_verify_reg_resp(uint8_t data_size,
+static inline int u2f_attest_verify_reg_resp(const uint8_t *user_secret,
+					     uint8_t data_size,
 					     const uint8_t *data)
 {
 	struct G2F_REGISTER_MSG *msg = (void *) data;
-	uint8_t unwrapped_kh[KH_LEN];
 
 	if (data_size != sizeof(struct G2F_REGISTER_MSG))
 		return VENDOR_RC_NOT_ALLOWED;
 
-	/* Check if we can decrypt keyhandle. */
-	if (wrap_kh(NULL, msg->key_handle, unwrapped_kh, DECRYPT_MODE))
+	if (!verify_kh_owned(user_secret, msg->app_id, msg->key_handle))
 		return VENDOR_RC_NOT_ALLOWED;
 
 	return VENDOR_RC_SUCCESS;
 }
 
-static int u2f_attest_verify(uint8_t format,
+static int u2f_attest_verify(const uint8_t *user_secret,
+			     uint8_t format,
 			     uint8_t data_size,
 			     const uint8_t *data)
 {
 	switch (format) {
 	case U2F_ATTEST_FORMAT_REG_RESP:
-		return u2f_attest_verify_reg_resp(data_size, data);
+		return u2f_attest_verify_reg_resp(user_secret, data_size, data);
 	default:
 		return VENDOR_RC_NOT_ALLOWED;
 	}
@@ -587,10 +593,15 @@ static enum vendor_cmd_rc u2f_attest(enum vendor_cmd_cc code,
 	/* Attestation key */
 	p256_int d, pk_x, pk_y;
 
-	if (input_size != sizeof(U2F_ATTEST_REQ))
+	if (input_size < 2 ||
+	    input_size < (2 + req->dataLen) ||
+	    input_size > sizeof(U2F_ATTEST_REQ) ||
+	    *response_size < sizeof(*resp))
 		return VENDOR_RC_BOGUS_ARGS;
 
-	verify_ret = u2f_attest_verify(req->format, req->dataLen,
+	verify_ret = u2f_attest_verify(req->userSecret,
+				       req->format,
+				       req->dataLen,
 				       req->data);
 
 	if (verify_ret != VENDOR_RC_SUCCESS)
diff --git a/include/u2f.h b/include/u2f.h
index b2749c7eea..003f047175 100644
--- a/include/u2f.h
+++ b/include/u2f.h
@@ -121,6 +121,7 @@ typedef struct {
 } U2F_SIGN_RESP;
 
 typedef struct {
+    uint8_t userSecret[U2F_P256_SIZE];
     uint8_t format;
     uint8_t dataLen;
     uint8_t data[U2F_MAX_ATTEST_SIZE];