Migrate OSSLDH to OpenSSL 3.0+ #817
Conversation
WalkthroughAdds OpenSSL-version-gated DH implementations: legacy DH APIs for OpenSSL < 3.0 and EVP_PKEY-based flows for OpenSSL ≥ 3.0 across core DH code, public/private DH key classes, PKCS#8 handling, memory management, and a test adjustment for private key length assertion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant OSSL_DH as OSSLDH
participant OpenSSL as OpenSSL
Caller->>OSSL_DH: generateKeyPair(params)
alt OpenSSL < 3.0
OSSL_DH->>OpenSSL: DH_new / DH_set0_pqg
OSSL_DH->>OpenSSL: DH_generate_key()
OpenSSL-->>OSSL_DH: DH object
OSSL_DH-->>Caller: DH keypair
else OpenSSL ≥ 3.0
OSSL_DH->>OpenSSL: OSSL_PARAM_BLD build (P,G[,priv_len])
OSSL_DH->>OpenSSL: EVP_PKEY_fromdata / EVP_PKEY_keygen
OpenSSL-->>OSSL_DH: EVP_PKEY
OSSL_DH-->>Caller: EVP_PKEY keypair
end
Caller->>OSSL_DH: deriveKey(private, peerPub)
alt OpenSSL < 3.0
OSSL_DH->>OpenSSL: DH_compute_key(peerPub)
OpenSSL-->>OSSL_DH: shared secret (may strip leading zeros)
OSSL_DH-->>Caller: shared secret
else OpenSSL ≥ 3.0
OSSL_DH->>OpenSSL: EVP_PKEY_derive_init / EVP_PKEY_derive (query length, allocate, derive)
OpenSSL-->>OSSL_DH: shared secret
OSSL_DH-->>Caller: shared secret
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| priv = (DHPrivateKey*) kp->getPrivateKey(); | ||
|
|
||
| CPPUNIT_ASSERT(priv->getX().bits() == 128); | ||
| CPPUNIT_ASSERT_EQUAL(size_t(1024), priv->getBitLength()); |
There was a problem hiding this comment.
This deserves special comment as I spent quite a lot of time trying to figure out why priv->getX().bits() no longer returns 128. This is because it is internally ignored when DH parameters are provided. This was the case for 1.1.1 too but it was saved directly to the structure so it didn't get reflected after generation. It doesn't really make sense to set it so the test is changed just to check the bit length.
|
So nice seeing devs for stopping using deprecated OSSL members. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
src/lib/crypto/OSSLDHPublicKey.h (2)
50-55: Make pointer-taking constructors explicit.Prevents accidental implicit conversions from DH*/EVP_PKEY* to OSSLDHPublicKey. Low-risk, local change.
-#if OPENSSL_VERSION_NUMBER < 0x30000000L - OSSLDHPublicKey(const DH* inDH); +#if OPENSSL_VERSION_NUMBER < 0x30000000L + explicit OSSLDHPublicKey(const DH* inDH); #else - OSSLDHPublicKey(const EVP_PKEY* inDH); + explicit OSSLDHPublicKey(const EVP_PKEY* inDH); #endif
86-93: Avoid double-free: delete copy/move or implement proper move semantics.This class owns a raw OpenSSL handle (DH*/EVP_PKEY*). The default copy/move will shallow-copy the pointer, risking double-free on destruction. If copying isn’t required (likely), delete copy/move for safety.
public: // Constructors OSSLDHPublicKey(); @@ virtual ~OSSLDHPublicKey(); + + // Non-copyable and non-movable to avoid double-free of the OpenSSL handle. + OSSLDHPublicKey(const OSSLDHPublicKey&) = delete; + OSSLDHPublicKey& operator=(const OSSLDHPublicKey&) = delete; + OSSLDHPublicKey(OSSLDHPublicKey&&) = delete; + OSSLDHPublicKey& operator=(OSSLDHPublicKey&&) = delete;If you need move, implement a custom move that nulls the source’s handle. I can draft that if desired.
src/lib/crypto/OSSLDHPrivateKey.h (2)
51-56: Mark the pointer-taking constructors explicit.Same rationale as the public key header.
-#if OPENSSL_VERSION_NUMBER < 0x30000000L - OSSLDHPrivateKey(const DH* inDH); +#if OPENSSL_VERSION_NUMBER < 0x30000000L + explicit OSSLDHPrivateKey(const DH* inDH); #else - OSSLDHPrivateKey(const EVP_PKEY* inDH); + explicit OSSLDHPrivateKey(const EVP_PKEY* inDH); #endif
95-103: Prevent shallow copies of the owned OpenSSL handle.Same ownership concern here; default copy/move can double-free DH*/EVP_PKEY*. If copying isn’t required in this codebase, delete copy/move.
public: // Constructors OSSLDHPrivateKey(); @@ virtual ~OSSLDHPrivateKey(); + + // Non-copyable and non-movable; avoids double-free of the OpenSSL handle. + OSSLDHPrivateKey(const OSSLDHPrivateKey&) = delete; + OSSLDHPrivateKey& operator=(const OSSLDHPrivateKey&) = delete; + OSSLDHPrivateKey(OSSLDHPrivateKey&&) = delete; + OSSLDHPrivateKey& operator=(OSSLDHPrivateKey&&) = delete;Given the maintainer’s preference for minimal scope (per #815), treating this as optional is fine; at least confirm these classes aren’t copied anywhere. I can provide a repo scan if helpful.
src/lib/crypto/test/DHTests.cpp (1)
94-101: Stale comment and hard-coded 1024; align assertion with loop key size.This block says “fixed private value length” but no fixed X bit-length is set. Also, hard-coding 1024 duplicates the previous assertion and will break if more sizes are added.
- // Generate key-pair with a fixed private value length + // Generate another key-pair; under OpenSSL 3.x the private key bit length equals the parameter size CPPUNIT_ASSERT(dh->generateKeyPair(&kp, p)); priv = (DHPrivateKey*) kp->getPrivateKey(); - CPPUNIT_ASSERT_EQUAL(size_t(1024), priv->getBitLength()); + CPPUNIT_ASSERT_EQUAL(size_t(*k), size_t(priv->getBitLength()));src/lib/crypto/OSSLDH.cpp (1)
356-365: Simplify shared secret handling in OpenSSL 3.0 pathThe OpenSSL 3.0 path creates a ByteString with
wipe(), but this is unnecessary sinceEVP_PKEY_derivewill write the entire buffer. The pattern differs from the pre-3.0 path which needs to handle leading zeros.Simplify the allocation:
ByteString secret; - secret.wipe(secretLen); + secret.resize(secretLen);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cpp(8 hunks)src/lib/crypto/OSSLDHPrivateKey.cpp(7 hunks)src/lib/crypto/OSSLDHPrivateKey.h(2 hunks)src/lib/crypto/OSSLDHPublicKey.cpp(4 hunks)src/lib/crypto/OSSLDHPublicKey.h(2 hunks)src/lib/crypto/test/DHTests.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cpp
🧬 Code graph analysis (5)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPublicKey.h (2)
src/lib/crypto/OSSLDHPublicKey.cpp (15)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)src/lib/crypto/DHPublicKey.h (1)
DHPublicKey(39-71)
src/lib/crypto/OSSLDHPrivateKey.h (1)
src/lib/crypto/OSSLDHPrivateKey.cpp (15)
OSSLDHPrivateKey(52-55)OSSLDHPrivateKey(71-74)inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)
src/lib/crypto/OSSLDHPublicKey.cpp (4)
src/lib/crypto/OSSLDHPublicKey.h (1)
OSSLDHPublicKey(44-97)src/lib/crypto/OSSLDHPrivateKey.cpp (23)
isOfType(81-84)isOfType(81-81)inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)setP(96-101)setP(96-96)setG(103-108)setG(103-103)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(333-381)createOSSLKey(333-333)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)src/lib/crypto/DHPublicKey.cpp (2)
setY(70-73)setY(70-70)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPrivateKey.cpp (3)
src/lib/crypto/OSSLDHPublicKey.cpp (21)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-282)createOSSLKey(234-234)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)src/lib/crypto/OSSLComp.cpp (4)
DH_get0_pqg(100-109)DH_get0_pqg(100-101)DH_get0_key(144-150)DH_get0_key(144-144)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
🔇 Additional comments (4)
src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: No change required — <openssl/core_names.h> is present.
Found in src/lib/crypto/OSSLDHPublicKey.cpp:44; header is fine as-is.src/lib/crypto/OSSLDHPrivateKey.h (1)
38-42: Version-gated includes are correct — core_names already included in OSSLDHPrivateKey.cppLocated at src/lib/crypto/OSSLDHPrivateKey.cpp:45.
src/lib/crypto/OSSLDH.cpp (2)
135-146: Memory leak on error path in OpenSSL < 3.0 key generationIf
DH_new()fails,bn_pandbn_gare properly freed. However, ifDH_set0_pqg()fails on line 150, only then arebn_pandbn_gfreed. According to OpenSSL documentation,DH_set0_pqg()takes ownership of the BIGNUMs on success, so freeing them after a failed call is correct. But this pattern is error-prone and could lead to double-free if misunderstood in future modifications.
220-230: Potential double key generation in OpenSSL 3.0 pathThe code creates an initial EVP_PKEY from parameters using
EVP_PKEY_fromdata, then immediately creates a new context from this key and callsEVP_PKEY_keygenagain. This appears to generate a second key, overwriting the first one. The initial key generation viaEVP_PKEY_fromdatawithEVP_PKEY_KEYPAIRshould already create a complete key pair.Consider removing the redundant key generation:
- 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; - }Likely an incorrect or invalid review comment.
| 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 |
There was a problem hiding this comment.
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.
| EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); | ||
| setFromOSSL(pkey); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing resource cleanup in PKCS8Decode for OpenSSL 3.0
The OpenSSL 3.0 version of PKCS8Decode doesn't free the PKCS8_PRIV_KEY_INFO structure after use, unlike the pre-3.0 version which properly calls PKCS8_PRIV_KEY_INFO_free(p8).
Add the missing cleanup:
EVP_PKEY* pkey = EVP_PKCS82PKEY(p8);
+ PKCS8_PRIV_KEY_INFO_free(p8);
setFromOSSL(pkey);
+ EVP_PKEY_free(pkey);
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); | |
| setFromOSSL(pkey); | |
| return true; | |
| } | |
| EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); | |
| PKCS8_PRIV_KEY_INFO_free(p8); | |
| setFromOSSL(pkey); | |
| EVP_PKEY_free(pkey); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPrivateKey.cpp around lines 283 to 286, the OpenSSL 3.0
branch of PKCS8Decode calls EVP_PKCS82PKEY(p8) and returns true without freeing
the PKCS8_PRIV_KEY_INFO pointer; ensure you call PKCS8_PRIV_KEY_INFO_free(p8)
after converting to EVP_PKEY (and in all early-return/error paths) so p8 is
always freed, i.e., store the result of EVP_PKCS82PKEY, call setFromOSSL on the
resulting EVP_PKEY as before, then call PKCS8_PRIV_KEY_INFO_free(p8) before
returning (and free p8 on error paths too).
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); | ||
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); | ||
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key); |
There was a problem hiding this comment.
Check return values of EVP_PKEY_get_bn_param
Similar to the issue in OSSLDH.cpp, the return values of EVP_PKEY_get_bn_param are not checked. Failed calls could leave the BIGNUM pointers as NULL.
Add error checking:
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p);
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g);
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key);
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p))
+ bn_p = NULL;
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g))
+ bn_g = NULL;
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key))
+ bn_priv_key = NULL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); | |
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); | |
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key); | |
| if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p)) | |
| bn_p = NULL; | |
| if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g)) | |
| bn_g = NULL; | |
| if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key)) | |
| bn_priv_key = NULL; |
| if (!ctx) | ||
| { | ||
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing resource cleanup on early returns in createOSSLKey
In the OpenSSL 3.0 version of createOSSLKey, if EVP_PKEY_CTX_new_id fails, the function returns without freeing the allocated BIGNUMs (bn_p, bn_g, bn_priv_key).
Add cleanup before returning:
if (!ctx)
{
ERROR_MSG("Could not create EVP_PKEY_CTX object");
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_priv_key);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ctx) | |
| { | |
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | |
| return; | |
| } | |
| if (!ctx) | |
| { | |
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | |
| BN_free(bn_p); | |
| BN_free(bn_g); | |
| BN_free(bn_priv_key); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPrivateKey.cpp around lines 342-346, the early return
when EVP_PKEY_CTX_new_id fails leaks the previously allocated BIGNUMs; before
returning free bn_p and bn_g with BN_free (or BN_clear_free for sensitive
buffers) and clear/free bn_priv_key with BN_clear_free, then proceed to return.
Ensure you only free pointers that are non-NULL to avoid double-free.
| { | ||
| ERROR_MSG("Could not create OSSL_PARAM_BLD"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing BIGNUM cleanup on OSSL_PARAM_BLD allocation failure
Similar resource leak when OSSL_PARAM_BLD_new() fails.
Add cleanup:
if (!bld)
{
ERROR_MSG("Could not create OSSL_PARAM_BLD");
EVP_PKEY_CTX_free(ctx);
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_priv_key);
return;
}🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPrivateKey.cpp around lines 350-354, when
OSSL_PARAM_BLD_new() fails the function logs an error, frees ctx and returns but
does not free the BIGNUMs allocated earlier; update the failure path to free all
BIGNUMs allocated in this function (the DH key component BNs created before the
OSSL_PARAM_BLD allocation) using BN_free (or appropriate cleanup) before freeing
ctx and returning so no BIGNUM memory is leaked.
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | ||
| OSSL_PARAM_BLD_free(bld); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing BIGNUM cleanup on OSSL_PARAM_BLD_to_param failure
Another resource leak when parameter building fails.
Add cleanup:
if (!params)
{
ERROR_MSG("OSSL_PARAM_BLD_to_param failed");
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_priv_key);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | |
| OSSL_PARAM_BLD_free(bld); | |
| EVP_PKEY_CTX_free(ctx); | |
| return; | |
| } | |
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | |
| OSSL_PARAM_BLD_free(bld); | |
| EVP_PKEY_CTX_free(ctx); | |
| BN_free(bn_p); | |
| BN_free(bn_g); | |
| BN_free(bn_priv_key); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPrivateKey.cpp around lines 363 to 367, the error path
that handles OSSL_PARAM_BLD_to_param failure frees bld and ctx then returns but
does not free previously allocated BIGNUMs; free all BIGNUMs allocated earlier
on this code path (use BN_clear_free for any private/secret BIGNUMs and BN_free
for public/non-secret ones) before freeing bld and ctx and returning so there is
no BIGNUM leak.
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); | ||
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); | ||
| EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key); |
There was a problem hiding this comment.
Check return values of EVP_PKEY_get_bn_param
The same issue as in OSSLDHPrivateKey.cpp - unchecked return values could lead to NULL pointers being passed to OSSL::bn2ByteString.
Add error checking:
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p);
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g);
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key);
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p))
+ bn_p = NULL;
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g))
+ bn_g = NULL;
+ if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key))
+ bn_pub_key = NULL;| if (!ctx) | ||
| { | ||
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing resource cleanup on early returns in createOSSLKey
Similar to OSSLDHPrivateKey.cpp, BIGNUMs are not freed when EVP_PKEY_CTX_new_id fails.
Add cleanup:
if (!ctx)
{
ERROR_MSG("Could not create EVP_PKEY_CTX object");
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_pub_key);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ctx) | |
| { | |
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | |
| return; | |
| } | |
| if (!ctx) | |
| { | |
| ERROR_MSG("Could not create EVP_PKEY_CTX object"); | |
| BN_free(bn_p); | |
| BN_free(bn_g); | |
| BN_free(bn_pub_key); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPublicKey.cpp around lines 243-247, the early return
when EVP_PKEY_CTX_new_id() fails leaks previously allocated BIGNUMs; before
returning free all BIGNUMs created earlier (e.g., BN_free(p), BN_free(g) and any
other BN_* variables allocated in this function) and release any other resources
allocated prior to the ctx creation (e.g., free any EVP_PKEY or parameter
objects if created) to ensure no memory is leaked on this error path.
| { | ||
| ERROR_MSG("Could not create OSSL_PARAM_BLD"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing BIGNUM cleanup on OSSL_PARAM_BLD allocation failure
BIGNUMs not freed when OSSL_PARAM_BLD_new() fails.
Add cleanup:
if (!bld)
{
ERROR_MSG("Could not create OSSL_PARAM_BLD");
EVP_PKEY_CTX_free(ctx);
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_pub_key);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| ERROR_MSG("Could not create OSSL_PARAM_BLD"); | |
| EVP_PKEY_CTX_free(ctx); | |
| return; | |
| } | |
| { | |
| ERROR_MSG("Could not create OSSL_PARAM_BLD"); | |
| EVP_PKEY_CTX_free(ctx); | |
| BN_free(bn_p); | |
| BN_free(bn_g); | |
| BN_free(bn_pub_key); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPublicKey.cpp around lines 251–255, when
OSSL_PARAM_BLD_new() fails the code logs an error and frees ctx but does not
free previously allocated BIGNUMs; update the failure path to BN_free each
BIGNUM allocated earlier (e.g. p, g, pub_key or whatever BIGNUM variables were
created) before calling EVP_PKEY_CTX_free(ctx) and returning, and also free any
other OpenSSL objects allocated prior to the OSSL_PARAM_BLD_new() call (cleanup
all allocated BIGNUMs and related objects on this error path).
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | ||
| OSSL_PARAM_BLD_free(bld); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing BIGNUM cleanup on OSSL_PARAM_BLD_to_param failure
BIGNUMs not freed when parameter building fails.
Add cleanup:
if (!params)
{
ERROR_MSG("OSSL_PARAM_BLD_to_param failed");
OSSL_PARAM_BLD_free(bld);
EVP_PKEY_CTX_free(ctx);
+ BN_free(bn_p);
+ BN_free(bn_g);
+ BN_free(bn_pub_key);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | |
| OSSL_PARAM_BLD_free(bld); | |
| EVP_PKEY_CTX_free(ctx); | |
| return; | |
| } | |
| ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); | |
| OSSL_PARAM_BLD_free(bld); | |
| EVP_PKEY_CTX_free(ctx); | |
| BN_free(bn_p); | |
| BN_free(bn_g); | |
| BN_free(bn_pub_key); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLDHPublicKey.cpp around lines 264-268, the error path after
OSSL_PARAM_BLD_to_param failure frees bld and ctx but never releases the BIGNUMs
allocated earlier; free the local BIGNUM variables (e.g., p, g, pub_key or
whatever BIGNUM names were allocated) using BN_free or BN_clear_free before
returning, then proceed to OSSL_PARAM_BLD_free(bld) and EVP_PKEY_CTX_free(ctx)
as already done to avoid memory leaks.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
bbf834f to
28c33ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/lib/crypto/OSSLDH.cpp (1)
476-478: Check return values ofEVP_PKEY_get_bn_param.The return values are not checked. If extraction fails,
bn_pandbn_gremain NULL, andOSSL::bn2ByteStringwill return empty ByteStrings, resulting in invalid DHParameters being stored.- 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; + }src/lib/crypto/OSSLDHPublicKey.cpp (2)
242-268: Missing BIGNUM cleanup on early return paths increateOSSLKey.BIGNUMs allocated at lines 238-240 are not freed when
EVP_PKEY_CTX_new_id(line 243),OSSL_PARAM_BLD_new(line 250), orOSSL_PARAM_BLD_to_param(line 262) fail.if (!ctx) { ERROR_MSG("Could not create EVP_PKEY_CTX object"); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; } OSSL_PARAM_BLD *bld = OSSL_PARAM_BLD_new(); if (!bld) { ERROR_MSG("Could not create OSSL_PARAM_BLD"); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; } ... if (!params) { ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); OSSL_PARAM_BLD_free(bld); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_free(bn_pub_key); return; }
199-202: UncheckedEVP_PKEY_get_bn_paramreturn values.While the code handles NULL BIGNUMs gracefully, silently ignoring extraction failures could mask underlying issues with malformed keys.
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key); + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p)) + bn_p = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g)) + bn_g = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PUB_KEY, &bn_pub_key)) + bn_pub_key = NULL;src/lib/crypto/OSSLDHPrivateKey.cpp (3)
276-286: Missing resource cleanup inPKCS8Decode.The
PKCS8_PRIV_KEY_INFO* p8(line 281) andEVP_PKEY* pkey(line 283) are not freed before returning, causing memory leaks.EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); + PKCS8_PRIV_KEY_INFO_free(p8); + if (pkey == NULL) return false; setFromOSSL(pkey); + EVP_PKEY_free(pkey); return true;
341-367: Missing BIGNUM cleanup on early return paths increateOSSLKey.BIGNUMs allocated at lines 337-339 are not freed when
EVP_PKEY_CTX_new_id,OSSL_PARAM_BLD_new, orOSSL_PARAM_BLD_to_paramfail. This is particularly concerning forbn_priv_keywhich contains sensitive key material.if (!ctx) { ERROR_MSG("Could not create EVP_PKEY_CTX object"); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; } OSSL_PARAM_BLD *bld = OSSL_PARAM_BLD_new(); if (!bld) { ERROR_MSG("Could not create OSSL_PARAM_BLD"); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; } ... if (!params) { ERROR_MSG("OSSL_PARAM_BLD_to_param failed"); OSSL_PARAM_BLD_free(bld); EVP_PKEY_CTX_free(ctx); + BN_free(bn_p); + BN_free(bn_g); + BN_clear_free(bn_priv_key); return; }Note: Use
BN_clear_freeforbn_priv_keyto securely wipe the private key material.
298-301: UncheckedEVP_PKEY_get_bn_paramreturn values.Same issue as in OSSLDHPublicKey.cpp - extraction failures are silently ignored.
- EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g); - EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key); + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_P, &bn_p)) + bn_p = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_FFC_G, &bn_g)) + bn_g = NULL; + if (!EVP_PKEY_get_bn_param(inDH, OSSL_PKEY_PARAM_PRIV_KEY, &bn_priv_key)) + bn_priv_key = NULL;
🧹 Nitpick comments (1)
src/lib/crypto/test/DHTests.cpp (1)
94-99: Stale comment after test modification.The comment "Generate key-pair with a fixed private value length" at line 94 is now misleading since the
setXBitLength(128)call was removed. The test no longer sets a fixed private value length; it simply generates another key-pair and verifies the overall key length.Consider updating the comment to reflect the actual test intent:
- // Generate key-pair with a fixed private value length + // Generate another key-pair and verify bit length CPPUNIT_ASSERT(dh->generateKeyPair(&kp, p));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cpp(8 hunks)src/lib/crypto/OSSLDHPrivateKey.cpp(7 hunks)src/lib/crypto/OSSLDHPrivateKey.h(2 hunks)src/lib/crypto/OSSLDHPublicKey.cpp(4 hunks)src/lib/crypto/OSSLDHPublicKey.h(2 hunks)src/lib/crypto/test/DHTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLDHPrivateKey.h
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-11-11T19:42:46.886Z
Learnt from: JoaoHenrique12
Repo: softhsm/SoftHSMv2 PR: 823
File: src/lib/SoftHSM.cpp:58-58
Timestamp: 2025-11-11T19:42:46.886Z
Learning: SoftHSM include policy: Algorithm key/parameter headers (e.g., SLHParameters, SLH*Key) are included unconditionally in SoftHSM.cpp/SoftHSM.h; feature gating is handled in mechanism tables and CryptoFactory (WITH_*). Avoid wrapping such includes in WITH_* guards.
Applied to files:
src/lib/crypto/OSSLDH.cpp
📚 Learning: 2025-10-13T09:42:31.270Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.270Z
Learning: In src/lib/SoftHSM.cpp, SoftHSM::AsymDecryptInit should use MechParamCheckRSAPKCSOAEP for CKM_RSA_PKCS_OAEP to keep OAEP parameter validation identical to AsymEncryptInit, and avoid redundant manual checks for pMechanism->pParameter and MGF allowlists.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-27T16:44:57.865Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:5550-5596
Timestamp: 2025-10-27T16:44:57.865Z
Learning: SoftHSM ML‑DSA additional context: avoid transient malloc in SoftHSM::{AsymSignInit, AsymVerifyInit}; let Session::setParameters deep‑copy SIGN_ADDITIONAL_CONTEXT by allocating one buffer for struct + context bytes and fixing the stored pointer; Session::resetOp frees m_pParameters so both struct and context are released.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:4193-4196
Timestamp: 2025-08-19T09:23:25.434Z
Learning: MLDSA parameter set inference: In src/lib/crypto/MLDSAPrivateKey.{h,cpp} and MLDSAPublicKey.{h,cpp}, getParameterSet() maps key material length to CKP_ML_DSA_44/65/87, and there is no setParameterSet(). Runtime MLDSA keys used by SoftHSM (e.g., in C_SignInit/C_VerifyInit via MLDSAUtil) rely on this inference; no explicit parameter-set assignment is required.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T17:54:37.645Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: m4/acx_openssl_mldsa.m4:10-35
Timestamp: 2025-08-18T17:54:37.645Z
Learning: For OpenSSL feature detection in autoconf macros, prefer functional tests (like EVP_PKEY_CTX_new_from_name) over version number checks, as the functional approach directly tests the actual capability and is more reliable across different OpenSSL builds where features may be backported or disabled.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-10-13T09:43:45.885Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.885Z
Learning: SoftHSM OAEP policy: In SoftHSM::MechParamCheckRSAPKCSOAEP (src/lib/SoftHSM.cpp), enforce that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 -> CKG_MGF1_SHA256). Mismatches should return CKR_ARGUMENTS_BAD with "Hash and MGF don't match". This mirrors the existing PSS enforcement.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/SoftHSM.cpp:10044-10055
Timestamp: 2025-08-18T19:46:46.659Z
Learning: In SoftHSM::generateMLDSA (src/lib/SoftHSM.cpp), the project prefers using CKP_ML_DSA_* constants for parameter set checks and logs "Unsupported parameter set: %lu". The code currently returns CKR_PARAMETER_SET_NOT_SUPPORTED for unsupported sets.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/test/DHTests.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPublicKey.cpp
📚 Learning: 2025-08-11T16:42:23.012Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 806
File: src/lib/test/p11test.cpp:81-84
Timestamp: 2025-08-11T16:42:23.012Z
Learning: In SoftHSM test files (like p11test.cpp and cryptotest.cpp), error handling for OSSL_PROVIDER_load() calls for "legacy" and "default" providers is intentionally omitted because these providers won't fail in practice and the test code is kept deliberately simple.
Applied to files:
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cpp
🧬 Code graph analysis (4)
src/lib/crypto/OSSLDH.cpp (1)
src/lib/crypto/OSSLUtil.cpp (4)
byteString2bn(56-61)byteString2bn(56-56)bn2ByteString(42-53)bn2ByteString(42-42)
src/lib/crypto/OSSLDHPublicKey.h (2)
src/lib/crypto/OSSLDHPublicKey.cpp (11)
OSSLDHPublicKey(54-57)OSSLDHPublicKey(79-82)inDH(69-76)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)src/lib/crypto/DHPublicKey.h (1)
DHPublicKey(39-71)
src/lib/crypto/OSSLDHPrivateKey.cpp (2)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)resetOSSLKey(143-150)resetOSSLKey(143-143)resetOSSLKey(224-231)resetOSSLKey(224-224)setG(93-98)setG(93-93)setFromOSSL(117-141)setFromOSSL(117-117)setFromOSSL(197-222)setFromOSSL(197-197)setP(86-91)setP(86-86)createOSSLKey(153-186)createOSSLKey(153-153)createOSSLKey(234-282)createOSSLKey(234-234)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPublicKey.cpp (1)
src/lib/crypto/OSSLDHPrivateKey.cpp (21)
inDH(61-68)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(323-330)resetOSSLKey(323-323)setP(96-101)setP(96-96)setG(103-108)setG(103-103)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(289-293)getOSSLKey(289-289)createOSSLKey(205-249)createOSSLKey(205-205)createOSSLKey(333-381)createOSSLKey(333-333)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(296-321)setFromOSSL(296-296)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
- GitHub Check: Linux with Botan
- GitHub Check: Linux with OpenSSL 1.1.1
🔇 Additional comments (5)
src/lib/crypto/OSSLDH.cpp (1)
316-367: OpenSSL 3.x derive path looks correct.The derivation flow properly initializes the context, sets the peer, queries the secret length, allocates the buffer, performs the derivation, and cleans up the context on all paths.
src/lib/crypto/OSSLDHPublicKey.h (1)
38-97: Header correctly implements dual-backend interface.The version-gated declarations maintain type consistency between the member variable, constructor, getter, and setter signatures. The
resetOSSLKey()helper centralizes cleanup logic.src/lib/crypto/OSSLDHPublicKey.cpp (1)
50-105: Clean implementation of type checking and setter invalidation.The
isOfTypemethod and setters that callresetOSSLKey()to invalidate the cached OpenSSL representation when components change follow a clean pattern consistent with the private key class.src/lib/crypto/OSSLDHPrivateKey.cpp (2)
168-202: Legacy path implementation is clean.The pre-3.0
setFromOSSL,resetOSSLKey, and unchangedcreateOSSLKeyfollow established patterns with proper resource management.
375-380: Final cleanup increateOSSLKeyhandles the success path correctly.All allocated resources (params, bld, ctx, BNs) are properly freed after successful key creation.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
2 participants
This removes OpenSSL 3.0+ deprecations by using the right APIs. Specifically it uses EVP_PKEY for keys and param API for operations. It tries to create a minimal diff without changing much for OpenSSL 1.1 and keep the common code the same. The idea is that when at some point in the future the 1.1 support is dropped, it will be easier to drop it from code - that's why it doesn't try to use some special compatibility macros.
This is the first part for DH and the idea is start working on other parts (DSA, RSA, EC...) once this is accepted.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.