From 22b55b0b9cd9bc475c3f48959939e31a06d92cbe Mon Sep 17 00:00:00 2001
From: tb <tb@openbsd.org>
Date: Mon, 14 Oct 2024 12:50:18 +0000
Subject: [PATCH] Fix field element encoding for elliptic curve coefficients

SEC 1, section 2.3.5, is explicit that the encoding of an element of the
field of definition for an elliptic curve needs to be a zero-padded octet
string whose length matches the byte size of the field's degree. So use
BN_bn2binpad() to fix this. Factor things into a simple helper to avoid
copy-pasting.

This gets rid of some of the most grotesque code in this file.

ok jsing
---
 lib/libcrypto/ec/ec_asn1.c | 81 +++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 45 deletions(-)

diff --git a/lib/libcrypto/ec/ec_asn1.c b/lib/libcrypto/ec/ec_asn1.c
index acab5137489..8d0f0329073 100644
--- a/lib/libcrypto/ec/ec_asn1.c
+++ b/lib/libcrypto/ec/ec_asn1.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_asn1.c,v 1.70 2024/10/14 12:42:52 tb Exp $ */
+/* $OpenBSD: ec_asn1.c,v 1.71 2024/10/14 12:50:18 tb Exp $ */
 /*
  * Written by Nils Larsch for the OpenSSL project.
  */
@@ -601,14 +601,40 @@ ec_asn1_group2fieldid(const EC_GROUP *group, X9_62_FIELDID *field)
 	return ret;
 }
 
+static int
+ec_asn1_encode_field_element(const EC_GROUP *group, const BIGNUM *bn,
+    ASN1_OCTET_STRING *os)
+{
+	unsigned char *buf;
+	int len;
+	int ret = 0;
+
+	/* Zero-pad field element per SEC 1, section 2.3.5. */
+	len = (EC_GROUP_get_degree(group) + 7) / 8;
+
+	/* One extra byte for historic NUL termination of ASN1_STRINGs. */
+	if ((buf = calloc(1, len + 1)) == NULL)
+		goto err;
+
+	if (BN_bn2binpad(bn, buf, len) != len)
+		goto err;
+
+	ASN1_STRING_set0(os, buf, len);
+	buf = NULL;
+	len = 0;
+
+	ret = 1;
+
+ err:
+	freezero(buf, len);
+
+	return ret;
+}
+
 static int
 ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve)
 {
 	BIGNUM *a = NULL, *b = NULL;
-	unsigned char *buffer_1 = NULL, *buffer_2 = NULL, *a_buf = NULL,
-	*b_buf = NULL;
-	size_t len_1, len_2;
-	unsigned char char_zero = 0;
 	int ret = 0;
 
 	if (!group || !curve || !curve->a || !curve->b)
@@ -619,50 +645,17 @@ ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve)
 		goto err;
 	}
 
-	/* get a and b */
 	if (!EC_GROUP_get_curve(group, NULL, a, b, NULL)) {
 		ECerror(ERR_R_EC_LIB);
 		goto err;
 	}
-	len_1 = (size_t) BN_num_bytes(a);
-	len_2 = (size_t) BN_num_bytes(b);
-
-	if (len_1 == 0) {
-		/* len_1 == 0 => a == 0 */
-		a_buf = &char_zero;
-		len_1 = 1;
-	} else {
-		if ((buffer_1 = malloc(len_1)) == NULL) {
-			ECerror(ERR_R_MALLOC_FAILURE);
-			goto err;
-		}
-		if ((len_1 = BN_bn2bin(a, buffer_1)) == 0) {
-			ECerror(ERR_R_BN_LIB);
-			goto err;
-		}
-		a_buf = buffer_1;
-	}
 
-	if (len_2 == 0) {
-		/* len_2 == 0 => b == 0 */
-		b_buf = &char_zero;
-		len_2 = 1;
-	} else {
-		if ((buffer_2 = malloc(len_2)) == NULL) {
-			ECerror(ERR_R_MALLOC_FAILURE);
-			goto err;
-		}
-		if ((len_2 = BN_bn2bin(b, buffer_2)) == 0) {
-			ECerror(ERR_R_BN_LIB);
-			goto err;
-		}
-		b_buf = buffer_2;
+	if (!ec_asn1_encode_field_element(group, a, curve->a)) {
+		ECerror(ERR_R_EC_LIB);
+		goto err;
 	}
-
-	/* set a and b */
-	if (!ASN1_STRING_set(curve->a, a_buf, len_1) ||
-	    !ASN1_STRING_set(curve->b, b_buf, len_2)) {
-		ECerror(ERR_R_ASN1_LIB);
+	if (!ec_asn1_encode_field_element(group, b, curve->b)) {
+		ECerror(ERR_R_EC_LIB);
 		goto err;
 	}
 
@@ -688,8 +681,6 @@ ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve)
 	ret = 1;
 
  err:
-	free(buffer_1);
-	free(buffer_2);
 	BN_free(a);
 	BN_free(b);
 
-- 
2.20.1