From: Michał Kępień <michal@isc.org>
Date: Fri, 19 Nov 2021 09:32:21 +0000 (+0100)
Subject: Fix parsing ECDSA keys
X-Git-Tag: v9.17.21~25^2~3
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a482a6b2049c62ef77b8b8f11d0a6c32f7ac05c3;p=thirdparty%2Fbind9.git

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.
---

diff --git a/lib/dns/opensslecdsa_link.c b/lib/dns/opensslecdsa_link.c
index 4dee158b319..08f061208c8 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 */