From a482a6b2049c62ef77b8b8f11d0a6c32f7ac05c3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= <michal@isc.org>
Date: Fri, 19 Nov 2021 10:32:21 +0100
Subject: [PATCH] Fix parsing ECDSA keys

raw_key_to_ossl() assumes fixed ECDSA private key sizes (32 bytes for
ECDSAP256SHA256, 48 bytes for ECDSAP384SHA384).  Meanwhile, in rare
cases, ECDSAP256SHA256 private keys are representable in 31 bytes or
less (similarly for ECDSAP384SHA384) and that is how they are then
stored in the "PrivateKey" field of the key file.  Nevertheless,
raw_key_to_ossl() always calls BN_bin2bn() with a fixed length argument,
which in the cases mentioned above leads to erroneously interpreting
uninitialized memory as a part of the private key.  This results in the
latter being malformed and broken signatures being generated.  Address
by using the key length provided by the caller rather than a fixed one.
Apply the same change to public key parsing code for consistency, adding
an INSIST() to prevent buffer overruns.
---
 lib/dns/opensslecdsa_link.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/lib/dns/opensslecdsa_link.c b/lib/dns/opensslecdsa_link.c
index 4dee158b31..08f061208c 100644
--- a/lib/dns/opensslecdsa_link.c
+++ b/lib/dns/opensslecdsa_link.c
@@ -66,24 +66,16 @@ raw_key_to_ossl(unsigned int key_alg, int private, const unsigned char *key,
 	OSSL_PARAM *params = NULL;
 	EVP_PKEY_CTX *ctx = NULL;
 	BIGNUM *priv = NULL;
-	size_t len = 0;
 	unsigned char buf[DNS_KEY_ECDSA384SIZE + 1];
 
 	if (key_alg == DST_ALG_ECDSA256) {
 		groupname = "P-256";
-		len = private ? DNS_KEY_ECDSA256SIZE / 2 : DNS_KEY_ECDSA256SIZE;
 	} else if (key_alg == DST_ALG_ECDSA384) {
 		groupname = "P-384";
-		len = private ? DNS_KEY_ECDSA384SIZE / 2 : DNS_KEY_ECDSA384SIZE;
 	} else {
 		DST_RET(ISC_R_NOTIMPLEMENTED);
 	}
 
-	ret = (private ? DST_R_INVALIDPRIVATEKEY : DST_R_INVALIDPUBLICKEY);
-	if (*key_len < len) {
-		DST_RET(ret);
-	}
-
 	bld = OSSL_PARAM_BLD_new();
 	if (bld == NULL) {
 		DST_RET(dst__openssl_toresult2("OSSL_PARAM_BLD_new",
@@ -98,7 +90,7 @@ raw_key_to_ossl(unsigned int key_alg, int private, const unsigned char *key,
 	}
 
 	if (private) {
-		priv = BN_bin2bn(key, len, NULL);
+		priv = BN_bin2bn(key, *key_len, NULL);
 		if (priv == NULL) {
 			DST_RET(dst__openssl_toresult2("BN_bin2bn",
 						       DST_R_OPENSSLFAILURE));
@@ -111,11 +103,12 @@ raw_key_to_ossl(unsigned int key_alg, int private, const unsigned char *key,
 						       DST_R_OPENSSLFAILURE));
 		}
 	} else {
+		INSIST(*key_len < sizeof(buf));
 		buf[0] = POINT_CONVERSION_UNCOMPRESSED;
-		memmove(buf + 1, key, len);
+		memmove(buf + 1, key, *key_len);
 
 		status = OSSL_PARAM_BLD_push_octet_string(
-			bld, OSSL_PKEY_PARAM_PUB_KEY, buf, 1 + len);
+			bld, OSSL_PKEY_PARAM_PUB_KEY, buf, 1 + *key_len);
 		if (status != 1) {
 			DST_RET(dst__openssl_toresult2("OSSL_PARAM_BLD_push_"
 						       "octet_string",
@@ -146,7 +139,6 @@ raw_key_to_ossl(unsigned int key_alg, int private, const unsigned char *key,
 					       DST_R_OPENSSLFAILURE));
 	}
 
-	*key_len = len;
 	ret = ISC_R_SUCCESS;
 
 err:
@@ -1184,8 +1176,6 @@ opensslecdsa_parse(dst_key_t *key, isc_lex_t *lexer, dst_key_t *pub) {
 #if OPENSSL_VERSION_NUMBER < 0x30000000L
 	EC_KEY *eckey = NULL;
 	EC_KEY *pubeckey = NULL;
-#else
-	size_t len;
 #endif /* OPENSSL_VERSION_NUMBER < 0x30000000L */
 	const char *engine = NULL;
 	const char *label = NULL;
@@ -1257,14 +1247,9 @@ opensslecdsa_parse(dst_key_t *key, isc_lex_t *lexer, dst_key_t *pub) {
 			key->keydata.pkey = NULL;
 		}
 
-		if (key->key_alg == DST_ALG_ECDSA256) {
-			len = DNS_KEY_ECDSA256SIZE / 2;
-		} else {
-			len = DNS_KEY_ECDSA384SIZE / 2;
-		}
-
 		ret = raw_key_to_ossl(key->key_alg, 1,
-				      priv.elements[privkey_index].data, &len,
+				      priv.elements[privkey_index].data,
+				      &priv.elements[privkey_index].length,
 				      &key->keydata.pkey);
 #endif /* OPENSSL_VERSION_NUMBER < 0x30000000L */