implement OAESP params for asymmetric encryption #820
+337
−84
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds OAEP parameter support end-to-end: SoftHSM now parses/validates/stores CK_RSA_PKCS_OAEP_PARAMS and propagates them through session/mechanism state; asymmetric interfaces gained optional OAEP params; OSSLRSA implements EVP-based OAEP (configurable hash/MGF/label); other algorithm classes updated signatures only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant PK11 as SoftHSM (PKCS#11)
participant Sess as Session State
participant RSA as OSSLRSA (EVP)
App->>PK11: C_EncryptInit(mech=CKM_RSA_PKCS_OAEP, pParameter)
PK11->>PK11: Parse CK_RSA_PKCS_OAEP_PARAMS (hash, mgf, label)
PK11->>Sess: Store mechanism + oaepParams
App->>PK11: C_Encrypt(data)
PK11->>Sess: Load stored oaepParams
PK11->>RSA: encrypt(pubKey, data, padding=OAEP, oaepParams)
RSA->>RSA: Configure EVP_PKEY_CTX (OAEP hash, MGF1 digest, label)
RSA-->>PK11: encryptedData / error
PK11-->>App: Return encryptedData or CKR_*
rect rgba(220,245,235,0.6)
note right of PK11: C_DecryptInit / C_Decrypt mirror Encrypt flow, passing oaepParams to OSSLRSA
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
🧹 Nitpick comments (13)
src/lib/crypto/OSSLECDSA.cpp (1)
334-338: Silence the unusedoaepParamsparameter
oaepParamsis never touched (these code paths return immediately), so this now trips-Wunused-parameterwarnings. The neighboring parameters already use the/*…*/convention to avoid that — please apply the same treatment here to keep warning-free builds.- ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/) @@ - ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)Also applies to: 343-347
src/lib/crypto/OSSLDH.cpp (1)
94-99: Silence unusedoaepParamsin DH stubsThese DH encrypt/decrypt stubs still short-circuit, so the freshly added
oaepParamsis unused and now raises-Wunused-parameter. Please treat it like the other ignored arguments to keep the build clean.- ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/) @@ - ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)Also applies to: 103-108
src/lib/crypto/OSSLDH.h (1)
57-61: OK to propagate OAEP param; ensure impl marks it unused to avoid warningsDH doesn’t support enc/dec; the extra param is fine for interface parity. In the .cpp, comment out the parameter name to silence -Wunused-parameter.
Example in src/lib/crypto/OSSLDH.cpp:
// Before bool OSSLDH::encrypt(PublicKey* /*publicKey*/, const ByteString& /*data*/, ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) // After bool OSSLDH::encrypt(PublicKey* /*publicKey*/, const ByteString& /*data*/, ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)src/lib/crypto/OSSLECDH.cpp (2)
97-101: Silence unused parameter warning (oaepParams)ECDH doesn’t support encryption; comment out the parameter name to match the existing style for other unused params.
Apply this diff:
- ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)
106-111: Silence unused parameter warning (oaepParams)Same for decrypt.
Apply this diff:
- ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)src/lib/crypto/OSSLDSA.cpp (2)
440-445: Silence unused parameter warning (oaepParams)DSA doesn’t support encryption; comment out the param name to avoid -Wunused-parameter.
Apply this diff:
- ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*encryptedData*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)
449-454: Silence unused parameter warning (oaepParams)Same for decrypt.
Apply this diff:
- ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* oaepParams) + ByteString& /*data*/, const AsymMech::Type /*padding*/, const CK_RSA_PKCS_OAEP_PARAMS* /*oaepParams*/)src/lib/crypto/AsymmetricAlgorithm.h (1)
142-147: Minor: prefer consistent NULL/nullptr usageProject mostly uses NULL; new defaults use nullptr. Consistency is cosmetic—ok to defer.
src/lib/SoftHSM.cpp (3)
116-132: Good addition: central OAEP hashLen mapping; consider default fallback(0→SHA‑1).Switch looks correct for SHA‑1/224/256/384/512. For broader interoperability (per issue #795), consider treating a “zero/unspecified” hashAlg as SHA‑1 instead of rejecting it.
Apply this diff to add a compatibility fallback:
static CK_ULONG getOAEPHashLength(CK_MECHANISM_TYPE hashAlg) { switch (hashAlg) { + case 0: + // Some HSMs treat 0 as "default". Fallback to SHA‑1 for compatibility. + return 20; case CKM_SHA_1: return 20; // 160 bits / 8 case CKM_SHA224: return 28; // 224 bits / 8 case CKM_SHA256: return 32; // 256 bits / 8 case CKM_SHA384: return 48; // 384 bits / 8 case CKM_SHA512: return 64; // 512 bits / 8 default: return 0; // Unsupported } }Based on learnings
6511-6526: Correct OAEP input bound; consider minor robustness.k−2−2·hLen bound is correct. Two small follow‑ups:
- If you ever revisit this block, consider rounding modulus bits to bytes as (bits+7)/8.
- Optional: if params->hashAlg is 0, default hashLen to SHA‑1 for compatibility (see earlier comment).
13221-13242: Allowing multiple digests/MGFs is good; decide on mgf==hash enforcement and default(0).
- Many stacks expect OAEP MGF1 hash to match hashAlg; others allow mismatch. If backends require equality, enforce it here.
- For compatibility with devices that pass 0 to mean “default”, consider normalizing 0→SHA‑1 (and mgf 0→CKG_MGF1_SHA1) rather than rejecting.
Apply this diff if you choose to normalize defaults:
CK_RSA_PKCS_OAEP_PARAMS_PTR params = (CK_RSA_PKCS_OAEP_PARAMS_PTR)pMechanism->pParameter; -switch (params->hashAlg) { +// Normalize "unspecified" to SHA-1 for compatibility +if (params->hashAlg == 0) { + params->hashAlg = CKM_SHA_1; +} +switch (params->hashAlg) { case CKM_SHA_1: case CKM_SHA256: case CKM_SHA224: case CKM_SHA384: case CKM_SHA512: break; // Valid hash algorithms default: ERROR_MSG("hashAlg must be CKM_SHA_1, CKM_SHA256, CKM_SHA224, CKM_SHA384, or CKM_SHA512"); return CKR_ARGUMENTS_BAD; } -switch (params->mgf) { +// Normalize "unspecified" to MGF1-SHA1 for compatibility +if (params->mgf == 0) { + params->mgf = CKG_MGF1_SHA1; +} +switch (params->mgf) { case CKG_MGF1_SHA1: case CKG_MGF1_SHA224: case CKG_MGF1_SHA256: case CKG_MGF1_SHA384: case CKG_MGF1_SHA512: break; // Valid MGF types default: ERROR_MSG("mgf must be CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, or CKG_MGF1_SHA512"); return CKR_ARGUMENTS_BAD; } +// Optional: enforce mgf/hash pairing if required by backends +// e.g., map expected MGF for given hash, and reject mismatches.If enforcing mgf==hash is needed, confirm with maintainers/backends first. Based on learnings
src/lib/crypto/OSSLRSA.cpp (2)
1353-1363: Resize to the actual ciphertext length returned by RSA_public_encrypt.Even though RSA_public_encrypt returns RSA_size(rsa), capturing the return and resizing improves consistency and future-proofs code.
Apply this diff:
- encryptedData.resize(RSA_size(rsa)); - - if (RSA_public_encrypt(data.size(), (unsigned char*) data.const_byte_str(), &encryptedData[0], rsa, osslPadding) == -1) + encryptedData.resize(RSA_size(rsa)); + int encLen = RSA_public_encrypt(data.size(), (unsigned char*) data.const_byte_str(), &encryptedData[0], rsa, osslPadding); + if (encLen == -1) { ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); return false; } + encryptedData.resize(encLen);
1284-1296: Optional: log when falling back to SHA‑1 defaults.Given Issue #795 mentions fallback behavior, consider logging a DEBUG/WARNING when hashAlg/mgf are unrecognized or zero so operators can diagnose unexpected defaults.
Also applies to: 1439-1449
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/lib/SoftHSM.cpp(8 hunks)src/lib/crypto/AsymmetricAlgorithm.h(2 hunks)src/lib/crypto/OSSLDH.cpp(2 hunks)src/lib/crypto/OSSLDH.h(1 hunks)src/lib/crypto/OSSLDSA.cpp(2 hunks)src/lib/crypto/OSSLDSA.h(1 hunks)src/lib/crypto/OSSLECDH.cpp(2 hunks)src/lib/crypto/OSSLECDH.h(1 hunks)src/lib/crypto/OSSLECDSA.cpp(2 hunks)src/lib/crypto/OSSLECDSA.h(1 hunks)src/lib/crypto/OSSLEDDSA.cpp(2 hunks)src/lib/crypto/OSSLEDDSA.h(1 hunks)src/lib/crypto/OSSLRSA.cpp(5 hunks)src/lib/crypto/OSSLRSA.h(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#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.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#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/OSSLEDDSA.hsrc/lib/crypto/OSSLECDSA.hsrc/lib/crypto/OSSLECDSA.cppsrc/lib/crypto/OSSLDH.hsrc/lib/crypto/OSSLDSA.hsrc/lib/crypto/OSSLECDH.cppsrc/lib/crypto/OSSLRSA.hsrc/lib/crypto/OSSLDH.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLDSA.cppsrc/lib/crypto/OSSLECDH.hsrc/lib/crypto/AsymmetricAlgorithm.h
📚 Learning: 2025-08-19T09:23:25.434Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLDSA.hsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLDSA.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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.cpp
🧬 Code graph analysis (8)
src/lib/crypto/OSSLEDDSA.h (1)
src/lib/crypto/OSSLEDDSA.cpp (4)
encrypt(210-216)encrypt(210-211)decrypt(219-225)decrypt(219-220)
src/lib/crypto/OSSLECDSA.h (2)
src/lib/crypto/OSSLECDSA.cpp (4)
encrypt(333-339)encrypt(333-334)decrypt(342-348)decrypt(342-343)src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1226-1366)encrypt(1226-1227)decrypt(1369-1500)decrypt(1369-1370)
src/lib/crypto/OSSLDH.h (1)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-99)encrypt(93-94)decrypt(102-108)decrypt(102-103)
src/lib/crypto/OSSLDSA.h (6)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-99)encrypt(93-94)decrypt(102-108)decrypt(102-103)src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-445)encrypt(439-440)decrypt(448-454)decrypt(448-449)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)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/OSSLRSA.cpp (4)
encrypt(1226-1366)encrypt(1226-1227)decrypt(1369-1500)decrypt(1369-1370)
src/lib/crypto/OSSLRSA.h (6)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-99)encrypt(93-94)decrypt(102-108)decrypt(102-103)src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-445)encrypt(439-440)decrypt(448-454)decrypt(448-449)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)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/OSSLRSA.cpp (4)
encrypt(1226-1366)encrypt(1226-1227)decrypt(1369-1500)decrypt(1369-1370)
src/lib/crypto/OSSLRSA.cpp (4)
src/lib/crypto/OSSLDH.cpp (2)
decrypt(102-108)decrypt(102-103)src/lib/crypto/OSSLDSA.cpp (2)
decrypt(448-454)decrypt(448-449)src/lib/crypto/OSSLECDH.cpp (2)
decrypt(105-111)decrypt(105-106)src/lib/crypto/OSSLECDSA.cpp (2)
decrypt(342-348)decrypt(342-343)
src/lib/crypto/OSSLECDH.h (2)
src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1226-1366)encrypt(1226-1227)decrypt(1369-1500)decrypt(1369-1370)
src/lib/crypto/AsymmetricAlgorithm.h (6)
src/lib/crypto/OSSLDH.cpp (4)
encrypt(93-99)encrypt(93-94)decrypt(102-108)decrypt(102-103)src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-445)encrypt(439-440)decrypt(448-454)decrypt(448-449)src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)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/OSSLRSA.cpp (4)
encrypt(1226-1366)encrypt(1226-1227)decrypt(1369-1500)decrypt(1369-1370)
🔇 Additional comments (9)
src/lib/crypto/AsymmetricAlgorithm.h (2)
44-45: Including pkcs11.h in a public header is acceptable hereNeeded for CK_RSA_PKCS_OAEP_PARAMS. No concerns.
142-147: OAEP param propagation in wrap/unwrap flows is out of scope
This PR only syncs headers and renames parameters; implement CK_RSA_PKCS_OAEP_PARAMS support for C_WrapKey/C_UnwrapKey in a follow-up issue.Likely an incorrect or invalid review comment.
src/lib/crypto/OSSLRSA.h (1)
63-67: Critical: MGF mapping bug in OAEP path (OSSLRSA.cpp)The OAEP MGF mapping shown in OSSLRSA.cpp uses numeric comparisons (1..5) and maps 3→SHA224 and 5→SHA512, which is incorrect per PKCS#11 (CKG_MGF1_SHA1=1, SHA256=2, SHA384=3, SHA512=4, SHA224=5). This will select the wrong MGF digest for SHA384 and SHA224, causing decryption failures and interop issues.
Fix suggestion (use the PKCS#11 enum constants directly and avoid ad‑hoc 0x… CKM values):
/* Replace the current mgfMD assignment with: */ const EVP_MD* mgfMD = EVP_sha1(); if (oaepParams != nullptr) { switch (oaepParams->mgf) { case CKG_MGF1_SHA1: mgfMD = EVP_sha1(); break; case CKG_MGF1_SHA224: mgfMD = EVP_sha224(); break; case CKG_MGF1_SHA256: mgfMD = EVP_sha256(); break; case CKG_MGF1_SHA384: mgfMD = EVP_sha384(); break; case CKG_MGF1_SHA512: mgfMD = EVP_sha512(); break; default: mgfMD = EVP_sha1(); break; } }Optional but recommended: also set the OAEP label when provided:
if (oaepParams && oaepParams->source == CKZ_DATA_SPECIFIED && oaepParams->pSourceData && oaepParams->ulSourceDataLen > 0) { unsigned char* label = (unsigned char*)OPENSSL_malloc(oaepParams->ulSourceDataLen); if (!label) { /* handle alloc error */ } memcpy(label, oaepParams->pSourceData, oaepParams->ulSourceDataLen); if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, label, oaepParams->ulSourceDataLen) <= 0) { OPENSSL_free(label); /* on error, since set0 wasn’t successful */ /* handle error */ } }⛔ Skipped due to learnings
Learnt from: bukka PR: softhsm/SoftHSMv2#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 PR: softhsm/SoftHSMv2#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.src/lib/SoftHSM.cpp (5)
3247-3250: Param validation for RSA‑OAEP on decrypt init is appropriate.MechParamCheckRSAPKCSOAEP here avoids misuse early. LGTM.
2670-2685: Approve OAEP parameter propagation
All AsymmetricAlgorithm::encrypt/decrypt overrides accept the new CK_RSA_PKCS_OAEP_PARAMS* signature without requiring a length parameter.
3419-3433: OAEP parameters are properly propagated and supported by all decrypt backends
2535-2535: No action needed:Session::setParametersdeep-copies the buffer viamalloc+memcpy, andgetParametersreturns the internal copy.
3293-3293: Deep copy confirmed inSession::setParametersmalloc + memcpy ensures parameter data is owned by the session, so passingpMechanism->pParameteris safe.src/lib/crypto/OSSLRSA.cpp (1)
1287-1310: Fix MGF mapping (3/4/5 are wrong) and add OAEP label support; also unify NULL usage.
- Current mapping treats mgf=3/4/5 as SHA224/384/512, but per PKCS#11 it must be 3→SHA384, 4→SHA512, 5→SHA224.
- OAEP label (source/pSourceData) is ignored; this breaks interop when callers set a label.
- Prefer consistent NULL over nullptr in this file.
Apply these diffs:
Correct mgf mapping and NULL:
- if (oaepParams != nullptr) { + if (oaepParams != NULL) { hashMD = getEVPMDFromCKM(oaepParams->hashAlg); - mgfMD = getEVPMDFromCKM(oaepParams->mgf == 1 ? 0x220 : // CKG_MGF1_SHA1 -> CKM_SHA_1 - oaepParams->mgf == 2 ? 0x250 : // CKG_MGF1_SHA256 -> CKM_SHA256 - oaepParams->mgf == 3 ? 0x240 : // CKG_MGF1_SHA224 -> CKM_SHA224 - oaepParams->mgf == 4 ? 0x260 : // CKG_MGF1_SHA384 -> CKM_SHA384 - oaepParams->mgf == 5 ? 0x270 : // CKG_MGF1_SHA512 -> CKM_SHA512 - 0x220); // Default to SHA-1 + switch (oaepParams->mgf) { + case CKG_MGF1_SHA1: mgfMD = EVP_sha1(); break; + case CKG_MGF1_SHA224: mgfMD = EVP_sha224(); break; + case CKG_MGF1_SHA256: mgfMD = EVP_sha256(); break; + case CKG_MGF1_SHA384: mgfMD = EVP_sha384(); break; + case CKG_MGF1_SHA512: mgfMD = EVP_sha512(); break; + default: /* keep default SHA-1 */ break; + } }Add OAEP label support:
if (EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hashMD) <= 0) { ERROR_MSG("Failed to set OAEP hash algorithm"); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; } if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgfMD) <= 0) { ERROR_MSG("Failed to set MGF1 hash algorithm"); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; } + + // Optional: set OAEP label if provided + if (oaepParams != NULL && + oaepParams->source == CKZ_DATA_SPECIFIED && + oaepParams->pSourceData != NULL && + oaepParams->ulSourceDataLen > 0) { + unsigned char* label = (unsigned char*) OPENSSL_malloc(oaepParams->ulSourceDataLen); + if (label == NULL) { + ERROR_MSG("Failed to allocate memory for OAEP label"); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); + return false; + } + memcpy(label, oaepParams->pSourceData, oaepParams->ulSourceDataLen); + if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, label, (int)oaepParams->ulSourceDataLen) <= 0) { + ERROR_MSG("Failed to set OAEP label"); + OPENSSL_free(label); // only free on failure; ownership transfers on success + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); + return false; + } + }⛔ Skipped due to learnings
Learnt from: bukka PR: softhsm/SoftHSMv2#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 PR: softhsm/SoftHSMv2#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.Learnt from: antoinelochet PR: softhsm/SoftHSMv2#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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/crypto/OSSLEDDSA.cpp(2 hunks)src/lib/crypto/OSSLRSA.cpp(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib/crypto/OSSLEDDSA.cpp
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#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
PR: softhsm/SoftHSMv2#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.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#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-08-18T17:59:30.693Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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/OSSLRSA.cpp
📚 Learning: 2025-08-18T19:46:46.659Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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-18T17:54:37.645Z
Learnt from: antoinelochet
PR: softhsm/SoftHSMv2#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
🧬 Code graph analysis (1)
src/lib/crypto/OSSLRSA.cpp (5)
src/lib/crypto/OSSLEDDSA.cpp (2)
decrypt(219-225)decrypt(219-220)src/lib/crypto/OSSLDH.cpp (2)
decrypt(102-108)decrypt(102-103)src/lib/crypto/OSSLECDH.cpp (2)
decrypt(105-111)decrypt(105-106)src/lib/crypto/OSSLDSA.cpp (2)
decrypt(448-454)decrypt(448-449)src/lib/crypto/OSSLECDSA.cpp (2)
decrypt(342-348)decrypt(342-343)
Comment on lines
+1285
to
+1335
| // Get hash algorithm from OAEP parameters | ||
| const EVP_MD* hashMD = EVP_sha1(); // Default to SHA-1 | ||
| const EVP_MD* mgfMD = EVP_sha1(); // Default to SHA-1 | ||
|
|
||
| if (oaepParams != NULL) { | ||
| hashMD = getEVPMDFromCKM(oaepParams->hashAlg); | ||
| switch (oaepParams->mgf) { | ||
| case CKG_MGF1_SHA1: mgfMD = EVP_sha1(); break; | ||
| case CKG_MGF1_SHA224: mgfMD = EVP_sha224(); break; | ||
| case CKG_MGF1_SHA256: mgfMD = EVP_sha256(); break; | ||
| case CKG_MGF1_SHA384: mgfMD = EVP_sha384(); break; | ||
| case CKG_MGF1_SHA512: mgfMD = EVP_sha512(); break; | ||
| default: /* keep default SHA-1 */ break; | ||
| } | ||
|
|
||
| osslPadding = RSA_PKCS1_OAEP_PADDING; | ||
| } | ||
|
|
||
| if (EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hashMD) <= 0) { | ||
| ERROR_MSG("Failed to set OAEP hash algorithm"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
| return false; | ||
| } | ||
|
|
||
| if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgfMD) <= 0) { | ||
| ERROR_MSG("Failed to set MGF1 hash algorithm"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
| return false; | ||
| } | ||
|
|
||
| // Perform encryption | ||
| size_t outlen; | ||
| if (EVP_PKEY_encrypt(ctx, NULL, &outlen, data.const_byte_str(), data.size()) <= 0) { | ||
| ERROR_MSG("Failed to determine output length for OAEP encryption"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
| return false; | ||
| } | ||
|
|
||
| encryptedData.resize(outlen); | ||
| int encryptResult = EVP_PKEY_encrypt(ctx, &encryptedData[0], &outlen, data.const_byte_str(), data.size()); | ||
|
|
||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
|
|
||
| if (encryptResult <= 0) { | ||
| ERROR_MSG("RSA OAEP encryption failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } | ||
|
|
||
| encryptedData.resize(outlen); |
There was a problem hiding this comment.
OAEP label ignored during encryption
We honor CK_RSA_PKCS_OAEP_PARAMS labels on the decrypt path but never set them during encryption, so any caller that supplies source == CKZ_DATA_SPECIFIED with non-empty pSourceData gets ciphertext encoded with an empty label. That breaks interoperability as the decrypt side (including our own) will refuse to unwrap the ciphertext because the label no longer matches. Please mirror the label-handling logic you added to the decrypt path here before calling EVP_PKEY_encrypt.
if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgfMD) <= 0) {
ERROR_MSG("Failed to set MGF1 hash algorithm");
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(pkey);
return false;
}
+
+ if (oaepParams != NULL &&
+ oaepParams->source == CKZ_DATA_SPECIFIED &&
+ oaepParams->pSourceData != NULL &&
+ oaepParams->ulSourceDataLen > 0) {
+ unsigned char* label = (unsigned char*) OPENSSL_malloc(oaepParams->ulSourceDataLen);
+ if (label == NULL) {
+ ERROR_MSG("Failed to allocate memory for OAEP label");
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ return false;
+ }
+ memcpy(label, oaepParams->pSourceData, oaepParams->ulSourceDataLen);
+ if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, label, (int)oaepParams->ulSourceDataLen) <= 0) {
+ ERROR_MSG("Failed to set OAEP label");
+ OPENSSL_free(label);
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ 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.
Suggested change
| // Get hash algorithm from OAEP parameters | |
| const EVP_MD* hashMD = EVP_sha1(); // Default to SHA-1 | |
| const EVP_MD* mgfMD = EVP_sha1(); // Default to SHA-1 | |
| if (oaepParams != NULL) { | |
| hashMD = getEVPMDFromCKM(oaepParams->hashAlg); | |
| switch (oaepParams->mgf) { | |
| case CKG_MGF1_SHA1: mgfMD = EVP_sha1(); break; | |
| case CKG_MGF1_SHA224: mgfMD = EVP_sha224(); break; | |
| case CKG_MGF1_SHA256: mgfMD = EVP_sha256(); break; | |
| case CKG_MGF1_SHA384: mgfMD = EVP_sha384(); break; | |
| case CKG_MGF1_SHA512: mgfMD = EVP_sha512(); break; | |
| default: /* keep default SHA-1 */ break; | |
| } | |
| osslPadding = RSA_PKCS1_OAEP_PADDING; | |
| } | |
| if (EVP_PKEY_CTX_set_rsa_oaep_md(ctx, hashMD) <= 0) { | |
| ERROR_MSG("Failed to set OAEP hash algorithm"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgfMD) <= 0) { | |
| ERROR_MSG("Failed to set MGF1 hash algorithm"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| // Perform encryption | |
| size_t outlen; | |
| if (EVP_PKEY_encrypt(ctx, NULL, &outlen, data.const_byte_str(), data.size()) <= 0) { | |
| ERROR_MSG("Failed to determine output length for OAEP encryption"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| encryptedData.resize(outlen); | |
| int encryptResult = EVP_PKEY_encrypt(ctx, &encryptedData[0], &outlen, data.const_byte_str(), data.size()); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| if (encryptResult <= 0) { | |
| ERROR_MSG("RSA OAEP encryption failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| encryptedData.resize(outlen); | |
| if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, mgfMD) <= 0) { | |
| ERROR_MSG("Failed to set MGF1 hash algorithm"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| // Mirror decrypt-side OAEP label handling for encryption | |
| if (oaepParams != NULL && | |
| oaepParams->source == CKZ_DATA_SPECIFIED && | |
| oaepParams->pSourceData != NULL && | |
| oaepParams->ulSourceDataLen > 0) { | |
| unsigned char* label = (unsigned char*) OPENSSL_malloc(oaepParams->ulSourceDataLen); | |
| if (label == NULL) { | |
| ERROR_MSG("Failed to allocate memory for OAEP label"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| memcpy(label, oaepParams->pSourceData, oaepParams->ulSourceDataLen); | |
| if (EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, label, (int)oaepParams->ulSourceDataLen) <= 0) { | |
| ERROR_MSG("Failed to set OAEP label"); | |
| OPENSSL_free(label); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| } | |
| // Perform encryption | |
| size_t outlen; | |
| if (EVP_PKEY_encrypt(ctx, NULL, &outlen, data.const_byte_str(), data.size()) <= 0) { | |
| ERROR_MSG("Failed to determine output length for OAEP encryption"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1285-1335, the OAEP label from
CK_RSA_PKCS_OAEP_PARAMS is not applied during encryption, so callers that set
source==CKZ_DATA_SPECIFIED produce ciphertext with an empty label; mirror the
decrypt-side handling by, when oaepParams != NULL and oaepParams->source ==
CKZ_DATA_SPECIFIED and oaepParams->pSourceData != NULL, set the OAEP label on
the EVP_PKEY_CTX before calling EVP_PKEY_encrypt. Do this by calling
EVP_PKEY_CTX_set0_rsa_oaep_label(ctx, oaepParams->pSourceData,
(int)oaepParams->ulSourceData) if available, and fall back to
EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_RSA, -1, EVP_PKEY_CTRL_RSA_OAEP_LABEL,
(int)oaepParams->ulSourceData, (void*)oaepParams->pSourceData) when set0 is not
present; handle errors from that call like the other EVP_PKEY_CTX_* calls and
free ctx/pkey on failure.
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #795
Summary by CodeRabbit
New Features
Bug Fixes
Refactor