Skip to content

Migrate OSSLDH to OpenSSL 3.0+ #817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bukka wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Migrate OSSLDH to OpenSSL 3.0+ #817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
202 changes: 195 additions & 7 deletions src/lib/crypto/OSSLDH.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,13 @@
#include "OSSLDHKeyPair.h"
#include "OSSLUtil.h"
#include <algorithm>
#if OPENSSL_VERSION_NUMBER < 0x30000000L
#include <openssl/dh.h>
#else
#include <openssl/core_names.h>
#include <openssl/param_build.h>
#include <openssl/provider.h>
#endif
#include <openssl/pem.h>
#include <openssl/err.h>

Expand Down Expand Up @@ -126,18 +132,21 @@ bool OSSLDH::generateKeyPair(AsymmetricKeyPair** ppKeyPair, AsymmetricParameters

DHParameters* params = (DHParameters*) parameters;

BIGNUM* bn_p = OSSL::byteString2bn(params->getP());
BIGNUM* bn_g = OSSL::byteString2bn(params->getG());

#if OPENSSL_VERSION_NUMBER < 0x30000000L
// Generate the key-pair
DH* dh = DH_new();
if (dh == NULL)
{
ERROR_MSG("Failed to instantiate OpenSSL DH object");
BN_free(bn_p);
BN_free(bn_g);

return false;
}

BIGNUM* bn_p = OSSL::byteString2bn(params->getP());
BIGNUM* bn_g = OSSL::byteString2bn(params->getG());

if (!DH_set0_pqg(dh, bn_p, NULL, bn_g))
{
ERROR_MSG("DH set pqg failed (0x%08X)", ERR_get_error());
Expand Down Expand Up @@ -169,6 +178,77 @@ bool OSSLDH::generateKeyPair(AsymmetricKeyPair** ppKeyPair, AsymmetricParameters

return false;
}
#else
EVP_PKEY_CTX* ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DH, NULL);
if (!ctx)
{
ERROR_MSG("Failed to create EVP_PKEY_CTX");
BN_free(bn_p);
BN_free(bn_g);
return false;
}

OSSL_PARAM_BLD* bld = OSSL_PARAM_BLD_new();
if (!bld)
{
ERROR_MSG("Failed to create OSSL_PARAM_BLD");
BN_free(bn_p);
BN_free(bn_g);
EVP_PKEY_CTX_free(ctx);
return false;
}

OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, bn_p);
OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, bn_g);
if (params->getXBitLength() > 0)
{
OSSL_PARAM_BLD_push_uint(bld, OSSL_PKEY_PARAM_DH_PRIV_LEN, params->getXBitLength());
}

OSSL_PARAM* params_built = OSSL_PARAM_BLD_to_param(bld);
if (!params_built)
{
ERROR_MSG("Failed to build OSSL_PARAM");
BN_free(bn_p);
BN_free(bn_g);
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);
return false;
}

EVP_PKEY* dh = NULL;
if (EVP_PKEY_fromdata_init(ctx) <= 0 || EVP_PKEY_fromdata(ctx, &dh, EVP_PKEY_KEYPAIR, params_built) <= 0)
{
ERROR_MSG("EVP_PKEY_fromdata failed");
BN_free(bn_p);
BN_free(bn_g);
OSSL_PARAM_free(params_built);
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(dh);
return false;
}

EVP_PKEY_CTX_free(ctx);
ctx = EVP_PKEY_CTX_new(dh, NULL);
if (EVP_PKEY_keygen_init(ctx) <= 0 || EVP_PKEY_keygen(ctx, &dh) <= 0) {
ERROR_MSG("DH key generation failed");
BN_free(bn_p);
BN_free(bn_g);
OSSL_PARAM_free(params_built);
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(dh);
return false;
}
Comment on lines +232 to +243
Copy link

@coderabbitai coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Memory leak: parameter key not freed before keygen.

