diff --git a/src/crypto/README.md b/src/crypto/README.md index ceefda03976ba9..51659f1f363b3b 100644 --- a/src/crypto/README.md +++ b/src/crypto/README.md @@ -82,7 +82,7 @@ using EVPKeyCtxPointer = DeleteFnPtr; using EVPMDPointer = DeleteFnPtr; using RSAPointer = DeleteFnPtr; using ECPointer = DeleteFnPtr; -using BignumPointer = DeleteFnPtr; +using BignumPointer = DeleteFnPtr; using NetscapeSPKIPointer = DeleteFnPtr; using ECGroupPointer = DeleteFnPtr; using ECPointPointer = DeleteFnPtr; diff --git a/src/crypto/crypto_dh.cc b/src/crypto/crypto_dh.cc index 408d6be2a9cfdb..1d3e7fec116503 100644 --- 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,15 +163,18 @@ 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(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(p), p_len, nullptr)); + BignumPointer bn_g(BN_new()); + if (!BN_set_word(bn_g.get(), g) || + !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(); } @@ -186,21 +190,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(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(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(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(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 index 1ce5f35a70a7c8..ac231f59907918 100644 --- a/src/crypto/crypto_util.h +++ b/src/crypto/crypto_util.h @@ -65,7 +65,7 @@ using EVPKeyCtxPointer = DeleteFnPtr; using EVPMDPointer = DeleteFnPtr; using RSAPointer = DeleteFnPtr; using ECPointer = DeleteFnPtr; -using BignumPointer = DeleteFnPtr; +using BignumPointer = DeleteFnPtr; using BignumCtxPointer = DeleteFnPtr; using NetscapeSPKIPointer = DeleteFnPtr; using ECGroupPointer = DeleteFnPtr;