cr50: update AES-CMAC implementation

Cr50 doesn't use CMAC, it is not even compiled, however during internal
review potential issues with branching on key values were spotted.

1) Fix key expansion to be constant time
2) Switch to enum dcrypto_result
3) Test commands updated to be compatible with FIPS build (use .rodata)
4) Clean up computed tag on stack during verification

BUG=None
TEST=make BOARD=cr50 CRYPTO_TEST=1 CMAC_TEST=1
in ccd: test_cmac 1 2 3 4
test_cmac_ver 1 2 3 4

Signed-off-by: Vadim Sukhomlinov <sukhomlinov@google.com>
Change-Id: Iff9b84dd8fb2baed9152f1ee5c40ef8e4198edd3
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3194972
Reviewed-by: Vadim Sukhomlinov <sukhomlinov@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>
Tested-by: Vadim Sukhomlinov <sukhomlinov@chromium.org>
Commit-Queue: Vadim Sukhomlinov <sukhomlinov@chromium.org>

3 files changed, 135 insertions, 118 deletions

diff --git a/board/cr50/build.mk b/board/cr50/build.mk
index f644a2c892..3f1c40f9c6 100644
--- a/board/cr50/build.mk
+++ b/board/cr50/build.mk
@@ -24,7 +24,7 @@ ifeq ($(BOARD_MK_INCLUDED_ONCE),)
 # command line.
 ENV_VARS := CR50_DEV CRYPTO_TEST H1_RED_BOARD U2F_TEST RND_TEST DRBG_TEST\
 	    ECDSA_TEST DCRYPTO_TEST P256_BIN_TEST SHA1_TEST SHA256_TEST\
-	    HMAC_SHA256_TEST
+	    HMAC_SHA256_TEST CMAC_TEST
 
 ifneq ($(CRYPTO_TEST),)
 CPPFLAGS += -DCRYPTO_TEST_SETUP
@@ -114,6 +114,9 @@ fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/dcrypto_p256.o
 fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/compare.o
 fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/dcrypto_runtime.o
 ifneq ($(CRYPTO_TEST),)
+ifneq ($(CMAC_TEST),)
+fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/aes_cmac.o
+endif
 fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/gcm.o
 fips-${CONFIG_DCRYPTO_BOARD} += dcrypto/hkdf.o
 endif
diff --git a/board/cr50/dcrypto/aes_cmac.c b/board/cr50/dcrypto/aes_cmac.c
index a921bc589b..d80d3c872e 100644
--- a/board/cr50/dcrypto/aes_cmac.c
+++ b/board/cr50/dcrypto/aes_cmac.c
@@ -4,38 +4,25 @@
  */
 
 /* AES-CMAC-128 implementation according to NIST SP 800-38B, RFC4493 */
-#include "console.h"
-#include "dcrypto.h"
+#include "internal.h"
 
 #define BSIZE 16 /* 16 bytes per 128-bit block */
 
-/* Given a 128-bit number in 32-bit chunks, shift to the left by one */
-static void shiftl_1(const uint8_t *in, uint8_t *out)
-{
-	int i;
-	uint8_t carry = 0;
-
-	for (i = 15; i >= 0; i--) {
-		out[i] = in[i] << 1;
-		out[i] |= carry;
-		carry = (in[i] & 0x80) ? 1 : 0;
-	}
-}
 
 static void xor128(const uint32_t in1[4], const uint32_t in2[4],
 		uint32_t out[4])
 {
-	int i;
+	size_t i;
 
 	for (i = 0; i < 4; i++)
 		out[i] = in1[i] ^ in2[i];
 }
 