At line 234, EVP_PKEY_keygen(ctx, &dh) overwrites dh with the newly generated key. However, dh already holds the parameter key created by EVP_PKEY_fromdata at line 220. This causes a memory leak of the parameter EVP_PKEY.

 	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY* dh_params = dh;
+	dh = NULL;
-	ctx = EVP_PKEY_CTX_new(dh, NULL);
+	ctx = EVP_PKEY_CTX_new(dh_params, NULL);
 	if (EVP_PKEY_keygen_init(ctx) <= 0 || EVP_PKEY_keygen(ctx, &dh) <= 0) {
 		ERROR_MSG("DH key generation failed");
 		BN_free(bn_p);
 		BN_free(bn_g);
 		OSSL_PARAM_free(params_built);
 		OSSL_PARAM_BLD_free(bld);
 		EVP_PKEY_CTX_free(ctx);
-		EVP_PKEY_free(dh);
+		EVP_PKEY_free(dh_params);
 		return false;
 	}
+	EVP_PKEY_free(dh_params);
🤖 Prompt for AI Agents 
In src/lib/crypto/OSSLDH.cpp around lines 232 to 243, the EVP_PKEY_keygen call
overwrites the existing dh parameter EVP_PKEY (created earlier) causing a memory
leak; before calling EVP_PKEY_keygen(ctx, &dh) either free the old dh
(EVP_PKEY_free(dh)) or use a separate EVP_PKEY* new_dh = NULL and call
EVP_PKEY_keygen(ctx, &new_dh) then EVP_PKEY_free(dh) and assign dh = new_dh (or
swap and free appropriately); ensure you only free each EVP_PKEY once and keep
the existing cleanup sequence intact so no double-free occurs.


BN_free(bn_p);
BN_free(bn_g);
OSSL_PARAM_free(params_built);
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);

#endif

// Create an asymmetric key-pair object to return
OSSLDHKeyPair* kp = new OSSLDHKeyPair();
Expand All @@ -179,7 +259,11 @@ bool OSSLDH::generateKeyPair(AsymmetricKeyPair** ppKeyPair, AsymmetricParameters
*ppKeyPair = kp;

// Release the key
#if OPENSSL_VERSION_NUMBER < 0x30000000L
DH_free(dh);
#else
EVP_PKEY_free(dh);
#endif

return true;
}
Expand All @@ -194,6 +278,7 @@ bool OSSLDH::deriveKey(SymmetricKey **ppSymmetricKey, PublicKey* publicKey, Priv
return false;
}

