From 1dc6524d3d4dc9ba2b661ffd9689dcca6bcebf55 Mon Sep 17 00:00:00 2001
From: Jeremy Barton <jbarton@microsoft.com>
Date: Fri, 29 Jul 2022 14:55:54 -0700
Subject: [PATCH] Appropriately free temporary values on EC import

EC import from parameters calls into a series of set-routines that neither
self-describe their ownership of the reference counted inputs (`set0` vs `set1`)
nor document them.

Looking at code usage examples, and underlying code, they are all logically
`set1`-style routines, in that they do not take ownership of the inputs, which
means we need to free the BN values on both success and failure.  This assertion
was verified by an intermediate change that reset all numbers to 1
(e.g. `BN_one(qxBn)`) and watched for test failures.  When none of our tests
failed with that change, it moved to the freeing form.

Verified with importing the cross-product of:
{ secp256r1, sect163k1 }
x
{ named curve, explicit parameters }
x
{ q + d, q only, d only }
---
 .../pal_ecc_import_export.c                        | 39 ++++++++++++----------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c b/src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c
index fedac75..99b2a58 100644
--- a/src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c
+++ b/src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c
@@ -327,10 +327,11 @@ int32_t CryptoNative_EcKeyCreateByKeyParameters(EC_KEY** key, const char* oid, u
     if (!nid)
         return -1;
 
-    *key = EC_KEY_new_by_curve_name(nid);
-    if (!(*key))
+    EC_KEY* tmpKey = EC_KEY_new_by_curve_name(nid);
+    if (tmpKey == NULL)
         return -1;
 
+    int ret = 0;
     BIGNUM* dBn = NULL;
     BIGNUM* qxBn = NULL;
     BIGNUM* qyBn = NULL;
@@ -344,7 +345,7 @@ int32_t CryptoNative_EcKeyCreateByKeyParameters(EC_KEY** key, const char* oid, u
         if (!qxBn || !qyBn)
             goto error;
 
-        if (!EC_KEY_set_public_key_affine_coordinates(*key, qxBn, qyBn))
+        if (!EC_KEY_set_public_key_affine_coordinates(tmpKey, qxBn, qyBn))
             goto error;
 
         // Set private key (optional)
@@ -354,12 +355,12 @@ int32_t CryptoNative_EcKeyCreateByKeyParameters(EC_KEY** key, const char* oid, u
             if (!dBn)
                 goto error;
 
-            if (!EC_KEY_set_private_key(*key, dBn))
+            if (!EC_KEY_set_private_key(tmpKey, dBn))
                 goto error;
         }
 
         // Validate key
-        if (!EC_KEY_check_key(*key))
+        if (!EC_KEY_check_key(tmpKey))
             goto error;
     }
 
@@ -373,10 +374,10 @@ int32_t CryptoNative_EcKeyCreateByKeyParameters(EC_KEY** key, const char* oid, u
         if (!dBn)
             goto error;
 
-        if (!EC_KEY_set_private_key(*key, dBn))
+        if (!EC_KEY_set_private_key(tmpKey, dBn))
             goto error;
 
-        const EC_GROUP* group = EC_KEY_get0_group(*key);
+        const EC_GROUP* group = EC_KEY_get0_group(tmpKey);
 
         if (!group)
             goto error;
@@ -389,27 +390,26 @@ int32_t CryptoNative_EcKeyCreateByKeyParameters(EC_KEY** key, const char* oid, u
         if (!EC_POINT_mul(group, pubG, dBn, NULL, NULL, NULL))
             goto error;
 
-        if (!EC_KEY_set_public_key(*key, pubG))
+        if (!EC_KEY_set_public_key(tmpKey, pubG))
             goto error;
 
-        if (!EC_KEY_check_key(*key))
+        if (!EC_KEY_check_key(tmpKey))
             goto error;
     }
 
     // Success
-    return 1;
+    *key = tmpKey;
+    tmpKey = NULL;
+    ret = 1;
 
 error:
     if (qxBn) BN_free(qxBn);
     if (qyBn) BN_free(qyBn);
     if (dBn) BN_clear_free(dBn);
     if (pubG) EC_POINT_free(pubG);
-    if (*key)
-    {
-        EC_KEY_free(*key);
-        *key = NULL;
-    }
-    return 0;
+    if (tmpKey) EC_KEY_free(tmpKey);
+
+    return ret;
 }
 
 EC_KEY* CryptoNative_EcKeyCreateByExplicitParameters(
@@ -436,6 +436,7 @@ EC_KEY* CryptoNative_EcKeyCreateByExplicitParameters(
     ERR_clear_error();
 
     EC_KEY* key = NULL;
+    EC_KEY* ret = NULL;
     EC_POINT* G = NULL;
     EC_POINT* pubG = NULL;
 
@@ -570,7 +571,8 @@ EC_KEY* CryptoNative_EcKeyCreateByExplicitParameters(
     }
 
     // Success
-    return key;
+    ret = key;
+    key = NULL;
 
 error:
     if (qxBn) BN_free(qxBn);
@@ -587,5 +589,6 @@ error:
     if (pubG) EC_POINT_free(pubG);
     if (group) EC_GROUP_free(group);
     if (key) EC_KEY_free(key);
-    return NULL;
+
+    return ret;
 }
-- 
2.7.4