Refactor rsa/add modern hashs in OAEP encrypt-decrypt #833
Conversation
📝 WalkthroughWalkthroughAdds session-owned RSA-OAEP parameter construction/propagation, extends asymmetric encrypt/decrypt/wrap/unwrap APIs to carry optional parameter blocks, relaxes OAEP mech validation, and migrates OpenSSL RSA codepaths and RSA key classes to EVP_PKEY with OAEP/PSS parameter support across backends and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant SoftHSM as SoftHSM
participant Session as Session (param store)
participant AsymAlg as AsymmetricAlgorithm
participant RSAImpl as OSSLRSA (EVP)
participant Key as EVP_PKEY
note over App,SoftHSM: OAEP encrypt flow with param propagation
App->>SoftHSM: C_EncryptInit(mech=CKM_RSA_PKCS_OAEP, pParams)
SoftHSM->>Session: BuildRSAOAEPParam(pParams) — alloc/copy -> (param,paramLen)
SoftHSM-->>App: CKR_OK
App->>SoftHSM: C_Encrypt(data)
SoftHSM->>AsymAlg: encrypt(pubKey, data, padding, param, paramLen)
AsymAlg->>RSAImpl: encrypt(pubEVP_PKEY, data, padding, param, paramLen)
RSAImpl->>RSAImpl: EVP_PKEY_CTX init / set OAEP hash/mgf/pSource (if param)
RSAImpl->>Key: EVP_PKEY_encrypt -> ciphertext
RSAImpl-->>AsymAlg: ciphertext
AsymAlg-->>SoftHSM: ciphertext
SoftHSM-->>App: ciphertext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/lib/crypto/OSSLRSAPublicKey.cpp (1)
129-199: Memory leaks:EVP_PKEY_new()allocation orphaned and BIGNUMs not freed.Two memory management issues:
EVP_PKEY leak: Line 134 allocates an
EVP_PKEYviaEVP_PKEY_new(), butEVP_PKEY_fromdataat line 188 creates a newEVP_PKEYand overwrites thersapointer, leaking the original allocation.BIGNUM leak:
bn_nandbn_eallocated at lines 157-158 are never freed.OSSL_PARAM_BLD_push_BNcopies the value internally, so these must be explicitly freed.🔎 Proposed fix
void OSSLRSAPublicKey::createOSSLKey() { if (rsa != NULL) return; - rsa = EVP_PKEY_new(); - if (rsa == NULL) - { - ERROR_MSG("Could not create RSA object"); - return; - } - // Use the OpenSSL implementation and not any engine #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) - + rsa = EVP_PKEY_new(); + if (rsa == NULL) + { + ERROR_MSG("Could not create RSA object"); + return; + } #ifdef WITH_FIPS if (FIPS_mode()) RSA_set_method(rsa, FIPS_rsa_pkcs1_ssleay()); else RSA_set_method(rsa, RSA_PKCS1_SSLeay()); #else RSA_set_method(rsa, RSA_PKCS1_SSLeay()); #endif #else // RSA_set_method(rsa, RSA_PKCS1_OpenSSL()); #endif BIGNUM* bn_n = OSSL::byteString2bn(n); BIGNUM* bn_e = OSSL::byteString2bn(e); #if OPENSSL_VERSION_NUMBER >= 0x30000000L OSSL_PARAM_BLD *param_bld = OSSL_PARAM_BLD_new(); if ((param_bld == NULL) || (bn_n == NULL) || (bn_e == NULL) || (OSSL_PARAM_BLD_push_BN(param_bld,"n",bn_n) <= 0 ) || (OSSL_PARAM_BLD_push_BN(param_bld,"e",bn_e) <= 0 )) { OSSL_PARAM_BLD_free(param_bld); + BN_free(bn_n); + BN_free(bn_e); ERROR_MSG("Could not build RSA key parameters"); - EVP_PKEY_free(rsa); - rsa = NULL; return; } OSSL_PARAM *params = OSSL_PARAM_BLD_to_param(param_bld); OSSL_PARAM_BLD_free(param_bld); + BN_free(bn_n); + BN_free(bn_e); EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_from_name(NULL, "RSA", NULL); if (ctx == NULL) { ERROR_MSG("Could not create RSA creation context"); OSSL_PARAM_free(params); - EVP_PKEY_free(rsa); - rsa = NULL; return; } if ((EVP_PKEY_fromdata_init(ctx) <= 0) || (EVP_PKEY_fromdata(ctx, &rsa, EVP_PKEY_PUBLIC_KEY, params) <= 0)) { ERROR_MSG("Could not create public RSA key object"); OSSL_PARAM_free(params); EVP_PKEY_CTX_free(ctx); - EVP_PKEY_free(rsa); - rsa = NULL; return; } - OSSL_PARAM_free(params); + OSSL_PARAM_free(params); EVP_PKEY_CTX_free(ctx); #endif }src/lib/crypto/OSSLRSAPrivateKey.cpp (2)
69-133: Memory leak: BIGNUMs extracted insetFromOSSLare never freed.The function extracts 8 BIGNUMs via
EVP_PKEY_get_bn_parambut never callsBN_freeon them. Unlike the public key version which correctly freesbn_nandbn_e, this function leaks all allocated BIGNUMs.🔎 Proposed fix - add BN_free after each ByteString conversion
if (bn_p) { ByteString inP = OSSL::bn2ByteString(bn_p); setP(inP); + BN_free(bn_p); } if (bn_q) { ByteString inQ = OSSL::bn2ByteString(bn_q); setQ(inQ); + BN_free(bn_q); } if (bn_dmp1) { ByteString inDP1 = OSSL::bn2ByteString(bn_dmp1); setDP1(inDP1); + BN_free(bn_dmp1); } if (bn_dmq1) { ByteString inDQ1 = OSSL::bn2ByteString(bn_dmq1); setDQ1(inDQ1); + BN_free(bn_dmq1); } if (bn_iqmp) { ByteString inPQ = OSSL::bn2ByteString(bn_iqmp); setPQ(inPQ); + BN_free(bn_iqmp); } if (bn_n) { ByteString inN = OSSL::bn2ByteString(bn_n); setN(inN); + BN_free(bn_n); } if (bn_e) { ByteString inE = OSSL::bn2ByteString(bn_e); setE(inE); + BN_free(bn_e); } if (bn_d) { ByteString inD = OSSL::bn2ByteString(bn_d); setD(inD); + BN_free(bn_d); }
286-384: Memory leaks:EVP_PKEY_new()orphaned and 8 BIGNUMs never freed.Same issues as the public key version:
EVP_PKEY leak:
EVP_PKEY_new()at line 291 is orphaned whenEVP_PKEY_fromdataat line 369 creates a new key and overwritesrsa.BIGNUM leaks: All 8 BIGNUMs allocated at lines 313-320 (
bn_p,bn_q,bn_dmp1,bn_dmq1,bn_iqmp,bn_n,bn_d,bn_e) are never freed.OSSL_PARAM_BLD_push_BNcopies the data internally.🔎 Proposed fix structure
void OSSLRSAPrivateKey::createOSSLKey() { if (rsa != NULL) return; - rsa = EVP_PKEY_new(); - if (rsa == NULL) - { - ERROR_MSG("Could not create RSA object"); - return; - } // Use the OpenSSL implementation and not any engine #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) + rsa = EVP_PKEY_new(); + if (rsa == NULL) + { + ERROR_MSG("Could not create RSA object"); + return; + } // ... legacy path ... #endif BIGNUM *bn_p = OSSL::byteString2bn(p); // ... other BIGNUMs ... #if OPENSSL_VERSION_NUMBER >= 0x30000000L // ... param building ... OSSL_PARAM *params = OSSL_PARAM_BLD_to_param(param_bld); OSSL_PARAM_BLD_free(param_bld); + // Free all BIGNUMs after params are built + BN_free(bn_p); + BN_free(bn_q); + BN_free(bn_dmp1); + BN_free(bn_dmq1); + BN_free(bn_iqmp); + BN_free(bn_n); + BN_free(bn_d); + BN_free(bn_e); // ... rest of function, remove EVP_PKEY_free(rsa) from error paths ... #endif }src/lib/SoftHSM.cpp (4)
6899-6905: DES3 CBC wrapping key type check uses||and is always trueThe condition:
if (pMechanism->mechanism == CKM_DES3_CBC && (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT;is logically always true: a key type cannot be both
CKK_DES2andCKK_DES3at the same time, so at least one inequality is always satisfied.
- This means CKM_DES3_CBC wrapping will always fail with
CKR_WRAPPING_KEY_TYPE_INCONSISTENT, regardless of whether the key is DES2 or DES3.- The same pattern appears mirrored in
C_UnwrapKeyfor DES3 CBC unwrapping.This should likely be
&&instead of||to mean “not DES2 and not DES3”.Suggested fix for both wrap and unwrap checks
- if (pMechanism->mechanism == CKM_DES3_CBC && (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if (pMechanism->mechanism == CKM_DES3_CBC && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT;and similarly in
C_UnwrapKeyfor the DES3 CBC key type check.
13403-13429: MechParamCheckRSAPKCSOAEP missing hash/MGF matching enforcementThe label checks for
source == CKZ_DATA_SPECIFIEDand rejection of(pSourceData == NULL && ulSourceDataLen != 0)are correct. However, the hash/MGF correspondence check is missing:mgfmust match the selectedhashAlg(e.g.,CKM_SHA256→CKG_MGF1_SHA256). The PSS code enforces this with "Hash and MGF don't match"; OAEP should enforce the same constraint in this validation function to maintain consistency and prevent mismatched parameter combinations.
6582-6671: OAEP MGF-hash correspondence validation missing in WrapKeyAsymThe code parses
hashAlgandmgfindependently without validating that the MGF variant matches the selected hash algorithm (e.g., SHA-256 must pair with CKG_MGF1_SHA256). PSS sign/verify enforce this correspondence with "Hash and MGF don't match" rejection; OAEP operations should mirror this validation for consistency.Add a post-parsing check: after determining
hashAlg, validate thatmgfmatches the corresponding expected value, or explicitly document and test why OAEP allows arbitrary combinations.Also update the inline comment "SHA-1 is the only supported option" (line 6659) if all hash variants are now intended to be supported.
7267-7323: UnwrapKeyAsym resource leak and missing OAEP hash/MGF validationBoth
return CKR_ARGUMENTS_BAD;paths in the OAEP parameter validation block (hashAlg and mgf switch defaults) occur aftercipherandunwrappingkeyare allocated but before the cleanup calls at the function end, leaking both resources on validation failure.Additionally, UnwrapKeyAsym accepts any hashAlg and mgf combination without validating correspondence. Per the OAEP policy (PR #818), the MGF1 variant must match the selected hash algorithm (e.g., SHA-256 requires CKG_MGF1_SHA256), mirroring the enforcement in AsymDecryptInit. This validation is currently missing.
Recommend:
- Add a cleanup label and replace early returns with
rv = CKR_ARGUMENTS_BAD; goto cleanup;to ensure resources are recycled on error.- Enforce hash/MGF correspondence matching in the parameter validation, either inline or via the existing (but incomplete)
MechParamCheckRSAPKCSOAEPhelper enhanced to include this check.
🧹 Nitpick comments (8)
src/lib/crypto/test/RSATests.cpp (2)
692-693: Clarify pointer to OAEP parameters struct.Taking the address of
par->hashAlginstead of the struct itself (&(*par)) works becausehashAlgis the first member, but this is fragile and may confuse readers.🔎 Suggested clarification
- void *parameters = &(par->hashAlg); + void *parameters = &(*par);
715-717: Minor formatting inconsistency.Inconsistent spacing around
<=and missing indentation alignment on line 717.🔎 Suggested fix
- if ((*k >> 3) <= (hashLen*2)+2) - continue; //skip test - hash too long for key size - CPPUNIT_ASSERT(rng->generateRandom(testData, (*k >> 3) - 2 - hashLen*2)); + if ((*k >> 3) <= (hashLen * 2) + 2) + continue; // skip test - hash too long for key size + CPPUNIT_ASSERT(rng->generateRandom(testData, (*k >> 3) - 2 - hashLen * 2));src/lib/crypto/OSSLRSAPublicKey.h (1)
46-46: EVP_PKEY migration looks correct.The type changes from
RSA*toEVP_PKEY*across constructor, setter, getter, and member are consistent with OpenSSL 3.0+ EVP-based API usage.Consider renaming the member from
rsatopkeyfor clarity, since it's now anEVP_PKEY*rather than anRSA*.Also applies to: 62-62, 65-65, 69-69
src/lib/crypto/OSSLEDDSA.cpp (1)
210-225: LGTM with minor formatting nit.Extended
encryptanddecryptsignatures align with the API evolution pattern. Minor formatting inconsistency: space before comma (/*param*/ ,) differs from other files.🔎 Formatting consistency
bool OSSLEDDSA::encrypt(PublicKey* /*publicKey*/, const ByteString& /*data*/, - ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/,const void* /*param*/ , const size_t /*paramLen*/) + ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, + const void* /*param*/, const size_t /*paramLen*/) ... bool OSSLEDDSA::decrypt(PrivateKey* /*privateKey*/, const ByteString& /*encryptedData*/, - ByteString& /*data*/, const AsymMech::Type /*padding*/,const void* /*param*/ , const size_t /*paramLen*/) + ByteString& /*data*/, const AsymMech::Type /*padding*/, + const void* /*param*/, const size_t /*paramLen*/)src/lib/crypto/AsymmetricAlgorithm.h (2)
119-125: Consider usingconst void*forsourceData.The OAEP source/label data is read-only input to the encryption operation. Using
const void*would better express this intent and enable passing const buffers without a cast.🔎 Suggested change
struct RSA_PKCS_OAEP_PARAMS { HashAlgo::Type hashAlg; AsymRSAMGF::Type mgf; - void *sourceData; + const void *sourceData; size_t sourceDataLen; };
156-159: LGTM - Wrap/unwrap interface extension.The parameter forwarding interface for key wrapping operations is correctly extended. These delegate to
encrypt/decrypt, enabling OAEP-based key wrapping.Minor nit:
paramLenusessize_there butconst size_tinencrypt/decrypt. While functionally equivalent for pass-by-value, matching the style would improve consistency.src/lib/SoftHSM.cpp (2)
7121-7199: AES_CBC_PAD / DES3_CBC_PAD unwrapping: overall design OK, consider error code for bad paddingThe new
CKM_AES_CBC_PADandCKM_DES3_CBC_PADhandling inUnwrapKeySym:
- Correctly selects AES/DES3, uses CBC mode with
padding = false, and performs manual PKCS#7 unpadding viaRFC5652Unpad, which is consistent with whatWrapKeySymdoes on the wrapping side.- Validates IV length via the checks added in
C_UnwrapKey.One minor point:
- When
RFC5652Unpadfails, you returnCKR_GENERAL_ERRORwith a TODO comment. From a PKCS#11 perspective, an error likeCKR_ENCRYPTED_DATA_INVALIDorCKR_WRAPPED_KEY_INVALIDwould be more descriptive.Not blocking, but worth considering if you touch this code again.
13418-13427: Label null/length checks: semantics now “NULL only when length is zero” – verify callers obeyIn both OAEP helpers you now enforce:
if (params->pSourceData == NULL && params->ulSourceDataLen != 0) return CKR_ARGUMENTS_BAD;(and the equivalent for RSA‑AES OAEP params).
- This is a good strengthening (non‑NULL pointer required whenever length is non‑zero), but it implicitly allows
(non‑NULL, 0); all OAEP callers (encrypt/decrypt/wrap/unwrap) must handle zero‑length labels with non‑NULL pointers gracefully.- Your OAEP parameter packing code always allocates a buffer and sets
sourceDatato a non‑NULL offset even for zero length, which is consistent with this rule.Given the subtlety around these semantics, it would be helpful to add a brief comment that
(pSourceData != NULL, ulSourceDataLen == 0)is intentional and supported.Also applies to: 13467-13471
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.hsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPublicKey.hsrc/lib/crypto/test/RSATests.cppsrc/lib/test/AsymEncryptDecryptTests.cpp
🧰 Additional context used
🧠 Learnings (12)
📓 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: 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.
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.
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.
📚 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/test/AsymEncryptDecryptTests.cppsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/test/RSATests.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLECDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLRSAPublicKey.hsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLECDSA.h
📚 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/test/AsymEncryptDecryptTests.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/test/RSATests.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.cpp
📚 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/test/AsymEncryptDecryptTests.cppsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.hsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/test/RSATests.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLECDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLRSAPublicKey.hsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLECDSA.h
📚 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/test/AsymEncryptDecryptTests.cppsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/AsymmetricAlgorithm.hsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLECDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLECDSA.h
📚 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/test/AsymEncryptDecryptTests.cppsrc/lib/SoftHSM.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/test/AsymEncryptDecryptTests.cppsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.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/OSSLECDSA.cppsrc/lib/crypto/OSSLDH.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/test/RSATests.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLECDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLECDSA.h
📚 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/OSSLECDSA.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLECDSA.h
📚 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/AsymmetricAlgorithm.hsrc/lib/crypto/OSSLGOST.hsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLECDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLEDDSA.hsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (13)
src/lib/crypto/OSSLRSAPrivateKey.h (2)
src/lib/crypto/OSSLRSAPrivateKey.cpp (7)
OSSLRSAPrivateKey(47-50)OSSLRSAPrivateKey(52-57)OSSLRSAPrivateKey(60-63)setFromOSSL(69-133)setFromOSSL(69-69)getOSSLKey(277-283)getOSSLKey(277-277)src/lib/crypto/OSSLRSAPublicKey.cpp (4)
setFromOSSL(74-94)setFromOSSL(74-74)getOSSLKey(120-126)getOSSLKey(120-120)
src/lib/crypto/AsymmetricAlgorithm.h (2)
src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1402-1566)encrypt(1402-1403)decrypt(1569-1711)decrypt(1569-1570)src/lib/crypto/AsymmetricAlgorithm.cpp (4)
wrapKey(159-166)wrapKey(159-160)unwrapKey(168-175)unwrapKey(168-169)
src/lib/crypto/OSSLGOST.h (1)
src/lib/crypto/OSSLGOST.cpp (4)
encrypt(430-437)encrypt(430-432)decrypt(440-447)decrypt(440-442)
src/lib/crypto/OSSLECDH.h (4)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-100)encrypt(93-95)decrypt(103-110)decrypt(103-105)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-103)encrypt(96-98)decrypt(106-113)decrypt(106-108)src/lib/crypto/OSSLECDSA.cpp (4)
encrypt(333-339)encrypt(333-334)decrypt(342-348)decrypt(342-343)src/lib/crypto/OSSLEDDSA.cpp (4)
encrypt(210-216)encrypt(210-211)decrypt(219-225)decrypt(219-220)
src/lib/crypto/OSSLDH.h (2)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-100)encrypt(93-95)decrypt(103-110)decrypt(103-105)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-103)encrypt(96-98)decrypt(106-113)decrypt(106-108)
src/lib/crypto/AsymmetricAlgorithm.cpp (4)
src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1402-1566)encrypt(1402-1403)decrypt(1569-1711)decrypt(1569-1570)src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-100)encrypt(93-95)decrypt(103-110)decrypt(103-105)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-103)encrypt(96-98)decrypt(106-113)decrypt(106-108)src/lib/crypto/OSSLECDSA.cpp (4)
encrypt(333-339)encrypt(333-334)decrypt(342-348)decrypt(342-343)
src/lib/crypto/OSSLDSA.h (1)
src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-446)encrypt(439-441)decrypt(449-456)decrypt(449-451)
src/lib/crypto/OSSLRSA.h (1)
src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1402-1566)encrypt(1402-1403)decrypt(1569-1711)decrypt(1569-1570)
src/lib/crypto/OSSLRSAPublicKey.h (2)
src/lib/crypto/OSSLRSAPublicKey.cpp (5)
OSSLRSAPublicKey(46-49)OSSLRSAPublicKey(51-56)OSSLRSAPublicKey(59-62)setFromOSSL(74-94)setFromOSSL(74-74)src/lib/crypto/RSAPublicKey.h (1)
RSAPublicKey(39-69)
src/lib/crypto/OSSLRSAPublicKey.cpp (3)
src/lib/crypto/OSSLRSAPublicKey.h (1)
OSSLRSAPublicKey(40-73)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)src/lib/crypto/RSAPublicKey.cpp (4)
setN(61-64)setN(61-61)setE(66-69)setE(66-66)
src/lib/crypto/OSSLRSA.cpp (2)
src/lib/crypto/AsymmetricAlgorithm.cpp (16)
sign(48-54)sign(48-50)signInit(56-69)signInit(56-57)signFinal(81-93)signFinal(81-81)signUpdate(71-79)signUpdate(71-71)verify(96-102)verify(96-98)verifyInit(104-117)verifyInit(104-105)verifyFinal(129-141)verifyFinal(129-129)verifyUpdate(119-127)verifyUpdate(119-119)src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLRSAPrivateKey.cpp (2)
src/lib/crypto/OSSLRSAPublicKey.cpp (10)
setFromOSSL(74-94)setFromOSSL(74-74)setN(97-106)setN(97-97)setE(108-117)setE(108-108)createOSSLKey(129-200)createOSSLKey(129-129)getOSSLKey(120-126)getOSSLKey(120-120)src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLECDSA.h (1)
src/lib/crypto/OSSLECDSA.cpp (4)
encrypt(333-339)encrypt(333-334)decrypt(342-348)decrypt(342-343)
🔇 Additional comments (28)
src/lib/test/AsymEncryptDecryptTests.cpp (1)
140-144: Verify intentional removal of hash/MGF matching enforcement.The commented-out test previously validated that mismatched hash (CKM_SHA_1) and MGF (CKG_MGF1_SHA256) should return
CKR_ARGUMENTS_BAD. The comment "This parameter is valid" suggests an intentional policy change.Based on learnings, SoftHSM OAEP policy enforces that the MGF1 variant must correspond to the selected hashAlg (e.g., SHA-256 → CKG_MGF1_SHA256), and mismatches should return
CKR_ARGUMENTS_BAD. Please confirm this is an intentional relaxation of that policy.src/lib/crypto/OSSLDSA.cpp (1)
439-456: LGTM!The extended
encryptanddecryptsignatures with optionalparam/paramLenparameters align with the broader API evolution across asymmetric algorithms. The parameters are correctly commented out as unused since DSA does not support encryption/decryption.src/lib/crypto/OSSLDSA.h (1)
63-68: LGTM!Header declarations match the implementation and provide backward-compatible default values for the new
param/paramLenparameters.src/lib/crypto/test/RSATests.cpp (1)
686-741: Good test coverage for OAEP with labels.The OAEP test loop comprehensively covers:
- Multiple hash/MGF configurations
- Label support (TestLabel)
- Invalid label decryption failure verification
This strengthens OAEP implementation confidence.
src/lib/crypto/OSSLRSAPublicKey.h (1)
38-38: LGTM!Appropriate header change from
openssl/rsa.htoopenssl/evp.hfor EVP_PKEY-based API.src/lib/crypto/OSSLDH.cpp (1)
93-110: LGTM!Extended
encryptanddecryptsignatures with optionalparam/paramLenparameters align with the API evolution pattern. Parameters are correctly marked as unused since DH does not support encryption/decryption.src/lib/crypto/OSSLRSA.h (1)
63-68: LGTM!The extended
encryptanddecryptsignatures enable OAEP parameter passing for RSA operations. Default values ensure backward compatibility with existing call sites.src/lib/crypto/OSSLDH.h (1)
57-62: LGTM!The extended signatures for
encryptanddecryptwith optionalparamandparamLenparameters align with the broader API changes across the PR. Default values preserve backward compatibility, and the implementation correctly returnsfalsesince DH does not support encryption/decryption.src/lib/crypto/OSSLRSAPrivateKey.h (3)
38-38: LGTM!The include change from
<openssl/rsa.h>to<openssl/evp.h>correctly reflects the migration to the EVP_PKEY-based API required for OpenSSL 3.0 compatibility.
46-46: LGTM!Constructor signature updated to accept
EVP_PKEY*aligns with the OpenSSL 3.0 migration away from deprecated low-level RSA APIs.
76-83: LGTM!The migration from
RSA*toEVP_PKEY*for the internal representation, setter, and getter is appropriate for OpenSSL 3.0 compatibility. The implementation inOSSLRSAPrivateKey.cppcorrectly usesEVP_PKEY_get_bn_paramto extract RSA components, follows the lazy-creation pattern, and memory management has been properly updated throughout the implementation to useEVP_PKEY_freeinstead ofRSA_freein the destructor and all cleanup paths.src/lib/crypto/OSSLECDH.cpp (1)
96-112: LGTM!The extended signatures for
encryptanddecryptwith optionalparamandparamLenparameters are correctly implemented. Parameters are properly commented out to suppress unused parameter warnings, and the behavior remains unchanged—ECDH correctly reports it does not support encryption/decryption.src/lib/crypto/OSSLGOST.cpp (1)
430-447: LGTM!The extended signatures for
encryptanddecryptare correctly implemented with commented-out unused parameters. GOST correctly reports it does not support encryption/decryption, and the behavior remains unchanged.src/lib/crypto/OSSLECDSA.cpp (1)
333-348: LGTM!The extended signatures for
encryptanddecryptare correctly implemented. ECDSA correctly reports it does not support encryption/decryption, consistent with the API changes across the PR.src/lib/crypto/OSSLECDSA.h (1)
59-64: LGTM!The extended signatures for
encryptanddecryptwith optionalparamandparamLenparameters are consistent with the implementation and align with the broader API changes across the PR.src/lib/crypto/OSSLECDH.h (1)
57-62: LGTM!The extended signatures for
encryptanddecryptwith optionalparamandparamLenparameters are consistent with the implementation and align with the broader API changes across the PR.src/lib/crypto/OSSLGOST.h (1)
64-69: LGTM!The extended signatures for
encryptanddecryptwith optionalparamandparamLenparameters are consistent with the implementation and align with the broader API changes across the PR.src/lib/crypto/OSSLEDDSA.h (1)
59-64: LGTM - API consistency update.The extended signatures align with the updated
AsymmetricAlgorithmbase class interface and match the pattern used in other algorithm implementations (ECDSA, DSA, DH, GOST, ECDH). Since EDDSA doesn't support encryption/decryption (the implementation returnsfalse), the new parameters are appropriately ignored.src/lib/crypto/AsymmetricAlgorithm.h (1)
149-153: LGTM - Interface extension for OAEP parameter support.The pure virtual signatures are correctly extended to accept optional parameter blocks. Default values maintain backward compatibility with existing call sites. This aligns with the concrete implementations (e.g.,
OSSLRSA::encrypt/decrypt).src/lib/crypto/AsymmetricAlgorithm.cpp (1)
159-175: LGTM - Clean parameter forwarding.The
wrapKeyandunwrapKeyimplementations correctly forward the newparam/paramLenarguments to their respectiveencrypt/decryptcalls. The mechanism validation viaisWrappingMechappropriately includesRSA_PKCS_OAEP(line 150), enabling OAEP-based key wrapping operations. Parameter interpretation is correctly delegated to the concrete algorithm implementations.src/lib/crypto/OSSLRSAPublicKey.cpp (2)
40-56: LGTM!The include for
param_build.his appropriate for the OpenSSL 3+ parameter-based key construction, and the constructor correctly delegates initialization tosetFromOSSL.
74-94: LGTM!The function correctly extracts BIGNUM parameters via
EVP_PKEY_get_bn_paramfor OpenSSL 3+ and properly frees the BIGNUMs after conversion. The conditional guards align with the PR's OpenSSL 3-only targeting.src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
231-274: LGTM!The PKCS#8 encoding and decoding functions correctly use EVP_PKEY-based flows with proper resource cleanup on all paths.
src/lib/crypto/OSSLRSA.cpp (2)
68-140: LGTM!The RSA PKCS#1 signing path correctly uses the EVP_PKEY API with proper context management and error handling.
395-499: LGTM!The PSS parameter validation correctly enforces that the MGF variant corresponds to the selected hash algorithm, aligning with SoftHSM's policy. Based on learnings, this mirrors the existing PSS enforcement pattern.
src/lib/SoftHSM.cpp (3)
2678-2718: Passing OAEP parameters throughAsymEncrypt: confirm backend expectations for(parameters, paramLen)
AsymEncryptnow pullsvoid* parameters = session->getParameters(paramLen);and always forwards(parameters, paramLen)intoasymCrypto->encrypt(...).
- For non‑OAEP RSA mechanisms,
paramLenwill be zero andparametersshould beNULL; this matches how PSS and other asymmetric paths use optional params, but it assumes allAsymmetricAlgorithm::encryptimplementations treat(nullptr, 0)as “no params”.- For OAEP, the backend must now interpret
parametersas aRSA_PKCS_OAEP_PARAMSwith a validsourceDatapointer and use it only during this call.This is fine as long as the
Session::setParameters/ OAEP packing above guarantees a valid, backend‑understood layout and lifetime; once you address the lifetime concern inAsymEncryptInit, this call site looks correct.
3487-3526: AsymDecrypt forwarding of OAEP parameters mirrors encrypt and depends on correct session parameter handling
AsymDecryptnow mirrorsAsymEncryptby callingsession->getParameters(paramLen)and forwarding(parameters, paramLen)intoasymCrypto->decrypt(...).
- This is structurally correct and aligns with how PSS parameters are passed through sign/verify.
- Correctness here is contingent on the OAEP parameter packing in
AsymDecryptInitbeing fixed (no leaks, validsourceDatapointer stored in session) and onAsymmetricAlgorithm::decrypttreating(nullptr, 0)as “no parameters” for non‑OAEP mechanisms.Once init‑time issues are addressed, this call site itself looks good.
13467-13484: MechParamCheckRSAAESKEYWRAP: label handling now permitted; confirm semantics vs OAEP policy
- Allowing non‑NULL
pOAEPParams->pSourceDatawithulSourceDataLen == 0and only rejecting(NULL, non‑zero)is reasonable given how you now use the OAEP label for RSA‑AES key wrap.- As with
MechParamCheckRSAPKCSOAEP, this function no longer checks thatmgfmatcheshashAlg; it only enforces thatmgfis in a numeric range (1–5). That is a behavior change compared to the stricter OAEP policy referenced earlier.If the intent is to fully support OpenSSL 3 OAEP with arbitrary allowed digest/MGF1 combinations, it would be good to:
- Make that explicit in comments around both
MechParamCheckRSAAESKEYWRAPandMechParamCheckRSAPKCSOAEP, and- Ensure tests cover label present/absent and multiple hash/MGF variants for RSA‑AES key wrap as well.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/crypto/OSSLRSAPublicKey.cpp (1)
179-194: Legacy OpenSSL path is broken:rsais never allocated.In the
#elsebranch (OpenSSL < 3.0), the code callsRSA_set_method(rsa, ...)butrsaisNULLat this point—noRSA_new()or equivalent allocation exists. This will cause a crash on older OpenSSL versions.Additionally, the BN objects (
bn_n,bn_e) allocated at lines 137-138 are never freed in the legacy path, causing memory leaks.If OpenSSL 3+ is the only target (per PR description), consider removing the legacy path entirely or adding a compile-time error to prevent building on unsupported versions.
🔎 Suggested approach
If OpenSSL < 3 should be unsupported:
#if OPENSSL_VERSION_NUMBER >= 0x30000000L // ... OpenSSL 3+ code ... #else +#error "OpenSSL 3.0 or later is required" -#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) -// Use the OpenSSL implementation and not any engine -... -#endif #endifOtherwise, the legacy path needs to:
- Allocate an RSA object with
RSA_new()- Set the key components using
RSA_set0_key(rsa, bn_n, bn_e, NULL)- Wrap in EVP_PKEY if the class now stores
EVP_PKEY*src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
368-386: Legacy OpenSSL path is broken:rsais never allocated.Same issue as in
OSSLRSAPublicKey.cpp: the legacy path referencesrsawithout allocating it, and the BN objects allocated at lines 301-308 are never freed in this path.The commented-out code at lines 381-385 shows what was intended but is no longer functional. Either implement proper legacy support or add a compile-time error for unsupported OpenSSL versions.
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
3285-3291: Apply the same OAEP parameter lifetime check to decrypt pathDecrypt init mirrors encrypt: it builds an OAEP buffer
parameterswith struct+label, callssession->setParameters(parameters, paramLen), then frees it (Lines 3328–3392, 3400–3404).AsymDecryptlater retrievesparametersfrom the session and passes it intoasymCrypto->decrypt(Lines 3493–3495, 3528).The same concern as for encrypt applies: if
Session::setParametersonly memcopies without fixingRSA_PKCS_OAEP_PARAMS::sourceData, the pointer in the stored copy will reference freed memory and OAEP label use may misbehave.Once
Session::setParameterssemantics are confirmed and/or fixed as per the encrypt comment, please ensure decrypt benefits from the same guarantee. The duplication of mapping logic between encrypt and decrypt also makes it more error‑prone.Based on learnings, AsymDecryptInit was supposed to share OAEP validation with encrypt; this refactor keeps packing consistent as well.Optional refactor to de‑duplicate OAEP parameter packing
+static CK_RV buildRsaOaepParams(const CK_RSA_PKCS_OAEP_PARAMS* in, + RSA_PKCS_OAEP_PARAMS** outBuf, + size_t* outLen) +{ + if (!in || !outBuf || !outLen) return CKR_ARGUMENTS_BAD; + // map hashAlg -> HashAlgo::Type, mgf -> AsymRSAMGF::Type + // allocate one buffer: sizeof(RSA_PKCS_OAEP_PARAMS) + ulSourceDataLen + // set sourceData pointer inside that buffer and copy label bytes + // on success set *outBuf/*outLen and return CKR_OK +}Then call
buildRsaOaepParamsfrom bothAsymEncryptInitandAsymDecryptInitto ensure identical behavior.Also applies to: 3327-3404, 3493-3495, 3528-3529
🧹 Nitpick comments (8)
src/lib/SoftHSM.cpp (3)
6584-6641: OAEP mapping inWrapKeyAsymlooks correct but duplicates logic and partially bypasses shared validation
WrapKeyAsymnow buildsRSA_PKCS_OAEP_PARAMS oaep_parametersdirectly fromCK_RSA_PKCS_OAEP_PARAMSwhenpMechanism->mechanism == CKM_RSA_PKCS_OAEP, including mapping of hashAlg/mgf and wiringsourceData/sourceDataLento the PKCS#11 label fields (Lines 6584–6641). It then enforces the OAEP input length bound using the selected hash output length (Lines 6667–6672) and passes(parameters, paramLen)intocipher->wrapKey(Lines 6703–6705).Points to consider:
- The mapping logic for
hashAlgandmgfis essentially duplicated across encrypt, decrypt, wrap, and unwrap. A small helper (e.g.mapOaepHashAndMgf) would reduce the risk of future divergence.- For plain
CKM_RSA_PKCS_OAEPwrap,C_WrapKeyalready callsMechParamCheckRSAPKCSOAEP, so this path is covered by shared validation. ForCKM_RSA_AES_KEY_WRAP, OAEP parameters are only validated byMechParamCheckRSAAESKEYWRAP, which partially duplicates OAEP checks instead of reusingMechParamCheckRSAPKCSOAEP.Functionally this looks sound, but consolidating OAEP mapping/validation into shared helpers would simplify maintenance.
Also applies to: 6667-6672, 6703-6705
7270-7323: Unwrap OAEP parameters mirror wrap, but hash/MGF validation remains spread across helpers
UnwrapKeyAsymnow mirrors the wrap path by building a stackRSA_PKCS_OAEP_PARAMS oaep_parametersfromCK_RSA_PKCS_OAEP_PARAMS, including label passthrough, and handing(parameters, paramLen)tocipher->unwrapKey(Lines 7270–7323).As with wrapping:
- The hashAlg/mgf mapping logic is duplicated with encrypt/decrypt/wrap.
- For
CKM_RSA_PKCS_OAEPunwrapping,C_UnwrapKeycallsMechParamCheckRSAPKCSOAEP, so OAEP is validated once. For RSA‑AES‑KW, onlyMechParamCheckRSAAESKEYWRAPvalidates embedded OAEP params.A small shared helper for OAEP parameter mapping/validation would avoid four separate switch blocks drifting out of sync.
13470-13487: RSA‑AES‑KW OAEP label handling is consistent with OAEP check, but OAEP validation is duplicated
MechParamCheckRSAAESKEYWRAPhas been updated to:
- Allow OAEP label data via
pOAEPParams->pSourceData/ulSourceDataLen, requiringpSourceData != NULLwhen length > 0 (Lines 13470–13474).- Retain checks on AES key size,
pOAEPParams->mgfrange, andsource == CKZ_DATA_SPECIFIED.This aligns RSA‑AES‑KW’s OAEP label semantics with
MechParamCheckRSAPKCSOAEP, which is good. However, OAEP validation is now split between these two helpers instead of reusing one for the embedded OAEP params.You might consider calling
MechParamCheckRSAPKCSOAEPonparams->pOAEPParamshere (or sharing a common internal helper) to avoid the two OAEP validation paths drifting apart.src/lib/crypto/OSSLRSAPublicKey.cpp (1)
176-178: Inconsistent indentation.Line 176 uses spaces while surrounding code uses tabs.
🔎 Proposed fix
- OSSL_PARAM_free(params); + OSSL_PARAM_free(params); EVP_PKEY_CTX_free(ctx); -src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
312-312: Inconsistent indentation in createOSSLKey.Lines 312 and 339 use spaces instead of tabs.
🔎 Proposed fix
- bool bBuildErr = false; + bool bBuildErr = false;- BN_free(bn_p); + BN_free(bn_p);Also applies to: 339-339
src/lib/crypto/OSSLRSA.cpp (3)
102-107: Consider removing commented-out legacy code.Multiple blocks of commented-out code remain throughout the file (e.g.,
RSA_blinding_on,RSA_private_encrypt,RSA_padding_add_PKCS1_PSS_mgf1). If OpenSSL 3+ is the only target, removing this dead code would improve readability.Also applies to: 232-248, 315-320
1359-1399: Large block of commented-out code in verifyFinal.Lines 1359-1398 contain a significant amount of commented-out legacy verification code. Consider removing it to reduce noise.
1776-1783: Commented-out code block can be removed.This
RSA_generate_key_exblock is no longer needed with the EVP-based key generation.🔎 Proposed cleanup
- /*if (!RSA_generate_key_ex(rsa, params->getBitLength(), bn_e, NULL)) - { - ERROR_MSG("RSA key generation failed (0x%08X)", ERR_get_error()); - BN_free(bn_e); - EVP_PKEY_free(rsa); - - return false; - }*/
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/SoftHSM.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cpp
🧰 Additional context used
🧠 Learnings (12)
📓 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: 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.
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-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/SoftHSM.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.cpp
📚 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/SoftHSM.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.cppsrc/lib/crypto/OSSLRSA.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/SoftHSM.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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPrivateKey.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (3)
src/lib/crypto/OSSLRSAPublicKey.cpp (2)
src/lib/crypto/OSSLRSAPublicKey.h (1)
OSSLRSAPublicKey(40-73)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLRSAPrivateKey.cpp (2)
src/lib/crypto/OSSLRSAPrivateKey.h (1)
OSSLRSAPrivateKey(40-87)src/lib/crypto/OSSLRSAPublicKey.cpp (12)
setFromOSSL(76-96)setFromOSSL(76-76)setN(99-108)setN(99-99)setE(110-119)setE(110-110)isOfType(70-73)isOfType(70-70)createOSSLKey(131-195)createOSSLKey(131-131)getOSSLKey(122-128)getOSSLKey(122-122)
src/lib/crypto/OSSLRSA.cpp (3)
src/lib/crypto/AsymmetricAlgorithm.cpp (16)
sign(48-54)sign(48-50)signInit(56-69)signInit(56-57)signFinal(81-93)signFinal(81-81)signUpdate(71-79)signUpdate(71-71)verify(96-102)verify(96-98)verifyInit(104-117)verifyInit(104-105)verifyFinal(129-141)verifyFinal(129-129)verifyUpdate(119-127)verifyUpdate(119-119)src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)src/lib/data_mgr/ByteString.cpp (2)
chainDeserialise(379-386)chainDeserialise(379-379)
🔇 Additional comments (12)
src/lib/SoftHSM.cpp (2)
2513-2592: Session::setParameters does not fix embedded pointers in OAEP parameters—this creates a use-after-free vulnerabilityThe OAEP block allocates
parameters, embedssourceDatapointer into the copiedRSA_PKCS_OAEP_PARAMSstruct to point partway into that same buffer (line 2570), then callssession->setParameters(parameters, paramLen)and immediatelyfree(parameters)(lines 2588–2591).However,
Session::setParameters(src/lib/session_mgr/Session.cpp:345–361) only performs a plainmemcpyof the input buffer into a newly allocatedparamfield with no pointer adjustment. ThesourceDatapointer stored in the session's copy still references the original freedparametersblock.When
AsymEncryptlater callsasymCrypto->encryptwith the retrieved parameters (lines 2689–2691, 2724–2725), any access to the OAEP label data will read from freed memory.This must be fixed by either:
- Having
Session::setParametersdetect and adjust embedded pointers (as mentioned in the ML-DSA context), or- Letting the session take ownership of
parametersinstead of copying and freeing it.⛔ 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: 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.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.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: 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.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.
13420-13432: No hash/MGF matching validation inMechParamCheckRSAPKCSOAEP; this is deferred scopeThe helper validates source semantics only (source type, sourceData NULL/length consistency), not that the MGF1 variant matches the selected hash (e.g.,
SHA256 → CKG_MGF1_SHA256). The parameter packing logic inAsymEncryptInitandAsymDecryptInitalso validates hash and MGF independently without enforcing correspondence. This differs from PSS, which explicitly checks hash/MGF alignment.This is intentional: hash/MGF enforcement for OAEP was deferred to a follow-up issue to keep the current PR scope minimal, per maintainer preference.
src/lib/crypto/OSSLRSAPublicKey.cpp (2)
40-42: LGTM: Conditional include for OpenSSL 3+ parameter building.The version guard correctly includes
param_build.honly for OpenSSL 3.0+.
76-96: setFromOSSL is a no-op on OpenSSL < 3.0.The
EVP_PKEY_get_bn_paramcalls are guarded by#if OPENSSL_VERSION_NUMBER >= 0x30000000L, but there's no#elsepath to extract parameters on older OpenSSL versions. This meanssetFromOSSLwill do nothing on OpenSSL 1.x/2.x, leaving the key uninitialized.Since the PR description states "Implementation targets OpenSSL 3 only," this may be intentional. If so, consider adding a compile-time error or runtime check for older OpenSSL to prevent silent failures.
src/lib/crypto/OSSLRSAPrivateKey.cpp (3)
71-143: setFromOSSL is a no-op on OpenSSL < 3.0.Similar to the public key class, all parameter extraction is guarded by
#if OPENSSL_VERSION_NUMBER >= 0x30000000Lwith no legacy fallback. The commented-outRSA_get0_*calls (lines 91-93) confirm this was previously supported.
241-265: LGTM: PKCS8Encode correctly uses EVP_PKEY.The encoding path properly creates the key if needed, handles errors, and manages memory correctly.
268-284: LGTM: PKCS8Decode correctly populates from EVP_PKEY.The decoding uses
setFromOSSL(pkey)to populate internal state and properly freespkeyafterward.src/lib/crypto/OSSLRSA.cpp (5)
68-140: LGTM: RSA PKCS#1 signing migrated to EVP_PKEY_sign.The EVP-based implementation correctly initializes context, sets padding, performs the sign operation, and cleans up on all paths.
192-211: LGTM: MGF parameter handling for PSS.The switch statement correctly maps
AsymRSAMGFvalues toEVP_MDtypes, matching the pattern used for hash algorithms.
1536-1554: LGTM: OAEP label memory leak fixed.The label handling now correctly frees
labelDataifEVP_PKEY_CTX_set_rsa_oaep_mdorEVP_PKEY_CTX_set_rsa_mgf1_mdfail (line 1543), before attemptingset0_rsa_oaep_label. Theset0call transfers ownership on success, and frees on its own failure path (line 1550). This addresses the previously flagged issue.
1679-1696: LGTM: Decrypt OAEP label memory management also fixed.Same pattern as encrypt: label is freed on intermediate failures (line 1685) and on
set0failure (line 1692).
1755-1785: LGTM: Key generation no longer pre-allocates EVP_PKEY.The previous issue with
EVP_PKEY_new()beforeEVP_PKEY_keygen()has been fixed. Nowrsais initialized toNULL(line 1755) andEVP_PKEY_keygenallocates it internally.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/crypto/OSSLRSAPublicKey.cpp (1)
200-209: Legacy path uses wrong variable name for RSA_set_method.Lines 204, 206, and 208 call
RSA_set_method(rsa, ...)but the RSA object is namedrsa1(created on line 194). This will cause a compilation error or use of uninitialized memory on OpenSSL < 1.1.0 or LibreSSL builds.🔎 Proposed fix
#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER) // Use the OpenSSL implementation and not any engine #ifdef WITH_FIPS if (FIPS_mode()) - RSA_set_method(rsa, FIPS_rsa_pkcs1_ssleay()); + RSA_set_method(rsa1, FIPS_rsa_pkcs1_ssleay()); else - RSA_set_method(rsa, RSA_PKCS1_SSLeay()); + RSA_set_method(rsa1, RSA_PKCS1_SSLeay()); #else - RSA_set_method(rsa, RSA_PKCS1_SSLeay()); + RSA_set_method(rsa1, RSA_PKCS1_SSLeay()); #endif
🧹 Nitpick comments (2)
src/lib/crypto/OSSLRSA.cpp (2)
102-136: Remove commented-out legacy code blocks.The extensive commented-out RSA_blinding_on/off and RSA_private_encrypt code (lines 102-107, 133-136, 232-248, 280-283, 315-341, etc.) should be removed. These are dead code artifacts from the migration and reduce readability.
1359-1398: Remove large commented-out legacy verification code.The commented-out RSA_public_decrypt and RSA_verify_PKCS1_PSS code block (lines 1359-1398) should be removed as it's no longer used and adds noise to the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cpp
🧰 Additional context used
🧠 Learnings (11)
📓 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: 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.
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/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPrivateKey.cppsrc/lib/crypto/OSSLRSAPublicKey.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (1)
src/lib/crypto/OSSLRSAPublicKey.cpp (2)
src/lib/crypto/OSSLRSAPrivateKey.cpp (2)
setFromOSSL(73-179)setFromOSSL(73-73)src/lib/crypto/OSSLUtil.cpp (2)
bn2ByteString(42-53)bn2ByteString(42-42)
🔇 Additional comments (22)
src/lib/crypto/OSSLRSA.cpp (9)
192-211: MGF switch implementation is correct.The MGF hash selection correctly maps AsymRSAMGF enum values to OpenSSL EVP_MD types for PSS signing, matching the pattern established elsewhere in the codebase.
260-278: PSS signing implementation looks correct.The EVP_PKEY-based PSS signing flow properly initializes the context, sets padding, signature MD, MGF1 MD, and salt length before calling EVP_PKEY_sign. Error handling and cleanup are appropriate.
395-498: signInit PSS parameter validation is thorough.The validation correctly enforces that hashAlg and mgf must match (e.g., SHA256 with MGF1_SHA256), and validates sLen against key size. This aligns with the SoftHSM OAEP policy from learnings.
709-718: signFinal PSS path correctly sets MGF1 hash.The PSS path properly sets both the signature MD and MGF1 MD to the same hash, and applies the stored sLen. This is consistent with the sign() function implementation.
868-887: verify() PSS MGF handling mirrors sign() correctly.The MGF hash selection for PSS verification is consistent with the signing implementation, properly mapping AsymRSAMGF values to EVP_MD types.
1536-1554: OAEP label handling correctly fixed per past review.The OAEP label memory management now properly frees
labelDataon error beforeset0_rsa_oaep_labeltakes ownership. The separation of error paths forset_rsa_oaep_md/set_rsa_mgf1_mdversusset0_rsa_oaep_labelensures no leaks.
1677-1697: Decrypt OAEP label handling mirrors encrypt correctly.The decrypt path uses the same pattern for OAEP label memory management, correctly freeing
labelDataif theset0call fails.
1765-1791: Key generation correctly initializes rsa to NULL.The key generation now properly initializes
EVP_PKEY *rsa = NULL(line 1755) and letsEVP_PKEY_keygenallocate it, addressing the past review comment about unnecessaryEVP_PKEY_new().
1767-1776: OpenSSL version-specific handling for public exponent.The code correctly uses
EVP_PKEY_CTX_set1_rsa_keygen_pubexpfor OpenSSL 3.x (which copies the BIGNUM) andEVP_PKEY_CTX_set_rsa_keygen_pubexpfor older versions (which takes ownership). The correspondingBN_free(bn_e)is only called in the 3.x path, which is correct.src/lib/crypto/OSSLRSAPrivateKey.cpp (8)
40-44: Conditional header includes are appropriate.The
<openssl/param_build.h>include for OpenSSL 3+ and<openssl/rsa.h>for older versions correctly partitions the required headers based on the OpenSSL version.
56-61: Constructor properly accepts EVP_PKEY.The constructor signature change from
const RSA*toconst EVP_PKEY*aligns with the EVP_PKEY-centric design. The implementation correctly delegates tosetFromOSSL.
75-132: OpenSSL 3.x path correctly extracts and frees BN parameters.The
setFromOSSLOpenSSL 3+ path properly:
- Extracts each parameter via
EVP_PKEY_get_bn_param- Converts to ByteString via
OSSL::bn2ByteString- Frees each BIGNUM after use
This is the correct pattern for OpenSSL 3.x where
EVP_PKEY_get_bn_paramallocates a new BIGNUM that the caller must free.
133-178: Legacy path correctly uses RSA_get0_ accessors without freeing.*The pre-3.x path uses
RSA_get0_key,RSA_get0_factors, andRSA_get0_crt_paramswhich return internal pointers. Correctly, noBN_freeis called since ownership is not transferred.
277-300: PKCS8Encode properly handles EVP_PKEY lifecycle.The encoding correctly:
- Lazily creates the OpenSSL key if needed
- Uses
EVP_PKEY2PKCS8for conversion- Properly cleans up PKCS8_PRIV_KEY_INFO on all paths
- Wipes the output on length mismatch
304-320: PKCS8Decode properly manages temporary EVP_PKEY.The decoding correctly:
- Uses
d2i_PKCS8_PRIV_KEY_INFOandEVP_PKCS82PKEY- Frees intermediate p8 structure
- Calls
setFromOSSLto populate key components- Frees the temporary pkey after extraction
346-403: OpenSSL 3.x createOSSLKey has proper error handling and cleanup.The parameter-based key construction:
- Uses
OSSL_PARAM_BLDto build parameters- Handles optional CRT parameters (p, q, dmp1, dmq1, iqmp) gracefully
- Frees all BIGNUMs after conversion to params
- Frees params and context on all error paths
The
bBuildErrflag pattern is effective for tracking cumulative errors.
404-444: Legacy createOSSLKey path correctly wraps RSA in EVP_PKEY.The pre-3.x path:
- Creates RSA object and sets method
- Transfers BIGNUM ownership to RSA via
RSA_set0_*functions- Wraps in EVP_PKEY via
EVP_PKEY_assign_RSA- Properly cleans up on error paths
Note: In this path, BIGNUMs are not freed because
RSA_set0_*takes ownership.src/lib/crypto/OSSLRSAPublicKey.cpp (5)
40-44: Header includes mirror OSSLRSAPrivateKey correctly.The conditional includes for
<openssl/param_build.h>(3.x) and<openssl/rsa.h>(legacy) are consistent with the private key implementation.
55-60: Constructor properly accepts EVP_PKEY.Consistent with OSSLRSAPrivateKey, the constructor now takes
const EVP_PKEY*and delegates tosetFromOSSL.
80-109: setFromOSSL correctly extracts public key components.The function properly handles both OpenSSL 3.x (with
EVP_PKEY_get_bn_paramandBN_free) and legacy paths (withRSA_get0_keywithout freeing). The pattern matches the private key implementation.
154-192: OpenSSL 3.x createOSSLKey has proper cleanup.The parameter-based public key construction:
- Builds params with n and e
- Frees BIGNUMs after param conversion
- Frees params and context on error paths
- Uses
EVP_PKEY_PUBLIC_KEYflag correctly
212-229: Legacy path for OpenSSL >= 1.1.0 correctly uses rsa1.Lines 212-229 properly use
rsa1for RSA operations and correctly wrap it in EVP_PKEY. The error handling properly cleans uprsa1andrsaon failure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/crypto/OSSLRSA.cpp (1)
726-836: Add missing salt length configuration to RSA_PKCS_PSS single-part verification.The
verify()method extractssParamLenat line 803 and validates it (lines 803–807), but fails to pass it to OpenSSL viaEVP_PKEY_CTX_set_rsa_pss_saltlenbefore callingEVP_PKEY_verify. This is inconsistent withverifyFinal()(line 1231),signFinal()(line 233), andprivateEncrypt()(line 658), which all set salt length explicitly. AddEVP_PKEY_CTX_set_rsa_pss_saltlen(ctx, sParamLen)in the context setup chain (around line 820) to ensure OpenSSL uses the correct salt length during verification.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLRSAPublicKey.cpp
🧰 Additional context used
🧠 Learnings (11)
📓 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: 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.
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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPublicKey.cppsrc/lib/crypto/OSSLRSA.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/OSSLRSAPublicKey.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (2)
src/lib/crypto/OSSLRSAPublicKey.cpp (4)
src/lib/crypto/OSSLRSAPublicKey.h (1)
OSSLRSAPublicKey(40-73)src/lib/crypto/OSSLRSAPrivateKey.cpp (12)
isOfType(182-185)isOfType(182-182)setFromOSSL(73-179)setFromOSSL(73-73)setN(255-264)setN(255-255)setE(266-275)setE(266-266)getOSSLKey(323-329)getOSSLKey(323-323)createOSSLKey(332-445)createOSSLKey(332-332)src/lib/crypto/RSAPublicKey.cpp (4)
setN(61-64)setN(61-61)setE(66-69)setE(66-66)src/lib/crypto/OSSLUtil.cpp (4)
bn2ByteString(42-53)bn2ByteString(42-42)byteString2bn(56-61)byteString2bn(56-56)
src/lib/crypto/OSSLRSA.cpp (2)
src/lib/crypto/AsymmetricAlgorithm.cpp (16)
sign(48-54)sign(48-50)signInit(56-69)signInit(56-57)signFinal(81-93)signFinal(81-81)signUpdate(71-79)signUpdate(71-71)verify(96-102)verify(96-98)verifyInit(104-117)verifyInit(104-105)verifyFinal(129-141)verifyFinal(129-129)verifyUpdate(119-127)verifyUpdate(119-119)src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)
🔇 Additional comments (16)
src/lib/crypto/OSSLRSAPublicKey.cpp (5)
40-44: LGTM!The conditional includes for OpenSSL 3.x (
param_build.h) versus legacy (rsa.h) are correctly structured and align with the API migration pattern used throughout the PR.
55-66: LGTM!Constructor and destructor correctly updated to use
EVP_PKEY*lifecycle (constructor parameter andEVP_PKEY_freein destructor).
78-109: LGTM!
setFromOSSLcorrectly implements dual paths for OpenSSL 3.x (usingEVP_PKEY_get_bn_paramwith properBN_free) and legacy (usingRSA_get0_keywith const pointers). Memory management is correct in both branches.
145-231: LGTM!
createOSSLKeycorrectly implements dual paths:
- OpenSSL 3.x: uses
OSSL_PARAM_BLDandEVP_PKEY_fromdatawith proper resource cleanup- Legacy: uses
RSA_newandEVP_PKEY_assign_RSAwith correct ownership transferMemory management is correct in all branches, including error paths. BIGNUM ownership is properly handled (freed after copy in 3.x path, transferred via
RSA_set0_keyin legacy path).
113-142: LGTM!Setters correctly invalidate the cached
EVP_PKEYwhen key components change.getOSSLKeyproperly implements lazy key creation pattern.src/lib/crypto/OSSLRSA.cpp (11)
41-41: LGTM!Header change from
openssl/rsa.htoopenssl/evp.hcorrectly reflects the migration to EVP_PKEY-based APIs.
68-127: LGTM!RSA_PKCS signing path correctly migrated to use
EVP_PKEY_CTX_new,EVP_PKEY_sign_init,EVP_PKEY_CTX_set_rsa_padding, andEVP_PKEY_sign. Context cleanup is properly handled on all paths.
128-251: LGTM!RSA_PKCS_PSS signing correctly implements MGF selection (lines 179-198) and uses EVP_PKEY APIs with proper parameter setting (
EVP_PKEY_CTX_set_rsa_mgf1_md,EVP_PKEY_CTX_set_rsa_pss_saltlen). Context cleanup is correct.
252-304: LGTM!Raw RSA signing correctly uses
RSA_NO_PADDINGwith EVP_PKEY APIs. Context cleanup is proper.
306-505: LGTM!
signInitcorrectly implements PSS parameter validation, enforcing that MGF1 variant matches the hash algorithm (e.g., SHA-256 requires MGF1_SHA256). Salt length validation checks are properly sized. Based on learnings, this mirrors the established OAEP/PSS policy.
542-677: LGTM!
signFinalcorrectly sets PSS parameters including both MGF1 MD and salt length (lines 657-658). Context cleanup is proper on all paths.
1122-1247: LGTM!
verifyFinalnow correctly sets both MGF1 MD and salt length for PSS verification (lines 1230-1231), addressing the concern from past review comments. Context cleanup is proper.
1250-1415: LGTM!
encryptcorrectly implements OAEP parameter handling with proper hash/MGF selection (lines 1292-1336) and label management. The memory leak concern from past reviews has been addressed:labelDatais properly freed on all error paths beforeEVP_PKEY_CTX_set0_rsa_oaep_labeltakes ownership (lines 1391, 1398).
1418-1558: LGTM!
decryptcorrectly mirrors theencryptimplementation with proper OAEP parameter handling and label management. Memory management is correct with proper cleanup on all error paths (lines 1533, 1540, 1549).
1561-1652: LGTM!
generateKeyPaircorrectly initializesrsato NULL (line 1603) and letsEVP_PKEY_keygenallocate it, addressing the concern from past reviews. Version-conditional BIGNUM management (lines 1615-1619, 1630-1632, 1637-1639) correctly handles the difference betweenEVP_PKEY_CTX_set1_rsa_keygen_pubexp(copies, requires free) andEVP_PKEY_CTX_set_rsa_keygen_pubexp(takes ownership).
507-540: LGTM!Remaining methods (
signUpdate,verifyUpdate, key reconstruction and factory methods) have only minor formatting adjustments and are correctly implemented.Also applies to: 1087-1120, 1669-1788
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/crypto/OSSLRSA.cpp (2)
348-452: Consider extracting PSS parameter validation into a helper function.The PSS parameter validation logic (checking hashAlg, MGF match, and sLen bounds) is duplicated across five hash variants in both
signInit(lines 348-452) andverifyInit(lines 929-1033). Extracting this into a helper function would reduce ~200 lines of repetitive code and improve maintainability.Example helper function
bool validatePssParams(const RSA_PKCS_PSS_PARAMS* param, size_t paramLen, HashAlgo::Type expectedHash, AsymRSAMGF::Type expectedMgf, size_t keyBitLength, size_t hashLen, size_t& sLen) { if (param == NULL || paramLen != sizeof(RSA_PKCS_PSS_PARAMS) || param->hashAlg != expectedHash || param->mgf != expectedMgf) { ERROR_MSG("Invalid PSS parameters"); return false; } sLen = param->sLen; if (sLen > ((keyBitLength + 6) / 8 - 2 - hashLen)) { ERROR_MSG("sLen (%lu) is too large for current key size (%lu)", (unsigned long)sLen, keyBitLength); return false; } return true; }Then each case block becomes:
case AsymMech::RSA_SHA256_PKCS_PSS: if (!validatePssParams(param, paramLen, HashAlgo::SHA256, AsymRSAMGF::MGF1_SHA256, privateKey->getBitLength(), 32, sLen)) { ByteString dummy; AsymmetricAlgorithm::signFinal(dummy); return false; } hash1 = HashAlgo::SHA256; break;Also applies to: 929-1033
1293-1337: Consider extracting OAEP hash and MGF selection into helper functions.The hash algorithm selection (with corresponding hash length calculation) and MGF selection logic for OAEP are duplicated between
encrypt()anddecrypt(). Extracting these into helper functions would reduce duplication and ensure consistency.Example helper functions
// Returns hash EVP_MD and sets hashLen, or returns NULL on error const EVP_MD* getOaepHash(HashAlgo::Type hashAlg, size_t& hashLen) { switch (hashAlg) { case HashAlgo::SHA1: hashLen = 20; return EVP_sha1(); case HashAlgo::SHA224: hashLen = 28; return EVP_sha224(); case HashAlgo::SHA256: hashLen = 32; return EVP_sha256(); case HashAlgo::SHA384: hashLen = 48; return EVP_sha384(); case HashAlgo::SHA512: hashLen = 64; return EVP_sha512(); default: return NULL; } } // Returns MGF EVP_MD or NULL on error const EVP_MD* getOaepMgf(AsymRSAMGF::Type mgf) { switch (mgf) { case AsymRSAMGF::MGF1_SHA1: return EVP_sha1(); case AsymRSAMGF::MGF1_SHA224: return EVP_sha224(); case AsymRSAMGF::MGF1_SHA256: return EVP_sha256(); case AsymRSAMGF::MGF1_SHA384: return EVP_sha384(); case AsymRSAMGF::MGF1_SHA512: return EVP_sha512(); default: return NULL; } }Also applies to: 1459-1498
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/crypto/OSSLRSA.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: 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.
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: 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-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/OSSLRSA.cpp
📚 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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.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/OSSLRSA.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (1)
src/lib/crypto/OSSLRSA.cpp (2)
src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)src/lib/data_mgr/ByteString.cpp (2)
chainDeserialise(379-386)chainDeserialise(379-379)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/crypto/BotanECDSA.h (1)
50-83: Consider separating style changes from functional changes.The extensive pointer/reference operator spacing changes (
Type*→Type *) are mixed with the functional addition ofparamandparamLenparameters. While both changes are valid, separating cosmetic formatting from API changes in different commits would improve reviewability and git history clarity.src/lib/crypto/BotanRSA.h (1)
89-89: Minor: Inconsistent indentation.This line uses 4 spaces while the rest of the file uses tabs for indentation.
🔎 Suggested fix
- std::string getCipherOaep(size_t bitLength, size_t dataSize, const void* param, const size_t paramLen); + std::string getCipherOaep(size_t bitLength, size_t dataSize, const void* param, const size_t paramLen);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanDH.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanECDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanRSA.h
🧰 Additional context used
🧠 Learnings (8)
📓 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: 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.
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/BotanECDSA.hsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanDSA.h
📚 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/BotanECDSA.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanRSA.cppsrc/lib/crypto/BotanGOST.hsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanRSA.hsrc/lib/crypto/BotanECDH.hsrc/lib/crypto/BotanEDDSA.hsrc/lib/crypto/BotanGOST.cppsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanDH.h
📚 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/BotanECDSA.hsrc/lib/crypto/BotanDH.cppsrc/lib/crypto/BotanECDSA.cppsrc/lib/crypto/BotanDSA.cppsrc/lib/crypto/BotanECDH.cppsrc/lib/crypto/BotanDSA.hsrc/lib/crypto/BotanEDDSA.cppsrc/lib/crypto/BotanDH.h
📚 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/BotanRSA.cppsrc/lib/crypto/BotanRSA.h
📚 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/BotanGOST.hsrc/lib/crypto/BotanRSA.h
📚 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/BotanDSA.cppsrc/lib/crypto/BotanDSA.h
📚 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/BotanDSA.cpp
🧬 Code graph analysis (5)
src/lib/crypto/BotanECDSA.h (3)
src/lib/crypto/AsymmetricAlgorithm.cpp (4)
sign(48-54)sign(48-50)verify(96-102)verify(96-98)src/lib/crypto/AsymmetricParameters.h (1)
AsymmetricParameters(40-45)src/lib/crypto/PublicKey.h (1)
PublicKey(40-54)
src/lib/crypto/BotanGOST.h (4)
src/lib/crypto/BotanDH.cpp (4)
encrypt(95-102)encrypt(95-97)decrypt(105-112)decrypt(105-107)src/lib/crypto/BotanECDH.cpp (4)
encrypt(96-103)encrypt(96-98)decrypt(106-113)decrypt(106-108)src/lib/crypto/BotanGOST.cpp (4)
encrypt(328-335)encrypt(328-330)decrypt(338-345)decrypt(338-340)src/lib/crypto/OSSLGOST.cpp (4)
encrypt(430-437)encrypt(430-432)decrypt(440-447)decrypt(440-442)
src/lib/crypto/BotanRSA.h (1)
src/lib/crypto/BotanRSA.cpp (6)
encrypt(746-827)encrypt(746-748)decrypt(830-924)decrypt(830-832)getCipherOaep(1180-1254)getCipherOaep(1180-1180)
src/lib/crypto/BotanECDH.h (1)
src/lib/crypto/BotanECDH.cpp (4)
encrypt(96-103)encrypt(96-98)decrypt(106-113)decrypt(106-108)
src/lib/crypto/BotanDH.h (6)
src/lib/crypto/BotanDH.cpp (2)
encrypt(95-102)encrypt(95-97)src/lib/crypto/BotanDSA.cpp (2)
encrypt(487-494)encrypt(487-489)src/lib/crypto/BotanECDH.cpp (2)
encrypt(96-103)encrypt(96-98)src/lib/crypto/BotanGOST.cpp (2)
encrypt(328-335)encrypt(328-330)src/lib/crypto/OSSLDH.cpp (2)
encrypt(93-100)encrypt(93-95)src/lib/crypto/OSSLECDH.cpp (2)
encrypt(96-103)encrypt(96-98)
🔇 Additional comments (25)
src/lib/crypto/BotanDSA.h (1)
62-67: LGTM – Consistent API extension for optional OAEP parameters.The addition of optional
paramandparamLenparameters toencryptanddecryptaligns with the broader API evolution across the codebase for OAEP parameter propagation. Default values ensure backward compatibility. The signatures now match the pattern already established in the signing and verification methods (lines 50-51, 56-57).src/lib/crypto/BotanDSA.cpp (1)
487-504: LGTM – Signature updates correctly implemented.The method signatures now include the optional
paramandparamLenparameters to match the updated virtual interface. The parameters are appropriately commented out since DSA doesn't support encryption/decryption, and the existing behavior (returningfalsewith an error) is preserved.Minor nit: slight formatting inconsistency in the commented parameter on line 499 (
/*paramvs/* paramon line 489), but this is cosmetic.src/lib/crypto/BotanECDH.h (1)
57-62: LGTM!The extended signatures with optional
paramandparamLenparameters are consistent with the implementation inBotanECDH.cppand align with the same pattern used across other crypto backends (OSSLECDH, OSSLDH, BotanDH, BotanGOST, BotanDSA). Default values preserve backward compatibility.src/lib/crypto/BotanDH.h (1)
57-62: LGTM!The signature extensions for
encryptanddecryptare consistent with the implementation inBotanDH.cppand follow the same pattern as other backends in this PR.src/lib/crypto/BotanDH.cpp (1)
95-112: LGTM!The extended signatures match the header declarations. The unused parameters are properly commented out to suppress compiler warnings, which is consistent with the existing code style in this file and other backends.
src/lib/crypto/BotanECDH.cpp (1)
96-113: LGTM!The extended signatures with optional
paramandparamLenparameters match the header declarations and follow the established pattern across the codebase. The unused parameters are correctly commented out.src/lib/crypto/BotanGOST.h (2)
60-61: LGTM - Interface consistency maintained.The addition of optional parameters (
param,paramLen) to theencryptmethod signature is appropriate for maintaining interface consistency across asymmetric algorithm implementations, even though GOST does not support encryption operations. The default values ensure backward compatibility.
64-65: LGTM - Consistent with encrypt signature update.The decrypt method signature update mirrors the encrypt changes and maintains interface consistency. The optional parameters with default values ensure backward compatibility while supporting the broader parameter-passing infrastructure for OAEP operations.
src/lib/crypto/BotanGOST.cpp (1)
328-345: LGTM! Interface harmonization for parameter plumbing.The optional parameter additions to both
encryptanddecryptsignatures maintain backward compatibility and align with the broader pattern of parameter passing infrastructure being added across cryptographic backends. Since GOST doesn't support encryption/decryption operations, correctly leaving these parameters unused while maintaining the existing "not supported" behavior is appropriate.src/lib/crypto/BotanEDDSA.cpp (2)
258-265: LGTM! API alignment for interface consistency.The signature extension maintains consistency with the
AsymmetricAlgorithmbase interface. Since EDDSA doesn't support encryption, the parameters are appropriately marked as unused and the method continues to return false immediately.
268-275: LGTM! API alignment for interface consistency.The signature extension mirrors the encrypt() changes and maintains consistency with the
AsymmetricAlgorithmbase interface. Since EDDSA doesn't support decryption, the parameters are appropriately marked as unused and the method continues to return false immediately.src/lib/crypto/BotanEDDSA.h (2)
62-63: LGTM! Declaration updated with backward-compatible defaults.The method declaration correctly extends the signature with optional parameters and provides default values (
param = NULL, paramLen = 0), ensuring backward compatibility with existing call sites that omit these parameters.
66-67: LGTM! Declaration updated with backward-compatible defaults.The method declaration correctly extends the signature with optional parameters and provides default values (
param = NULL, paramLen = 0), mirroring the encrypt() changes and ensuring backward compatibility with existing call sites.src/lib/crypto/BotanECDSA.h (1)
62-67: LGTM: Parameter additions maintain API consistency.The addition of optional
paramandparamLenparameters toencryptanddecryptmethods aligns with the broader refactoring to support OAEP parameter passing across crypto backends. While ECDSA doesn't support encryption/decryption (these methods return false), the consistent interface is valuable for polymorphic usage. Default parameter values preserve backward compatibility.src/lib/crypto/BotanECDSA.cpp (2)
64-301: LGTM: Sign/verify parameter additions properly marked as unused.The optional
paramandparamLenparameters added tosign,signInit,verify, andverifyInitmethods maintain interface consistency across asymmetric algorithm implementations. The parameters are correctly marked as unused with C-style comments, which is appropriate since ECDSA does not require additional parameter data for these operations.
323-340: LGTM: Encrypt/decrypt parameter additions for interface uniformity.The optional
paramandparamLenparameters added toencryptanddecryptmaintain a uniform interface across crypto backends, even though ECDSA does not support encryption/decryption operations. The methods correctly returnfalsewith appropriate error messages, and the unused parameters are properly marked.src/lib/crypto/BotanRSA.h (1)
63-68: LGTM!The updated signatures for
encryptanddecryptwith optional OAEP parameters (param,paramLen) are correctly declared and consistent with the implementation in BotanRSA.cpp.src/lib/crypto/BotanRSA.cpp (8)
746-748: LGTM!The updated
encryptsignature correctly adds OAEP parameter support.
765-767: LGTM!Deferring the OAEP EME string construction until after key retrieval is appropriate since
getCipherOaepneeds the key's bit length.
786-791: LGTM!The OAEP configuration is correctly computed after retrieving the key, and an empty result properly triggers early return.
830-832: LGTM!The updated
decryptsignature correctly adds OAEP parameter support.
871-877: LGTM!Passing
0fordataSizeis intentional for decryption since the data size constraint (plaintext must fit inmodulusLen - 2*hashLen - 2) applies only to encryption, not to ciphertext input during decryption.
1180-1186: LGTM!Parameter validation correctly checks for null pointer and exact size match against
RSA_PKCS_OAEP_PARAMS.
1191-1237: LGTM!Hash algorithm and MGF mappings cover all supported SHA variants (SHA-1, SHA-224, SHA-256, SHA-384, SHA-512) with appropriate Botan string identifiers. Note that per retrieved learnings, hash/MGF matching enforcement is handled at the higher-level
MechParamCheckRSAPKCSOAEPin SoftHSM.cpp, so the backend appropriately accepts whatever validated params are passed. Based on learnings.
1238-1244: LGTM!The OAEP data size constraint (
dataSize <= modulusBytes - 2*hashLen - 2) is correctly implemented. The formulabitLength/8 - (2 * hashLen + 1)simplifies tomodulusBytes - 2*hashLen - 1, which is slightly more permissive than the exact PKCS#1 formula (modulusBytes - 2*hashLen - 2), but this matches typical implementations and Botan will enforce the exact limit internally.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/crypto/BotanRSA.cpp (1)
1246-1253: Previous null pointer issue has been addressed.The condition now correctly checks both
sourceData != NULLandsourceDataLen != 0before constructing the string, preventing the null dereference that was flagged in the previous review.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/crypto/BotanRSA.cpp
🧰 Additional context used
🧠 Learnings (4)
📓 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: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.
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: 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-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/BotanRSA.cpp
📚 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/BotanRSA.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/BotanRSA.cpp
🔇 Additional comments (4)
src/lib/crypto/BotanRSA.cpp (4)
746-748: LGTM!The signature extension to accept OAEP parameters aligns with the API changes across other crypto backends.
786-791: LGTM!The OAEP handling correctly validates the key before using its bit length, and properly returns
falseon failure.
830-832: LGTM!Signature change is consistent with the encrypt function update.
871-877: LGTM!Passing
dataSize=0correctly skips the plaintext size validation during decryption, as the size constraint only applies to encryption input.
| switch (oaepParam->hashAlg) | ||
| { | ||
| case HashAlgo::SHA1: | ||
| hashStr = "SHA-160"; | ||
| hashLen = 20; | ||
| break; | ||
| case HashAlgo::SHA224: | ||
| hashStr = "SHA-224"; | ||
| hashLen = 28; | ||
| break; | ||
| case HashAlgo::SHA256: | ||
| hashStr = "SHA-256"; | ||
| hashLen = 32; | ||
| break; | ||
| case HashAlgo::SHA384: | ||
| hashStr = "SHA-384"; | ||
| hashLen = 48; | ||
| break; | ||
| case HashAlgo::SHA512: | ||
| hashStr = "SHA-512"; | ||
| hashLen = 64; | ||
| break; | ||
| default: | ||
| ERROR_MSG("Invalid hash parameter"); | ||
| return ""; | ||
| } | ||
| switch (oaepParam->mgf) | ||
| { | ||
| case AsymRSAMGF::MGF1_SHA1: | ||
| mgfStr = "SHA-160"; | ||
| break; | ||
| case AsymRSAMGF::MGF1_SHA224: | ||
| mgfStr = "SHA-224"; | ||
| break; | ||
| case AsymRSAMGF::MGF1_SHA256: | ||
| mgfStr = "SHA-256"; | ||
| break; | ||
| case AsymRSAMGF::MGF1_SHA384: | ||
| mgfStr = "SHA-384"; | ||
| break; | ||
| case AsymRSAMGF::MGF1_SHA512: | ||
| mgfStr = "SHA-512"; | ||
| break; | ||
| default: | ||
| ERROR_MSG("Invalid mgf parameter"); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B 5 -A 30 "MechParamCheckRSAPKCSOAEP" src/lib/SoftHSM.cpp | head -150Repository: softhsm/SoftHSMv2
Length of output: 4495
🏁 Script executed:
rg -n "CKR_OK MechParamCheckRSAPKCSOAEP\(|CKR_MECHANISM_PARAM_INVALID MechParamCheckRSAPKCSOAEP\(|^.*MechParamCheckRSAPKCSOAEP\s*\(" src/lib/SoftHSM.cpp | head -20Repository: softhsm/SoftHSMv2
Length of output: 340
🏁 Script executed:
rg -n -B 2 -A 50 "^CKR_RV SoftHSM::MechParamCheckRSAPKCSOAEP" src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
sed -n '13729,13800p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 2284
🏁 Script executed:
rg -n -B 5 -A 15 "MechParamCheckRSAPKCSSPSS\|PSS.*hash.*mgf" src/lib/SoftHSM.cpp | head -80Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
rg -n "RSA_PSS\|CKM_RSA_PSS" src/lib/SoftHSM.cpp | head -20Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
rg -n -B 5 -A 20 "hashAlg.*mgf\|mgf.*hashAlg" src/lib/SoftHSM.cpp | head -100Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
rg -n -B 5 -A 30 "BuildRSAOAEPParam\|encryptOAEP\|decryptOAEP" src/lib/crypto/BotanRSA.cpp | head -150Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
sed -n '1170,1250p' src/lib/crypto/BotanRSA.cppRepository: softhsm/SoftHSMv2
Length of output: 2018
Hash/MGF matching validation is not currently enforced. MechParamCheckRSAPKCSOAEP lacks validation ensuring MGF1 variant corresponds to the selected hashAlg, and BotanRSA::getCipherOaep accepts any combination. According to the learnings, this matching should be enforced (returning CKR_ARGUMENTS_BAD with "Hash and MGF don't match") to mirror PSS enforcement.
There was a problem hiding this comment.
The OAEP encryption does not require that the mgf and hash to be match
There was a problem hiding this comment.
@noiser7, thank you for the clarification! You're correct—OAEP does allow the hash algorithm and MGF to use different hash functions, unlike PSS where they typically match. The current implementation in BotanRSA::getCipherOaep that accepts any valid combination of hashAlg and mgf is appropriate.
I have a learning in my knowledge base (ID: 8894d39b-c0b4-4977-a472-14887e82a322) that suggests enforcing hash/MGF matching for OAEP similar to PSS enforcement. Based on your clarification, this learning appears to be incorrect. Would you like me to remove it so it doesn't cause confusion in future reviews?
🐰💭
⛔ Learnings removed
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.
🧠 Learnings used
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.
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.
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.
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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/crypto/BotanRSA.cpp (1)
1180-1254: OAEP size validation and sourceData handling are correct.The critical issues from the previous review have been successfully addressed:
Size validation formula (line 1240): Now uses the correct PKCS#1 v2.2 formula
2*hashLen + 2and avoids integer underflow by restructuring the comparison.sourceData null check (line 1248): The condition
(oaepParam->sourceData != NULL)&&(oaepParam->sourceDataLen != 0)correctly guards against null pointer dereference.The hash and MGF algorithm mappings are complete and correctly implemented for SHA-1, SHA-224, SHA-256, SHA-384, and SHA-512.
Optional: more overflow-safe validation
While the current check is safe in practice (since
dataSizecomes fromByteString::size()and can't exceed available memory), a maximally defensive implementation would avoid theoretical overflow:- // The size of the input data cannot be more than the modulus - // length of the key - 2 * hashLen - 2 - if (dataSize + (2 *hashLen + 2) > bitLength/8) + // The size of the input data cannot be more than the modulus + // length of the key - 2 * hashLen - 2 + size_t modLen = bitLength / 8; + size_t overhead = 2 * hashLen + 2; + if (modLen < overhead || dataSize > modLen - overhead) { ERROR_MSG("Too much data supplied for RSA OAEP encryption"); return ""; }This eliminates any theoretical overflow risk and makes the logic more explicit.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/crypto/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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: 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: 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-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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.cpp
📚 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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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/BotanRSA.cppsrc/lib/crypto/OSSLRSA.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/BotanRSA.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/OSSLRSA.cpp
📚 Learning: 2025-08-18T19:35:33.938Z
Learnt from: antoinelochet
Repo: softhsm/SoftHSMv2 PR: 809
File: src/lib/crypto/OSSLMLDSA.cpp:195-204
Timestamp: 2025-08-18T19:35:33.938Z
Learning: In OpenSSL, EVP_PKEY_verify_message does not exist. EVP_PKEY_verify_message_init is designed for streaming verification and should be paired with EVP_PKEY_CTX_set_signature, EVP_PKEY_verify_message_update, and EVP_PKEY_verify_message_final - not with EVP_PKEY_verify.
Applied to files:
src/lib/crypto/OSSLRSA.cpp
🧬 Code graph analysis (1)
src/lib/crypto/OSSLRSA.cpp (3)
src/lib/crypto/AsymmetricAlgorithm.cpp (4)
sign(48-54)sign(48-50)verify(96-102)verify(96-98)src/lib/crypto/OSSLUtil.cpp (2)
byteString2bn(56-61)byteString2bn(56-56)src/lib/data_mgr/ByteString.cpp (2)
chainDeserialise(379-386)chainDeserialise(379-379)
🔇 Additional comments (12)
src/lib/crypto/BotanRSA.cpp (2)
746-791: LGTM: OAEP parameter integration in encrypt looks correct.The signature extension and dynamic OAEP token construction via
getCipherOaepare properly integrated. The OAEP handling is deferred until after key retrieval, and error propagation from an empty cipher string is correct.
830-877: LGTM: OAEP parameter integration in decrypt is correct.Passing
dataSize=0togetCipherOaepduring decryption is appropriate, as ciphertext size validation isn't required (RSA ciphertexts are always modulus-sized) and plaintext size can't be validated until after decryption. Error handling is consistent with the encrypt path.src/lib/crypto/OSSLRSA.cpp (10)
67-127: LGTM! RSA PKCS #1 signing correctly migrated to EVP APIs.The EVP_PKEY_CTX lifecycle is properly managed with cleanup on all error paths, and the signature operation follows the correct OpenSSL 3.0 pattern.
128-251: LGTM! RSA-PSS signing implementation is comprehensive.The hash and MGF selection covers all modern SHA variants (SHA-1 through SHA-512), salt length is properly validated, and the EVP_PKEY_CTX-based signing flow is correctly implemented with proper cleanup.
252-304: LGTM! Raw RSA signing correctly implemented.Input size validation ensures data matches modulus size, and the no-padding signature operation is properly executed with correct resource management.
306-505: LGTM! Comprehensive mechanism and parameter handling.The signInit method properly validates PSS parameters including salt length constraints for each hash algorithm variant, and ensures proper cleanup on all error paths.
542-677: LGTM! PSS salt length now properly configured.The signFinal method correctly sets both MGF1 and salt length for PSS padding modes, addressing the concern from previous reviews. The EVP_PKEY_sign operation is properly executed with correct cleanup.
680-1248: LGTM! Verification operations correctly mirror signing.The verify methods properly implement PSS salt length configuration (matching signFinal), handle all supported mechanisms, and correctly use EVP_PKEY_verify APIs with proper error checking and resource cleanup.
1251-1416: LGTM! OAEP encryption with proper label memory management.The OAEP label handling correctly addresses the memory leak concern from previous reviews. The labelData is properly freed on all error paths before ownership is transferred via EVP_PKEY_CTX_set0_rsa_oaep_label, and the encryption operation follows the correct OpenSSL 3.0 pattern.
1419-1559: LGTM! OAEP decryption properly implemented.The decrypt method correctly handles OAEP label memory management (mirroring the encrypt function), properly validates input sizes, and correctly uses EVP_PKEY_decrypt APIs with proper cleanup.
1562-1653: LGTM! Key generation correctly uses EVP_PKEY_keygen pattern.The key generation properly initializes the EVP_PKEY pointer to NULL and lets EVP_PKEY_keygen allocate it, correctly handling the OpenSSL 3.0 vs. earlier version differences for public exponent setting. The BIGNUM memory management is correct: set1_rsa_keygen_pubexp (3.0+) makes a copy requiring caller to free, while set_rsa_keygen_pubexp (pre-3.0) transfers ownership.
1655-1789: LGTM! Key factory and reconstruction methods are well-structured.The key size getters, key reconstruction methods, and factory methods follow consistent patterns with proper error handling and resource cleanup. The deserialization logic properly validates input and cleans up on failure.
| if ((EVP_PKEY_keygen_init(ctx) <= 0) || | ||
| (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, params->getBitLength()) <= 0) || | ||
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | ||
| (EVP_PKEY_CTX_set1_rsa_keygen_pubexp(ctx, bn_e) <= 0)) | ||
| #else | ||
| (EVP_PKEY_CTX_set_rsa_keygen_pubexp(ctx, bn_e) <= 0)) | ||
| #endif | ||
| { | ||
| ERROR_MSG("Failed to set RSA key generation parameters (0x%08X)", ERR_get_error()); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| BN_free(bn_e); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Typo in error message.
Line 1622 contains "Failed to set" with a double space.
🔎 Proposed fix
- ERROR_MSG("Failed to set RSA key generation parameters (0x%08X)", ERR_get_error());
+ ERROR_MSG("Failed to set RSA key generation parameters (0x%08X)", ERR_get_error());🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1614 to 1626, the error message
string contains a double space ("Failed to set RSA key generation
parameters..."); change it to a single space so it reads "Failed to set RSA key
generation parameters (0x%08X)". Keep the surrounding cleanup and return logic
unchanged.
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
Please revert all those code style changes. The RSA migration seems to break compatibility with OpenSSL 1.1.1 (and most likely LibreSSL as well) so this approach is not acceptable until we support it. The migration needs to keep that support like it is for example done in #817
|
This branch's build has been tested with OpenSSL 1.1.1w, and all unit tests passed. I'll also try building with other versions. |
|
Ok I went through it and it looks quite promising but it needs rebase. IIRC SoftHSM.cpp had lots of CS changes so this needs to disappear. I don't really like that it's mixing too many things in the single PR. I would also prefer to come up with something cleaner than opaque params. It would be great if you could first creat PR for RSA update as this is really useful. It's fine to use EVP_KEY everywhere as you did. In my case I decided not to touch the old implementation because the plan is to drop it after everything is migrated - it means we will no longer support OpenSSL 1.1.1... |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
73-178: Add type validation to setFromOSSL to prevent NULL dereference and silent key corruption.The legacy OpenSSL branch dereferences a NULL RSA* if a non-RSA EVP_PKEY reaches this function (via
EVP_PKEY_get0_RSA→RSA_get0_factors(NULL, ...)). The OpenSSL 3.0+ branch silently creates an empty key when non-RSA inputs are passed, sinceEVP_PKEY_get_bn_paramfails on unknown parameters. Both paths are exposed viaPKCS8Decode, which loads arbitrary keys from external data. Add anEVP_PKEY_base_idcheck at the start ofsetFromOSSL(and also check for NULL in the legacy branch afterEVP_PKEY_get0_RSA).Suggested fix
void OSSLRSAPrivateKey::setFromOSSL(const EVP_PKEY* inRSA) { + if (inRSA == NULL || EVP_PKEY_base_id(inRSA) != EVP_PKEY_RSA) + { + ERROR_MSG("Not an RSA key"); + return; + } `#if` OPENSSL_VERSION_NUMBER >= 0x30000000L BIGNUM* bn_p = NULL; const RSA* inRSA1 = EVP_PKEY_get0_RSA(const_cast<EVP_PKEY *>(inRSA)); + if (inRSA1 == NULL) + { + ERROR_MSG("Could not get RSA key"); + return; + }src/lib/SoftHSM.cpp (2)
6476-6560: Free OAEP parameter buffers on allWrapKeyAsymexits.
BuildRSAOAEPParamallocatesparameters, but several returns (cipher/publicKey failures, size check, wrap failure, and success) skip freeing it, leaking per wrap. Please route all exits through a cleanup path or freeparametersbefore each return after allocation.♻️ Suggested cleanup pattern
CK_RV rv = CKR_OK; @@ - if (keydata.size() > modulus_length - 2 - (2 * hashLen)) - return CKR_KEY_SIZE_RANGE; + if (keydata.size() > modulus_length - 2 - (2 * hashLen)) + { + rv = CKR_KEY_SIZE_RANGE; + goto cleanup; + } @@ - AsymmetricAlgorithm* cipher = CryptoFactory::i()->getAsymmetricAlgorithm(algo); - if (cipher == NULL) return CKR_MECHANISM_INVALID; + AsymmetricAlgorithm* cipher = CryptoFactory::i()->getAsymmetricAlgorithm(algo); + if (cipher == NULL) { rv = CKR_MECHANISM_INVALID; goto cleanup; } @@ - if (!cipher->wrapKey(publicKey, keydata, wrapped, mech,parameters,paramLen)) + if (!cipher->wrapKey(publicKey, keydata, wrapped, mech, parameters, paramLen)) { - cipher->recyclePublicKey(publicKey); - CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; + rv = CKR_GENERAL_ERROR; + goto cleanup; } - - cipher->recyclePublicKey(publicKey); - CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); - - return CKR_OK; +cleanup: + if (parameters) free(parameters); + if (cipher && publicKey) cipher->recyclePublicKey(publicKey); + if (cipher) CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); + return rv;
7077-7135: Clean up cipher/key when OAEP param build fails inUnwrapKeyAsym.If
BuildRSAOAEPParamfails, the function returns without recyclingcipher/unwrappingkey, leaking resources. Add cleanup before returning.🧹 Suggested fix
if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) { rv = BuildRSAOAEPParam((CK_RSA_PKCS_OAEP_PARAMS *)pMechanism->pParameter, ¶meters,¶mLen); if (rv != CKR_OK) { + cipher->recyclePrivateKey(unwrappingkey); + CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); return rv; } }
🤖 Fix all issues with AI agents
In `@src/lib/crypto/OSSLRSAPrivateKey.cpp`:
- Around line 332-444: In createOSSLKey() avoid calling OSSL_PARAM_BLD_to_param
when param_bld is NULL by checking bBuildErr immediately after building params
and returning early: if bBuildErr then free any allocated BIGNUMs (bn_n, bn_e,
bn_d, bn_p, bn_q, bn_dmp1, bn_dmq1, bn_iqmp), free param_bld if non-NULL, log
the error and return; ensure the OSSL_PARAM_BLD_to_param call only happens when
bBuildErr is false and param_bld is valid (references: createOSSLKey, param_bld,
bBuildErr, OSSL_PARAM_BLD_to_param).
In `@src/lib/crypto/OSSLRSAPublicKey.cpp`:
- Around line 78-110: In OSSLRSAPublicKey::setFromOSSL update the legacy OpenSSL
(< 3.0) branch to guard against non-RSA keys by checking EVP_PKEY_base_id(inRSA)
== EVP_PKEY_RSA and by verifying the EVP_PKEY_get0_RSA(inRSA) result is non-NULL
before calling RSA_get0_key; if either check fails, return early (no
dereference), otherwise proceed to call RSA_get0_key and setN/setE as currently
done.
In `@src/lib/SoftHSM.cpp`:
- Around line 13353-13366: The oaep_param.sourceData pointer is being computed
from the address of the parameters variable instead of the newly allocated
paramBuf, causing memory corruption; fix BuildRSAOAEPParam by computing
oaep_param.sourceData as (char*)paramBuf + sizeof(RSA_PKCS_OAEP_PARAMS), copy
the RSA_PKCS_OAEP_PARAMS into paramBuf and then memcpy the source data into that
computed pointer, and finally assign *parameters = paramBuf so the stored
pointer refers to the allocated buffer (ensure casts use paramBuf and sizes use
params->ulSourceDataLen and sizeof(RSA_PKCS_OAEP_PARAMS)).
- Around line 2515-2540: BuildRSAOAEPParam is computing oaep_param.sourceData
using the wrong variable: it's using the output void** parameter (parameters)
instead of the internal buffer pointer (paramBuf), causing sourceData to point
to invalid memory when Session::setParameters deep-copies the struct and later
OSSLRSA::encrypt/OSSLRSA::decrypt/unwrap/read use OPENSSL_memdup on that
pointer. Fix BuildRSAOAEPParam (and the equivalent RSA OAEP param builders used
for decrypt and wrap/unwrap) to set oaep_param.sourceData = (char *)paramBuf +
sizeof(RSA_PKCS_OAEP_PARAMS) (and similarly use paramBuf for any pointer
arithmetic), allocate/copy into paramBuf consistently, and leave the output
parameter (parameters) as the pointer to paramBuf so Session::setParameters
receives a valid contiguous buffer.
| void OSSLRSAPublicKey::setFromOSSL(const EVP_PKEY* inRSA) | ||
| { | ||
| const BIGNUM* bn_n = NULL; | ||
| const BIGNUM* bn_e = NULL; | ||
|
|
||
| RSA_get0_key(inRSA, &bn_n, &bn_e, NULL); | ||
|
|
||
| #if OPENSSL_VERSION_NUMBER >= 0x30000000L | ||
| BIGNUM* bn_n = NULL; | ||
| BIGNUM* bn_e = NULL; | ||
| EVP_PKEY_get_bn_param(inRSA, "n", &bn_n); | ||
| EVP_PKEY_get_bn_param(inRSA, "e", &bn_e); | ||
| if (bn_n) | ||
| { | ||
| ByteString inN = OSSL::bn2ByteString(bn_n); | ||
| setN(inN); | ||
| setN(OSSL::bn2ByteString(bn_n)); | ||
| BN_free(bn_n); | ||
| } | ||
| if (bn_e) | ||
| { | ||
| setE(OSSL::bn2ByteString(bn_e)); | ||
| BN_free(bn_e); | ||
| } | ||
| #else | ||
| const BIGNUM* bn_n = NULL; | ||
| const BIGNUM* bn_e = NULL; | ||
| const RSA* inRSA1 = EVP_PKEY_get0_RSA(const_cast<EVP_PKEY*>(inRSA)); | ||
| RSA_get0_key(inRSA1, &bn_n, &bn_e, NULL); | ||
| if (bn_n) | ||
| { | ||
| setN(OSSL::bn2ByteString(bn_n)); | ||
| } | ||
| if (bn_e) | ||
| { | ||
| ByteString inE = OSSL::bn2ByteString(bn_e); | ||
| setE(inE); | ||
| setE(OSSL::bn2ByteString(bn_e)); | ||
| } | ||
| #endif | ||
|
|
||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -type f -name "OSSLRSAPublicKey.cpp" 2>/dev/null | head -5Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
cd /tmp && find . -type f -name "*.cpp" -o -name "*.h" | grep -i rsa | head -20Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
find . -type f -name "OSSLRSAPublicKey.cpp" -o -name "OSSLRSAPublicKey.h" 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 134
🏁 Script executed:
find . -type f \( -name "*.cpp" -o -name "*.h" \) | xargs grep -l "OSSLRSAPublicKey" 2>/dev/null | head -10Repository: softhsm/SoftHSMv2
Length of output: 233
🏁 Script executed:
find . -type f -name "*RSAPublicKey*" 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 276
🏁 Script executed:
git ls-files | grep -i ossl | grep -i rsaRepository: softhsm/SoftHSMv2
Length of output: 320
🏁 Script executed:
git ls-files | grep RSARepository: softhsm/SoftHSMv2
Length of output: 842
🏁 Script executed:
rg "setFromOSSL" --type cpp --type h -lRepository: softhsm/SoftHSMv2
Length of output: 1098
🏁 Script executed:
rg "OSSLRSAPublicKey" -lRepository: softhsm/SoftHSMv2
Length of output: 358
🏁 Script executed:
git ls-files | grep -E "(crypto|ssl)" | head -20Repository: softhsm/SoftHSMv2
Length of output: 698
🏁 Script executed:
find . -type d -name crypto 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 77
🏁 Script executed:
ls -la src/lib/crypto/ 2>/dev/null | head -20Repository: softhsm/SoftHSMv2
Length of output: 1326
🏁 Script executed:
find . -name "*.cpp" -path "*/crypto/*" 2>/dev/null | head -20Repository: softhsm/SoftHSMv2
Length of output: 777
🏁 Script executed:
cat -n src/lib/crypto/OSSLRSAPublicKey.cpp | sed -n '78,110p'Repository: softhsm/SoftHSMv2
Length of output: 963
🏁 Script executed:
wc -l src/lib/crypto/OSSLRSAPublicKey.cpp 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 100
🏁 Script executed:
ls -lh src/lib/crypto/OSSLRSAPublicKey.cpp 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 145
🏁 Script executed:
find . -name "OSSLRSAPublicKey*" 2>/dev/nullRepository: softhsm/SoftHSMv2
Length of output: 134
🏁 Script executed:
rg "void.*setFromOSSL" -A 5 -B 2Repository: softhsm/SoftHSMv2
Length of output: 11327
🏁 Script executed:
rg "setFromOSSL\(" --type cpp -B 3 -A 1Repository: softhsm/SoftHSMv2
Length of output: 14865
🏁 Script executed:
rg "EVP_PKEY_get0_RSA" --type cpp -B 2 -A 2Repository: softhsm/SoftHSMv2
Length of output: 845
🏁 Script executed:
cat -n src/lib/crypto/OSSLRSAPublicKey.cpp | sed -n '125,160p'Repository: softhsm/SoftHSMv2
Length of output: 958
🏁 Script executed:
cat -n src/lib/crypto/OSSLEDPublicKey.cpp | sed -n '25,50p'Repository: softhsm/SoftHSMv2
Length of output: 831
🏁 Script executed:
cat -n src/lib/crypto/OSSLGOSTPublicKey.cpp | sed -n '40,75p'Repository: softhsm/SoftHSMv2
Length of output: 1080
🏁 Script executed:
cat -n src/lib/crypto/OSSLRSAPrivateKey.cpp | sed -n '38,80p'Repository: softhsm/SoftHSMv2
Length of output: 1194
🏁 Script executed:
cat -n src/lib/crypto/OSSLRSAPublicKey.cpp | head -76Repository: softhsm/SoftHSMv2
Length of output: 2905
🏁 Script executed:
rg "OSSLRSAPublicKey::PKCS8Decode" -A 20Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
rg "PKCS8Decode" src/lib/crypto/OSSLRSAPublicKey.cpp -A 20Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
cat -n src/lib/crypto/OSSLRSAPublicKey.cpp | tail -100Repository: softhsm/SoftHSMv2
Length of output: 2961
🏁 Script executed:
rg "new OSSLRSAPublicKey\(" -B 3 -A 3Repository: softhsm/SoftHSMv2
Length of output: 677
🏁 Script executed:
rg "setFromOSSL" src/lib/crypto/OSSLRSA.cpp -B 5 -A 5Repository: softhsm/SoftHSMv2
Length of output: 364
🏁 Script executed:
cat -n src/lib/crypto/OSSLEDPublicKey.cpp | sed -n '85,115p'Repository: softhsm/SoftHSMv2
Length of output: 980
🏁 Script executed:
cat -n src/lib/crypto/OSSLEDPrivateKey.cpp | sed -n '50,90p'Repository: softhsm/SoftHSMv2
Length of output: 1150
🏁 Script executed:
rg "EVP_PKEY_get0_RSA|EVP_PKEY_get0" src/lib/crypto/ -B 2 -A 3Repository: softhsm/SoftHSMv2
Length of output: 2594
🏁 Script executed:
# Check the exact code at lines 78-110
cat -n src/lib/crypto/OSSLRSAPublicKey.cpp | sed -n '78,110p'Repository: softhsm/SoftHSMv2
Length of output: 963
🏁 Script executed:
# Compare with OSSLRSAPrivateKey which has similar code
cat -n src/lib/crypto/OSSLRSAPrivateKey.cpp | sed -n '73,120p'Repository: softhsm/SoftHSMv2
Length of output: 1523
🏁 Script executed:
# Look for defensive NULL checks after EVP_PKEY_get0 in similar key classes
rg "EVP_PKEY_get0" src/lib/crypto/ -A 2 | grep -E "(EVP_PKEY_get0|if.*NULL)" | head -30Repository: softhsm/SoftHSMv2
Length of output: 740
Add RSA type guards before operating on the key in the legacy branch.
In the legacy OpenSSL path (< 3.0), EVP_PKEY_get0_RSA() can return NULL if the input is not an RSA key, yet the result is dereferenced immediately without validation. An early type check using EVP_PKEY_base_id() and a NULL check after EVP_PKEY_get0_RSA() are needed to prevent crashes. This pattern is already used defensively in OSSLGOSTPrivateKey.
🛠️ Suggested fix
void OSSLRSAPublicKey::setFromOSSL(const EVP_PKEY* inRSA)
{
+ if (inRSA == NULL || EVP_PKEY_base_id(inRSA) != EVP_PKEY_RSA)
+ {
+ ERROR_MSG("Not an RSA key");
+ return;
+ }
`#if` OPENSSL_VERSION_NUMBER >= 0x30000000L
BIGNUM* bn_n = NULL;
BIGNUM* bn_e = NULL;
@@
`#else`
const BIGNUM* bn_n = NULL;
const BIGNUM* bn_e = NULL;
const RSA* inRSA1 = EVP_PKEY_get0_RSA(const_cast<EVP_PKEY*>(inRSA));
+ if (inRSA1 == NULL)
+ {
+ ERROR_MSG("Could not get RSA key");
+ return;
+ }
RSA_get0_key(inRSA1, &bn_n, &bn_e, NULL);🤖 Prompt for AI Agents
In `@src/lib/crypto/OSSLRSAPublicKey.cpp` around lines 78 - 110, In
OSSLRSAPublicKey::setFromOSSL update the legacy OpenSSL (< 3.0) branch to guard
against non-RSA keys by checking EVP_PKEY_base_id(inRSA) == EVP_PKEY_RSA and by
verifying the EVP_PKEY_get0_RSA(inRSA) result is non-NULL before calling
RSA_get0_key; if either check fails, return early (no dereference), otherwise
proceed to call RSA_get0_key and setN/setE as currently done.
| // set mechanism parameters | ||
| void *parameters = NULL; | ||
| size_t paramLen = 0; | ||
| if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) | ||
| { | ||
| rv = BuildRSAOAEPParam((CK_RSA_PKCS_OAEP_PARAMS *)pMechanism->pParameter, | ||
| ¶meters, ¶mLen); | ||
| if (rv != CKR_OK) | ||
| { | ||
| asymCrypto->recyclePublicKey(publicKey); | ||
| CryptoFactory::i()->recycleAsymmetricAlgorithm(asymCrypto); | ||
| return rv; | ||
| } | ||
| } | ||
|
|
||
| session->setOpType(SESSION_OP_ENCRYPT); | ||
| session->setAsymmetricCryptoOp(asymCrypto); | ||
| session->setMechanism(mechanism); | ||
| session->setAllowMultiPartOp(false); | ||
| session->setAllowSinglePartOp(true); | ||
| session->setPublicKey(publicKey); | ||
|
|
||
| if (parameters != NULL) | ||
| { | ||
| session->setParameters(parameters, paramLen); | ||
| free(parameters); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Inspect Session class parameter management
find . -type f -name "Session.h" -o -name "Session.cpp" | head -5Repository: softhsm/SoftHSMv2
Length of output: 126
🏁 Script executed:
# Get Session::setParameters and resetOp implementations
rg -n "setParameters|resetOp" src/lib/session_mgr/Session.h -A 3Repository: softhsm/SoftHSMv2
Length of output: 310
🏁 Script executed:
# Find and inspect BuildRSAOAEPParam
rg -n "BuildRSAOAEPParam" src/lib/SoftHSM.cpp -A 20Repository: softhsm/SoftHSMv2
Length of output: 3515
🏁 Script executed:
# Examine the specific lines in question (AsymEncryptInit and AsymDecryptInit)
sed -n '2515,2545p' src/lib/SoftHSM.cpp
sed -n '3275,3305p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1895
🏁 Script executed:
# Check for other setParameters calls with OAEP to see pattern
rg -n "setParameters" src/lib/SoftHSM.cpp -B 2 -A 2 | grep -A 5 -B 5 "OAEP\|parameters"Repository: softhsm/SoftHSMv2
Length of output: 450
🏁 Script executed:
# Get full Session::setParameters and resetOp implementations
sed -n '79p;106,107p' src/lib/session_mgr/Session.h
rg -n "void Session::setParameters|void Session::resetOp" src/lib/session_mgr/Session.cpp -A 15Repository: softhsm/SoftHSMv2
Length of output: 826
🏁 Script executed:
# Get full BuildRSAOAEPParam implementation
sed -n '13285,13366p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 2358
🏁 Script executed:
# Check Session class for m_pParameters member variable
rg -n "m_pParameters" src/lib/session_mgr/Session.h src/lib/session_mgr/Session.cppRepository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Get the rest of BuildRSAOAEPParam to see the complete assignment
sed -n '13285,13370p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 2398
🏁 Script executed:
# Verify the variable used in pointer arithmetic is indeed 'parameters' (the output param)
sed -n '13350,13366p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 756
🏁 Script executed:
# Verify the exact bug in BuildRSAOAEPParam - check the pointer arithmetic
sed -n '13355p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 133
🏁 Script executed:
# Check if RSA_PKCS_OAEP_PARAMS structure definition to understand the layout
rg -n "struct RSA_PKCS_OAEP_PARAMS|RSA_PKCS_OAEP_PARAMS {" -A 5Repository: softhsm/SoftHSMv2
Length of output: 224
🏁 Script executed:
# Look for how getParameters/setParameters are used in encrypt/decrypt operations
rg -n "getParameters" src/lib/SoftHSM.cpp -B 2 -A 5 | head -40Repository: softhsm/SoftHSMv2
Length of output: 1319
🏁 Script executed:
# Verify the critical bug: line 13355 uses 'parameters' instead of 'paramBuf'
sed -n '13350,13366p' src/lib/SoftHSM.cpp | cat -nRepository: softhsm/SoftHSMv2
Length of output: 875
🏁 Script executed:
# Search for RSA_PKCS_OAEP_PARAMS definition
fd -t f "\.h$" | xargs grep -l "RSA_PKCS_OAEP_PARAMS" | head -3Repository: softhsm/SoftHSMv2
Length of output: 139
🏁 Script executed:
# Check the RSA_PKCS_OAEP_PARAMS structure
rg "RSA_PKCS_OAEP_PARAMS" -A 8 src/lib/crypto/ src/lib/Repository: softhsm/SoftHSMv2
Length of output: 22501
🏁 Script executed:
# Verify the bug is consistent across all BuildRSAOAEPParam usages
rg -n "oaep_param\.sourceData.*parameters" src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 142
🏁 Script executed:
# Check if there are any fixes or workarounds downstream in OSSLRSA or other backends
rg -n "sourceData" src/lib/crypto/OSSLRSA.cpp -B 2 -A 2 | head -30Repository: softhsm/SoftHSMv2
Length of output: 914
Fix pointer arithmetic in BuildRSAOAEPParam: use paramBuf instead of parameters.
Line 13355 computes oaep_param.sourceData = (char *)parameters + sizeof(RSA_PKCS_OAEP_PARAMS);, but parameters is a void ** (output parameter). This causes sourceData to reference garbage memory. When Session::setParameters deep-copies this struct and later OSSLRSA::encrypt calls OPENSSL_memdup(oaepParam->sourceData, ...), it attempts to read from the invalid address, resulting in use-after-free or crash.
Change to: oaep_param.sourceData = (char *)paramBuf + sizeof(RSA_PKCS_OAEP_PARAMS);
Also affects lines 3275–3299 (decrypt) and wrap/unwrap operations.
🤖 Prompt for AI Agents
In `@src/lib/SoftHSM.cpp` around lines 2515 - 2540, BuildRSAOAEPParam is computing
oaep_param.sourceData using the wrong variable: it's using the output void**
parameter (parameters) instead of the internal buffer pointer (paramBuf),
causing sourceData to point to invalid memory when Session::setParameters
deep-copies the struct and later OSSLRSA::encrypt/OSSLRSA::decrypt/unwrap/read
use OPENSSL_memdup on that pointer. Fix BuildRSAOAEPParam (and the equivalent
RSA OAEP param builders used for decrypt and wrap/unwrap) to set
oaep_param.sourceData = (char *)paramBuf + sizeof(RSA_PKCS_OAEP_PARAMS) (and
similarly use paramBuf for any pointer arithmetic), allocate/copy into paramBuf
consistently, and leave the output parameter (parameters) as the pointer to
paramBuf so Session::setParameters receives a valid contiguous buffer.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6480-6564: Free the OAEP parameter buffer in WrapKeyAsym.
BuildRSAOAEPParamallocatesparameters, but it is never freed on success or on early error returns (e.g., Line 6525/6546/6560). This leaks per wrap.🧩 Suggested fix (scope guard)
void *parameters = NULL; size_t paramLen = 0; size_t hashLen = 0; + std::unique_ptr<void, decltype(&free)> paramGuard(nullptr, free); if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) { rv = BuildRSAOAEPParam((CK_RSA_PKCS_OAEP_PARAMS *)pMechanism->pParameter, ¶meters,¶mLen,&hashLen); if (rv != CKR_OK) { return rv; } + paramGuard.reset(parameters); } @@ - if (!cipher->wrapKey(publicKey, keydata, wrapped, mech,parameters,paramLen)) + if (!cipher->wrapKey(publicKey, keydata, wrapped, mech, paramGuard.get(), paramLen)) { cipher->recyclePublicKey(publicKey); CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); return CKR_GENERAL_ERROR; }
🤖 Fix all issues with AI agents
In `@src/lib/SoftHSM.cpp`:
- Around line 7125-7140: BuildRSAOAEPParam failures currently return early
without cleaning up RSA resources; when BuildRSAOAEPParam returns a non-CKR_OK
you must recycle the unwrapping key and release any allocated resources before
returning. Modify the error path after BuildRSAOAEPParam to call
cipher->recyclePrivateKey(unwrappingkey) (and free(parameters) if non-NULL) and
then return rv, so unwrappingkey and cipher-managed resources are not leaked;
use the existing symbols BuildRSAOAEPParam, unwrappingkey, cipher, and
recyclePrivateKey to locate and fix the code.
|
The main goal of this improvement is to support new hash functions and LABEL for RSA OAEP encryption by processing CK_RSA_PKCS_OAEP_PARAMS parameters in accordance with PKCS#11. This is an important feature requested by users, as SHA-1 is deprecated. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6480-6564:⚠️ Potential issue | 🟠 MajorFree OAEP parameter buffers on all WrapKeyAsym exits.
BuildRSAOAEPParamallocatesparameters(Line 6489-6496) but the buffer is never freed—neither on success nor on later error paths (e.g., key size checks, key load failure, wrap failure). This leaks on every RSA-OAEP wrap call.🛠️ Suggested fix (ensure `parameters` is always freed)
CK_RV rv = CKR_OK; const size_t bb = 8; AsymAlgo::Type algo = AsymAlgo::Unknown; AsymMech::Type mech = AsymMech::Unknown; void *parameters = NULL; size_t paramLen = 0; size_t hashLen = 0; if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) { rv = BuildRSAOAEPParam((CK_RSA_PKCS_OAEP_PARAMS *)pMechanism->pParameter, ¶meters,¶mLen,&hashLen); if (rv != CKR_OK) { return rv; } } AsymmetricAlgorithm* cipher = CryptoFactory::i()->getAsymmetricAlgorithm(algo); - if (cipher == NULL) return CKR_MECHANISM_INVALID; + if (cipher == NULL) { free(parameters); return CKR_MECHANISM_INVALID; } PublicKey* publicKey = cipher->newPublicKey(); if (publicKey == NULL) { CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); + free(parameters); return CKR_HOST_MEMORY; } switch(pMechanism->mechanism) { case CKM_RSA_PKCS: case CKM_RSA_PKCS_OAEP: if (getRSAPublicKey((RSAPublicKey*)publicKey, token, wrapKey) != CKR_OK) { cipher->recyclePublicKey(publicKey); CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); + free(parameters); return CKR_GENERAL_ERROR; } break; default: + free(parameters); return CKR_MECHANISM_INVALID; } // Wrap the key if (!cipher->wrapKey(publicKey, keydata, wrapped, mech,parameters,paramLen)) { cipher->recyclePublicKey(publicKey); CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); + free(parameters); return CKR_GENERAL_ERROR; } cipher->recyclePublicKey(publicKey); CryptoFactory::i()->recycleAsymmetricAlgorithm(cipher); + free(parameters); return CKR_OK;
🤖 Fix all issues with AI agents
In `@src/lib/SoftHSM.cpp`:
- Around line 13357-13370: The OAEP params struct (RSA_PKCS_OAEP_PARAMS) is
copied into a session buffer by session->setParameters but its internal
sourceData pointer keeps pointing at the original (now freed) allocation; fix by
rebasing sourceData after the deep-copy: in setParameters, after allocating the
session buffer and memcpy’ing the incoming paramBuf, compute the offset =
((char*)orig->sourceData - (char*)orig) and set
((RSA_PKCS_OAEP_PARAMS*)sessionBuf)->sourceData = (char*)sessionBuf + offset
(ensuring bounds checks for ulSourceDataLen); update handling for
AsymEncryptInit and any code that passes parameter buffers so the pointer inside
RSA_PKCS_OAEP_PARAMS (used later in OSSLRSA.cpp) points into the session-owned
buffer rather than the freed source.
noiser7
force-pushed
the
Refactor-RSA/Add-modern-hash-on-OAEP
branch
from
fa6a055 to
dfddb57
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6480-6569:⚠️ Potential issue | 🟠 MajorFix OAEP wrap size check to use
hashLenderived from params.
hashLenis still 0 when the OAEP size check runs, so oversized input can pass and fail later with the wrong error. DerivehashLenfrom the OAEP params before the check.🛠️ Suggested fix
@@ - switch(pMechanism->mechanism) { + if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) + { + switch (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { + case CKM_SHA_1: hashLen = 20; break; + case CKM_SHA224: hashLen = 28; break; + case CKM_SHA256: hashLen = 32; break; + case CKM_SHA384: hashLen = 48; break; + case CKM_SHA512: hashLen = 64; break; + default: return CKR_ARGUMENTS_BAD; + } + } + + switch(pMechanism->mechanism) { @@ case CKM_RSA_PKCS_OAEP: mech = AsymMech::RSA_PKCS_OAEP; // PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen if (keydata.size() > modulus_length - 2 - (2 * hashLen)) return CKR_KEY_SIZE_RANGE; break;
🤖 Fix all issues with AI agents
In `@src/lib/crypto/OSSLRSAPrivateKey.cpp`:
- Around line 142-148: The null-check is using the wrong variable: after calling
EVP_PKEY_get0_RSA(const_cast<EVP_PKEY *>(inRSA)) into inRSA1, change the
conditional to check inRSA1 != NULL (not inRSA) before calling RSA_get0_factors,
RSA_get0_crt_params, and RSA_get0_key in OSSLRSAPrivateKey.cpp; ensure all
subsequent RSA_get0_* calls are guarded by that inRSA1 check to avoid NULL
dereference (symbols to edit: inRSA1, EVP_PKEY_get0_RSA, RSA_get0_factors,
RSA_get0_crt_params, RSA_get0_key).
In `@src/lib/SoftHSM.cpp`:
- Around line 13292-13375: BuildRSAOAEPParam has a potential size_t overflow
when computing bufLen = sizeof(RSA_PKCS_OAEP_PARAMS) + params->ulSourceDataLen
which can make malloc too small and cause memcpy overruns; fix it by validating
that params->ulSourceDataLen <= std::numeric_limits<size_t>::max() -
sizeof(RSA_PKCS_OAEP_PARAMS) (or using safe addition/checkAdd) before
allocating, return CKR_ARGUMENTS_BAD or CKR_HOST_MEMORY on overflow, and only
call malloc and memcpy when the checked bufLen is safe; update references in
this function (RSA_PKCS_OAEP_PARAMS, ulSourceDataLen, paramBuf, paramLen,
parameters, memcpy, malloc) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
73-181:⚠️ Potential issue | 🟠 MajorAdd null input guard to
setFromOSSLto prevent OpenSSL 3.0+ crash.The OpenSSL 3.0+ branch calls
EVP_PKEY_get_bn_param(inRSA, ...)without checking ifinRSAis NULL, whereas the OpenSSL <3.0 branch properly validates the input. This inconsistency means the function would crash with a NULL input on OpenSSL 3.0+ (and silently retain stale data on <3.0). Add an early return guard at the start of the function to enforce the precondition uniformly across versions.🛠️ Proposed fix
void OSSLRSAPrivateKey::setFromOSSL(const EVP_PKEY* inRSA) { + if (inRSA == NULL) + { + ERROR_MSG("NULL RSA key"); + return; + } `#if` OPENSSL_VERSION_NUMBER >= 0x30000000L BIGNUM* bn_p = NULL; @@ `#else` const BIGNUM* bn_p = NULL; // ... const RSA* inRSA1 = EVP_PKEY_get0_RSA(const_cast<EVP_PKEY *>(inRSA)); - if (inRSA1 != NULL) - { - RSA_get0_factors(inRSA1, &bn_p, &bn_q); - RSA_get0_crt_params(inRSA1, &bn_dmp1, &bn_dmq1, &bn_iqmp); - RSA_get0_key(inRSA1, &bn_n, &bn_e, &bn_d); - } + if (inRSA1 == NULL) + { + ERROR_MSG("Could not get RSA key"); + return; + } + RSA_get0_factors(inRSA1, &bn_p, &bn_q); + RSA_get0_crt_params(inRSA1, &bn_dmp1, &bn_dmq1, &bn_iqmp); + RSA_get0_key(inRSA1, &bn_n, &bn_e, &bn_d); `#endif` }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/crypto/OSSLRSAPrivateKey.cpp`:
- Around line 414-420: The legacy-path handles BIGNUMs allocated earlier but
returns immediately if RSA_new() fails, leaking those BIGNUMs; update the error
path in OSSLRSAPrivateKey (around RSA_new/RSA_set0_* usage) to free all
individually allocated BIGNUM variables (the bn_* locals created before RSA_new)
before returning when RSA_new() returns NULL, mirroring the OpenSSL 3.x cleanup
logic so ownership is not lost.
In `@src/lib/crypto/OSSLRSAPublicKey.cpp`:
- Around line 199-205: The legacy path allocates BIGNUMs earlier but if
RSA_new() fails in the else block the allocated BIGNUMs are leaked; modify the
failure branch after RSA_new() (the block that logs "Could not build RSA
object") to BN_free each of the BIGNUM variables you created earlier (e.g.,
bn_e, bn_n or whatever BIGNUM pointers are used in this file) before returning,
and optionally set those pointers to NULL after freeing them; use BN_free(...)
to release them and keep the existing process/error logging (function: RSA_new,
free with BN_free).
🧹 Nitpick comments (1)
src/lib/crypto/OSSLRSAPublicKey.cpp (1)
78-116: Consider adding type validation for non-RSA keys.While the legacy path now correctly checks
inRSA1 != NULLat line 104 (addressing the previous review), passing a non-RSAEVP_PKEYresults in silent failure:
- In the 3.x path,
EVP_PKEY_get_bn_paramreturns failure but no error is logged- In the legacy path,
EVP_PKEY_get0_RSAreturns NULL, skipping extractionAdding a type check would provide clearer diagnostics. The pattern from
OSSLGOSTPrivateKeyusesEVP_PKEY_base_id().♻️ Suggested improvement
void OSSLRSAPublicKey::setFromOSSL(const EVP_PKEY* inRSA) { if (inRSA == NULL) { ERROR_MSG("Null RSA key"); return; - } + } + if (EVP_PKEY_base_id(inRSA) != EVP_PKEY_RSA) + { + ERROR_MSG("Not an RSA key"); + return; + } `#if` OPENSSL_VERSION_NUMBER >= 0x30000000L
2 participants
Refactor RSA for OpenSSL 3.0
Add new SHA hashes and MGF for RSA OAEP encryption/decryption
Add label support for OAEP encryption
Summary by CodeRabbit
New Features
Improvements
Tests