#if OPENSSL_VERSION_NUMBER < 0x30000000L
// Get keys
DH *pub = ((OSSLDHPublicKey *)publicKey)->getOSSLKey();
DH *priv = ((OSSLDHPrivateKey *)privateKey)->getOSSLKey();
Expand Down Expand Up @@ -228,6 +313,58 @@ bool OSSLDH::deriveKey(SymmetricKey **ppSymmetricKey, PublicKey* publicKey, Priv

// We compensate that OpenSSL removes leading zeros
memcpy(&secret[0] + size - keySize, &derivedSecret[0], keySize);
#else
// Get keys
EVP_PKEY *pub = ((OSSLDHPublicKey *)publicKey)->getOSSLKey();
EVP_PKEY *priv = ((OSSLDHPrivateKey *)privateKey)->getOSSLKey();
if (pub == NULL || priv == NULL)
{
ERROR_MSG("Failed to get OpenSSL DH keys");
return false;
}

EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(priv, NULL);
if (!ctx)
{
ERROR_MSG("Failed to create EVP_PKEY_CTX");
return false;
}

if (EVP_PKEY_derive_init(ctx) <= 0)
{
ERROR_MSG("EVP_PKEY_derive_init failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

if (EVP_PKEY_derive_set_peer(ctx, pub) <= 0)
{
ERROR_MSG("EVP_PKEY_derive_set_peer failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

// Determine buffer length
size_t secretLen = 0;
if (EVP_PKEY_derive(ctx, NULL, &secretLen) <= 0)
{
ERROR_MSG("EVP_PKEY_derive size query failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

ByteString secret;
secret.wipe(secretLen);

if (EVP_PKEY_derive(ctx, &secret[0], &secretLen) <= 0)
{
ERROR_MSG("EVP_PKEY_derive failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

EVP_PKEY_CTX_free(ctx);
#endif

*ppSymmetricKey = new SymmetricKey(secret.size() * 8);
if (*ppSymmetricKey == NULL)
Expand Down Expand Up @@ -273,6 +410,7 @@ bool OSSLDH::generateParameters(AsymmetricParameters** ppParams, void* parameter
return false;
}

#if OPENSSL_VERSION_NUMBER < 0x30000000L
DH* dh = DH_new();
if (dh == NULL)
{
Expand All @@ -290,19 +428,69 @@ bool OSSLDH::generateParameters(AsymmetricParameters** ppParams, void* parameter
return false;
}

// Store the DH parameters
DHParameters* params = new DHParameters();

const BIGNUM* bn_p = NULL;
const BIGNUM* bn_g = NULL;

DH_get0_pqg(dh, &bn_p, NULL, &bn_g);
#else
EVP_PKEY_CTX* ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DH, NULL);
if (!ctx)
{
ERROR_MSG("Failed to create EVP_PKEY_CTX");
return false;
}

if (EVP_PKEY_paramgen_init(ctx) <= 0)
{
ERROR_MSG("EVP_PKEY_paramgen_init failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

if (EVP_PKEY_CTX_set_dh_paramgen_prime_len(ctx, bitLen) <= 0)
{
ERROR_MSG("EVP_PKEY_CTX_set_dh_paramgen_prime_len failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

if (EVP_PKEY_CTX_set_dh_paramgen_generator(ctx, 2) <= 0)
{
ERROR_MSG("EVP_PKEY_CTX_set_dh_paramgen_generator failed");
EVP_PKEY_CTX_free(ctx);
return false;
}

EVP_PKEY* dh_params = NULL;
if (EVP_PKEY_paramgen(ctx, &dh_params) <= 0)
{
ERROR_MSG("Failed to generate DH parameters");
EVP_PKEY_CTX_free(ctx);
return false;
}

EVP_PKEY_CTX_free(ctx);

BIGNUM* bn_p = NULL;
BIGNUM* bn_g = NULL;

EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p);
EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g);
#endif
Comment on lines +476 to +478
Copy link

@coderabbitai coderabbitai bot Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check return values of EVP_PKEY_get_bn_param

The OpenSSL 3.0 path doesn't check the return values of EVP_PKEY_get_bn_param. If these calls fail, bn_p and bn_g could be NULL, leading to potential issues when passed to OSSL::bn2ByteString.

Add error checking:

-	EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p);
-	EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g);
+	if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p) ||
+	    !EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g))
+	{
+		ERROR_MSG("Failed to get DH parameters");
+		EVP_PKEY_free(dh_params);
+		BN_free(bn_p);
+		BN_free(bn_g);
+		return false;
+	}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/lib/crypto/OSSLDH.cpp around lines 476 to 478, the OpenSSL 3.0 calls to
EVP_PKEY_get_bn_param are made without checking their return values so bn_p and
bn_g may be NULL; modify the code to check the int return value of each
EVP_PKEY_get_bn_param call, handle failures by cleaning up any allocated
resources and returning or throwing an error (or logging and returning an error
code) before calling OSSL::bn2ByteString, and ensure bn_p/bn_g are only used
when the getters succeed and are freed appropriately on error paths.


// Store the DH parameters
DHParameters* params = new DHParameters();
ByteString p = OSSL::bn2ByteString(bn_p); params->setP(p);
ByteString g = OSSL::bn2ByteString(bn_g); params->setG(g);

*ppParams = params;

#if OPENSSL_VERSION_NUMBER < 0x30000000L
DH_free(dh);
#else
EVP_PKEY_free(dh_params);
BN_free(bn_p);
BN_free(bn_g);
#endif

return true;
}
Expand Down
Loading
Loading