src: use BignumPointer and use BN_clear_free

PR-URL: #50454 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
nodejs · Mar 25, 2024 · 9b25268 · 9b25268
1 parent a8fd01a
commit 9b25268
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 18 deletions.
diff --git a/src/crypto/README.md b/src/crypto/README.md
@@ -82,7 +82,7 @@ using EVPKeyCtxPointer = DeleteFnPtr<EVP_PKEY_CTX, EVP_PKEY_CTX_free>;
 using EVPMDPointer = DeleteFnPtr<EVP_MD_CTX, EVP_MD_CTX_free>;
 using RSAPointer = DeleteFnPtr<RSA, RSA_free>;
 using ECPointer = DeleteFnPtr<EC_KEY, EC_KEY_free>;
-using BignumPointer = DeleteFnPtr<BIGNUM, BN_free>;
+using BignumPointer = DeleteFnPtr<BIGNUM, BN_clear_free>;
 using NetscapeSPKIPointer = DeleteFnPtr<NETSCAPE_SPKI, NETSCAPE_SPKI_free>;
 using ECGroupPointer = DeleteFnPtr<EC_GROUP, EC_GROUP_free>;
 using ECPointPointer = DeleteFnPtr<EC_POINT, EC_POINT_free>;

diff --git a/src/crypto/crypto_dh.cc b/src/crypto/crypto_dh.cc
@@ -2,6 +2,7 @@
 #include "async_wrap-inl.h"
 #include "base_object-inl.h"
 #include "crypto/crypto_keys.h"
+#include "crypto/crypto_util.h"
 #include "env-inl.h"
 #include "memory_tracker-inl.h"
 #include "threadpoolwork-inl.h"
@@ -162,13 +163,11 @@ bool DiffieHellman::Init(const char* p, int p_len, int g) {
       DH_R_BAD_GENERATOR, __FILE__, __LINE__);
     return false;
   }
-  BIGNUM* bn_p =
-      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr);
-  BIGNUM* bn_g = BN_new();
-  if (!BN_set_word(bn_g, g) ||
-      !DH_set0_pqg(dh_.get(), bn_p, nullptr, bn_g)) {
-    BN_free(bn_p);
-    BN_free(bn_g);
+  BignumPointer bn_p(
+      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr));
+  BignumPointer bn_g(BN_new());
+  if (bn_p == nullptr || bn_g == nullptr || !BN_set_word(bn_g.get(), g) ||
+      !DH_set0_pqg(dh_.get(), bn_p.release(), nullptr, bn_g.release())) {
     return false;
   }
   return VerifyContext();
@@ -186,21 +185,23 @@ bool DiffieHellman::Init(const char* p, int p_len, const char* g, int g_len) {
       DH_R_BAD_GENERATOR, __FILE__, __LINE__);
     return false;
   }
-  BIGNUM* bn_g =
-      BN_bin2bn(reinterpret_cast<const unsigned char*>(g), g_len, nullptr);
-  if (BN_is_zero(bn_g) || BN_is_one(bn_g)) {
-    BN_free(bn_g);
+  BignumPointer bn_g(
+      BN_bin2bn(reinterpret_cast<const unsigned char*>(g), g_len, nullptr));
+  if (BN_is_zero(bn_g.get()) || BN_is_one(bn_g.get())) {
     ERR_put_error(ERR_LIB_DH, DH_F_DH_BUILTIN_GENPARAMS,
       DH_R_BAD_GENERATOR, __FILE__, __LINE__);
     return false;
   }
-  BIGNUM* bn_p =
-      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr);
-  if (!DH_set0_pqg(dh_.get(), bn_p, nullptr, bn_g)) {
-    BN_free(bn_p);
-    BN_free(bn_g);
+  BignumPointer bn_p(
+      BN_bin2bn(reinterpret_cast<const unsigned char*>(p), p_len, nullptr));
+  if (!DH_set0_pqg(dh_.get(), bn_p.get(), nullptr, bn_g.get())) {
     return false;
   }
+  // The DH_set0_pqg call above takes ownership of the bignums on success,
+  // so we should release them here so we don't end with a possible
+  // use-after-free or double free.
+  bn_p.release();
+  bn_g.release();
   return VerifyContext();
 }
 

diff --git a/src/crypto/crypto_util.h b/src/crypto/crypto_util.h
@@ -65,7 +65,7 @@ using EVPKeyCtxPointer = DeleteFnPtr<EVP_PKEY_CTX, EVP_PKEY_CTX_free>;
 using EVPMDPointer = DeleteFnPtr<EVP_MD_CTX, EVP_MD_CTX_free>;
 using RSAPointer = DeleteFnPtr<RSA, RSA_free>;
 using ECPointer = DeleteFnPtr<EC_KEY, EC_KEY_free>;
-using BignumPointer = DeleteFnPtr<BIGNUM, BN_free>;
+using BignumPointer = DeleteFnPtr<BIGNUM, BN_clear_free>;
 using BignumCtxPointer = DeleteFnPtr<BN_CTX, BN_CTX_free>;
 using NetscapeSPKIPointer = DeleteFnPtr<NETSCAPE_SPKI, NETSCAPE_SPKI_free>;
 using ECGroupPointer = DeleteFnPtr<EC_GROUP, EC_GROUP_free>;