-static void get_and_xor(const uint8_t *arr, const uint32_t nBytes, int i,
+static void get_and_xor(const uint8_t *arr, const size_t nBytes, size_t i,
 			const uint8_t *xor_term, uint8_t *out)
 {
-	int j;
-	int k;
+	size_t j;
+	size_t k;
 
 	for (j = 0; j < 16; j++) {
 		k = i*16 + j; /* index in arr */
@@ -49,44 +36,66 @@ static void get_and_xor(const uint8_t *arr, const uint32_t nBytes, int i,
 	}
 }
 
+static const uint32_t zero[4] = {0, 0, 0, 0};
+
 /* Wrapper for initializing and calling AES-128 */
-static int aes128(const uint8_t *K, const uint32_t in[4], uint32_t out[4])
+static enum dcrypto_result aes128(const uint8_t *K, const uint32_t in[4],
+				  uint32_t out[4])
 {
 	const uint32_t zero[4] = {0, 0, 0, 0};
 
 	if (DCRYPTO_aes_init((const uint8_t *)K, 128, (const uint8_t *)zero,
 			     CIPHER_MODE_ECB, ENCRYPT_MODE) != DCRYPTO_OK)
-		return 0;
+		return DCRYPTO_FAIL;
 	if (DCRYPTO_aes_block((const uint8_t *)in, (uint8_t *)out) !=
 	    DCRYPTO_OK)
-		return 0;
-	return 1;
+		return DCRYPTO_FAIL;
+	return DCRYPTO_OK;
 }
 
-static int gen_subkey(const uint8_t *K, uint32_t k1[4], uint32_t k2[4])
+static void compute_subkey(uint32_t out_words[4], const uint32_t in_words[4])
 {
-	uint32_t L[4];
-	uint32_t tmp[4];
-	const uint32_t *xor_term;
-	static const uint32_t zero[4] = {0, 0, 0, 0};
-	static const uint32_t Rb[4] = {0, 0, 0, 0x87000000};
+	const uint32_t msb = (in_words[0] >> 7) & 1;
 
-	if (!aes128(K, zero, L))
-		return 0;
+	/**
+	 * Shift left by 1 bit without branches. The counter is big-endian,
+	 * while process it on little-endian arch.
+	 */
+	for (size_t index = 0; index < 3; ++index) {
+		/* bit 7 in each byte will become a bit zero in next byte. */
+		out_words[index] = (in_words[index] & 0x80808080) >> 15;
+		/* shift bits left and clear overflows, then combine. */
+		out_words[index] |= (in_words[index] << 1) & 0xfefefefe;
+		/**
+		 * and add highest bit of next word which will be in lowest
+		 * byte of next word.
+		 */
+		out_words[index] |= ((in_words[index + 1] & 0x80) << 17);
+	}
+	out_words[3] = (in_words[3] & 0x80808080) >> 15;
+	out_words[3] |= (in_words[3] << 1) & 0xfefefefe;
+	out_words[3] ^= ((0 - msb) & 0x87) << 24;
+}
+
+static enum dcrypto_result gen_subkey(const uint8_t *K, uint32_t k1[4],
+				      uint32_t k2[4])
+{
+	uint32_t L[4];
+	enum dcrypto_result result;
 
-	xor_term = (L[0] & 0x00000080) ? Rb : zero;
-	shiftl_1((const uint8_t *)L, (uint8_t *)tmp);
-	xor128(tmp, xor_term, k1);
+	result = aes128(K, zero, L);
+	if (result != DCRYPTO_OK)
+		return result;
 
-	xor_term = (k1[0] & 0x00000080) ? Rb : zero;
-	shiftl_1((const uint8_t *) k1, (uint8_t *) tmp);
-	xor128(tmp, xor_term, k2);
+	compute_subkey(k1, L);
+	compute_subkey(k2, k1);
 
-	return 1;
+	/* result should be DCRYPTO_OK at this point. */
+	return result;
 }
 
-int DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M, const uint32_t len,
-		uint32_t T[4])
+enum dcrypto_result DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M,
+				     size_t len, uint32_t T[4])
 {
 	uint32_t n;
 	int i;
@@ -96,10 +105,12 @@ int DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M, const uint32_t len,
 	uint32_t M_last[4];
 	uint32_t Y[4];
 	uint32_t X[4] = {0, 0, 0, 0};
+	enum dcrypto_result result;
 
 	/* Generate the subkeys K1 and K2 */
-	if (!gen_subkey(K, k1, k2))
-		return 0;
+	result = gen_subkey(K, k1, k2);
+	if (result != DCRYPTO_OK)
+		return result;
 
 	/* Set n and flag.
 	 * flag = 1 if the last block has a full 128 bits; 0 otherwise
@@ -115,14 +126,15 @@ int DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M, const uint32_t len,
 	}
 
 	/* M_last = padded(last 128-bit block of M) ^ (flag ? k1 : k2) */
-	get_and_xor(M, len, n-1, (uint8_t *) (flag ? k1 : k2),
-			(uint8_t *) M_last);
+	get_and_xor(M, len, n - 1, (uint8_t *)(flag ? k1 : k2),
+		    (uint8_t *)M_last);
 
-	for (i = 0; i < n - 1; i++) {
+	for (i = 0; i < (int)n - 1; i++) {
 		/* Y = padded(nth 128-bit block of M) ^ (flag ? k1 : k2) */
 		get_and_xor(M, len, i, (uint8_t *)X, (uint8_t *)Y);
-		if (!aes128(K, Y, X))
-			return 0;
+		result = aes128(K, Y, X);
+		if (result != DCRYPTO_OK)
+			return result;
 	}
 
 	/* TODO: This block is separate from the main loop in the RFC. However,
@@ -130,29 +142,31 @@ int DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M, const uint32_t len,
 	 * for one more step, which might be a nicer way to write it.
 	 */
 	xor128(X, M_last, Y);
-	if (!aes128(K, Y, T))
-		return 0;
-	return 1;
+	return aes128(K, Y, T);
 }
 
-int DCRYPTO_aes_cmac_verify(const uint8_t *key, const uint8_t *M, const int len,
-		const uint32_t T[4])
+enum dcrypto_result DCRYPTO_aes_cmac_verify(const uint8_t *key,
+					    const uint8_t *M, size_t len,
+					    const uint32_t T[4])
 {
-	int i;
+	size_t i;
 	uint32_t T2[4];
-	int match = 1;
+	uint32_t match = 0;
 
-	if (!DCRYPTO_aes_cmac(key, M, len, T2))
-		return -EC_ERROR_UNKNOWN;
+	if (DCRYPTO_aes_cmac(key, M, len, T2) != DCRYPTO_OK)
+		return DCRYPTO_FAIL;
 
-	for (i = 0; i < 4; i++) {
-		if (T[i] != T2[i])
-			match = 0;
-	}
-	return match;
+	for (i = 0; i < 4; i++)
+		match |= T[i] ^ T2[i];
+
+	always_memset(T2, 0, sizeof(T2));
+
+	return dcrypto_ok_if_zero(match);
 }
 
 #ifdef CRYPTO_TEST_SETUP
+#include "console.h"
+
 static int check_answer(const uint32_t expected[4], uint32_t actual[4])
 {
 	int i;
@@ -174,20 +188,27 @@ static int check_answer(const uint32_t expected[4], uint32_t actual[4])
 	return success;
 }
 
+/* K:  2b7e1516 28aed2a6 abf71588 09cf4f3c
+ * k1: fbeed618 35713366 7c85e08f 7236a8de
+ * k2: f7ddac30 6ae266cc f90bc11e e46d513b
+ */
+static const uint32_t K[4] = {0x16157e2b, 0xa6d2ae28, 0x8815f7ab, 0x3c4fcf09};
+static const uint32_t k1e[4] = {0x18d6eefb, 0x66337135, 0x8fe0857c,
+				0xdea83672};
+static const uint32_t k2e[4] = {0x30acddf7, 0xcc66e26a, 0x1ec10bf9,
+				0x3b516de4};
+
 static int command_test_aes_block(int argc, char **argv)
 {
 	uint32_t actual[4];
-	const uint32_t zero[4] = {0, 0, 0, 0};
-	const uint32_t K[4] = {0x16157e2b, 0xa6d2ae28, 0x8815f7ab, 0x3c4fcf09};
-	const uint32_t expected[4] = {0x0c6bf77d, 0xb399b81a, 0x47f0423e,
-				      0x6f541bb9};
+	static const uint32_t expected[4] = {0x0c6bf77d, 0xb399b81a,
+					     0x47f0423e, 0x6f541bb9};
 
-	aes128((const uint8_t *) K, zero, actual);
+	aes128((const uint8_t *)K, zero, actual);
 	check_answer(expected, actual);
 
 	return 0;
 }
-
 DECLARE_SAFE_CONSOLE_COMMAND(test_aesbk, command_test_aes_block, NULL,
 			     "Test AES block in AES-CMAC subkey generation");
 
@@ -195,17 +216,8 @@ static int command_test_subkey_gen(int argc, char **argv)
 {
 	uint32_t k1[4];
 	uint32_t k2[4];
-	/* K:  2b7e1516 28aed2a6 abf71588 09cf4f3c
-	 * k1: fbeed618 35713366 7c85e08f 7236a8de
-	 * k2: f7ddac30 6ae266cc f90bc11e e46d513b
-	 */
-	const uint32_t K[4] = {0x16157e2b, 0xa6d2ae28, 0x8815f7ab, 0x3c4fcf09};
-	const uint32_t k1e[4] = {0x18d6eefb, 0x66337135, 0x8fe0857c,
-				 0xdea83672};
-	const uint32_t k2e[4] = {0x30acddf7, 0xcc66e26a, 0x1ec10bf9,
-				 0x3b516de4};
 
-	gen_subkey((const uint8_t *) K, k1, k2);
+	gen_subkey((const uint8_t *)K, k1, k2);
 
 	ccprintf("Checking K1: ");
 	check_answer(k1e, k1);
@@ -227,30 +239,41 @@ struct cmac_test_param {
 
 /* N.B. The order of bytes in each 32-bit block is reversed from the form in
  * which they are written in the RFC.
+ * Make sure it is placed in .rodata, not .data
  */
-const struct cmac_test_param rfctests[4] = {
+static const uint8_t M0[0] = {};
+static const uint32_t M1[] = {0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373};
+static const uint32_t M2[] = {0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373,
+			      0x578a2dae, 0x9cac031e, 0xac6fb79e, 0x518eaf45,
+			      0x461cc830, 0x11e45ca3};
+static const uint32_t M3[] = {0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373,
+			      0x578a2dae, 0x9cac031e, 0xac6fb79e, 0x518eaf45,
+			      0x461cc830, 0x11e45ca3, 0x19c1fbe5, 0xef520a1a,
+			      0x45249ff6, 0x179b4fdf, 0x7b412bad, 0x10376ce6};
+
+static const struct cmac_test_param rfctests[4] = {
 	/*  --------------------------------------------------
 	 *  Example 1: len = 0
 	 *  M	      <empty string>
 	 *  AES-CMAC       bb1d6929 e9593728 7fa37d12 9b756746
 	 *  --------------------------------------------------
 	 */
-	{ .len = 0,
-	  .M   = (uint8_t *) "",
-	  .Te  = {0x29691dbb, 0x283759e9, 0x127da37f, 0x4667759b},
-	  },
+	{
+		.len = 0,
+		.M = (uint8_t *)M0,
+		.Te = {0x29691dbb, 0x283759e9, 0x127da37f, 0x4667759b},
+	},
 	/*  --------------------------------------------------
 	 *  Example 2: len = 16
 	 *  M	      6bc1bee2 2e409f96 e93d7e11 7393172a
 	 *  AES-CMAC       070a16b4 6b4d4144 f79bdd9d d04a287c
 	 *  --------------------------------------------------
 	 */
-	{ .len = 16,
-	  .M   = (uint8_t *) (uint32_t[]) {
-		0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373
-		},
-	  .Te  = {0xb4160a07, 0x44414d6b, 0x9ddd9bf7, 0x7c284ad0},
-	  },
+	{
+		.len = sizeof(M1),
+		.M = (uint8_t *)M1,
+		.Te = {0xb4160a07, 0x44414d6b, 0x9ddd9bf7, 0x7c284ad0},
+	},
 	/*  --------------------------------------------------
 	 *  Example 3: len = 40
 	 *  M	      6bc1bee2 2e409f96 e93d7e11 7393172a
@@ -259,14 +282,11 @@ const struct cmac_test_param rfctests[4] = {
 	 *  AES-CMAC       dfa66747 de9ae630 30ca3261 1497c827
 	 *  --------------------------------------------------
 	 */
-	{ .len = 40,
-	  .M   = (uint8_t *) (uint32_t[]) {
-		0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373,
-		0x578a2dae, 0x9cac031e, 0xac6fb79e, 0x518eaf45,
-		0x461cc830, 0x11e45ca3
-		},
-	  .Te  = {0x4767a6df, 0x30e69ade, 0x6132ca30, 0x27c89714},
-	  },
+	{
+		.len = sizeof(M2),
+		.M = (uint8_t *)M2,
+		.Te = {0x4767a6df, 0x30e69ade, 0x6132ca30, 0x27c89714},
+	},
 	/*  --------------------------------------------------
 	 *  Example 4: len = 64
 	 *  M	      6bc1bee2 2e409f96 e93d7e11 7393172a
@@ -276,15 +296,11 @@ const struct cmac_test_param rfctests[4] = {
 	 *  AES-CMAC       51f0bebf 7e3b9d92 fc497417 79363cfe
 	 *  --------------------------------------------------
 	 */
-	{ .len = 64,
-	  .M   = (uint8_t *) (uint32_t[]) {
-		0xe2bec16b, 0x969f402e, 0x117e3de9, 0x2a179373,
-		0x578a2dae, 0x9cac031e, 0xac6fb79e, 0x518eaf45,
-		0x461cc830, 0x11e45ca3, 0x19c1fbe5, 0xef520a1a,
-		0x45249ff6, 0x179b4fdf, 0x7b412bad, 0x10376ce6
-		},
-	  .Te  = {0xbfbef051, 0x929d3b7e, 0x177449fc, 0xfe3c3679},
-	  },
+	{
+		.len = sizeof(M3),
+		.M = (uint8_t *)M3,
+		.Te = {0xbfbef051, 0x929d3b7e, 0x177449fc, 0xfe3c3679},
+	},
 };
 
 static int command_test_aes_cmac(int argc, char **argv)
@@ -293,7 +309,6 @@ static int command_test_aes_cmac(int argc, char **argv)
 	uint32_t T[4];
 	int testN;
 	struct cmac_test_param param;
-	const uint32_t K[4] = {0x16157e2b, 0xa6d2ae28, 0x8815f7ab, 0x3c4fcf09};
 
 	for (i = 1; i < argc; i++) {
 		testN = strtoi(argv[i], NULL, 10);
@@ -302,6 +317,7 @@ static int command_test_aes_cmac(int argc, char **argv)
 		ccprintf("Testing RFC Example #%d (%d-byte message)...", testN,
 			 param.len);
 
+		memset(T, 0, sizeof(T));
 		DCRYPTO_aes_cmac((const uint8_t *)K, param.M, param.len, T);
 		check_answer(param.Te, T);
 	}
@@ -317,24 +333,21 @@ static int command_test_verify(int argc, char **argv)
 {
 	int i;
 	int testN;
-	int result;
+	enum dcrypto_result result;
 	struct cmac_test_param param;
-	const uint32_t K[4] = {0x16157e2b, 0xa6d2ae28, 0x8815f7ab, 0x3c4fcf09};
 
 	for (i = 1; i < argc; i++) {
 		testN = strtoi(argv[i], NULL, 10);
-		param = rfctests[testN-1];
+		param = rfctests[testN - 1];
 
 		ccprintf("Testing RFC Example #%d (%d-byte message)...", testN,
 			 param.len);
 
 		result = DCRYPTO_aes_cmac_verify((const uint8_t *)K, param.M,
-				param.len, param.Te);
-		if (result == 1)
+						 param.len, param.Te);
+		if (result == DCRYPTO_OK)
 			ccprintf("SUCCESS\n");
-		else if (result == 0)
-			ccprintf("FAILURE: verify returned INVALID\n");
-		else if (result == -EC_ERROR_UNKNOWN)
+		else
 			ccprintf("FAILURE: verify returned ERROR\n");
 	}
 
diff --git a/board/cr50/dcrypto/dcrypto.h b/board/cr50/dcrypto/dcrypto.h
index c162b1c66c..0a8c05a536 100644
--- a/board/cr50/dcrypto/dcrypto.h
+++ b/board/cr50/dcrypto/dcrypto.h
@@ -689,14 +689,15 @@ void DCRYPTO_gcm_finish(struct GCM_CTX *ctx);
  * Writes 128-bit tag to T; returns 0 if an error is encountered and 1
  * otherwise.
  */
-int DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M, const uint32_t len,
-		     uint32_t T[4]);
+enum dcrypto_result DCRYPTO_aes_cmac(const uint8_t *K, const uint8_t *M,
+				     size_t len, uint32_t T[4]);
 /* key: 128-bit key, M: message, len: number of bytes in M,
  *    T: tag to be verified
  * Returns 1 if the tag is correct and 0 otherwise.
  */
-int DCRYPTO_aes_cmac_verify(const uint8_t *key, const uint8_t *M, const int len,
-			    const uint32_t T[4]);
+enum dcrypto_result DCRYPTO_aes_cmac_verify(const uint8_t *key,
+					    const uint8_t *M, size_t len,
+					    const uint32_t T[4]);
 
 /*
  * BIGNUM utility methods.