Migrate OSSLDH to OpenSSL 3.0+ #817
Conversation
📝 WalkthroughWalkthroughAdds OpenSSL-version-gated DH implementations: legacy DH APIs for OpenSSL < 3.0 and EVP_PKEY/OSSL_PARAM_BLD flows for OpenSSL ≥ 3.0 across core DH logic, 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 OSSLDH as OSSLDH
participant OpenSSL as OpenSSL
Caller->>OSSLDH: generateKeyPair(params)
alt OpenSSL < 3.0
OSSLDH->>OpenSSL: DH_new / DH_set0_pqg
OSSLDH->>OpenSSL: DH_generate_key()
OpenSSL-->>OSSLDH: DH keypair
OSSLDH-->>Caller: DH keypair
else OpenSSL ≥ 3.0
OSSLDH->>OpenSSL: OSSL_PARAM_BLD build (P,G[,priv_len])
OSSLDH->>OpenSSL: EVP_PKEY_fromdata / EVP_PKEY_keygen
OpenSSL-->>OSSLDH: EVP_PKEY keypair
OSSLDH-->>Caller: EVP_PKEY keypair
end
Caller->>OSSLDH: deriveKey(private, peerPub)
alt OpenSSL < 3.0
OSSLDH->>OpenSSL: DH_compute_key(peerPub)
OpenSSL-->>OSSLDH: shared secret
OSSLDH-->>Caller: shared secret
else OpenSSL ≥ 3.0
OSSLDH->>OpenSSL: EVP_PKEY_derive_init / EVP_PKEY_derive (query, alloc, derive)
OpenSSL-->>OSSLDH: shared secret
OSSLDH-->>Caller: shared secret
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
src/lib/crypto/test/DHTests.cpp
Outdated
| 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.
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.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
28c33ea to
2683431
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/lib/crypto/OSSLDHPrivateKey.cpp:
- Around line 276-286: PKCS8Decode leaks both PKCS8_PRIV_KEY_INFO* p8 and
EVP_PKEY* pkey; after calling d2i_PKCS8_PRIV_KEY_INFO and EVP_PKCS82PKEY, ensure
you free p8 with PKCS8_PRIV_KEY_INFO_free(p8) and free pkey with
EVP_PKEY_free(pkey) once setFromOSSL has consumed the EVP_PKEY (and also on any
early returns/failure paths), updating OSSLDHPrivateKey::PKCS8Decode to call
those frees and to null-check before freeing.
In @src/lib/crypto/OSSLDHPublicKey.cpp:
- Around line 238-268: The BIGNUMs bn_p, bn_g and bn_pub_key allocated by
OSSL::byteString2bn are leaked on the failure branches; update createOSSLKey so
that any early return after allocating bn_p/bn_g/bn_pub_key frees them (BN_free)
— specifically free bn_p, bn_g, bn_pub_key before returning when
EVP_PKEY_CTX_new_id fails, when OSSL_PARAM_BLD_new fails, and when
OSSL_PARAM_BLD_to_param fails; preserve existing cleanup for ctx and bld
(EVP_PKEY_CTX_free, OSSL_PARAM_BLD_free) and ensure no double-free if ownership
is transferred later (only free the bn_* on error paths prior to successful
ownership transfer).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/test/DHTests.cpp
🧰 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: 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.
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.
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.
📚 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/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPublicKey.hsrc/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.cppsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPrivateKey.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
📚 Learning: 2025-08-18T17:59:30.693Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/MLDSAParameters.h:42-44
Timestamp: 2025-08-18T17:59:30.693Z
Learning: In MLDSAParameters.h, the author prefers to keep PKCS#11 constant references (CKP_ML_DSA_*) rather than explicit numeric values for better code clarity and maintainability.
Applied to files:
src/lib/crypto/OSSLDHPublicKey.cpp
🧬 Code graph analysis (5)
src/lib/crypto/OSSLDHPublicKey.h (5)
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/OSSLDHPrivateKey.cpp (13)
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/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)src/lib/crypto/OSSLDSAPrivateKey.cpp (4)
getOSSLKey(203-208)getOSSLKey(203-203)setFromOSSL(68-98)setFromOSSL(68-68)src/lib/crypto/OSSLRSAPublicKey.cpp (4)
getOSSLKey(116-121)getOSSLKey(116-116)setFromOSSL(73-90)setFromOSSL(73-73)
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/OSSLDHPrivateKey.h (5)
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 (13)
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/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)src/lib/crypto/OSSLDSAPrivateKey.cpp (4)
getOSSLKey(203-208)getOSSLKey(203-203)setFromOSSL(68-98)setFromOSSL(68-68)src/lib/crypto/OSSLRSAPrivateKey.cpp (4)
getOSSLKey(272-277)getOSSLKey(272-272)setFromOSSL(68-123)setFromOSSL(68-68)
src/lib/crypto/OSSLDHPublicKey.cpp (2)
src/lib/crypto/OSSLDHPrivateKey.cpp (17)
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)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/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLDHPrivateKey.cpp (1)
src/lib/crypto/OSSLDHPublicKey.cpp (17)
inDH(69-76)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)
⏰ 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 (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
- GitHub Check: macOS (openssl)
- GitHub Check: Windows (x64, botan)
🔇 Additional comments (18)
src/lib/crypto/OSSLDHPrivateKey.h (3)
38-42: LGTM: Version-gated includes are correctly structured.The conditional includes correctly pull in
<openssl/dh.h>for legacy OpenSSL and<openssl/evp.h>for OpenSSL 3.0+.
51-55: LGTM: Constructor overloads are correctly version-gated.The constructor signatures properly switch between
const DH*andconst EVP_PKEY*based on OpenSSL version.
79-103: LGTM: Public API and internal representation are consistently version-gated.The
getOSSLKey(),setFromOSSL(), internaldhmember, andresetOSSLKey()helper are all properly aligned with the version-specific types.src/lib/crypto/OSSLDHPrivateKey.cpp (6)
40-48: LGTM: Version-gated includes are correctly structured.The conditional includes properly add
<openssl/core_names.h>,<openssl/param_build.h>, and<openssl/provider.h>for OpenSSL 3.0+.
57-68: LGTM: Version-gated constructor implementation.The constructor correctly uses the version-appropriate parameter type and delegates to
setFromOSSL().
71-74: LGTM: Centralized cleanup via resetOSSLKey().The destructor and all setters correctly call
resetOSSLKey()to ensure the internal OpenSSL representation is invalidated when key components change.Also applies to: 86-108
168-202: LGTM: Pre-3.0 setFromOSSL and resetOSSLKey implementations.The legacy path correctly extracts DH parameters and uses
DH_free()for cleanup.
110-158: LGTM: Pre-3.0 PKCS8Encode and PKCS8Decode implementations.Resource management is correct with proper cleanup of
PKCS8_PRIV_KEY_INFO,EVP_PKEY, andDHobjects on all paths.
253-273: LGTM: OpenSSL 3.0 PKCS8Encode implementation.The encoding path correctly handles cleanup of
PKCS8_PRIV_KEY_INFOon all paths.src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: LGTM: Header declarations are correctly version-gated and consistent with private key counterpart.The public key header follows the same pattern as the private key header, with appropriate type switches for OpenSSL 3.0+.
Also applies to: 50-54, 70-93
src/lib/crypto/OSSLDH.cpp (4)
42-48: LGTM: Version-gated includes for OpenSSL 3.0+.
316-367: LGTM: OpenSSL 3.0 deriveKey implementation.The EVP_PKEY_derive flow is correctly implemented with proper error handling and cleanup on all paths.
281-315: LGTM: Pre-3.0 deriveKey implementation with leading zero compensation.The legacy path correctly handles the DH key derivation including compensation for OpenSSL removing leading zeros.
413-433: LGTM: Pre-3.0 generateParameters implementation.The legacy path correctly uses
DH_generate_parameters_exand extracts parameters viaDH_get0_pqg.src/lib/crypto/OSSLDHPublicKey.cpp (4)
39-47: LGTM: Version-gated includes.
50-105: LGTM: Common code including type definition, constructors, destructor, and setters.All setters correctly invalidate the internal OpenSSL representation by calling
resetOSSLKey().
107-186: LGTM: Pre-3.0 implementation of getOSSLKey, setFromOSSL, resetOSSLKey, and createOSSLKey.The legacy path correctly handles DH object creation and cleanup.
224-231: LGTM: OpenSSL 3.0 resetOSSLKey implementation.Correctly uses
EVP_PKEY_free()for the 3.0+ path.
bukka
force-pushed
the
openssl-3-migration-ossldh
branch
from
2683431 to
30c4746
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/lib/crypto/OSSLDH.cpp:
- Around line 473-488: The code calls EVP_PKEY_CTX_free(ctx) early and then
attempts to free ctx again in the EVP_PKEY_get_bn_param error branches, causing
a double-free; fix by removing the redundant EVP_PKEY_CTX_free(ctx) calls in the
error branches (leave the single free already done) or alternatively move the
initial EVP_PKEY_CTX_free(ctx) to after the EVP_PKEY_get_bn_param checks so ctx
is freed exactly once; ensure BN_free(bn_p) is still called when appropriate and
keep ERROR_MSG and return false behavior intact for the
OSSL_PKEY_PARAM_FFC_P/FFC_G error paths.
- Around line 219-230: Replace the EVP_PKEY_fromdata call that currently uses
EVP_PKEY_KEYPAIR with EVP_PKEY_KEY_PARAMETERS since only DH domain parameters
(P, G and optional length) are being initialized (before key generation). Update
the call site EVP_PKEY_fromdata(ctx, &dh, EVP_PKEY_KEYPAIR, params_built) to use
EVP_PKEY_KEY_PARAMETERS so EVP_PKEY_fromdata, the local variable dh, and
subsequent EVP_PKEY_keygen use the parameter-only initialization path correctly;
keep the existing error handling and cleanup logic intact.
In @src/lib/crypto/OSSLDHPrivateKey.cpp:
- Around line 276-288: In OSSLDHPrivateKey::PKCS8Decode, after calling
EVP_PKCS82PKEY(p8) you must check whether the returned EVP_PKEY* pkey is NULL
before calling setFromOSSL and EVP_PKEY_free; if pkey is NULL return false
(ensuring PKCS8_PRIV_KEY_INFO_free(p8) has already been called as in the current
flow), so add a null-check for pkey immediately after the EVP_PKCS82PKEY call
and only call setFromOSSL/EVP_PKEY_free when pkey is non-NULL.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDHPrivateKey.cppsrc/lib/crypto/OSSLDHPrivateKey.hsrc/lib/crypto/OSSLDHPublicKey.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/test/DHTests.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/crypto/test/DHTests.cpp
- src/lib/crypto/OSSLDHPublicKey.cpp
🧰 Additional context used
🧠 Learnings (9)
📓 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: 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.
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.
📚 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.hsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPrivateKey.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/OSSLDHPrivateKey.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/OSSLDHPrivateKey.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.cppsrc/lib/crypto/OSSLDHPublicKey.hsrc/lib/crypto/OSSLDHPrivateKey.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/OSSLDHPrivateKey.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.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 (2)
src/lib/crypto/OSSLDHPublicKey.h (4)
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/OSSLDHPrivateKey.cpp (13)
inDH(61-68)getOSSLKey(161-166)getOSSLKey(161-161)getOSSLKey(291-295)getOSSLKey(291-291)setFromOSSL(169-193)setFromOSSL(169-169)setFromOSSL(298-323)setFromOSSL(298-298)resetOSSLKey(195-202)resetOSSLKey(195-195)resetOSSLKey(325-332)resetOSSLKey(325-325)src/lib/crypto/OSSLRSAPublicKey.cpp (4)
getOSSLKey(116-121)getOSSLKey(116-116)setFromOSSL(73-90)setFromOSSL(73-73)src/lib/crypto/OSSLDSAPublicKey.cpp (4)
getOSSLKey(151-156)getOSSLKey(151-151)setFromOSSL(67-97)setFromOSSL(67-67)
src/lib/crypto/OSSLDHPrivateKey.cpp (5)
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-291)createOSSLKey(234-234)getOSSLKey(109-114)getOSSLKey(109-109)getOSSLKey(190-194)getOSSLKey(190-190)src/lib/crypto/OSSLDSAPrivateKey.cpp (14)
setG(142-151)setG(142-142)setFromOSSL(68-98)setFromOSSL(68-68)setP(120-129)setP(120-120)PKCS8Encode(154-181)PKCS8Encode(154-154)createOSSLKey(211-256)createOSSLKey(211-211)PKCS8Decode(184-200)PKCS8Decode(184-184)getOSSLKey(203-208)getOSSLKey(203-203)src/lib/crypto/OSSLDSAPublicKey.cpp (10)
setG(128-137)setG(128-128)setFromOSSL(67-97)setFromOSSL(67-67)setP(106-115)setP(106-106)createOSSLKey(159-193)createOSSLKey(159-159)getOSSLKey(151-156)getOSSLKey(151-151)src/lib/crypto/OSSLRSAPrivateKey.cpp (12)
setFromOSSL(68-123)setFromOSSL(68-68)setP(132-141)setP(132-132)PKCS8Encode(223-250)PKCS8Encode(223-223)createOSSLKey(280-319)createOSSLKey(280-280)PKCS8Decode(253-269)PKCS8Decode(253-253)getOSSLKey(272-277)getOSSLKey(272-272)src/lib/crypto/OSSLRSAPublicKey.cpp (6)
setFromOSSL(73-90)setFromOSSL(73-73)createOSSLKey(124-155)createOSSLKey(124-124)getOSSLKey(116-121)getOSSLKey(116-116)
⏰ 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 (x86, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with Botan
- GitHub Check: macOS (botan)
- GitHub Check: macOS (openssl)
🔇 Additional comments (7)
src/lib/crypto/OSSLDHPrivateKey.h (1)
38-42: Well-structured version-gated API declarations.The header cleanly separates legacy DH* and OpenSSL 3.x EVP_PKEY* paths with consistent version guards. The introduction of
resetOSSLKey()centralizes cleanup logic. The approach of keeping the member nameddhregardless of type aligns with the PR's goal of minimal changes.Also applies to: 51-55, 79-91, 95-102
src/lib/crypto/OSSLDHPublicKey.h (1)
38-42: Consistent with private key header structure.The public key header mirrors the private key header's version-gated pattern, ensuring a uniform API surface for both key types across OpenSSL versions.
Also applies to: 50-54, 70-82, 86-93
src/lib/crypto/OSSLDH.cpp (2)
318-369: OpenSSL 3.x key derivation path looks correct.The two-phase derivation (size query, then actual derive) follows the standard OpenSSL 3.x pattern. The leading-zero compensation from the legacy path is not needed here because
EVP_PKEY_derivereturns the properly-sized secret.
42-48: Appropriate conditional includes for OpenSSL 3.x APIs.The includes for
core_names.h,param_build.h, andprovider.hare correctly gated for OpenSSL 3.x and match the APIs used in the implementation.src/lib/crypto/OSSLDHPrivateKey.cpp (3)
297-323: Proper BIGNUM extraction and cleanup insetFromOSSL.The OpenSSL 3.x version correctly extracts parameters using
EVP_PKEY_get_bn_paramand frees eachBIGNUMafter conversion toByteString. The conditional setting pattern matches the legacy implementation.
168-193: Correct legacysetFromOSSLimplementation.The extraction of P, G, and private key from the
DH*object usingDH_get0_pqgandDH_get0_keyfollows the established pattern. The returnedconst BIGNUM*pointers are internal to the DH object and correctly not freed here.
334-392: The OpenSSL 3.x implementation is missing the public key derivation—this is a critical bug, not approved code.The
createOSSLKeymethod (lines 335-392) only provides P, G, and the private key toEVP_PKEY_fromdata, but does not compute the public key component. Unlike the OpenSSL < 3.0 version which explicitly computes the public key viaBN_mod_exp(bn_pub_key, bn_g, bn_priv_key, bn_p, ctx), the OpenSSL 3.0+ path relies on a false assumption.
EVP_PKEY_fromdatadoes not automatically derive the public key from only the private key and domain parameters. This can be verified by observing:
- The
generateKeyPairmethod (lines 220-234) explicitly callsEVP_PKEY_keygenafterEVP_PKEY_fromdatato generate the complete keypairOSSLDHPublicKey::createOSSLKeyexplicitly pushesOSSL_PKEY_PARAM_PUB_KEYto the paramsOSSLDHPrivateKey::setFromOSSLonly extracts P, G, and priv_key but not the public key parameterThe fix should compute the public key before calling
EVP_PKEY_fromdata, similar to the < 3.0 implementation, or callEVP_PKEY_keygenafterward likegenerateKeyPairdoes. The current code results in an incomplete key that will fail during serialization/deserialization or operations requiring the public key component.⛔ Skipped due to learnings
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: 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.
| EVP_PKEY* dh = NULL, *new_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; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine OSSLDH.cpp
find . -name "OSSLDH.cpp" -type fRepository: softhsm/SoftHSMv2
Length of output: 88
🏁 Script executed:
# Read the relevant section of the code with context
cat -n src/lib/crypto/OSSLDH.cpp | sed -n '200,240p'Repository: softhsm/SoftHSMv2
Length of output: 1476
🌐 Web query:
OpenSSL EVP_PKEY_fromdata EVP_PKEY_KEYPAIR vs EVP_PKEY_KEY_PARAMETERS DH parameter selection
💡 Result:
Short answer
- EVP_PKEY_fromdata's "selection" controls what you create: EVP_PKEY_KEY_PARAMETERS builds only domain/algorithm parameters (p,g, q/seed/etc. for FFC); EVP_PKEY_KEYPAIR builds a full keypair (parameters + public and private components). [1]
When to use which for DH
- Use EVP_PKEY_KEY_PARAMETERS when you only want/receive DH domain parameters (p,g and optional q/metadata) or when you intend to share/validate or later generate keys from those parameters. This is the choice for parameter negotiation or storage. [1][2]
- Use EVP_PKEY_KEYPAIR when you have the private key material (and public) in addition to the parameters and want a usable EVP_PKEY containing the private key (for key agreement, signing where applicable). [1]
Additional practical notes
- For DH OpenSSL encourages using named safe-prime groups (set "group"/OSSL_PKEY_PARAM_GROUP_NAME) rather than ad-hoc p/g; named groups avoid expensive prime checks and simplify validation. If you construct non-named FIPS186-style parameters you may need extra generation/validation fields (seed, pcounter, gindex) for full FIPS validation. Use KEY_PARAMETERS to hold those domain values; use KEYPAIR only when you also supply private/public key components. [2]
- The provider will ignore params not allowed for the selection; check EVP_PKEY_fromdata_settable() to see valid OSSL_PARAMs for the selection. [1]
References
[1] EVP_PKEY_fromdata (OpenSSL docs).
[2] EVP_PKEY-DH / EVP_PKEY-FFC (OpenSSL docs).
Use EVP_PKEY_KEY_PARAMETERS for domain parameter initialization.
At line 220, EVP_PKEY_fromdata is called with EVP_PKEY_KEYPAIR, but only DH domain parameters (P, G) and optional key-length metadata are provided—no actual key material. The selection should be EVP_PKEY_KEY_PARAMETERS, which is semantically correct for parameter-only initialization. The actual keypair is subsequently generated via EVP_PKEY_keygen (lines 233–234).
🤖 Prompt for AI Agents
In @src/lib/crypto/OSSLDH.cpp around lines 219 - 230, Replace the
EVP_PKEY_fromdata call that currently uses EVP_PKEY_KEYPAIR with
EVP_PKEY_KEY_PARAMETERS since only DH domain parameters (P, G and optional
length) are being initialized (before key generation). Update the call site
EVP_PKEY_fromdata(ctx, &dh, EVP_PKEY_KEYPAIR, params_built) to use
EVP_PKEY_KEY_PARAMETERS so EVP_PKEY_fromdata, the local variable dh, and
subsequent EVP_PKEY_keygen use the parameter-only initialization path correctly;
keep the existing error handling and cleanup logic intact.
| EVP_PKEY_CTX_free(ctx); | ||
|
|
||
| BIGNUM* bn_p = NULL; | ||
| BIGNUM* bn_g = NULL; | ||
|
|
||
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p)) { | ||
| ERROR_MSG("Failed to get DH domain parameter p"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return false; | ||
| } | ||
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g)) { | ||
| ERROR_MSG("Failed to get DH domain parameter g"); | ||
| BN_free(bn_p); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Double-free of ctx in error paths.
After EVP_PKEY_CTX_free(ctx) at line 473, the error handlers at lines 480 and 486 still attempt to free ctx again, causing a double-free.
🐛 Proposed fix
EVP_PKEY_CTX_free(ctx);
BIGNUM* bn_p = NULL;
BIGNUM* bn_g = NULL;
if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p)) {
ERROR_MSG("Failed to get DH domain parameter p");
- EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(dh_params);
return false;
}
if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g)) {
ERROR_MSG("Failed to get DH domain parameter g");
BN_free(bn_p);
- EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(dh_params);
return false;
}📝 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_CTX_free(ctx); | |
| BIGNUM* bn_p = NULL; | |
| BIGNUM* bn_g = NULL; | |
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p)) { | |
| ERROR_MSG("Failed to get DH domain parameter p"); | |
| EVP_PKEY_CTX_free(ctx); | |
| return false; | |
| } | |
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g)) { | |
| ERROR_MSG("Failed to get DH domain parameter g"); | |
| BN_free(bn_p); | |
| EVP_PKEY_CTX_free(ctx); | |
| return false; | |
| } | |
| EVP_PKEY_CTX_free(ctx); | |
| BIGNUM* bn_p = NULL; | |
| BIGNUM* bn_g = NULL; | |
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_P, &bn_p)) { | |
| ERROR_MSG("Failed to get DH domain parameter p"); | |
| EVP_PKEY_free(dh_params); | |
| return false; | |
| } | |
| if (!EVP_PKEY_get_bn_param(dh_params, OSSL_PKEY_PARAM_FFC_G, &bn_g)) { | |
| ERROR_MSG("Failed to get DH domain parameter g"); | |
| BN_free(bn_p); | |
| EVP_PKEY_free(dh_params); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In @src/lib/crypto/OSSLDH.cpp around lines 473 - 488, The code calls
EVP_PKEY_CTX_free(ctx) early and then attempts to free ctx again in the
EVP_PKEY_get_bn_param error branches, causing a double-free; fix by removing the
redundant EVP_PKEY_CTX_free(ctx) calls in the error branches (leave the single
free already done) or alternatively move the initial EVP_PKEY_CTX_free(ctx) to
after the EVP_PKEY_get_bn_param checks so ctx is freed exactly once; ensure
BN_free(bn_p) is still called when appropriate and keep ERROR_MSG and return
false behavior intact for the OSSL_PKEY_PARAM_FFC_P/FFC_G error paths.
| bool OSSLDHPrivateKey::PKCS8Decode(const ByteString& ber) | ||
| { | ||
| int len = ber.size(); | ||
| if (len <= 0) return false; | ||
| const unsigned char* priv = ber.const_byte_str(); | ||
| PKCS8_PRIV_KEY_INFO* p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &priv, len); | ||
| if (p8 == NULL) return false; | ||
| EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); | ||
| PKCS8_PRIV_KEY_INFO_free(p8); | ||
| setFromOSSL(pkey); | ||
| EVP_PKEY_free(pkey); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing null check for pkey after EVP_PKCS82PKEY.
The legacy path (line 151) checks if (pkey == NULL) return false, but the OpenSSL 3.x path at lines 283-285 passes pkey directly to setFromOSSL without checking for NULL. If decoding fails, this will dereference a null pointer.
🐛 Proposed fix
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;📝 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.
| bool OSSLDHPrivateKey::PKCS8Decode(const ByteString& ber) | |
| { | |
| int len = ber.size(); | |
| if (len <= 0) return false; | |
| const unsigned char* priv = ber.const_byte_str(); | |
| PKCS8_PRIV_KEY_INFO* p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &priv, len); | |
| if (p8 == NULL) return false; | |
| EVP_PKEY* pkey = EVP_PKCS82PKEY(p8); | |
| PKCS8_PRIV_KEY_INFO_free(p8); | |
| setFromOSSL(pkey); | |
| EVP_PKEY_free(pkey); | |
| return true; | |
| } | |
| bool OSSLDHPrivateKey::PKCS8Decode(const ByteString& ber) | |
| { | |
| int len = ber.size(); | |
| if (len <= 0) return false; | |
| const unsigned char* priv = ber.const_byte_str(); | |
| PKCS8_PRIV_KEY_INFO* p8 = d2i_PKCS8_PRIV_KEY_INFO(NULL, &priv, len); | |
| if (p8 == NULL) return false; | |
| 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; | |
| } |
🤖 Prompt for AI Agents
In @src/lib/crypto/OSSLDHPrivateKey.cpp around lines 276 - 288, In
OSSLDHPrivateKey::PKCS8Decode, after calling EVP_PKCS82PKEY(p8) you must check
whether the returned EVP_PKEY* pkey is NULL before calling setFromOSSL and
EVP_PKEY_free; if pkey is NULL return false (ensuring
PKCS8_PRIV_KEY_INFO_free(p8) has already been called as in the current flow), so
add a null-check for pkey immediately after the EVP_PKCS82PKEY call and only
call setFromOSSL/EVP_PKEY_free when pkey is non-NULL.
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.