rsa-oaep-support #821
rsa-oaep-support #821
Conversation
WalkthroughRefactors RSA-OAEP parameter handling to a helper that validates and derives OAEP settings, threads optional param/paramLen through encrypt/decrypt/wrap/unwrap APIs, and adapts RSA backends (Botan/OpenSSL) to consume OAEP parameters. Updates headers, propagates optional args across non-RSA backends, and extends tests to cover multiple OAEP configurations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant HSM as SoftHSM
participant Algo as AsymmetricAlgorithm
participant RSA as RSA Backend (Botan/OpenSSL)
rect rgb(245,245,255)
note over App,HSM: RSA OAEP Encrypt
App->>HSM: C_EncryptInit(mech=CKM_RSA_PKCS_OAEP, pParameter)
HSM->>HSM: MechParamCheckRSAPKCSOAEP(pParameter) -> OAEP params
App->>HSM: C_Encrypt(data)
HSM->>Algo: encrypt(pubKey, data, padding=OAEP, param=&OAEP, len)
Algo->>RSA: encrypt(pubKey, data, OAEP, param, len)
RSA-->>Algo: ciphertext
Algo-->>HSM: ciphertext
HSM-->>App: ciphertext
end
rect rgb(245,255,245)
note over App,HSM: RSA OAEP Decrypt
App->>HSM: C_DecryptInit(mech=CKM_RSA_PKCS_OAEP, pParameter)
HSM->>HSM: MechParamCheckRSAPKCSOAEP(pParameter) -> OAEP params
App->>HSM: C_Decrypt(ciphertext)
HSM->>Algo: decrypt(privKey, ciphertext, OAEP, param=&OAEP, len)
Algo->>RSA: decrypt(privKey, ciphertext, OAEP, param, len)
RSA-->>Algo: plaintext
Algo-->>HSM: plaintext
HSM-->>App: plaintext
end
sequenceDiagram
autonumber
actor App
participant HSM as SoftHSM
participant Algo as AsymmetricAlgorithm
participant RSA as RSA Backend
rect rgb(255,245,245)
note over App,HSM: Wrap / Unwrap with OAEP
App->>HSM: C_WrapKey(mech=CKM_RSA_PKCS_OAEP, pParameter, pubKey, key)
HSM->>HSM: Validate OAEP params
HSM->>Algo: wrapKey(pubKey, keyBlob, OAEP, param, len)
Algo->>RSA: encrypt(..., OAEP, param, len)
RSA-->>Algo: wrapped
Algo-->>HSM: wrapped
HSM-->>App: wrapped
App->>HSM: C_UnwrapKey(mech=CKM_RSA_PKCS_OAEP, pParameter, privKey, wrapped)
HSM->>HSM: Validate OAEP params
HSM->>Algo: unwrapKey(privKey, wrapped, OAEP, param, len)
Algo->>RSA: decrypt(..., OAEP, param, len)
RSA-->>Algo: keyBlob
Algo-->>HSM: keyBlob
HSM-->>App: new key handle
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 4
🧹 Nitpick comments (13)
src/lib/crypto/OSSLRSA.cpp (4)
41-100: Helper mapping OK; consider simplifying digest lookupMinor: you can return EVP_sha{1,224,256,384,512}() directly instead of EVP_get_digestbynid, and share the same switch for both hash and MGF to reduce duplication.
-const EVP_MD* getHashMD(HashAlgo::Type hash) { ... return EVP_get_digestbynid(nid); } -const EVP_MD* getMGFMD(AsymRSAMGF::Type mgf) { ... return EVP_get_digestbynid(nid); } +const EVP_MD* getHashMD(HashAlgo::Type h) { + switch (h) { + case HashAlgo::SHA1: return EVP_sha1(); + case HashAlgo::SHA224: return EVP_sha224(); + case HashAlgo::SHA256: return EVP_sha256(); + case HashAlgo::SHA384: return EVP_sha384(); + case HashAlgo::SHA512: return EVP_sha512(); + default: return NULL; + } +} +const EVP_MD* getMGFMD(AsymRSAMGF::Type m) { + switch (m) { + case AsymRSAMGF::MGF1_SHA1: return EVP_sha1(); + case AsymRSAMGF::MGF1_SHA224: return EVP_sha224(); + case AsymRSAMGF::MGF1_SHA256: return EVP_sha256(); + case AsymRSAMGF::MGF1_SHA384: return EVP_sha384(); + case AsymRSAMGF::MGF1_SHA512: return EVP_sha512(); + default: return NULL; + } +}
1269-1337: Optionally handle OAEP label (or explicitly reject non-empty labels)Current code always passes NULL label to RSA_padding_add_PKCS1_OAEP_mgf1. If label support is out of scope now, explicitly reject non-empty labels to avoid silent mis-encryption; otherwise plumb and pass them.
- if (!RSA_padding_add_PKCS1_OAEP_mgf1(padded.data(), padded.size(), - data.const_byte_str(), data.size(), - NULL, 0, hash, mgfHash)) + const unsigned char* lbl = NULL; + size_t lblLen = 0; + if (oaep && oaep->label && oaep->labelLen > 0) { + lbl = oaep->label; + lblLen = oaep->labelLen; + } + if (!RSA_padding_add_PKCS1_OAEP_mgf1(padded.data(), padded.size(), + data.const_byte_str(), data.size(), + lbl, lblLen, hash, mgfHash)) { ERROR_MSG("RSA OAEP padding failed (0x%08X)", ERR_get_error()); return false; }If you’d rather defer label support, add this instead near parameter parsing:
+ if (oaep && oaep->labelLen > 0) { + ERROR_MSG("OAEP labels are not supported in this backend"); + return false; + }Based on learnings.
1389-1496: Add blinding for private decrypt and clear sensitive buffersRSA private ops should use blinding; also cleanse temporary buffers holding plaintext to reduce residual data risk.
#include <openssl/err.h> +#include <openssl/crypto.h> @@ - std::vector<unsigned char> padded(RSA_size(rsa)); - int decSize = RSA_private_decrypt(encryptedData.size(), - (unsigned char*) encryptedData.const_byte_str(), - padded.data(), rsa, RSA_NO_PADDING); + std::vector<unsigned char> padded(RSA_size(rsa)); + if (!RSA_blinding_on(rsa, NULL)) + { + ERROR_MSG("Failed to turn on blinding for OpenSSL RSA key"); + return false; + } + int decSize = RSA_private_decrypt(encryptedData.size(), + (unsigned char*) encryptedData.const_byte_str(), + padded.data(), rsa, RSA_NO_PADDING); + RSA_blinding_off(rsa); @@ - std::vector<unsigned char> recovered(RSA_size(rsa)); + std::vector<unsigned char> recovered(RSA_size(rsa)); int outLen = RSA_padding_check_PKCS1_OAEP_mgf1(recovered.data(), recovered.size(), padded.data(), decSize, RSA_size(rsa), - NULL, 0, hash, mgfHash); + NULL, 0, hash, mgfHash); @@ - data.resize(outLen); - memcpy(&data[0], recovered.data(), outLen); + data.resize(outLen); + memcpy(&data[0], recovered.data(), outLen); + OPENSSL_cleanse(padded.data(), padded.size()); + OPENSSL_cleanse(recovered.data(), recovered.size());If labels are supported (see encrypt), pass the same label to RSA_padding_check_PKCS1_OAEP_mgf1.
1389-1496: Reject inconsistent hashLen on decrypt tooEven if unused by OpenSSL here, reject hashLen mismatches for symmetry and clearer failures.
- const EVP_MD* hash = getHashMD(hashType); + const EVP_MD* hash = getHashMD(hashType); const EVP_MD* mgfHash = getMGFMD(mgfType); @@ - if (hash == NULL || mgfHash == NULL) + if (hash == NULL || mgfHash == NULL) { ERROR_MSG("Unsupported OAEP parameters for OpenSSL backend"); return false; } + if (param != NULL && oaep->hashLen != (size_t)EVP_MD_size(hash)) + { + ERROR_MSG("OAEP hashLen (%lu) does not match digest size (%d)", + (unsigned long)oaep->hashLen, EVP_MD_size(hash)); + return false; + }src/lib/crypto/AsymmetricAlgorithm.h (2)
148-156: API surface LGTM; add invariants to doc/commentsConsider documenting that: (1) For RSA_PKCS_OAEP, param must point to RSA_PKCS_OAEP_PARAMS; (2) hashLen must equal digest size of hashAlg; (3) label semantics (unsupported vs supported). This will prevent future misuse across providers.
112-117: Keep PSS/OAEP param structs consistent (size_t vs fixed width)You use size_t for sLen/hashLen. That ties ABI to platform word size across modules. If these structs cross shared-library boundaries, prefer uint32_t for length fields (PKCS#11 ulX are CK_ULONG). If strictly internal, ignore.
src/lib/crypto/BotanRSA.cpp (3)
745-819: OAEP param → Botan EME mapping is correct; consider future-proofingCurrent mapping correctly restricts to MGF1(hash) combos Botan supports and returns specific EMEs. Optionally: add SHA‑3 mappings if/when HashAlgo adds them, and improve error messages to print both hash/mgf values received for faster triage.
820-851: Optional: pre-check OAEP input length for consistent errors with OpenSSL pathOpenSSL path validates max input size (nBytes - 2 - 2*hashLen) and emits a clear error. Botan path relies on exceptions and returns a generic error. You could mirror the size check (derive hashLen from params) to align UX across backends.
- case AsymMech::RSA_PKCS_OAEP: - if (!getBotanOAEPParameters(param, paramLen, eme)) - return false; + case AsymMech::RSA_PKCS_OAEP: + if (!getBotanOAEPParameters(param, paramLen, eme)) + return false; + // Optional: size check aligned with OpenSSL + if (param != NULL && paramLen == sizeof(RSA_PKCS_OAEP_PARAMS)) { + const RSA_PKCS_OAEP_PARAMS* oaep = static_cast<const RSA_PKCS_OAEP_PARAMS*>(param); + const size_t hashLen = oaep->hashLen; + const size_t modLen = pk->getN().size(); + if (data.size() > (modLen - 2 - 2*hashLen)) { + ERROR_MSG("Too much data supplied for RSA OAEP encryption"); + return false; + } + } break;
902-933: Confirm Botan EME name on decrypt() and consider RAII
- Same EME string concern as encrypt(); verify for PKCS#1 v1.5.
- Minor: wrap encryptor/decryptor in std::unique_ptr to avoid manual deletes on all paths.
- Botan::PK_Decryptor_EME* decryptor = NULL; + std::unique_ptr<Botan::PK_Decryptor_EME> decryptor; try { BotanRNG* rng = (BotanRNG*)BotanCryptoFactory::i()->getRNG(); - decryptor = new Botan::PK_Decryptor_EME(*botanKey, *rng->getRNG(), eme); + decryptor.reset(new Botan::PK_Decryptor_EME(*botanKey, *rng->getRNG(), eme)); } @@ - delete decryptor; + decryptor.reset();To keep style consistent, apply the same RAII change to encrypt().
src/lib/SoftHSM.cpp (4)
13177-13205: Optional: add SHA‑3 hashes if supportedIf the build/toolchain already supports SHA‑3, extend
getHashProperties()for CKM_SHA3_*.
13206-13229: Optional: add MGF1 with SHA‑3 variantsConsider mapping CKG_MGF1_SHA3_* to internal MGF types (when PKCS#11 v3.0+ is enabled).
13343-13366: Minor: duplicate OAEP validation
C_WrapKey()already callsMechParamCheckRSAPKCSOAEPfor CKM_RSA_PKCS_OAEP; calling it again here is redundant. Consider removing one check.
6471-6487: PublicKey::getOutputLength() available—refactor viable but defer to follow-up
RSAPublicKey::getOutputLength()(and its PublicKey base) is defined and used throughout SoftHSM (e.g., SoftHSM.cpp lines 2628, 5621, 5804; RSAPublicKey.h/CPP) so swapping out CKA_MODULUS_BITS–derived checks forpublicKey->getOutputLength()is safe. Given the minimal scope of this PR, address this optimization in a subsequent follow-up issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
src/lib/SoftHSM.cpp(11 hunks)src/lib/SoftHSM.h(1 hunks)src/lib/crypto/AsymmetricAlgorithm.cpp(1 hunks)src/lib/crypto/AsymmetricAlgorithm.h(2 hunks)src/lib/crypto/BotanDH.cpp(2 hunks)src/lib/crypto/BotanDH.h(1 hunks)src/lib/crypto/BotanDSA.cpp(2 hunks)src/lib/crypto/BotanDSA.h(1 hunks)src/lib/crypto/BotanECDH.cpp(2 hunks)src/lib/crypto/BotanECDH.h(1 hunks)src/lib/crypto/BotanECDSA.cpp(2 hunks)src/lib/crypto/BotanECDSA.h(1 hunks)src/lib/crypto/BotanEDDSA.cpp(2 hunks)src/lib/crypto/BotanEDDSA.h(1 hunks)src/lib/crypto/BotanGOST.cpp(2 hunks)src/lib/crypto/BotanGOST.h(1 hunks)src/lib/crypto/BotanRSA.cpp(2 hunks)src/lib/crypto/BotanRSA.h(1 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/OSSLGOST.cpp(2 hunks)src/lib/crypto/OSSLGOST.h(1 hunks)src/lib/crypto/OSSLRSA.cpp(2 hunks)src/lib/crypto/OSSLRSA.h(1 hunks)src/lib/test/AsymEncryptDecryptTests.cpp(2 hunks)src/lib/test/AsymWrapUnwrapTests.cpp(4 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/OSSLRSA.hsrc/lib/SoftHSM.hsrc/lib/test/AsymWrapUnwrapTests.cppsrc/lib/crypto/AsymmetricAlgorithm.cppsrc/lib/crypto/OSSLGOST.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/BotanRSA.cppsrc/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.h
📚 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.hsrc/lib/SoftHSM.cpp
📚 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.cppsrc/lib/crypto/OSSLRSA.cppsrc/lib/crypto/OSSLEDDSA.cppsrc/lib/crypto/OSSLDSA.h
🧬 Code graph analysis (21)
src/lib/crypto/OSSLRSA.h (2)
src/lib/crypto/BotanRSA.cpp (4)
encrypt(821-899)encrypt(821-823)decrypt(902-990)decrypt(902-904)src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1268-1385)encrypt(1268-1270)decrypt(1388-1497)decrypt(1388-1390)
src/lib/SoftHSM.h (1)
src/lib/SoftHSM.cpp (2)
MechParamCheckRSAPKCSOAEP(13230-13315)MechParamCheckRSAPKCSOAEP(13230-13230)
src/lib/crypto/BotanGOST.h (1)
src/lib/crypto/BotanGOST.cpp (4)
encrypt(328-334)encrypt(328-329)decrypt(337-343)decrypt(337-338)
src/lib/test/AsymWrapUnwrapTests.cpp (3)
src/lib/SoftHSM.cpp (11)
C_WrapKey(6631-6920)C_WrapKey(6631-6639)C_UnwrapKey(7244-7589)C_UnwrapKey(7244-7254)data(2588-2588)data(2748-2748)data(3491-3491)data(3739-3739)data(3786-3786)data(4592-4592)data(5583-5583)src/lib/test/SignVerifyTests.cpp (2)
generateAesKey(883-902)generateAesKey(883-883)src/lib/test/SymmetricAlgorithmTests.cpp (2)
generateAesKey(681-701)generateAesKey(681-681)
src/lib/crypto/BotanEDDSA.h (1)
src/lib/crypto/BotanEDDSA.cpp (4)
encrypt(258-264)encrypt(258-259)decrypt(267-273)decrypt(267-268)
src/lib/crypto/AsymmetricAlgorithm.cpp (1)
src/lib/crypto/BotanRSA.cpp (4)
encrypt(821-899)encrypt(821-823)decrypt(902-990)decrypt(902-904)
src/lib/crypto/OSSLEDDSA.h (2)
src/lib/crypto/BotanRSA.cpp (4)
encrypt(821-899)encrypt(821-823)decrypt(902-990)decrypt(902-904)src/lib/crypto/OSSLEDDSA.cpp (4)
encrypt(210-216)encrypt(210-211)decrypt(219-225)decrypt(219-220)
src/lib/crypto/OSSLECDH.h (1)
src/lib/crypto/OSSLECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)
src/lib/crypto/OSSLGOST.h (1)
src/lib/crypto/OSSLGOST.cpp (4)
encrypt(430-436)encrypt(430-431)decrypt(439-445)decrypt(439-440)
src/lib/crypto/BotanECDSA.h (1)
src/lib/crypto/BotanECDSA.cpp (4)
encrypt(323-329)encrypt(323-324)decrypt(332-338)decrypt(332-333)
src/lib/crypto/BotanECDH.h (2)
src/lib/crypto/BotanDH.cpp (4)
encrypt(95-101)encrypt(95-96)decrypt(104-110)decrypt(104-105)src/lib/crypto/BotanECDH.cpp (4)
encrypt(96-102)encrypt(96-97)decrypt(105-111)decrypt(105-106)
src/lib/crypto/OSSLECDSA.h (1)
src/lib/crypto/OSSLECDSA.cpp (4)
encrypt(333-339)encrypt(333-334)decrypt(342-348)decrypt(342-343)
src/lib/crypto/BotanDSA.h (2)
src/lib/crypto/BotanDSA.cpp (4)
encrypt(487-493)encrypt(487-488)decrypt(496-502)decrypt(496-497)src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-445)encrypt(439-440)decrypt(448-454)decrypt(448-449)
src/lib/crypto/OSSLRSA.cpp (1)
src/lib/crypto/BotanRSA.cpp (2)
decrypt(902-990)decrypt(902-904)
src/lib/crypto/BotanRSA.cpp (2)
src/lib/crypto/BotanDH.cpp (2)
encrypt(95-101)encrypt(95-96)src/lib/crypto/OSSLRSA.cpp (2)
encrypt(1268-1385)encrypt(1268-1270)
src/lib/crypto/BotanRSA.h (2)
src/lib/crypto/BotanRSA.cpp (4)
encrypt(821-899)encrypt(821-823)decrypt(902-990)decrypt(902-904)src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1268-1385)encrypt(1268-1270)decrypt(1388-1497)decrypt(1388-1390)
src/lib/crypto/BotanDH.h (1)
src/lib/crypto/BotanDH.cpp (4)
encrypt(95-101)encrypt(95-96)decrypt(104-110)decrypt(104-105)
src/lib/test/AsymEncryptDecryptTests.cpp (1)
src/lib/SoftHSM.cpp (8)
C_EncryptInit(2529-2535)C_EncryptInit(2529-2529)C_Encrypt(2678-2704)C_Encrypt(2678-2678)C_DecryptInit(3275-3281)C_DecryptInit(3275-3275)C_Decrypt(3419-3444)C_Decrypt(3419-3419)
src/lib/crypto/AsymmetricAlgorithm.h (3)
src/lib/crypto/BotanDH.cpp (4)
encrypt(95-101)encrypt(95-96)decrypt(104-110)decrypt(104-105)src/lib/crypto/OSSLRSA.cpp (4)
encrypt(1268-1385)encrypt(1268-1270)decrypt(1388-1497)decrypt(1388-1390)src/lib/crypto/AsymmetricAlgorithm.cpp (4)
wrapKey(159-165)wrapKey(159-159)unwrapKey(167-173)unwrapKey(167-167)
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 (1)
src/lib/crypto/OSSLDSA.cpp (4)
encrypt(439-445)encrypt(439-440)decrypt(448-454)decrypt(448-449)
🔇 Additional comments (26)
src/lib/crypto/BotanEDDSA.h (1)
62-65: Signature update keeps override alignedThanks for threading the optional mechanism parameters through the EDDSA encrypt/decrypt prototypes; this keeps the override in lock-step with the updated AsymmetricAlgorithm interface without changing behavior.
src/lib/crypto/BotanGOST.cpp (1)
328-339: OAEP plumbing propagated cleanlyExtending the Botan GOST stubs with the new param/paramLen pair keeps the backend conformant with the widened interface while preserving the explicit “not supported” guardrails—looks good.
src/lib/crypto/BotanDH.h (1)
57-60: Interface compliance maintainedAdding the optional parameter block to the DH encrypt/decrypt declarations ensures the class stays ABI-compatible with the updated base class and keeps existing call sites intact via the defaults.
src/lib/crypto/OSSLEDDSA.cpp (1)
210-221: EDDSA overrides now match widened APIThe OpenSSL EDDSA backend now mirrors the new encrypt/decrypt signature shape, so callers can pass mechanism-specific data uniformly while unsupported operations still fail fast—nice and tidy.
src/lib/crypto/OSSLECDH.cpp (1)
96-107: ECDH stubs updated consistentlyGood to see the ECDH encrypt/decrypt placeholders updated with the optional parameter pair, keeping this backend consistent with the broader OAEP plumbing work without altering behavior.
src/lib/crypto/OSSLDSA.h (1)
63-66: Signature update keeps overrides alignedThe added
param/paramLendefaults match the new base-class signature, so OSSLDSA continues to overrideencrypt/decryptcorrectly without altering behavior.src/lib/crypto/BotanGOST.h (1)
59-63: Optional parameters threaded through correctlyPropagating the optional parameter pair keeps this stub in sync with the updated interface while maintaining the existing “unsupported” behavior.
src/lib/crypto/OSSLECDSA.h (1)
59-62: Interface parity maintainedThe signature change mirrors the base class update, ensuring OSSLECDSA still overrides
encrypt/decryptcleanly with no behavior change.src/lib/crypto/OSSLDSA.cpp (1)
439-450: Stub remains accurate after signature changeThe implementation still returns the expected “unsupported” error while picking up the new parameters, keeping linkage with the header update.
src/lib/crypto/OSSLEDDSA.h (1)
59-62: Override stays in lockstep with baseAdding the optional parameters preserves the override and keeps EDDSA behavior unchanged.
src/lib/test/AsymEncryptDecryptTests.cpp (1)
113-129: Great to see OAEP permutations exercisedThe table-driven loop keeps the test succinct while covering the new hash/MGF combinations plus the legacy defaults—nice way to guard the RSA plumbing changes.
src/lib/crypto/AsymmetricAlgorithm.cpp (1)
158-173: Param threading in wrap/unwrap looks correctForwarding param/paramLen to encrypt/decrypt is consistent and scoped by isWrappingMech.
src/lib/crypto/AsymmetricAlgorithm.h (2)
147-156: Verify encrypt/decrypt overrides include new parameters
Manually confirm every derivedencrypt/decryptimplementation across all providers incorporates and forwards the newly addedparamandparamLenarguments.
119-124: Upstream rejects non-empty OAEP labels in MechParamCheckRSAPKCSOAEP
MechParamCheckRSAPKCSOAEP returns CKR_ARGUMENTS_BAD when pSourceData≠NULL or ulSourceDataLen≠0 (SoftHSM.cpp 13296–13305); no change needed.src/lib/crypto/BotanDH.cpp (2)
94-101: API extension on encrypt(): looks goodSignature now accepts param/paramLen while preserving existing behavior. No functional risk.
103-110: API extension on decrypt(): looks goodSame as encrypt(): compatible extension with no behavior change.
src/lib/crypto/BotanRSA.cpp (1)
820-851: No action needed — “PKCS1v15” is the correct EME spec string for PKCS#1 v1.5 in Botan 2.x; “EME-PKCS1-v1_5” is just a deprecated alias.src/lib/SoftHSM.cpp (9)
2654-2658: LGTM: OAEP params threaded into encrypt()Plumbing via
getParameters()looks correct.
3215-3227: Same potential UAF as encrypt pathSame stack-allocated
oaepParamspassed throughsetParameters(); see earlier note.
3391-3395: LGTM: OAEP params threaded into decrypt()Passing
param/paramLentodecrypt()matches encrypt path.
6447-6453: LGTM: OAEP context state for wrappingState vars (
oaepParams,hasOaepParams) are clear.
6517-6525: LGTM: Passing normalized OAEP params to wrapKey()
param/paramLenconstruction aligns with encrypt/decrypt.
7045-7065: LGTM: OAEP unwrap mapping and validationMechanism mapping and delegated check are consistent.
7091-7099: LGTM: OAEP params forwarded to unwrapKey()Forwarding
param/paramLenmatches wrap path.
2474-2487: setParameters safely copies parameters
Session::setParametersallocates heap memory and usesmemcpyto copyinParamcontents, so it does not store the stack pointer and there is no UAF.
13230-13315: Remove MGF–hash equality enforcement and refine OAEP label checks
- Drop the entire
mgfMatchesHashblock: PKCS#11 v3.2 defineshashAlgandmgfas independent parameters and does not require them to match (docs.oasis-open.org).- Replace the over-strict label checks so only non-empty labels are rejected; e.g.:
if (params->pSourceData != NULL) {ERROR_MSG("pSourceData must be NULL");return CKR_ARGUMENTS_BAD;}if (params->ulSourceDataLen != 0) {ERROR_MSG("ulSourceDataLen must be 0");return CKR_ARGUMENTS_BAD;}
// only reject non-empty OAEP labelif (params->pSourceData != NULL_PTR && params->ulSourceDataLen > 0) {ERROR_MSG("OAEP label not supported");return CKR_MECHANISM_PARAM_INVALID;}⛔ 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.
| struct RSA_PKCS_OAEP_PARAMS | ||
| { | ||
| HashAlgo::Type hashAlg; | ||
| AsymRSAMGF::Type mgf; | ||
| size_t hashLen; | ||
| }; |
There was a problem hiding this comment.
OAEP label not represented; risks silent label ignore
PKCS#11 CK_RSA_PKCS_OAEP_PARAMS includes label fields; the current struct lacks them. Either add and plumb label support or ensure upstream rejects non-empty labels to avoid non-compliance and silent behavior differences.
struct RSA_PKCS_OAEP_PARAMS
{
HashAlgo::Type hashAlg;
AsymRSAMGF::Type mgf;
size_t hashLen;
+ // Optional OAEP label (may be NULL when labelLen == 0)
+ const unsigned char* label;
+ size_t labelLen;
};If adding later, document that labelLen must be 0 for now and enforce rejection in backends. Based on learnings.
📝 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.
| struct RSA_PKCS_OAEP_PARAMS | |
| { | |
| HashAlgo::Type hashAlg; | |
| AsymRSAMGF::Type mgf; | |
| size_t hashLen; | |
| }; | |
| struct RSA_PKCS_OAEP_PARAMS | |
| { | |
| HashAlgo::Type hashAlg; | |
| AsymRSAMGF::Type mgf; | |
| size_t hashLen; | |
| // Optional OAEP label (may be NULL when labelLen == 0) | |
| const unsigned char* label; | |
| size_t labelLen; | |
| }; |
| ByteString& encryptedData, const AsymMech::Type padding, | ||
| const void* param /* = NULL */, const size_t paramLen /* = 0 */) | ||
| { | ||
| // Check if the public key is the right type | ||
| if (!publicKey->isOfType(OSSLRSAPublicKey::type)) | ||
| { | ||
| ERROR_MSG("Invalid key type supplied"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Retrieve the OpenSSL key object | ||
| RSA* rsa = ((OSSLRSAPublicKey*) publicKey)->getOSSLKey(); | ||
|
|
||
| // Check the data and padding algorithm | ||
| int osslPadding = 0; | ||
|
|
||
| if (padding == AsymMech::RSA_PKCS) | ||
| { | ||
| // The size of the input data cannot be more than the modulus | ||
| // length of the key - 11 | ||
| if (data.size() > (size_t) (RSA_size(rsa) - 11)) | ||
| { | ||
| ERROR_MSG("Too much data supplied for RSA PKCS #1 encryption"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| osslPadding = RSA_PKCS1_PADDING; | ||
| } | ||
| else if (padding == AsymMech::RSA_PKCS_OAEP) | ||
| { | ||
| // The size of the input data cannot be more than the modulus | ||
| // length of the key - 41 | ||
| if (data.size() > (size_t) (RSA_size(rsa) - 41)) | ||
| { | ||
| ERROR_MSG("Too much data supplied for RSA OAEP encryption"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| osslPadding = RSA_PKCS1_OAEP_PADDING; | ||
| } | ||
| else if (padding == AsymMech::RSA) | ||
| { | ||
| // The size of the input data should be exactly equal to the modulus length | ||
| if (data.size() != (size_t) RSA_size(rsa)) | ||
| { | ||
| ERROR_MSG("Incorrect amount of input data supplied for raw RSA encryption"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| osslPadding = RSA_NO_PADDING; | ||
| } | ||
| else | ||
| { | ||
| ERROR_MSG("Invalid padding mechanism supplied (%i)", padding); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Perform the RSA operation | ||
| encryptedData.resize(RSA_size(rsa)); | ||
|
|
||
| if (RSA_public_encrypt(data.size(), (unsigned char*) data.const_byte_str(), &encryptedData[0], rsa, osslPadding) == -1) | ||
| { | ||
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| // Check if the public key is the right type | ||
| if (!publicKey->isOfType(OSSLRSAPublicKey::type)) | ||
| { | ||
| ERROR_MSG("Invalid key type supplied"); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Retrieve the OpenSSL key object | ||
| RSA* rsa = ((OSSLRSAPublicKey*) publicKey)->getOSSLKey(); | ||
|
|
||
| if (padding == AsymMech::RSA_PKCS_OAEP) | ||
| { | ||
| const RSA_PKCS_OAEP_PARAMS* oaep = NULL; | ||
|
|
||
| if (param != NULL) | ||
| { | ||
| if (paramLen != sizeof(RSA_PKCS_OAEP_PARAMS)) | ||
| { | ||
| ERROR_MSG("Invalid OAEP parameter size"); | ||
| return false; | ||
| } | ||
|
|
||
| oaep = static_cast<const RSA_PKCS_OAEP_PARAMS*>(param); | ||
| } | ||
|
|
||
| HashAlgo::Type hashType = oaep ? oaep->hashAlg : HashAlgo::SHA1; | ||
| AsymRSAMGF::Type mgfType = oaep ? oaep->mgf : AsymRSAMGF::MGF1_SHA1; | ||
|
|
||
| const EVP_MD* hash = getHashMD(hashType); | ||
| const EVP_MD* mgfHash = getMGFMD(mgfType); | ||
|
|
||
| if (hash == NULL || mgfHash == NULL) | ||
| { | ||
| ERROR_MSG("Unsupported OAEP parameters for OpenSSL backend"); | ||
| return false; | ||
| } | ||
|
|
||
| size_t hashLen = oaep ? oaep->hashLen : static_cast<size_t>(EVP_MD_size(hash)); | ||
|
|
||
| if (data.size() > (size_t) (RSA_size(rsa) - 2 - 2 * hashLen)) | ||
| { | ||
| ERROR_MSG("Too much data supplied for RSA OAEP encryption"); | ||
| return false; | ||
| } | ||
|
|
||
| std::vector<unsigned char> padded(RSA_size(rsa)); | ||
|
|
||
| if (!RSA_padding_add_PKCS1_OAEP_mgf1(padded.data(), padded.size(), | ||
| data.const_byte_str(), data.size(), | ||
| NULL, 0, hash, mgfHash)) | ||
| { | ||
| ERROR_MSG("RSA OAEP padding failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } | ||
|
|
||
| encryptedData.resize(RSA_size(rsa)); | ||
|
|
||
| if (RSA_public_encrypt(padded.size(), padded.data(), &encryptedData[0], rsa, RSA_NO_PADDING) == -1) | ||
| { | ||
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Validate hashLen against selected digest; compute bound from EVP_MD_size(hash)
Relying on caller‑supplied hashLen can desync the length check from the actual digest, causing spurious accepts/rejects. Validate or ignore param->hashLen and use EVP_MD_size(hash) for the bound.
- size_t hashLen = oaep ? oaep->hashLen : static_cast<size_t>(EVP_MD_size(hash));
-
- if (data.size() > (size_t) (RSA_size(rsa) - 2 - 2 * hashLen))
+ const size_t hLen = static_cast<size_t>(EVP_MD_size(hash));
+ if (param != NULL && oaep->hashLen != hLen)
+ {
+ ERROR_MSG("OAEP hashLen (%lu) does not match digest size (%lu)",
+ (unsigned long)oaep->hashLen, (unsigned long)hLen);
+ return false;
+ }
+
+ if (data.size() > (size_t) (RSA_size(rsa) - 2 - 2 * hLen))
{
ERROR_MSG("Too much data supplied for RSA OAEP encryption");
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.
| ByteString& encryptedData, const AsymMech::Type padding, | |
| const void* param /* = NULL */, const size_t paramLen /* = 0 */) | |
| { | |
| // Check if the public key is the right type | |
| if (!publicKey->isOfType(OSSLRSAPublicKey::type)) | |
| { | |
| ERROR_MSG("Invalid key type supplied"); | |
| return false; | |
| } | |
| // Retrieve the OpenSSL key object | |
| RSA* rsa = ((OSSLRSAPublicKey*) publicKey)->getOSSLKey(); | |
| // Check the data and padding algorithm | |
| int osslPadding = 0; | |
| if (padding == AsymMech::RSA_PKCS) | |
| { | |
| // The size of the input data cannot be more than the modulus | |
| // length of the key - 11 | |
| if (data.size() > (size_t) (RSA_size(rsa) - 11)) | |
| { | |
| ERROR_MSG("Too much data supplied for RSA PKCS #1 encryption"); | |
| return false; | |
| } | |
| osslPadding = RSA_PKCS1_PADDING; | |
| } | |
| else if (padding == AsymMech::RSA_PKCS_OAEP) | |
| { | |
| // The size of the input data cannot be more than the modulus | |
| // length of the key - 41 | |
| if (data.size() > (size_t) (RSA_size(rsa) - 41)) | |
| { | |
| ERROR_MSG("Too much data supplied for RSA OAEP encryption"); | |
| return false; | |
| } | |
| osslPadding = RSA_PKCS1_OAEP_PADDING; | |
| } | |
| else if (padding == AsymMech::RSA) | |
| { | |
| // The size of the input data should be exactly equal to the modulus length | |
| if (data.size() != (size_t) RSA_size(rsa)) | |
| { | |
| ERROR_MSG("Incorrect amount of input data supplied for raw RSA encryption"); | |
| return false; | |
| } | |
| osslPadding = RSA_NO_PADDING; | |
| } | |
| else | |
| { | |
| ERROR_MSG("Invalid padding mechanism supplied (%i)", padding); | |
| return false; | |
| } | |
| // Perform the RSA operation | |
| encryptedData.resize(RSA_size(rsa)); | |
| if (RSA_public_encrypt(data.size(), (unsigned char*) data.const_byte_str(), &encryptedData[0], rsa, osslPadding) == -1) | |
| { | |
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| return true; | |
| // Check if the public key is the right type | |
| if (!publicKey->isOfType(OSSLRSAPublicKey::type)) | |
| { | |
| ERROR_MSG("Invalid key type supplied"); | |
| return false; | |
| } | |
| // Retrieve the OpenSSL key object | |
| RSA* rsa = ((OSSLRSAPublicKey*) publicKey)->getOSSLKey(); | |
| if (padding == AsymMech::RSA_PKCS_OAEP) | |
| { | |
| const RSA_PKCS_OAEP_PARAMS* oaep = NULL; | |
| if (param != NULL) | |
| { | |
| if (paramLen != sizeof(RSA_PKCS_OAEP_PARAMS)) | |
| { | |
| ERROR_MSG("Invalid OAEP parameter size"); | |
| return false; | |
| } | |
| oaep = static_cast<const RSA_PKCS_OAEP_PARAMS*>(param); | |
| } | |
| HashAlgo::Type hashType = oaep ? oaep->hashAlg : HashAlgo::SHA1; | |
| AsymRSAMGF::Type mgfType = oaep ? oaep->mgf : AsymRSAMGF::MGF1_SHA1; | |
| const EVP_MD* hash = getHashMD(hashType); | |
| const EVP_MD* mgfHash = getMGFMD(mgfType); | |
| if (hash == NULL || mgfHash == NULL) | |
| { | |
| ERROR_MSG("Unsupported OAEP parameters for OpenSSL backend"); | |
| return false; | |
| } | |
| size_t hashLen = oaep ? oaep->hashLen : static_cast<size_t>(EVP_MD_size(hash)); | |
| if (data.size() > (size_t) (RSA_size(rsa) - 2 - 2 * hashLen)) | |
| { | |
| ERROR_MSG("Too much data supplied for RSA OAEP encryption"); | |
| return false; | |
| } | |
| std::vector<unsigned char> padded(RSA_size(rsa)); | |
| if (!RSA_padding_add_PKCS1_OAEP_mgf1(padded.data(), padded.size(), | |
| data.const_byte_str(), data.size(), | |
| NULL, 0, hash, mgfHash)) | |
| { | |
| ERROR_MSG("RSA OAEP padding failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| encryptedData.resize(RSA_size(rsa)); | |
| if (RSA_public_encrypt(padded.size(), padded.data(), &encryptedData[0], rsa, RSA_NO_PADDING) == -1) | |
| { | |
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); | |
| return false; | |
| } | |
| return true; | |
| } | |
| const EVP_MD* hash = getHashMD(hashType); | |
| const EVP_MD* mgfHash = getMGFMD(mgfType); | |
| if (hash == NULL || mgfHash == NULL) | |
| { | |
| ERROR_MSG("Unsupported OAEP parameters for OpenSSL backend"); | |
| return false; | |
| } | |
| const size_t hLen = static_cast<size_t>(EVP_MD_size(hash)); | |
| if (param != NULL && oaep->hashLen != hLen) | |
| { | |
| ERROR_MSG("OAEP hashLen (%lu) does not match digest size (%lu)", | |
| (unsigned long)oaep->hashLen, (unsigned long)hLen); | |
| return false; | |
| } | |
| if (data.size() > (size_t)(RSA_size(rsa) - 2 - 2 * hLen)) | |
| { | |
| ERROR_MSG("Too much data supplied for RSA OAEP encryption"); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1269 to 1337, the code uses
oaep->hashLen (caller-supplied) to bound OAEP payload size which can diverge
from the actual digest size; replace that behavior by obtaining the digest
length from EVP_MD_size(hash) and use that as the authoritative hashLen, and if
an OAEP param was provided validate that param->hashLen matches
EVP_MD_size(hash) (or reject the params) before using it; specifically set
size_t hashLen = static_cast<size_t>(EVP_MD_size(hash)); if oaep != NULL then
check if oaep->hashLen == hashLen (log/error and return false on mismatch)
otherwise ignore oaep->hashLen and proceed.
| ); | ||
|
|
||
| CK_RV MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism); | ||
| CK_RV MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism, RSA_PKCS_OAEP_PARAMS* outParams = NULL); |
There was a problem hiding this comment.
Restore missing declaration for RSA_PKCS_OAEP_PARAMS
SoftHSM.h now uses RSA_PKCS_OAEP_PARAMS in the public signature, but the header neither includes the definition nor forward declares the struct. Compiling a TU that includes SoftHSM.h before any other OAEP headers currently fails with “RSA_PKCS_OAEP_PARAMS has not been declared.” Please add a declaration (or the defining include) in this header so it remains self-contained.
+#include "AsymmetricAlgorithm.h"📝 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.
| CK_RV MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism, RSA_PKCS_OAEP_PARAMS* outParams = NULL); | |
| #include "AsymmetricAlgorithm.h" | |
| CK_RV MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism, RSA_PKCS_OAEP_PARAMS* outParams = NULL); |
🤖 Prompt for AI Agents
In src/lib/SoftHSM.h around line 510 the signature MechParamCheckRSAPKCSOAEP
references RSA_PKCS_OAEP_PARAMS but the header neither forward-declares nor
includes its definition; fix by either adding a forward declaration (e.g., a
typedef struct RSA_PKCS_OAEP_PARAMS RSA_PKCS_OAEP_PARAMS;) near the top of
SoftHSM.h or by including the header that defines RSA_PKCS_OAEP_PARAMS (for
example the appropriate pkcs11 header used in this project), and ensure the
include is guarded and placed before the function prototype so the file becomes
self-contained.
| std::vector<CK_RSA_PKCS_OAEP_PARAMS> paramsList = { | ||
| { CKM_SHA_1, CKG_MGF1_SHA1, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { CKM_SHA224, CKG_MGF1_SHA224, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { CKM_SHA256, CKG_MGF1_SHA256, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { CKM_SHA384, CKG_MGF1_SHA384, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { CKM_SHA512, CKG_MGF1_SHA512, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { CKM_SHA256, 0, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }, | ||
| { 0, 0, CKZ_DATA_SPECIFIED, NULL_PTR, 0 } | ||
| }; |
There was a problem hiding this comment.
Invalid OAEP “default” values violate PKCS#11
Both parameter lists include entries where hashAlg or mgf is set to 0, and the tests assert CKR_OK. PKCS#11 v3.1 (see §2.40.8 and Table 427) requires hashAlg to name a valid hash mechanism (e.g. CKM_SHA_*) and mgf to be one of the defined CKG_MGF1_* constants; 0 is not a legal value in either field. By hard-coding these vectors and expecting success, we force the library to accept spec-incompliant OAEP parameters and mask bugs the new validator should flag. Please drop the zero-valued cases (or expect the appropriate failure code) so the test suite keeps us aligned with the standard.
You can remove the offending combinations like so:
- { CKM_SHA256, 0, CKZ_DATA_SPECIFIED, NULL_PTR, 0 },
- { 0, 0, CKZ_DATA_SPECIFIED, NULL_PTR, 0 }Also applies to: 291-298
🤖 Prompt for AI Agents
In src/lib/test/AsymWrapUnwrapTests.cpp around lines 153 to 161 (and similarly
for 291-298), the OAEP paramsList contains entries with hashAlg or mgf set to 0
which violates PKCS#11; remove the zero-valued combinations from the hard-coded
vectors (or alternatively change those cases to assert the appropriate failure
code instead of CKR_OK) so that only legally defined CKM_SHA_* and CKG_MGF1_*
values remain and the test asserts success only for valid parameter
combinations.
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests