Refactor rsa/add modern hashs in OAEP encrypt-decrypt #833
Conversation
📝 WalkthroughWalkthroughThis PR introduces OAEP parameter handling infrastructure across RSA cryptographic operations by extending asymmetric algorithm API signatures to accept optional parameter blocks (param, paramLen), migrating OSSLRSA from legacy RSA OpenSSL APIs to EVP_PKEY-based operations, and updating multiple crypto algorithm implementations to propagate parameters through encryption/decryption and wrap/unwrap flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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.
| if ((param == NULL)||(paramLen != sizeof(RSA_PKCS_OAEP_PARAMS))) | ||
| { | ||
| ERROR_MSG("Invalid RSA enryption OAEP parameter supplied"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Typo in error message.
"enryption" should be "encryption".
🔎 Proposed fix
- ERROR_MSG("Invalid RSA enryption OAEP parameter supplied");
+ ERROR_MSG("Invalid RSA encryption OAEP parameter supplied");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((param == NULL)||(paramLen != sizeof(RSA_PKCS_OAEP_PARAMS))) | |
| { | |
| ERROR_MSG("Invalid RSA enryption OAEP parameter supplied"); | |
| return false; | |
| } | |
| if ((param == NULL)||(paramLen != sizeof(RSA_PKCS_OAEP_PARAMS))) | |
| { | |
| ERROR_MSG("Invalid RSA encryption OAEP parameter supplied"); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1437 to 1441 the error message string
contains a typo "enryption"; update the ERROR_MSG call to correct the spelling
to "encryption" (i.e., change "Invalid RSA enryption OAEP parameter supplied" to
"Invalid RSA encryption OAEP parameter supplied") ensuring no other text or
formatting is altered.
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(rsa, NULL); | ||
| if (ctx == NULL) | ||
| { | ||
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); | ||
| ERROR_MSG("An error occurred while creating RSA signature context"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Misleading error messages reference "signature context" in encrypt/decrypt functions.
Lines 1524 and 1666 both say "RSA signature context" but are in encrypt/decrypt paths. Consider updating to "RSA encryption context" and "RSA decryption context" respectively for clarity during debugging.
Also applies to: 1663-1668
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1521-1526 and 1663-1668 the ERROR_MSG
strings incorrectly refer to an "RSA signature context" inside RSA
encrypt/decrypt code paths; change the messages to accurately say "RSA
encryption context" for the encrypt path (around 1521-1526) and "RSA decryption
context" for the decrypt path (around 1663-1668) so logs reflect the correct
operation being performed.
| if (osslPadding == RSA_PKCS1_OAEP_PADDING) | ||
| { | ||
| void *labelData=NULL; | ||
| if (oaepParam->sourceDataLen != 0) | ||
| labelData = OPENSSL_memdup(oaepParam->sourceData,oaepParam->sourceDataLen); | ||
|
|
||
| if ((EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hash) <= 0) || | ||
| (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0) || | ||
| (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0)) | ||
| { | ||
| EVP_PKEY_CTX_free(ctx); | ||
| ERROR_MSG("RSA encrypt set oaep parameters failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential memory leak in OAEP label handling.
If any of the EVP_PKEY_CTX_set_* calls fail, labelData (allocated via OPENSSL_memdup) may leak. The set0 function takes ownership only on success; on failure, the caller must free it.
🔎 Proposed fix
if (osslPadding == RSA_PKCS1_OAEP_PADDING)
{
void *labelData=NULL;
if (oaepParam->sourceDataLen != 0)
labelData = OPENSSL_memdup(oaepParam->sourceData,oaepParam->sourceDataLen);
if ((EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hash) <= 0) ||
- (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0) ||
- (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0))
+ (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0))
{
+ OPENSSL_free(labelData);
EVP_PKEY_CTX_free(ctx);
ERROR_MSG("RSA encrypt set oaep parameters failed (0x%08X)", ERR_get_error());
return false;
}
+ if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0)
+ {
+ OPENSSL_free(labelData);
+ EVP_PKEY_CTX_free(ctx);
+ ERROR_MSG("RSA encrypt set oaep label failed (0x%08X)", ERR_get_error());
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (osslPadding == RSA_PKCS1_OAEP_PADDING) | |
| { | |
| void *labelData=NULL; | |
| if (oaepParam->sourceDataLen != 0) | |
| labelData = OPENSSL_memdup(oaepParam->sourceData,oaepParam->sourceDataLen); | |
| if ((EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hash) <= 0) || | |
| (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0) || | |
| (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0)) | |
| { | |
| EVP_PKEY_CTX_free(ctx); | |
| ERROR_MSG("RSA encrypt set oaep parameters failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| } | |
| if (osslPadding == RSA_PKCS1_OAEP_PADDING) | |
| { | |
| void *labelData=NULL; | |
| if (oaepParam->sourceDataLen != 0) | |
| labelData = OPENSSL_memdup(oaepParam->sourceData,oaepParam->sourceDataLen); | |
| if ((EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hash) <= 0) || | |
| (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0)) | |
| { | |
| OPENSSL_free(labelData); | |
| EVP_PKEY_CTX_free(ctx); | |
| ERROR_MSG("RSA encrypt set oaep parameters failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0) | |
| { | |
| OPENSSL_free(labelData); | |
| EVP_PKEY_CTX_free(ctx); | |
| ERROR_MSG("RSA encrypt set oaep label failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1534-1548, the OAEP labelData
allocated with OPENSSL_memdup can leak if any EVP_PKEY_CTX_set_* call fails
because EVP_PKEY_CTX_set0_rsa_oaep_label only takes ownership on success; modify
the logic so that you allocate labelData as before but check the return of
EVP_PKEY_CTX_set_rsa_oaep_md and EVP_PKEY_CTX_set_rsa_mgf1_md first, and if
either fails free labelData (using OPENSSL_free) before freeing ctx and
returning false, or alternatively call EVP_PKEY_CTX_set0_rsa_oaep_label only
after the other two succeed and if that final call fails free labelData before
cleanup; ensure you only avoid freeing labelData when set0 succeeds.
| if ((EVP_PKEY_decrypt_init(ctx) <= 0) || | ||
| (EVP_PKEY_CTX_set_rsa_padding(ctx, osslPadding) <= 0)) | ||
| { | ||
| EVP_PKEY_CTX_free(ctx); | ||
| ERROR_MSG("RSA encrypt failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Incorrect error message in decrypt function.
Line 1673 says "RSA encrypt failed" but this is the decrypt path.
🔎 Proposed fix
- ERROR_MSG("RSA encrypt failed (0x%08X)", ERR_get_error());
+ ERROR_MSG("RSA decrypt init failed (0x%08X)", ERR_get_error());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((EVP_PKEY_decrypt_init(ctx) <= 0) || | |
| (EVP_PKEY_CTX_set_rsa_padding(ctx, osslPadding) <= 0)) | |
| { | |
| EVP_PKEY_CTX_free(ctx); | |
| ERROR_MSG("RSA encrypt failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| if ((EVP_PKEY_decrypt_init(ctx) <= 0) || | |
| (EVP_PKEY_CTX_set_rsa_padding(ctx, osslPadding) <= 0)) | |
| { | |
| EVP_PKEY_CTX_free(ctx); | |
| ERROR_MSG("RSA decrypt init failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1669 to 1675, the error message
incorrectly says "RSA encrypt failed" inside the decrypt path; update the log to
reflect decryption failure and include the OpenSSL error code. Replace the
message string with something like "RSA decrypt failed (0x%08X)" so the log
accurately describes the operation and retains the ERR_get_error() value for
diagnostics.
| if (osslPadding == RSA_PKCS1_OAEP_PADDING) | ||
| { | ||
| void *labelData=NULL; | ||
| if (oaepParam->sourceDataLen != 0) | ||
| labelData = OPENSSL_memdup(oaepParam->sourceData,oaepParam->sourceDataLen); | ||
| if ((EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hash) <= 0) || | ||
| (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgf) <= 0) || | ||
| (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, labelData, oaepParam->sourceDataLen) <= 0)) | ||
| { | ||
| EVP_PKEY_CTX_free(ctx); | ||
| ERROR_MSG("RSA verify set mgf1 failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same OAEP label memory leak issue as in encrypt.
The decrypt path has the same potential memory leak where labelData may not be freed if earlier EVP_PKEY_CTX_set_* calls fail before set0_rsa_oaep_label takes ownership.
Apply the same fix pattern as suggested for the encrypt function.
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1676 to 1689, the OAEP label
allocation (labelData) may leak if EVP_PKEY_CTX_set_rsa_oaep_md or
EVP_PKEY_CTX_set_rsa_mgf1_md fail before ownership is transferred to
EVP_PKEY_CTX_set0_rsa_oaep_label; change the sequence to: allocate labelData as
you do, call EVP_PKEY_CTX_set_rsa_oaep_md and EVP_PKEY_CTX_set_rsa_mgf1_md
first, and if either of those fails free labelData before freeing ctx and
returning false; only call EVP_PKEY_CTX_set0_rsa_oaep_label (which takes
ownership) after the prior calls succeed, and on failure of set0_rsa_oaep_label
free ctx but do not free labelData (since ownership was transferred) — mirror
the same safe free-on-error pattern used in the encrypt function.
| EVP_PKEY *rsa = EVP_PKEY_new(); | ||
| if (rsa == NULL) | ||
| { | ||
| ERROR_MSG("Failed to instantiate OpenSSL RSA object"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| BIGNUM* bn_e = OSSL::byteString2bn(params->getE()); | ||
|
|
||
| BIGNUM *bn_e = OSSL::byteString2bn(params->getE()); | ||
| // Check if the key was successfully generated | ||
| if (!RSA_generate_key_ex(rsa, params->getBitLength(), bn_e, NULL)) | ||
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, NULL); | ||
| if (ctx == NULL) | ||
| { | ||
| ERROR_MSG("Failed to create RSA key creation context"); | ||
| BN_free(bn_e); | ||
| EVP_PKEY_free(rsa); | ||
| return false; | ||
| } | ||
| if ((EVP_PKEY_keygen_init(ctx) <= 0) || | ||
| (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, params->getBitLength()) <= 0) || | ||
| (EVP_PKEY_CTX_set1_rsa_keygen_pubexp(ctx, bn_e) <= 0) || | ||
| (EVP_PKEY_keygen(ctx, &rsa) <= 0)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSSL 3 EVP_PKEY_keygen ppkey pointer NULL pre-allocated documentation
💡 Result:
Short answer: pass a NULL EVP_PKEY* (i.e. EVP_PKEY *pkey = NULL; EVP_PKEY_keygen(ctx, &pkey)) and let OpenSSL allocate it. The EVP docs state "If *ppkey is NULL when this function is called, it will be allocated, and should be freed by the caller using EVP_PKEY_free()." Use EVP_PKEY_free() when done. [1][2]
If you pass a non-NULL *ppkey you must manage its lifetime yourself; the docs only guarantee allocation when ppkey is NULL, so the safe common pattern is to initialize the EVP_PKEY to NULL and let EVP_PKEY_keygen/EVP_PKEY_generate set it. [1][2]
Sources:
- EVP_PKEY_keygen / EVP_PKEY_generate documentation. [1]
- EVP_PKEY_new / EVP_PKEY_free documentation (allocation/free semantics). [2]
Remove unnecessary EVP_PKEY_new() before EVP_PKEY_keygen() at line 1756.
OpenSSL 3 documentation specifies that EVP_PKEY_keygen() allocates the key internally when passed a NULL pointer. Pre-allocating an empty key with EVP_PKEY_new() violates the documented API contract and can lead to a memory leak or undefined behavior. Initialize rsa to NULL instead and let EVP_PKEY_keygen() handle allocation: EVP_PKEY *rsa = NULL; followed by EVP_PKEY_keygen(ctx, &rsa) at line 1777.
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1756 to 1777, remove the
pre-allocation of an EVP_PKEY with EVP_PKEY_new() and instead initialize rsa to
NULL so EVP_PKEY_keygen() can allocate it; also remove the corresponding
EVP_PKEY_free() path that would free the pre-allocated object on error and
ensure any cleanup only frees bn_e and the ctx when appropriate, relying on
EVP_PKEY_keygen(ctx, &rsa) to set rsa on success and freeing rsa only when it
was actually allocated.
| // set mechanism parameters | ||
| void *parameters = NULL; | ||
| size_t paramLen = 0; | ||
| RSA_PKCS_OAEP_PARAMS oaep_param; | ||
| if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) | ||
| { | ||
| CK_RSA_PKCS_OAEP_PARAMS* par1 = (CK_RSA_PKCS_OAEP_PARAMS*) pMechanism->pParameter; | ||
| switch (par1->hashAlg) | ||
| { | ||
| case CKM_SHA_1: | ||
| oaep_param.hashAlg = HashAlgo::SHA1; | ||
| break; | ||
| case CKM_SHA224: | ||
| oaep_param.hashAlg = HashAlgo::SHA224; | ||
| break; | ||
| case CKM_SHA256: | ||
| oaep_param.hashAlg = HashAlgo::SHA256; | ||
| break; | ||
| case CKM_SHA384: | ||
| oaep_param.hashAlg = HashAlgo::SHA384; | ||
| break; | ||
| case CKM_SHA512: | ||
| oaep_param.hashAlg = HashAlgo::SHA512; | ||
| break; | ||
| default: | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| switch (par1->mgf) | ||
| { | ||
| case CKG_MGF1_SHA1: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA1; | ||
| break; | ||
| case CKG_MGF1_SHA224: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA224; | ||
| break; | ||
| case CKG_MGF1_SHA256: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA256; | ||
| break; | ||
| case CKG_MGF1_SHA384: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA384; | ||
| break; | ||
| case CKG_MGF1_SHA512: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA512; | ||
| break; | ||
| default: | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| // need copy parameters to session context | ||
| // label source data will be copyed to end of parameter block | ||
| paramLen = sizeof(RSA_PKCS_OAEP_PARAMS) + par1->ulSourceDataLen; | ||
| parameters = malloc(paramLen); | ||
| if (parameters == NULL) | ||
| return CKR_HOST_MEMORY; | ||
| oaep_param.sourceData = (char*)parameters + sizeof(RSA_PKCS_OAEP_PARAMS); | ||
| oaep_param.sourceDataLen = par1->ulSourceDataLen; | ||
| memcpy(parameters,&oaep_param,sizeof(RSA_PKCS_OAEP_PARAMS)); | ||
| memcpy(oaep_param.sourceData,par1->pSourceData,par1->ulSourceDataLen); | ||
| } | ||
|
|
||
| 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:
# First, find the Session class definition and setParameters method
fd -e h -e cpp | head -20Repository: softhsm/SoftHSMv2
Length of output: 735
🏁 Script executed:
# Search for Session::setParameters implementation
rg "class Session" -A 5 --type cpp --type hRepository: softhsm/SoftHSMv2
Length of output: 3351
🏁 Script executed:
# Find setParameters method definition
rg "setParameters" -B 2 -A 10 --type cpp --type h | head -100Repository: softhsm/SoftHSMv2
Length of output: 4150
🏁 Script executed:
# Get the full AsymEncryptInit context around the OAEP block to see cleanup patterns
rg "AsymEncryptInit" -A 100 --type cpp | head -150Repository: softhsm/SoftHSMv2
Length of output: 7096
🏁 Script executed:
# Check how publicKey/asymCrypto are typically recycled
rg "recyclePublicKey|recycleAsymmetricAlgorithm" -B 3 -A 1 --type cpp | head -80Repository: softhsm/SoftHSMv2
Length of output: 3360
🏁 Script executed:
# Get the full Session::setParameters implementation
rg "void Session::setParameters" -A 20 --type cppRepository: softhsm/SoftHSMv2
Length of output: 1100
🏁 Script executed:
# Also check Session::resetOp to see how parameters are cleaned up
rg "Session::resetOp" -A 20 --type cppRepository: softhsm/SoftHSMv2
Length of output: 1045
🏁 Script executed:
# Check if there's a destructor or cleanup that frees the param member
rg "~Session|resetOp|free(param)" -B 2 -A 3 --type cpp | head -100Repository: softhsm/SoftHSMv2
Length of output: 3586
🏁 Script executed:
# Get full AsymEncryptInit to see exact line numbers and flow
sed -n '2515,2590p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 2300
🏁 Script executed:
# Find where publicKey and asymCrypto are allocated to confirm they're before the OAEP block
sed -n '2500,2580p' src/lib/SoftHSM.cpp | head -80Repository: softhsm/SoftHSMv2
Length of output: 2369
🏁 Script executed:
# Look at a few lines before line 2515 to see the allocation
sed -n '2480,2520p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1030
Add error path cleanup to prevent leaking publicKey and asymCrypto on OAEP parameter validation failures
The OAEP parameter mapping block contains two validation default: branches and one allocation check that return CKR_ARGUMENTS_BAD or CKR_HOST_MEMORY without freeing publicKey and asymCrypto, which were allocated earlier in the function. These early returns must clean up both resources before returning.
Use a common cleanup path or add paired recyclePublicKey/recycleAsymmetricAlgorithm calls at each error site:
+ CK_RV rv = CKR_OK;
void *parameters = NULL;
// ...
if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP)
{
// ...
default:
- return CKR_ARGUMENTS_BAD;
+ rv = CKR_ARGUMENTS_BAD;
+ goto cleanup;
// ...
default:
- return CKR_ARGUMENTS_BAD;
+ rv = CKR_ARGUMENTS_BAD;
+ goto cleanup;
// ...
if (parameters == NULL)
- return CKR_HOST_MEMORY;
+ {
+ rv = CKR_HOST_MEMORY;
+ goto cleanup;
+ }
}
+
+ // ... rest of setOpType/setMechanism/etc ...
+ return CKR_OK;
+
+cleanup:
+ if (publicKey) asymCrypto->recyclePublicKey(publicKey);
+ if (asymCrypto) CryptoFactory::i()->recycleAsymmetricAlgorithm(asymCrypto);
+ if (parameters) free(parameters);
+ return rv;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 2515 to 2584 the OAEP parameter validation
and allocation paths return CKR_ARGUMENTS_BAD or CKR_HOST_MEMORY on error but do
not free previously acquired resources (publicKey and asymCrypto) nor any
partially allocated parameters, causing leaks; update each early-return path to
first clean up: if publicKey is non-NULL call recyclePublicKey(publicKey); if
asymCrypto is non-NULL call recycleAsymmetricAlgorithm(asymCrypto); if
parameters was allocated (non-NULL) free it before returning; alternatively
implement a single labeled cleanup path (e.g. err:) that performs these
deallocations and returns the error code, and replace the direct returns with
setting the error code and goto err.
| // set mechanism parameters | ||
| void *parameters = NULL; | ||
| size_t paramLen = 0; | ||
| RSA_PKCS_OAEP_PARAMS oaep_param; | ||
| if (pMechanism->mechanism == CKM_RSA_PKCS_OAEP) | ||
| { | ||
| CK_RSA_PKCS_OAEP_PARAMS* par1 = (CK_RSA_PKCS_OAEP_PARAMS*) pMechanism->pParameter; | ||
| switch (par1->hashAlg) | ||
| { | ||
| case CKM_SHA_1: | ||
| oaep_param.hashAlg = HashAlgo::SHA1; | ||
| break; | ||
| case CKM_SHA224: | ||
| oaep_param.hashAlg = HashAlgo::SHA224; | ||
| break; | ||
| case CKM_SHA256: | ||
| oaep_param.hashAlg = HashAlgo::SHA256; | ||
| break; | ||
| case CKM_SHA384: | ||
| oaep_param.hashAlg = HashAlgo::SHA384; | ||
| break; | ||
| case CKM_SHA512: | ||
| oaep_param.hashAlg = HashAlgo::SHA512; | ||
| break; | ||
| default: | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| switch (par1->mgf) | ||
| { | ||
| case CKG_MGF1_SHA1: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA1; | ||
| break; | ||
| case CKG_MGF1_SHA224: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA224; | ||
| break; | ||
| case CKG_MGF1_SHA256: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA256; | ||
| break; | ||
| case CKG_MGF1_SHA384: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA384; | ||
| break; | ||
| case CKG_MGF1_SHA512: | ||
| oaep_param.mgf = AsymRSAMGF::MGF1_SHA512; | ||
| break; | ||
| default: | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| // need copy parameters to session context | ||
| // label source data will be copyed to end of parameter block | ||
| paramLen = sizeof(RSA_PKCS_OAEP_PARAMS) + par1->ulSourceDataLen; | ||
| parameters = malloc(paramLen); | ||
| if (parameters == NULL) | ||
| return CKR_HOST_MEMORY; | ||
| oaep_param.sourceData = (char*)parameters + sizeof(RSA_PKCS_OAEP_PARAMS); | ||
| oaep_param.sourceDataLen = par1->ulSourceDataLen; | ||
| memcpy(parameters,&oaep_param,sizeof(RSA_PKCS_OAEP_PARAMS)); | ||
| memcpy(oaep_param.sourceData,par1->pSourceData,par1->ulSourceDataLen); | ||
| } | ||
|
|
||
| session->setOpType(SESSION_OP_DECRYPT); | ||
| session->setAsymmetricCryptoOp(asymCrypto); | ||
| session->setMechanism(mechanism); | ||
| session->setAllowMultiPartOp(false); | ||
| session->setAllowSinglePartOp(true); | ||
| session->setPrivateKey(privateKey); | ||
|
|
||
| if (parameters != NULL) | ||
| { | ||
| session->setParameters(parameters,paramLen); | ||
| free(parameters); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the AsymDecryptInit function and examine the code around line 3332-3401
rg -n "AsymDecryptInit" src/lib/SoftHSM.cpp | head -5Repository: softhsm/SoftHSMv2
Length of output: 235
🏁 Script executed:
# Check the structure of the file to understand line numbers
wc -l src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 86
🏁 Script executed:
# Search for MechParamCheckRSAPKCSOAEP to verify it exists and how it's used
rg -n "MechParamCheckRSAPKCSOAEP" src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 366
🏁 Script executed:
# Look for AsymEncryptInit to compare the patterns
rg -n "AsymEncryptInit" src/lib/SoftHSM.cpp | head -5Repository: softhsm/SoftHSMv2
Length of output: 235
🏁 Script executed:
# Read AsymDecryptInit from the function definition to see the full implementation
sed -n '3213,3420p' src/lib/SoftHSM.cpp | head -150Repository: softhsm/SoftHSMv2
Length of output: 4350
🏁 Script executed:
# Check AsymEncryptInit to compare the OAEP handling patterns
sed -n '2419,2600p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 5392
🏁 Script executed:
# Read MechParamCheckRSAPKCSOAEP to see what validation it performs
sed -n '13402,13450p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1694
🏁 Script executed:
# Check if there's cleanup code on error paths in decrypt - look for asymCrypto deallocation
sed -n '3300,3410p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 3192
🏁 Script executed:
# Verify wrap/unwrap OAEP usage
rg -B5 -A5 "MechParamCheckRSAPKCSOAEP" src/lib/SoftHSM.cpp | head -80Repository: softhsm/SoftHSMv2
Length of output: 1422
🏁 Script executed:
# Let's see the exact line numbers and structure of the hard-coded SHA-1/MGF1-SHA1 checks in decrypt
sed -n '3275,3310p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1090
🏁 Script executed:
# Check what happens if the hash/mgf switch in the parameter packing fails - is asymCrypto already allocated?
sed -n '3310,3360p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1306
🏁 Script executed:
# Check Session::setParameters to understand the use-after-free concern
rg -n "void.*setParameters" src/lib/session.hRepository: softhsm/SoftHSMv2
Length of output: 118
🏁 Script executed:
# Find Session class definition
find src -name "*.h" | xargs grep -l "class Session"Repository: softhsm/SoftHSMv2
Length of output: 380
🏁 Script executed:
# Let me check the actual error handling in the parameter packing - if a switch returns CKR_ARGUMENTS_BAD, is asymCrypto cleaned up?
sed -n '3332,3390p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1776
🏁 Script executed:
# Check the Session class for setParameters
rg -n "setParameters" src/lib/session.h src/lib/*.cpp | grep -E "(void|CK_RV).*setParameters"Repository: softhsm/SoftHSMv2
Length of output: 118
🏁 Script executed:
# Search for setParameters in the correct Session.h location
rg -n "setParameters" src/lib/session_mgr/Session.hRepository: softhsm/SoftHSMv2
Length of output: 119
🏁 Script executed:
# Also check for the full implementation
rg -A10 "void.*setParameters" src/lib/session_mgr/Session.h src/lib/session_mgr/Session.cppRepository: softhsm/SoftHSMv2
Length of output: 1292
🏁 Script executed:
# Get the full context to see when asymCrypto is allocated vs when the parameter switches happen
sed -n '3213,3410p' src/lib/SoftHSM.cpp | tail -100Repository: softhsm/SoftHSMv2
Length of output: 2898
🏁 Script executed:
# Check Session::setParameters full implementation for the malloc issue
rg -A15 "void Session::setParameters" src/lib/session_mgr/Session.cppRepository: softhsm/SoftHSMv2
Length of output: 350
🏁 Script executed:
# Verify that MechParamCheckRSAPKCSOAEP does NOT validate hash/MGF matching
sed -n '13402,13430p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 905
🏁 Script executed:
# Final check: Verify wrap/unwrap also have the same issues or use MechParamCheckRSAPKCSOAEP
# First find wrap function
rg -n "AsymWrapKey" src/lib/SoftHSM.cpp | head -3Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Check unwrap for comparison
rg -n "AsymUnwrapKey" src/lib/SoftHSM.cpp | head -3Repository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Search for wrap/unwrap init functions
rg -n "WrapKey|UnwrapKey" src/lib/SoftHSM.cpp | head -10Repository: softhsm/SoftHSMv2
Length of output: 712
🏁 Script executed:
# Also check C_WrapKey and C_UnwrapKey which are the main entry points
rg -n "C_WrapKey\|C_UnwrapKey" src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 43
🏁 Script executed:
# Get context around the wrap OAEP validation call
sed -n '6840,6860p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 678
🏁 Script executed:
# Get context around the unwrap OAEP validation call
sed -n '7510,7530p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 640
🏁 Script executed:
# Confirm the exact sequence: are asymCrypto/privateKey allocated before the parameter packing code?
sed -n '3295,3335p' src/lib/SoftHSM.cppRepository: softhsm/SoftHSMv2
Length of output: 1061
Replace hard-coded SHA-1/MGF1-SHA1 checks with MechParamCheckRSAPKCSOAEP to align decrypt with encrypt/wrap/unwrap, and fix resource leak in parameter packing error paths.
AsymDecryptInit enforces hashAlg == CKM_SHA_1 and mgf == CKG_MGF1_SHA1 (lines 3282–3295), rejecting all other hash/MGF combinations. However, AsymEncryptInit uses MechParamCheckRSAPKCSOAEP and the parameter packing code below (lines 3332–3390) supports SHA-224/256/384/512 and corresponding MGF1 variants via switch statements—these are only reachable after the hard-coded checks pass. This creates an inconsistency: encrypt/wrap/unwrap accept multiple hashes, decrypt rejects them.
Additionally, if either hash/MGF switch in the parameter packing block hits the default case and returns CKR_ARGUMENTS_BAD, asymCrypto and privateKey are already allocated (lines 3301–3326) but not cleaned up, causing a resource leak.
Consider replacing the hard-coded checks with MechParamCheckRSAPKCSOAEP(pMechanism) (as encrypt does) and consolidating the parameter packing logic to eliminate duplication and resource management errors.
1 participant
Refactor RSA for OpenSSL 3.0
Add new SHA hashes and MGF for RSA OAEP encryption/decryption
Add label support for OAEP encryption
Only OpenSSL 3 supported
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.