CR50: add a constant time buffer equals implementation

Various cryptographic operations leak timing
information if comparisons are not executed
in constant time.  This change adds DCRYPTO_equals(),
a constant runtime comparator.

Also replace crypto related callsites that used
memcmp() as a binary comparator.

BUG=none
BRANCH=none
TEST=tcg tests pass

Change-Id: I3d3da3c0524c3a349d60675902d1f2d338ad455f
Signed-off-by: nagendra modadugu <ngm@google.com>
Reviewed-on: https://chromium-review.googlesource.com/410163
Commit-Ready: Nagendra Modadugu <ngm@google.com>
Tested-by: Nagendra Modadugu <ngm@google.com>
Reviewed-by: Marius Schilder <mschilder@chromium.org>
Reviewed-by: Bill Richardson <wfrichar@chromium.org>

9 files changed, 49 insertions, 18 deletions

diff --git a/board/cr50/tpm2/NVMem.c b/board/cr50/tpm2/NVMem.c
index f78308033c..7874a89e86 100644
--- a/board/cr50/tpm2/NVMem.c
+++ b/board/cr50/tpm2/NVMem.c
@@ -135,8 +135,7 @@ _plat__NvIsDifferent(unsigned int startOffset,
 #ifdef CONFIG_FLASH_NVMEM
 	return (nvmem_is_different(startOffset, size, data, NVMEM_TPM) != 0);
 #else
-	/* Do we need a safe memcmp here? */
-	return (memcmp(&s_NV[startOffset], data, size) != 0);
+	return !DCRYPTO_equals(&s_NV[startOffset], data, size);
 #endif
 }
 
