CR50: remove DCRYPTO_p256_points_mul, add DCRYPTO_p256_point_mul

points_mul (variable time) is only necessary for
ECDSA verification, and is not required as part of
the public dcrypto API.  Replaced wih (constant time)
point_mul, and add corresponding parameter checks to
the tpm2 interface call _cpri__EccPointMultiply.

BRANCH=none
BUG=chrome-os-partner:43025,chrome-os-partner:47524
TEST=tests in test/tpm/tpmtest.py pass

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

3 files changed, 23 insertions, 48 deletions

diff --git a/board/cr50/tpm2/ecc.c b/board/cr50/tpm2/ecc.c
index 30963aa756..838fbc1143 100644
--- a/board/cr50/tpm2/ecc.c
+++ b/board/cr50/tpm2/ecc.c
@@ -57,7 +57,7 @@ BOOL _cpri__EccIsPointOnCurve(TPM_ECC_CURVE curve_id, TPMS_ECC_POINT *q)
 	}
 }
 
-/* out = n1*G + n2*in */
+/* out = n1*G, or out = n2*in */
 CRYPT_RESULT _cpri__EccPointMultiply(
 	TPMS_ECC_POINT *out, TPM_ECC_CURVE curve_id,
 	TPM2B_ECC_PARAMETER *n1, TPMS_ECC_POINT *in, TPM2B_ECC_PARAMETER *n2)
@@ -66,16 +66,23 @@ CRYPT_RESULT _cpri__EccPointMultiply(
 
 	switch (curve_id) {
 	case TPM_ECC_NIST_P256:
-		if (!check_p256_param(n1))
+		if ((n1 != NULL && n2 != NULL) ||
+			(n1 == NULL && n2 == NULL))
+			/* Only one of n1 or n2 must be specified. */
 			return CRYPT_PARAMETER;
-		if (in != NULL && !check_p256_point(in))
+		if ((n2 != NULL && in == NULL) ||
+			(n2 == NULL && in != NULL))
+			return CRYPT_PARAMETER;
+		if (n1 != NULL && !check_p256_param(n1))
 			return CRYPT_PARAMETER;
+		if (in != NULL && !check_p256_point(in))
+			return CRYPT_POINT;
 		if (n2 != NULL && !check_p256_param(n2))
 			return CRYPT_PARAMETER;
 
 		reverse_tpm2b(&n1->b);
 
-		if (in == NULL || n2 == NULL) {
+		if (n1 != NULL) {
 			result = DCRYPTO_p256_base_point_mul(
 				(p256_int *) out->x.b.buffer,
 				(p256_int *) out->y.b.buffer,
@@ -85,10 +92,9 @@ CRYPT_RESULT _cpri__EccPointMultiply(
 			reverse_tpm2b(&in->x.b);
 			reverse_tpm2b(&in->y.b);
 
-			result = DCRYPTO_p256_points_mul(
+			result = DCRYPTO_p256_point_mul(
 				(p256_int *) out->x.b.buffer,
 				(p256_int *) out->y.b.buffer,
-				(p256_int *) n1->b.buffer,
 				(p256_int *) n2->b.buffer,
 				(p256_int *) in->x.b.buffer,
 				(p256_int *) in->y.b.buffer);
@@ -111,7 +117,7 @@ CRYPT_RESULT _cpri__EccPointMultiply(
 			return CRYPT_NO_RESULT;
 		}
 	default:
-		return CRYPT_FAIL;
+		return CRYPT_PARAMETER;
 	}
 }
 
diff --git a/chip/g/dcrypto/dcrypto.h b/chip/g/dcrypto/dcrypto.h
index aec3292dae..8092a0a576 100644
--- a/chip/g/dcrypto/dcrypto.h
+++ b/chip/g/dcrypto/dcrypto.h
@@ -140,9 +140,9 @@ int DCRYPTO_rsa_key_compute(struct BIGNUM *N, struct BIGNUM *d,
 int DCRYPTO_p256_valid_point(const p256_int *x, const p256_int *y);
 int DCRYPTO_p256_base_point_mul(p256_int *out_x, p256_int *out_y,
 				const p256_int *n);
-int DCRYPTO_p256_points_mul(p256_int *out_x, p256_int *out_y,
-			const p256_int *n1, const p256_int *n2,
-			const p256_int *in_x, const p256_int *in_y);
+int DCRYPTO_p256_point_mul(p256_int *out_x, p256_int *out_y,
+			const p256_int *n, const p256_int *in_x,
+			const p256_int *in_y);
 int DCRYPTO_p256_key_from_bytes(p256_int *x, p256_int *y, p256_int *d,
 				const uint8_t key_bytes[P256_NBYTES]);
 
diff --git a/chip/g/dcrypto/p256_ec.c b/chip/g/dcrypto/p256_ec.c
index 8bb4a0e308..12423fb50a 100644
--- a/chip/g/dcrypto/p256_ec.c
+++ b/chip/g/dcrypto/p256_ec.c
@@ -1356,48 +1356,17 @@ void p256_points_mul_vartime(
 	from_montgomery(out_y, py);
 }
 
-/* p256_points_mul sets {out_x,out_y} = n1*G + n2*{in_x,in_y}, where
- * n1 and n2 are < the order of the group.
- *
- * As indicated by the name, this function operates in variable time. This
- * is safe because it's used for signature validation which doesn't deal
- * with secrets. */
-int DCRYPTO_p256_points_mul(p256_int *out_x, p256_int *out_y,
-			const p256_int *n1, const p256_int *n2,
-			const p256_int *in_x, const p256_int *in_y)
+/* DCRYPTO_p256_point_mul sets {out_x,out_y} = n*{in_x,in_y}, where n is <
+ * the order of the group. */
+int DCRYPTO_p256_point_mul(p256_int *out_x, p256_int *out_y,
+			const p256_int *n, const p256_int *in_x,
+			const p256_int *in_y)
 {
-	felem x1, y1, z1, x2, y2, z2, px, py;
-
-	/* If both scalars are zero, then the result is the point at
-	 * infinity. */
-	if (p256_is_zero(n1) != 0 && p256_is_zero(n2) != 0) {
+	if (p256_is_zero(n) != 0) {
 		p256_clear(out_x);
 		p256_clear(out_y);
 		return 0;
 	}
-
-	to_montgomery(px, in_x);
-	to_montgomery(py, in_y);
-	scalar_base_mult(x1, y1, z1, n1);
-	scalar_mult(x2, y2, z2, px, py, n2);
-
-	if (p256_is_zero(n2) != 0) {
-		/* If n2 == 0, then {x2,y2,z2} is zero and the result is just
-		 * {x1,y1,z1}. */
-	} else if (p256_is_zero(n1) != 0) {
-		/* If n1 == 0, then {x1,y1,z1} is zero and the result is just
-		 * {x2,y2,z2}. */
-		memcpy(x1, x2, sizeof(x2));
-		memcpy(y1, y2, sizeof(y2));
-		memcpy(z1, z2, sizeof(z2));
-	} else {
-		/* This function handles the case where
-		 * {x1,y1,z1} == {x2,y2,z2}. */
-		point_add_or_double_vartime(x1, y1, z1, x1, y1, z1, x2, y2, z2);
-	}
-
-	point_to_affine(px, py, x1, y1, z1);
-	from_montgomery(out_x, px);
-	from_montgomery(out_y, py);
+	p256_point_mul(n, in_x, in_y, out_x, out_y);
 	return 1;
 }