Support different OAEP modes #818
Conversation
WalkthroughAdds explicit RSA OAEP mechanisms for SHA1/224/256/384/512 across API, validation, backends, and tests; enforces MGF1/hash correspondence and padding-aware length checks; refactors OpenSSL RSA to use EVP_PKEY encrypt/decrypt; tweaks OpenSSL detection macro and README OpenSSL minimum version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant SoftHSM
participant MechCheck as OAEP_Param_Check
participant Backend as Crypto_Backend
participant OpenSSL as OpenSSL_EVP
participant Botan as Botan
App->>SoftHSM: AsymEncryptInit(mech=CKM_RSA_PKCS_OAEP_ SHAx, params)
SoftHSM->>MechCheck: validate(hashAlg, mgf, label)
MechCheck-->>SoftHSM: OK / CKR_ARGUMENTS_BAD
alt valid
SoftHSM->>Backend: Encrypt(mech=RSA_PKCS_OAEP_SHAx)
alt OpenSSL provider
Backend->>OpenSSL: EVP_PKEY_encrypt(ctx, in)
OpenSSL-->>Backend: ciphertext
else Botan provider
Backend->>Botan: RSA/OAEP(hash) encrypt
Botan-->>Backend: ciphertext
end
Backend-->>SoftHSM: ciphertext
SoftHSM-->>App: CKR_OK + ciphertext
else invalid
SoftHSM-->>App: CKR_ARGUMENTS_BAD
end
sequenceDiagram
autonumber
participant App
participant OSSLRSA as OSSLRSA.cpp
participant EVP as EVP_PKEY/CTX
App->>OSSLRSA: Decrypt(padding=PKCS|OAEP-SHAx, data)
OSSLRSA->>EVP: create EVP_PKEY from RSA
OSSLRSA->>EVP: EVP_PKEY_decrypt_init + set_padding
OSSLRSA->>EVP: EVP_PKEY_decrypt(NULL,&outLen,data) %% preflight
OSSLRSA->>EVP: allocate buffer & EVP_PKEY_decrypt(buf,&outLen,data)
EVP-->>OSSLRSA: plaintext or error
OSSLRSA-->>App: result (resized to outLen) or failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 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 |
LawrenceHunter
force-pushed
the
lawrence/oaep
branch
from
62f514c to
22174b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/crypto/BotanRSA.cpp (1)
1178-1183: PSS raw salt-length bound uses SHA1 constant (20) for all hashes.This permits oversize salts for SHA-2 variants. Use the computed allowedLen instead.
Apply:
- if (sLen > ((bitLength+6)/8-2-20)) + if (sLen > ((bitLength+6)/8-2-allowedLen))src/lib/crypto/OSSLRSA.cpp (1)
41-44: Missing <openssl/evp.h> include (likely cause of Windows OpenSSL CI failure).This file uses EVP_PKEY/EVP_PKEY_CTX but doesn’t include evp.h.
Apply:
#include <openssl/rsa.h> #include <openssl/pem.h> +#include <openssl/evp.h> #include <openssl/err.h>
🧹 Nitpick comments (5)
m4/acx_crypto_backend.m4 (1)
78-83: Prefer functional feature checks for OAEP over version gating.To avoid false negatives on custom or backported builds, add a small probe for EVP OAEP setters (EVP_PKEY_CTX_set_rsa_oaep_md / EVP_PKEY_CTX_set_rsa_mgf1_md) in addition to ACX_OPENSSL(1,0,2).
Example (sketch):
AC_LINK_IFELSE([AC_LANG_PROGRAM([[ #include <openssl/evp.h> #include <openssl/rsa.h> ]], [[ EVP_PKEY_CTX *c = (EVP_PKEY_CTX*)0; EVP_PKEY_CTX_set_rsa_oaep_md(c, EVP_sha256()); EVP_PKEY_CTX_set_rsa_mgf1_md(c, EVP_sha256()); ]])], [AC_DEFINE([HAVE_OPENSSL_EVP_OAEP_MD], [1], [OpenSSL has EVP OAEP MD setters])], [AC_MSG_WARN([OpenSSL lacks EVP OAEP MD setters])])src/lib/crypto/OSSLRSA.cpp (1)
1334-1406: Decrypt path frees resources correctly; consider improving error logging and return checks.Swap ret in logs for ERR_get_error and check init/setter returns for robustness.
src/lib/SoftHSM.cpp (3)
3241-3249: Reuse the common OAEP param checker for decrypt initMirror AsymEncryptInit to keep validation identical (incl. source/pSourceData constraints).
Apply:
- if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS)) - { - DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS"); - return CKR_ARGUMENTS_BAD; - } + rv = MechParamCheckRSAPKCSOAEP(pMechanism); + if (rv != CKR_OK) + return rv;
6505-6515: Round RSA modulus bytes up, not down, for OAEP size checksUsing bits/8 truncates; ceil(bits/8) is correct per PKCS#1 “k” definition.
- modulus_length = wrapKey->getUnsignedLongValue(CKA_MODULUS_BITS, 0); - // adjust key bit length - modulus_length /= bb; + CK_ULONG modulus_bits = wrapKey->getUnsignedLongValue(CKA_MODULUS_BITS, 0); + // k = ceil(modulus_bits/8) + modulus_length = (modulus_bits + 7) / 8;
13309-13316: Nit: fix MGF names in error messageUse CKG_* constants consistently.
- ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKM_MGF1_SHA224, CKM_MGF1_SHA256, CKM_MGF1_SHA384, CKM_MGF1_SHA512"); + ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)m4/acx_crypto_backend.m4(1 hunks)src/lib/SoftHSM.cpp(9 hunks)src/lib/crypto/AsymmetricAlgorithm.cpp(1 hunks)src/lib/crypto/AsymmetricAlgorithm.h(1 hunks)src/lib/crypto/BotanRSA.cpp(2 hunks)src/lib/crypto/OSSLRSA.cpp(2 hunks)src/lib/crypto/OSSLRSAPrivateKey.cpp(1 hunks)src/lib/crypto/test/RSATests.cpp(2 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.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
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.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.cppsrc/lib/crypto/AsymmetricAlgorithm.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
📚 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:
m4/acx_crypto_backend.m4
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.0
🔇 Additional comments (11)
README.md (1)
40-40: Docs version bump aligns with new OAEP/EVP usage.OpenSSL minimum 1.0.2 matches the new EVP OAEP APIs used elsewhere. LGTM.
src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
39-39: Include of <openssl/evp.h> is correct and necessary.Enables PKCS#8 EVP paths; no issues spotted.
src/lib/crypto/BotanRSA.cpp (2)
764-778: OAEP per-hash mapping (encrypt) looks correct.Mappings to OAEP(SHA-160/224/256/384/512) are consistent with Botan.
855-869: OAEP per-hash mapping (decrypt) looks correct.Symmetric with encrypt path. LGTM.
src/lib/crypto/AsymmetricAlgorithm.cpp (1)
150-155: Confirm no remaining RSA_PKCS_OAEP usagesWrap-mech whitelist updated to OAEP per-hash — good; ripgrep returned no output, so confirm there are no callers referencing the removed RSA_PKCS_OAEP by running these locally:
rg -nP '\bRSA_PKCS_OAEP\b' -S rg -nP '\bRSA_PKCS_OAEP\b' --hidden --no-ignore -S git grep -n --extended-regexp '\bRSA_PKCS_OAEP\b' || trueMigrate any found callers to the corresponding RSA_PKCS_OAEP_SHA1|SHA224|SHA256|SHA384|SHA512 enum.
src/lib/crypto/AsymmetricAlgorithm.h (1)
68-73: Confirmed — no remaining RSA_PKCS_OAEP references; only per-hash OAEP enums are used.rg output shows AsymMech::RSA_PKCS_OAEP_SHA1/224/256/384/512 in SoftHSM.cpp, src/lib/crypto/AsymmetricAlgorithm.{h,cpp}, BotanRSA.cpp, OSSLRSA.cpp and RSATests.cpp; no plain RSA_PKCS_OAEP occurrences.
src/lib/crypto/test/RSATests.cpp (3)
622-627: Expanded padding set to cover all OAEP variants.Good coverage expansion.
648-649: PKCS#1 max message length corrected to k/8 - 11.This fixes the classic off-by-one. LGTM.
650-670: OAEP data sizing per hash is correct; SHA-512 guard for small moduli is sensible.Solid sizing logic; matches RFC 8017 limits.
src/lib/crypto/OSSLRSA.cpp (1)
1276-1285: Raw RSA input length: confirm desired policy.For RSA_NO_PADDING, EVP allows shorter inputs (left-padded with zeros). If you want strict parity with signing (exact modulus length), enforce equality here too.
src/lib/SoftHSM.cpp (1)
2460-2512: OAEP mode mapping and MGF enforcement in encrypt init look solidPer-hash OAEP selection and MGF match check are correct and tightly validate params. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/SoftHSM.cpp (1)
7141-7169: MGF check now uses OAEP params (fixes earlier cast bug).This resolves the prior issue where PSS params were used for OAEP MGF validation.
🧹 Nitpick comments (4)
src/lib/SoftHSM.cpp (4)
2482-2512: OAEP mapping + MGF enforcement look correct; consider de-duplicating this logic.You map hashAlg to specific OAEP mechanisms and enforce the matching MGF. Same code appears in AsymDecryptInit, WrapKeyAsym, and UnwrapKeyAsym; a small helper would reduce drift.
Example helper (add near other statics) and use here/elsewhere:
static bool oaepMap(CK_MECHANISM_PTR mech, AsymMech::Type& outMech, unsigned long& expectedMgf) { const auto* p = CK_RSA_PKCS_OAEP_PARAMS_PTR(mech->pParameter); switch (p->hashAlg) { case CKM_SHA_1: outMech = AsymMech::RSA_PKCS_OAEP_SHA1; expectedMgf = CKG_MGF1_SHA1; return true; case CKM_SHA224: outMech = AsymMech::RSA_PKCS_OAEP_SHA224; expectedMgf = CKG_MGF1_SHA224; return true; case CKM_SHA256: outMech = AsymMech::RSA_PKCS_OAEP_SHA256; expectedMgf = CKG_MGF1_SHA256; return true; case CKM_SHA384: outMech = AsymMech::RSA_PKCS_OAEP_SHA384; expectedMgf = CKG_MGF1_SHA384; return true; case CKM_SHA512: outMech = AsymMech::RSA_PKCS_OAEP_SHA512; expectedMgf = CKG_MGF1_SHA512; return true; default: return false; } }
3250-3279: Same OAEP mapping/validation duplicated; reuse a helper.Functionally fine, but it repeats the AsymEncryptInit logic. Reuse a shared mapper to keep behavior identical across init paths.
6530-6574: OAEP input-length checks per hash are correct; enforce via shared helper.The k − 2 − 2*hLen bounds are right. Consider centralizing the hash→expectedMgf mapping and the length computation to one helper to avoid inconsistencies.
13300-13318: Fix typo in MGF error message (CKG vs CKM).Log uses CKM_* for MGF constants; should be CKG_*.
- ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKM_MGF1_SHA224, CKM_MGF1_SHA256, CKM_MGF1_SHA384, CKM_MGF1_SHA512"); + ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512");Optional: move the “hashAlg ↔ mgf must match” check into MechParamCheckRSAPKCSOAEP to remove duplication across four call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/SoftHSM.cpp(9 hunks)src/lib/crypto/OSSLRSA.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/crypto/OSSLRSA.cpp
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
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.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, botan)
- GitHub Check: macOS (botan)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
🔇 Additional comments (1)
src/lib/SoftHSM.cpp (1)
7141-7169: No OAEP/PSS param mix-ups found.Grep output shows CK_RSA_PKCS_PSS_PARAMS_PTR only in PSS handlers; no OAEP code paths cast to PSS params.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/lib/crypto/OSSLRSA.cpp (2)
1306-1330: Fix error text and format specifier; prefer ERR_get_error() and “public key” wording.
Current logs use %08X and one path says “private key encryption” for a public-key op.Apply:
- if (ret <= 0) { - ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); + if (ret <= 0) { + ERROR_MSG("RSA public key encryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; } ... - if (ret <= 0) { - ERROR_MSG("RSA private key encryption failed (0x%08X)", ERR_get_error()); + if (ret <= 0) { + ERROR_MSG("RSA public key encryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; }
1223-1232: Memory leak on EVP_PKEY_set1_RSA failure (free pkey before returning).
If EVP_PKEY_set1_RSA fails, pkey is leaked.Apply:
if (EVP_PKEY_set1_RSA(pkey, rsa) != 1) { ERROR_MSG("EVP_PKEY_set1_RSA failed (0x%08lX)", ERR_get_error()); - return false; + EVP_PKEY_free(pkey); + return false; }
🧹 Nitpick comments (3)
src/lib/crypto/OSSLRSA.cpp (3)
1295-1304: Message-length check is fine; consider deriving hLen from EVP to avoid hard-coding.
Using EVP_MD_size(EVP_shaX()) to compute 2*hLen+2 would reduce duplication and chances of mismatch.
1401-1419: Use OpenSSL error code in decrypt logs; avoid printing ret.
ret can be 0/−2 (not an error code). Prefer ERR_get_error() and %08lX for consistency.Apply:
- if (ret <= 0) { - ERROR_MSG("RSA private key decryption failed (0x%08X)", ret); + if (ret <= 0) { + ERROR_MSG("RSA private key decryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; } ... - if (ret <= 0) { - ERROR_MSG("RSA private key decryption failed (0x%08X)", ret); + if (ret <= 0) { + ERROR_MSG("RSA private key decryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; }
1223-1244: Optional: reduce duplication with small RAII helpers for EVP objects.
Creating/freeing pkey/ctx is duplicated in encrypt/decrypt. A tiny local RAII wrapper (or early-return cleanup lambda) would simplify error paths and prevent future leaks.Also applies to: 1355-1359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/SoftHSM.cpp(9 hunks)src/lib/crypto/OSSLRSA.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/SoftHSM.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Windows (x64, botan)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: macOS (botan)
- GitHub Check: Linux with OpenSSL 3.0
- GitHub Check: macOS (openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Linux with Botan
🔇 Additional comments (1)
src/lib/crypto/OSSLRSA.cpp (1)
43-43: EVP include looks correct for the new EVP-based RSA path.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/SoftHSM.cpp (4)
2460-2501: OAEP: enforce hash↔MGF match centrally; drop unused variable here
- Remove the dead
expectedMgf.- Rely on a single source of truth by enforcing the hash/MGF interlock inside
MechParamCheckRSAPKCSOAEP(see suggested diff in that function), so all OAEP call sites are covered consistently.AsymMech::Type mechanism; -unsigned long expectedMgf; bool isRSA = false; @@ - switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { + switch (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
3215-3261: Missing OAEP parameter validation in AsymDecryptInit (security correctness)
AsymDecryptInitdoes not callMechParamCheckRSAPKCSOAEP, so it skips source/MGF checks and (after the refactor) the hash↔MGF interlock. Add the check and remove the unusedexpectedMgf.AsymMech::Type mechanism = AsymMech::Unknown; -unsigned long expectedMgf; bool isRSA = false; @@ - case CKM_RSA_PKCS_OAEP: + case CKM_RSA_PKCS_OAEP: if (keyType != CKK_RSA) return CKR_KEY_TYPE_INCONSISTENT; - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS)) - { - DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS"); - return CKR_ARGUMENTS_BAD; - } + if (pMechanism->pParameter == NULL_PTR || + pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS)) + { + DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS"); + return CKR_ARGUMENTS_BAD; + } + rv = MechParamCheckRSAPKCSOAEP(pMechanism); + if (rv != CKR_OK) return rv; - switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { + switch (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
1461-1478: Add strict OAEP hash↔MGF interlock in MechParamCheckRSAPKCSOAEPCurrently it only whitelists values; it should reject mismatched hash/MGF pairs (e.g., SHA-256 with MGF1-SHA1). This prevents silent misconfigurations and keeps parity with PSS handling elsewhere.
- if (params->hashAlg != CKM_SHA_1 && - params->hashAlg != CKM_SHA224 && - params->hashAlg != CKM_SHA256 && - params->hashAlg != CKM_SHA384 && - params->hashAlg != CKM_SHA512) + if (params->hashAlg != CKM_SHA_1 && + params->hashAlg != CKM_SHA224 && + params->hashAlg != CKM_SHA256 && + params->hashAlg != CKM_SHA384 && + params->hashAlg != CKM_SHA512) { ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512"); return CKR_ARGUMENTS_BAD; } - if (params->mgf != CKG_MGF1_SHA1 && - params->mgf != CKG_MGF1_SHA224 && - params->mgf != CKG_MGF1_SHA256 && - params->mgf != CKG_MGF1_SHA384 && - params->mgf != CKG_MGF1_SHA512) + if (params->mgf != CKG_MGF1_SHA1 && + params->mgf != CKG_MGF1_SHA224 && + params->mgf != CKG_MGF1_SHA256 && + params->mgf != CKG_MGF1_SHA384 && + params->mgf != CKG_MGF1_SHA512) { ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512"); return CKR_ARGUMENTS_BAD; } + // Enforce matching MGF for selected hash + switch (params->hashAlg) { + case CKM_SHA_1: if (params->mgf != CKG_MGF1_SHA1) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; } break; + case CKM_SHA224: if (params->mgf != CKG_MGF1_SHA224) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; } break; + case CKM_SHA256: if (params->mgf != CKG_MGF1_SHA256) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; } break; + case CKM_SHA384: if (params->mgf != CKG_MGF1_SHA384) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; } break; + case CKM_SHA512: if (params->mgf != CKG_MGF1_SHA512) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; } break; + }
2460-2501: Enforce OAEP param validation and remove unused expectedMgf
- AsymDecryptInit — src/lib/SoftHSM.cpp (≈lines 3168–3316): CKM_RSA_PKCS_OAEP path does not call MechParamCheckRSAPKCSOAEP; add
rv = MechParamCheckRSAPKCSOAEP(pMechanism);and returnrvif notCKR_OK.- AsymEncryptInit — src/lib/SoftHSM.cpp (declaration at line 2460):
unsigned long expectedMgf;is declared but unused — remove it. Also remove the unusedexpectedMgfat line 3215 in AsymDecryptInit if you add the param check and it remains unused.- Leave PSS use of
expectedMgfintact (src/lib/SoftHSM.cpp ≈5245–5277).- Inspect/remove other
expectedMgfdeclarations at ≈6483 and ≈7102 if they are unused.
🧹 Nitpick comments (2)
src/lib/SoftHSM.cpp (2)
6483-6544: OAEP wrap length checks look good; remove unusedexpectedMgf
- The per-hash input length bounds are correct. Nice.
expectedMgfis declared but unused; remove it to avoid warnings.- With the proposed central hash↔MGF interlock in
MechParamCheckRSAPKCSOAEP, this path stays consistent.AsymMech::Type mech = AsymMech::Unknown; -unsigned long expectedMgf; @@ - case CKM_RSA_PKCS_OAEP: - switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { + case CKM_RSA_PKCS_OAEP: + switch (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
7102-7130: OAEP unwrap: remove unusedexpectedMgf; rely on central param check
- Variable is unused; remove.
C_UnwrapKeyalready callsMechParamCheckRSAPKCSOAEP. After adding the hash↔MGF interlock there, this path is covered.AsymMech::Type mode = AsymMech::Unknown; -unsigned long expectedMgf; @@ - case CKM_RSA_PKCS_OAEP: + case CKM_RSA_PKCS_OAEP: algo = AsymAlgo::RSA; - switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { + switch (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/SoftHSM.cpp(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
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.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Windows (x64, openssl)
- GitHub Check: Windows (x86, openssl)
- GitHub Check: Linux with OpenSSL 1.1.1
- GitHub Check: Windows (x64, botan)
- GitHub Check: Linux with OpenSSL 3.0
EVP interface allows us to use different OAEP padding modes.
From what I see, EVP is introduced with version 1.0.2 and we require it for the OAEP methods.
Allow hash-mgf mismatch in RSA OAEP
LawrenceHunter
force-pushed
the
lawrence/oaep
branch
from
a4dbf00 to
e12fe72
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/lib/crypto/OSSLRSA.cpp (3)
1345-1345: Misleading log message: this is public-key encryption, not private. Also fix format.- ERROR_MSG("RSA private key encryption failed (0x%08X)", ERR_get_error()); + ERROR_MSG("RSA public key encryption failed (0x%08lX)", ERR_get_error());
1229-1232: Leak on EVP_PKEY_set1_RSA failure. Free pkey before returning.- if (EVP_PKEY_set1_RSA(pkey, rsa) != 1) { - ERROR_MSG("EVP_PKEY_set1_RSA failed (0x%08lX)", ERR_get_error()); - return false; - } + if (EVP_PKEY_set1_RSA(pkey, rsa) != 1) { + ERROR_MSG("EVP_PKEY_set1_RSA failed (0x%08lX)", ERR_get_error()); + EVP_PKEY_free(pkey); + return false; + }
1406-1460: Decrypt: check padding setter returns for PKCS#1 v1.5 and NO_PADDING too.Mirror OAEP branches—bail out with cleanup if a setter fails.
case AsymMech::RSA_PKCS: - EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); + if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING) <= 0) { + ERROR_MSG("Configuring RSA PKCS#1 v1.5 padding failed (0x%08lX)", ERR_get_error()); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); + return false; + } break; ... case AsymMech::RSA: - EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_NO_PADDING); + if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_NO_PADDING) <= 0) { + ERROR_MSG("Configuring RSA_NO_PADDING failed (0x%08lX)", ERR_get_error()); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(pkey); + return false; + } break;
🧹 Nitpick comments (3)
src/lib/crypto/OSSLRSA.cpp (2)
1334-1334: Fix OpenSSL error formatting spec.Use %08lX for ERR_get_error().
- ERROR_MSG("RSA public key encryption failed (0x%08X)", ERR_get_error()); + ERROR_MSG("RSA public key encryption failed (0x%08lX)", ERR_get_error());
1470-1477: Decrypt: log OpenSSL error code, not ret; fix format.ret≤0 doesn’t carry ERR state. Use ERR_get_error() and %08lX.
- if (ret <= 0) { - ERROR_MSG("RSA private key decryption failed (0x%08X)", ret); + if (ret <= 0) { + ERROR_MSG("RSA private key decryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; } ... - if (ret <= 0) { - ERROR_MSG("RSA private key decryption failed (0x%08X)", ret); + if (ret <= 0) { + ERROR_MSG("RSA private key decryption failed (0x%08lX)", ERR_get_error()); EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); return false; }Also applies to: 1481-1487
src/lib/SoftHSM.cpp (1)
6517-6551: OAEP input length bound per-hash — LGTM; minor de-dup possibleThe k − 2 − 2*hLen bound per hash is correct. Consider computing hLen once and using a single formula to reduce repetition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)m4/acx_crypto_backend.m4(1 hunks)src/lib/SoftHSM.cpp(5 hunks)src/lib/crypto/AsymmetricAlgorithm.cpp(1 hunks)src/lib/crypto/AsymmetricAlgorithm.h(1 hunks)src/lib/crypto/BotanRSA.cpp(2 hunks)src/lib/crypto/OSSLRSA.cpp(3 hunks)src/lib/crypto/OSSLRSAPrivateKey.cpp(1 hunks)src/lib/crypto/test/RSATests.cpp(2 hunks)src/lib/test/AsymEncryptDecryptTests.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- src/lib/test/AsymEncryptDecryptTests.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- src/lib/crypto/OSSLRSAPrivateKey.cpp
- src/lib/crypto/AsymmetricAlgorithm.h
- m4/acx_crypto_backend.m4
- src/lib/crypto/AsymmetricAlgorithm.cpp
- src/lib/crypto/test/RSATests.cpp
🧰 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.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
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.
📚 Learning: 2025-09-11T16:54:00.347Z
Learnt from: bukka
PR: softhsm/SoftHSMv2#815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.347Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/SoftHSM.cppsrc/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/SoftHSM.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
🔇 Additional comments (3)
src/lib/crypto/OSSLRSA.cpp (1)
43-43: Necessary include for EVP path: LGTM.src/lib/SoftHSM.cpp (2)
2481-2501: OAEP per-hash mechanism mapping in EncryptInit — LGTMClean mapping from hashAlg to the new per-hash AsymMech variants with clear error on unsupported values.
7116-7134: UnwrapKeyAsym OAEP per-hash mapping — LGTMCorrect cast to CK_RSA_PKCS_OAEP_PARAMS_PTR and clean mapping to per-hash modes.
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | ||
| maxPad = 11; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check return values when setting RSA padding for PKCS#1 v1.5 and NO_PADDING.
Setter calls can fail (provider mismatch/unsupported). Abort on error with cleanup for consistency with OAEP branches.
case AsymMech::RSA_PKCS:
- EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING);
+ if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING) <= 0) {
+ ERROR_MSG("Configuring RSA PKCS#1 v1.5 padding failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ return false;
+ }
maxPad = 11;
break;
...
case AsymMech::RSA:
- EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_NO_PADDING);
+ if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_NO_PADDING) <= 0) {
+ ERROR_MSG("Configuring RSA_NO_PADDING failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ return false;
+ }
break;Also applies to: 1310-1311
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1251-1253 (and similarly at
1310-1311), the calls to EVP_PKEY_CTX_set_rsa_padding for RSA_PKCS1_PADDING and
RSA_NO_PADDING are not checked for errors; update these sites to check the
function's return value, and if it returns <= 0 abort the operation and perform
the same cleanup used in the OAEP branches (free/cleanup ctx and any allocated
resources, set appropriate error/return code) so the function exits on failure
rather than continuing after a failed setter call.
There was a problem hiding this comment.
Not sure if this can fail in reality but for API correctness, it should be done.
There was a problem hiding this comment.
@bukka, thank you for confirming! You're absolutely right that for API correctness and consistency, these return values should be checked even if failures are unlikely in practice. This ensures robust error handling and maintains the same defensive programming pattern used in the OAEP branches.
Note that the same fix should also be applied in the decrypt() function around lines 1368-1369 (RSA_PKCS1_PADDING) and 1423-1424 (RSA_NO_PADDING), where the padding setters are also not checked. This will ensure consistent error handling across both encryption and decryption operations.
🧠 Learnings used
Learnt from: bukka
PR: softhsm/SoftHSMv2#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
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.
| switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) { | ||
| case CKM_SHA_1: | ||
| mechanism = AsymMech::RSA_PKCS_OAEP_SHA1; | ||
| break; | ||
| case CKM_SHA224: | ||
| mechanism = AsymMech::RSA_PKCS_OAEP_SHA224; | ||
| break; | ||
| case CKM_SHA256: | ||
| mechanism = AsymMech::RSA_PKCS_OAEP_SHA256; | ||
| break; | ||
| case CKM_SHA384: | ||
| mechanism = AsymMech::RSA_PKCS_OAEP_SHA384; | ||
| break; | ||
| case CKM_SHA512: | ||
| mechanism = AsymMech::RSA_PKCS_OAEP_SHA512; | ||
| break; | ||
| default: | ||
| DEBUG_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512"); | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA1) | ||
|
|
||
| if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA1 && | ||
| CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA224 && | ||
| CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA256 && | ||
| CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA384 && | ||
| CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA512) | ||
| { | ||
| DEBUG_MSG("mgf must be CKG_MGF1_SHA1"); | ||
| DEBUG_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512"); | ||
| return CKR_ARGUMENTS_BAD; |
There was a problem hiding this comment.
DecryptInit: run the same OAEP parameter validator as EncryptInit (and drop redundant checks)
AsymDecryptInit currently only checks struct size and that mgf is one of the allowed enums. It skips the stricter OAEP param checks (source, pSourceData, ulSourceDataLen) enforced in EncryptInit via MechParamCheckRSAPKCSOAEP, leading to inconsistent acceptance of parameters across encrypt/decrypt. Call the validator here and remove the duplicate mgf-allowlist block.
Apply this diff:
case CKM_RSA_PKCS_OAEP:
if (keyType != CKK_RSA)
return CKR_KEY_TYPE_INCONSISTENT;
- if (pMechanism->pParameter == NULL_PTR ||
- pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS))
- {
- DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS");
- return CKR_ARGUMENTS_BAD;
- }
+ if (pMechanism->pParameter == NULL_PTR ||
+ pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS))
+ {
+ DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS");
+ return CKR_ARGUMENTS_BAD;
+ }
+ // Keep OAEP param policy identical to EncryptInit.
+ rv = MechParamCheckRSAPKCSOAEP(pMechanism);
+ if (rv != CKR_OK)
+ return rv;
@@
switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
@@
default:
DEBUG_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512");
return CKR_ARGUMENTS_BAD;
}
-
- if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA1 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA224 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA256 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA384 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA512)
- {
- DEBUG_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512");
- return CKR_ARGUMENTS_BAD;
- }🤖 Prompt for AI Agents
In src/lib/SoftHSM.cpp around lines 3238 to 3266, DecryptInit duplicates an mgf
allowlist check and omits the stricter OAEP parameter validation used by
EncryptInit; replace the manual mgf checks by invoking
MechParamCheckRSAPKCSOAEP(pMechanism) (or the existing OAEP validator used by
EncryptInit) and remove the redundant mgf-if block so both EncryptInit and
DecryptInit run the same OAEP parameter validation (including source,
pSourceData and ulSourceDataLen) and return CKR_ARGUMENTS_BAD on failure.
There was a problem hiding this comment.
This is good idea with minor thing that pMechanism->pParameter does not need to be checked either as they are checked in MechParamCheckRSAPKCSOAEP
There was a problem hiding this comment.
@bukka Thanks, agreed. I’ve removed the explicit pMechanism->pParameter NULL/size check and now rely solely on MechParamCheckRSAPKCSOAEP, and I’ve dropped the redundant mgf allowlist. Here’s the updated diff for AsymDecryptInit:
diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp
index 0000000..0000001 100644
--- a/src/lib/SoftHSM.cpp
+++ b/src/lib/SoftHSM.cpp
@@ -3214,31 +3214,24 @@ CK_RV SoftHSM::AsymDecryptInit(CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMechanism, CK_OBJECT_HANDLE hKey)
case CKM_RSA_PKCS_OAEP:
if (keyType != CKK_RSA)
return CKR_KEY_TYPE_INCONSISTENT;
- if (pMechanism->pParameter == NULL_PTR ||
- pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS))
- {
- DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS");
- return CKR_ARGUMENTS_BAD;
- }
+ // Keep OAEP param policy identical to EncryptInit
+ rv = MechParamCheckRSAPKCSOAEP(pMechanism);
+ if (rv != CKR_OK)
+ return rv;
switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
case CKM_SHA_1:
mechanism = AsymMech::RSA_PKCS_OAEP_SHA1;
break;
case CKM_SHA224:
mechanism = AsymMech::RSA_PKCS_OAEP_SHA224;
break;
case CKM_SHA256:
mechanism = AsymMech::RSA_PKCS_OAEP_SHA256;
break;
case CKM_SHA384:
mechanism = AsymMech::RSA_PKCS_OAEP_SHA384;
break;
case CKM_SHA512:
mechanism = AsymMech::RSA_PKCS_OAEP_SHA512;
break;
default:
DEBUG_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512");
return CKR_ARGUMENTS_BAD;
}
-
- if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA1 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA224 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA256 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA384 &&
- CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != CKG_MGF1_SHA512)
- {
- DEBUG_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512");
- return CKR_ARGUMENTS_BAD;
- }
isRSA = true;
break;This makes DecryptInit consistent with EncryptInit’s OAEP validation and removes duplicate checks as you suggested.
✏️ Learnings added
Learnt from: bukka
PR: softhsm/SoftHSMv2#818
File: src/lib/SoftHSM.cpp:3238-3266
Timestamp: 2025-10-13T09:42:31.258Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
| if (params->hashAlg != CKM_SHA_1 && | ||
| params->hashAlg != CKM_SHA224 && | ||
| params->hashAlg != CKM_SHA256 && | ||
| params->hashAlg != CKM_SHA384 && | ||
| params->hashAlg != CKM_SHA512) | ||
| { | ||
| ERROR_MSG("hashAlg must be CKM_SHA_1"); | ||
| ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512"); | ||
| return CKR_ARGUMENTS_BAD; | ||
| } | ||
| if (params->mgf != CKG_MGF1_SHA1) | ||
| if (params->mgf != CKG_MGF1_SHA1 && | ||
| params->mgf != CKG_MGF1_SHA224 && | ||
| params->mgf != CKG_MGF1_SHA256 && | ||
| params->mgf != CKG_MGF1_SHA384 && | ||
| params->mgf != CKG_MGF1_SHA512) | ||
| { | ||
| ERROR_MSG("mgf must be CKG_MGF1_SHA1"); | ||
| ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512"); | ||
| return CKR_ARGUMENTS_BAD; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce OAEP MGF to match the selected hashAlg to avoid silently ignoring mgf
Backends don’t receive mgf separately via AsymMech, so a mismatched mgf would be accepted here yet ignored later, surprising callers. Require mgf to correspond to hashAlg (as done for PSS), and keep the existing policy for source/pSourceData.
Apply this diff:
CK_RSA_PKCS_OAEP_PARAMS_PTR params = (CK_RSA_PKCS_OAEP_PARAMS_PTR)pMechanism->pParameter;
-if (params->hashAlg != CKM_SHA_1 &&
- params->hashAlg != CKM_SHA224 &&
- params->hashAlg != CKM_SHA256 &&
- params->hashAlg != CKM_SHA384 &&
- params->hashAlg != CKM_SHA512)
-{
- ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512");
- return CKR_ARGUMENTS_BAD;
-}
-if (params->mgf != CKG_MGF1_SHA1 &&
- params->mgf != CKG_MGF1_SHA224 &&
- params->mgf != CKG_MGF1_SHA256 &&
- params->mgf != CKG_MGF1_SHA384 &&
- params->mgf != CKG_MGF1_SHA512)
-{
- ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512");
- return CKR_ARGUMENTS_BAD;
-}
+if (params->hashAlg != CKM_SHA_1 &&
+ params->hashAlg != CKM_SHA224 &&
+ params->hashAlg != CKM_SHA256 &&
+ params->hashAlg != CKM_SHA384 &&
+ params->hashAlg != CKM_SHA512)
+{
+ ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512");
+ return CKR_ARGUMENTS_BAD;
+}
+unsigned long expectedMgf = 0;
+switch (params->hashAlg) {
+ case CKM_SHA_1: expectedMgf = CKG_MGF1_SHA1; break;
+ case CKM_SHA224: expectedMgf = CKG_MGF1_SHA224; break;
+ case CKM_SHA256: expectedMgf = CKG_MGF1_SHA256; break;
+ case CKM_SHA384: expectedMgf = CKG_MGF1_SHA384; break;
+ case CKM_SHA512: expectedMgf = CKG_MGF1_SHA512; break;
+}
+if (params->mgf != expectedMgf) {
+ ERROR_MSG("Hash and MGF don't match");
+ return CKR_ARGUMENTS_BAD;
+}📝 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 (params->hashAlg != CKM_SHA_1 && | |
| params->hashAlg != CKM_SHA224 && | |
| params->hashAlg != CKM_SHA256 && | |
| params->hashAlg != CKM_SHA384 && | |
| params->hashAlg != CKM_SHA512) | |
| { | |
| ERROR_MSG("hashAlg must be CKM_SHA_1"); | |
| ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512"); | |
| return CKR_ARGUMENTS_BAD; | |
| } | |
| if (params->mgf != CKG_MGF1_SHA1) | |
| if (params->mgf != CKG_MGF1_SHA1 && | |
| params->mgf != CKG_MGF1_SHA224 && | |
| params->mgf != CKG_MGF1_SHA256 && | |
| params->mgf != CKG_MGF1_SHA384 && | |
| params->mgf != CKG_MGF1_SHA512) | |
| { | |
| ERROR_MSG("mgf must be CKG_MGF1_SHA1"); | |
| ERROR_MSG("mgf must be one of: CKG_MGF1_SHA1, CKG_MGF1_SHA224, CKG_MGF1_SHA256, CKG_MGF1_SHA384, CKG_MGF1_SHA512"); | |
| return CKR_ARGUMENTS_BAD; | |
| } | |
| CK_RSA_PKCS_OAEP_PARAMS_PTR params = (CK_RSA_PKCS_OAEP_PARAMS_PTR)pMechanism->pParameter; | |
| if (params->hashAlg != CKM_SHA_1 && | |
| params->hashAlg != CKM_SHA224 && | |
| params->hashAlg != CKM_SHA256 && | |
| params->hashAlg != CKM_SHA384 && | |
| params->hashAlg != CKM_SHA512) | |
| { | |
| ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512"); | |
| return CKR_ARGUMENTS_BAD; | |
| } | |
| unsigned long expectedMgf = 0; | |
| switch (params->hashAlg) { | |
| case CKM_SHA_1: expectedMgf = CKG_MGF1_SHA1; break; | |
| case CKM_SHA224: expectedMgf = CKG_MGF1_SHA224; break; | |
| case CKM_SHA256: expectedMgf = CKG_MGF1_SHA256; break; | |
| case CKM_SHA384: expectedMgf = CKG_MGF1_SHA384; break; | |
| case CKM_SHA512: expectedMgf = CKG_MGF1_SHA512; break; | |
| } | |
| if (params->mgf != expectedMgf) { | |
| ERROR_MSG("Hash and MGF don't match"); | |
| return CKR_ARGUMENTS_BAD; | |
| } |
There was a problem hiding this comment.
@bukka Thanks! I’ll update MechParamCheckRSAPKCSOAEP to require MGF1 to match the selected hashAlg (like PSS). Here’s the patch:
// src/lib/SoftHSM.cpp
CK_RV SoftHSM::MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism)
{
// This is a programming error
if (pMechanism->mechanism != CKM_RSA_PKCS_OAEP) {
ERROR_MSG("MechParamCheckRSAPKCSOAEP called on wrong mechanism");
return CKR_GENERAL_ERROR;
}
if (pMechanism->pParameter == NULL_PTR ||
pMechanism->ulParameterLen != sizeof(CK_RSA_PKCS_OAEP_PARAMS))
{
ERROR_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS");
return CKR_ARGUMENTS_BAD;
}
CK_RSA_PKCS_OAEP_PARAMS_PTR params = (CK_RSA_PKCS_OAEP_PARAMS_PTR)pMechanism->pParameter;
if (params->hashAlg != CKM_SHA_1 &&
params->hashAlg != CKM_SHA224 &&
params->hashAlg != CKM_SHA256 &&
params->hashAlg != CKM_SHA384 &&
params->hashAlg != CKM_SHA512)
{
ERROR_MSG("hashAlg must be one of: CKM_SHA_1, CKM_SHA224, CKM_SHA256, CKM_SHA384, CKM_SHA512");
return CKR_ARGUMENTS_BAD;
}
unsigned long expectedMgf = 0;
switch (params->hashAlg) {
case CKM_SHA_1: expectedMgf = CKG_MGF1_SHA1; break;
case CKM_SHA224: expectedMgf = CKG_MGF1_SHA224; break;
case CKM_SHA256: expectedMgf = CKG_MGF1_SHA256; break;
case CKM_SHA384: expectedMgf = CKG_MGF1_SHA384; break;
case CKM_SHA512: expectedMgf = CKG_MGF1_SHA512; break;
}
if (params->mgf != expectedMgf) {
ERROR_MSG("Hash and MGF don't match");
return CKR_ARGUMENTS_BAD;
}
if (params->source != CKZ_DATA_SPECIFIED)
{
ERROR_MSG("source must be CKZ_DATA_SPECIFIED");
return CKR_ARGUMENTS_BAD;
}
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;
}
return CKR_OK;
}Optional follow-up: For consistency, we can route AsymDecryptInit’s CKM_RSA_PKCS_OAEP branch through MechParamCheckRSAPKCSOAEP (and drop its local whitelist) so both encrypt and decrypt paths use the same policy. Want me to include that tweak as well?
✏️ Learnings added
Learnt from: bukka
PR: softhsm/SoftHSMv2#818
File: src/lib/SoftHSM.cpp:13265-13282
Timestamp: 2025-10-13T09:43:45.860Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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.
3 participants
Continues the work of #583 to support different OAEP padding modes, such as OAEP-SHA224, OAEP-SHA256, OAEP-SHA384, OAEP-SHA512.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Refactor
Documentation
Tests