diff --git a/board/cr50/tpm2/ecc.c b/board/cr50/tpm2/ecc.c
index 9f61ab86a9..0cb8493b72 100644
--- a/board/cr50/tpm2/ecc.c
+++ b/board/cr50/tpm2/ecc.c
@@ -394,11 +394,18 @@ static const struct TPM2B_ECC_PARAMETER_aligned NIST_P256_qy = {
 
 static int point_equals(const TPMS_ECC_POINT *a, const TPMS_ECC_POINT *b)
 {
-	return a->x.b.size == b->x.b.size &&
-		a->y.b.size == b->y.b.size &&
-		memcmp(a->x.b.buffer, b->x.b.buffer, a->x.b.size) == 0 &&
-		memcmp(a->y.b.buffer, b->y.b.buffer, a->y.b.size) == 0;
+	int diff = 0;
+
+	diff = a->x.b.size != b->x.b.size;
+	diff |= a->y.b.size != b->y.b.size;
+	if (!diff) {
+		diff |= !DCRYPTO_equals(
+			a->x.b.buffer, b->x.b.buffer, a->x.b.size);
+		diff |= !DCRYPTO_equals(
+			a->y.b.buffer, b->y.b.buffer, a->y.b.size);
+	}
 
+	return !diff;
 }
 
 static void ecc_command_handler(void *cmd_body, size_t cmd_size,
diff --git a/board/cr50/tpm2/endorsement.c b/board/cr50/tpm2/endorsement.c
index ff80327e9b..3b4f0d99c6 100644
--- a/board/cr50/tpm2/endorsement.c
+++ b/board/cr50/tpm2/endorsement.c
@@ -688,8 +688,8 @@ int tpm_endorse(void)
 		HASH_update(&hmac.hash, "RSA", 4);
 		DCRYPTO_HMAC_SHA256_init(&hmac, DCRYPTO_HMAC_final(&hmac), 32);
 		HASH_update(&hmac.hash, p, RO_CERTS_REGION_SIZE - 32);
-		if (memcmp(p + RO_CERTS_REGION_SIZE - 32,
-				DCRYPTO_HMAC_final(&hmac), 32) != 0) {
+		if (!DCRYPTO_equals(p + RO_CERTS_REGION_SIZE - 32,
+				   DCRYPTO_HMAC_final(&hmac), 32)) {
 			CPRINTF("%s: bad cert region hmac; falling back\n"
 				"    to fixed endorsement\n", __func__);
 
diff --git a/board/cr50/tpm2/rsa.c b/board/cr50/tpm2/rsa.c
index 290ad9cfe5..8b156feb75 100644
--- a/board/cr50/tpm2/rsa.c
+++ b/board/cr50/tpm2/rsa.c
@@ -363,8 +363,8 @@ CRYPT_RESULT _cpri__GenerateKeyRSA(
 	 * template instead.
 	 */
 	if (extra->size == sizeof(TPM2_RSA_EK_NAME_TEMPLATE) &&
-		memcmp(extra->buffer, TPM2_RSA_EK_NAME_TEMPLATE,
-			sizeof(TPM2_RSA_EK_NAME_TEMPLATE)) == 0 &&
+		DCRYPTO_equals(extra->buffer, TPM2_RSA_EK_NAME_TEMPLATE,
+			       sizeof(TPM2_RSA_EK_NAME_TEMPLATE)) &&
 		seed == &endorsement_seed->b) {
 		memcpy(local_extra.b.buffer, TPM2_RSA_EK_NAME_CR50,
 			sizeof(TPM2_RSA_EK_NAME_CR50));
@@ -376,7 +376,7 @@ CRYPT_RESULT _cpri__GenerateKeyRSA(
 	 */
 #ifdef CRYPTO_TEST_SETUP
 	if (seed->size == sizeof(VERIFY_SEED) &&
-	    memcmp(seed->buffer, VERIFY_SEED, seed->size) == 0) {
+		DCRYPTO_equals(seed->buffer, VERIFY_SEED, seed->size)) {
 		/* Test seed has already been hashed down. */
 		memcpy(local_seed.t.buffer, seed->buffer, seed->size);
 	} else
diff --git a/chip/g/build.mk b/chip/g/build.mk
index 5c31358439..bb0573958b 100644
--- a/chip/g/build.mk
+++ b/chip/g/build.mk
@@ -31,6 +31,7 @@ endif
 chip-$(CONFIG_DCRYPTO)+= dcrypto/aes.o
 chip-$(CONFIG_DCRYPTO)+= dcrypto/bn.o
 chip-$(CONFIG_DCRYPTO)+= dcrypto/bn_hw.o
+chip-$(CONFIG_DCRYPTO)+= dcrypto/compare.o
 chip-$(CONFIG_DCRYPTO)+= dcrypto/dcrypto_runtime.o
 chip-$(CONFIG_DCRYPTO)+= dcrypto/hkdf.o
 chip-$(CONFIG_DCRYPTO)+= dcrypto/hmac.o
diff --git a/chip/g/dcrypto/compare.c b/chip/g/dcrypto/compare.c
new file mode 100644
index 0000000000..db6193752b
--- /dev/null
+++ b/chip/g/dcrypto/compare.c
@@ -0,0 +1,20 @@
+/* Copyright 2016 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "dcrypto.h"
+
+/* Constant time comparator. */
+int DCRYPTO_equals(const void *a, const void *b, size_t len)
+{
+	size_t i;
+	const uint8_t *pa = a;
+	const uint8_t *pb = b;
+	uint8_t diff = 0;
+
+	for (i = 0; i < len; i++)
+		diff |= pa[i] ^ pb[i];
+
+	return !diff;
+}
diff --git a/chip/g/dcrypto/dcrypto.h b/chip/g/dcrypto/dcrypto.h
index 4f4c2df5ff..cfe24ff0f6 100644
--- a/chip/g/dcrypto/dcrypto.h
+++ b/chip/g/dcrypto/dcrypto.h
@@ -179,4 +179,9 @@ int DCRYPTO_bn_div(struct LITE_BIGNUM *quotient, struct LITE_BIGNUM *remainder,
 int DCRYPTO_x509_verify(const uint8_t *cert, size_t len,
 			const struct RSA *ca_pub_key);
 
+/*
+ * Memory related functions.
+ */
+int DCRYPTO_equals(const void *a, const void *b, size_t len);
+
 #endif  /* ! __EC_CHIP_G_DCRYPTO_DCRYPTO_H */
diff --git a/chip/g/dcrypto/p256_ecies.c b/chip/g/dcrypto/p256_ecies.c
index 8272014495..8164ff87db 100644
--- a/chip/g/dcrypto/p256_ecies.c
+++ b/chip/g/dcrypto/p256_ecies.c
@@ -161,9 +161,8 @@ size_t DCRYPTO_ecies_decrypt(
 	hmac_key = &key[AES_KEY_BYTES];
 	DCRYPTO_HMAC_SHA256_init(&ctx, hmac_key, HMAC_KEY_BYTES);
 	HASH_update(&ctx.hash, inp, in_len);
-	/* TODO(ngm): replace with constant time verify. */
-	if (memcmp(inp + in_len, DCRYPTO_HMAC_final(&ctx),
-			SHA256_DIGEST_SIZE) != 0)
+	if (!DCRYPTO_equals(inp + in_len, DCRYPTO_HMAC_final(&ctx),
+				SHA256_DIGEST_SIZE))
 		return 0;
 
 	memmove(outp, inp, auth_data_len);
diff --git a/chip/g/dcrypto/rsa.c b/chip/g/dcrypto/rsa.c
index ace9961169..45ec414dc7 100644
--- a/chip/g/dcrypto/rsa.c
+++ b/chip/g/dcrypto/rsa.c
@@ -137,7 +137,7 @@ static int check_oaep_pad(uint8_t *out, uint32_t *out_len,
 		DCRYPTO_SHA256_init(&ctx, 0);
 	HASH_update(&ctx, label, label ? strlen(label) + 1 : 0);
 
-	bad = memcmp(phash, HASH_final(&ctx), hash_size);
+	bad = !DCRYPTO_equals(phash, HASH_final(&ctx), hash_size);
 	bad |= padded[0];
 
 	for (i = PS - padded; i <  padded_len; i++) {
@@ -295,10 +295,10 @@ static int check_pkcs1_type1_pad(const uint8_t *msg, uint32_t msg_len,
 
 	if (padded[i++] != 0)
 		return 0;
-	if (memcmp(&padded[i], der, der_size) != 0)
+	if (!DCRYPTO_equals(&padded[i], der, der_size))
 		return 0;
 	i += der_size;
-	return memcmp(msg, &padded[i], hash_size) == 0;
+	return DCRYPTO_equals(msg, &padded[i], hash_size);
 }
 
 /* sign */
@@ -398,7 +398,7 @@ static int check_pkcs1_pss_pad(const uint8_t *in, uint32_t in_len,
 	HASH_update(&ctx, zeros, sizeof(zeros));
 	HASH_update(&ctx, in, in_len);
 	HASH_update(&ctx, padded + db_len - salt_len, salt_len);
-	bad |= memcmp(padded + db_len, HASH_final(&ctx), hash_size);
+	bad |= !DCRYPTO_equals(padded + db_len, HASH_final(&ctx), hash_size);
 	return !bad;
 }