Support different OAEP modes using OpenSSL EVP interface #583
Conversation
EVP interface allows us to use different OAEP padding modes.
|
I've tested decryption across all of them. Could someone help me test encryption? |
According to the spec, it's keylen/8 - 2 - 2*hashLen
borancar
changed the title
|
Implement all OAEP tests and they all pass now. The only workaround is not to use key size of 1024 for OAEP-SHA512. |
|
Thanks for this contribution; once you think it is ready for review, can you create a pull request so we can review the code? |
|
Had a quick look at the code, looks good at first glance. To bump the OpenSSL version, you should also update |
From what I see, EVP is introduced with version 1.0.2 and we require it for the OAEP methods.
|
Done, thanks! |
|
Hello, Just came across and I am happy that there is already a solution for supporting different hash algorithm other than SHA1 . |
|
Please rebase on develop and mark as ready when ready. |
| break; | ||
| case CKM_SHA224: | ||
| mech = AsymMech::RSA_PKCS_OAEP_SHA224; | ||
| expectedMgf = CKG_MGF1_SHA1; |
There was a problem hiding this comment.
This (and the other expectedMgf below for the other hash functions) is not the same as the other expectedMgf is the modification above.
It seems weird.
I think it should be CKG_MGF1_SHA224 which would make more sense.
|
SoftHSM does not compile with Botan backend. |
|
I am very keen for this PR and therefore am more than happy to help where I can. I was planning to pick this up myself but @antoinelochet seems to be actively working on it therefore please reach out if there's anything you need help with |
Thank you but I just pull this PR as-is on my fork so I can have the features I need on my side. |
|
Is anyone willing to take over merging this to master/develop/wherever? I tried getting this in 2020 and it was taking too long. I have since abandoned working on it due to lack of time, and am more than happy for someone else to take credit - as long as the thing gets merged. |
I use it daily so I can tell you it is ready for merging at least. |
Hello, the company I work for would be willing to dedicate some time to doing so. Although what are the barriers to merging? |
Co-authored-by: Lawrence Hunter <[email protected]>
WalkthroughSynchronizes OpenSSL minimum to 1.0.2 in docs and build m4. Expands RSA-OAEP support to SHA-1/224/256/384/512 with MGF1 consistency checks across init/wrap/unwrap. Updates enum to explicit OAEP variants, adjusts wrapping detection, migrates RSA encrypt/decrypt to EVP APIs, adds EVP include, and extends RSA tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant HSM as SoftHSM (PKCS#11)
participant ALG as AsymmetricAlgorithm
participant RSA as OSSLRSA
participant EVP as OpenSSL EVP
App->>HSM: C_EncryptInit(mech=RSA_PKCS_OAEP_SHA256, params{mgf=MGF1-SHA256})
HSM->>HSM: Validate OAEP params (hash ↔ MGF1 match)
HSM->>ALG: Resolve mech type (OAEP_SHA256)
App->>HSM: C_Encrypt(data)
HSM->>RSA: encrypt(mech=OAEP_SHA256, data)
RSA->>EVP: EVP_PKEY_CTX_new/init
RSA->>EVP: set_rsa_padding(OAEP)
RSA->>EVP: set_oaep_md(SHA-256), set_mgf1_md(SHA-256)
RSA->>EVP: EVP_PKEY_encrypt(outlen query → allocate)
RSA->>EVP: EVP_PKEY_encrypt(out, in)
EVP-->>RSA: ciphertext / status
RSA-->>HSM: ciphertext or error
HSM-->>App: ciphertext or CKR_ARGUMENTS_BAD/FAILURE
note over HSM,EVP: Similar flow for Decrypt/Wrap/Unwrap with same OAEP hash/MGF checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
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. ✨ 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 |
|
We can merge once @bjosv says OK and all tests run without any issues. |
|
Looks like Botan still fails, so this can't be merged |
At the moment, Botan build failing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/SoftHSM.cpp (1)
6725-6763: Bug: Using CK_RSA_PKCS_PSS_PARAMS_PTR to read OAEP params (UnwrapKeyAsym).The MGF check dereferences OAEP parameters through the PSS struct macro. While the layout happens to align for the first two fields, it’s the wrong type and fragile.
Apply:
- if (CK_RSA_PKCS_PSS_PARAMS_PTR(pMechanism->pParameter)->mgf != expectedMgf) { + if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->mgf != expectedMgf) { ERROR_MSG("Hash and MGF don't match"); return CKR_ARGUMENTS_BAD; }
♻️ Duplicate comments (2)
m4/acx_crypto_backend.m4 (1)
68-73: Both FIPS and non-FIPS branches set OpenSSL to 1.0.2 (with rationale).The explanatory comments address the earlier “branches are the same” concern. Looks good.
src/lib/SoftHSM.cpp (1)
6276-6347: Bug: expectedMgf is wrong for OAEP-SHA224/256/384/512 in WrapKeyAsym (forces SHA-1 MGF).This incorrectly rejects valid OAEP requests using matching MGF1 hashes for non‑SHA1 modes and only allows CKG_MGF1_SHA1. Fix by aligning expectedMgf with the selected hash.
Apply:
case CKM_SHA224: mech = AsymMech::RSA_PKCS_OAEP_SHA224; - expectedMgf = CKG_MGF1_SHA1; + expectedMgf = CKG_MGF1_SHA224; // PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen if (keydata.size() > modulus_length - 2 - 2 * 224 / 8) return CKR_KEY_SIZE_RANGE; break; case CKM_SHA256: mech = AsymMech::RSA_PKCS_OAEP_SHA256; - expectedMgf = CKG_MGF1_SHA1; + expectedMgf = CKG_MGF1_SHA256; if (keydata.size() > modulus_length - 2 - 2 * 256 / 8) return CKR_KEY_SIZE_RANGE; break; case CKM_SHA384: mech = AsymMech::RSA_PKCS_OAEP_SHA384; - expectedMgf = CKG_MGF1_SHA1; + expectedMgf = CKG_MGF1_SHA384; if (keydata.size() > modulus_length - 2 - 2 * 384 / 8) return CKR_KEY_SIZE_RANGE; break; case CKM_SHA512: mech = AsymMech::RSA_PKCS_OAEP_SHA512; - expectedMgf = CKG_MGF1_SHA1; + expectedMgf = CKG_MGF1_SHA512; if (keydata.size() > modulus_length - 2 - 2 * 512 / 8) return CKR_KEY_SIZE_RANGE; break;
🧹 Nitpick comments (4)
m4/acx_crypto_backend.m4 (1)
68-73: Consider capability probing over hard version pinning (future-proofing).Prefer a functional check for required EVP OAEP controls (e.g., EVP_PKEY_encrypt/decrypt plus EVP_PKEY_CTX_set_rsa_oaep_md and EVP_PKEY_CTX_set_rsa_mgf1_md) and fall back/err if missing. This better handles backports or vendor builds.
I can draft an AC_CHECK_FUNCS/compile test macro (ACX_OPENSSL_OAEP_FEATURES) if you want to move in this direction.
src/lib/crypto/test/RSATests.cpp (1)
666-670: Avoid hard-coding 1280-bit threshold; compute from hLen instead.This makes the test future-proof for other key sizes (e.g., 1536-bit) and other digests.
Apply:
- else if (*pad == AsymMech::RSA_PKCS_OAEP_SHA512) - { - if (*k < 1280) continue; - CPPUNIT_ASSERT(rng->generateRandom(testData, (*k >> 3) - (2*512/8+2))); - } + else if (*pad == AsymMech::RSA_PKCS_OAEP_SHA512) + { + const size_t kBytes = (*k >> 3); + const size_t hLen = 512/8; // SHA-512 + if (kBytes <= (2*hLen + 2)) continue; + CPPUNIT_ASSERT(rng->generateRandom(testData, kBytes - (2*hLen + 2))); + }src/lib/SoftHSM.cpp (1)
12517-12533: Nit: Log text lists MGF constants with CKM_ instead of CKG_.The message should reference CKG_MGF1_* to match the enum. Also consider mirroring the style used elsewhere.
Apply:
- 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");src/lib/crypto/OSSLRSA.cpp (1)
1231-1264: Optional: compute OAEP overhead via EVP_MD_size instead of hard-coded constants.Reduces duplication and avoids arithmetic errors if new hashes are added.
Illustration:
- size_t maxPad = 0; + size_t maxPad = 0; + const EVP_MD* oaep_md = NULL; @@ - EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); - EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); - EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); - maxPad = 2 * 256/8 + 2; + EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); + oaep_md = EVP_sha256(); + EVP_PKEY_CTX_set_rsa_oaep_md(ctx, oaep_md); + EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, oaep_md); + /* maxPad computed after switch */ break; @@ - // The size of the input data cannot be more than the modulus + if (oaep_md) { + maxPad = 2 * (size_t)EVP_MD_size(oaep_md) + 2; + } + // The size of the input data cannot be more than the modulusAlso applies to: 1276-1279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/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-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
📚 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
🔇 Additional comments (8)
src/lib/crypto/OSSLRSAPrivateKey.cpp (1)
39-39: Good include to unlock PKCS#8 EVP APIs.Explicitly including <openssl/evp.h> ensures prototypes for EVP_PKEY2PKCS8/EVP_PKCS82PKEY are available and avoids transitive-include brittleness. LGTM.
README.md (1)
39-39: OpenSSL min version bump is consistent with the build macros.This matches ACX_OPENSSL(1,0,2) in the autoconf path and aligns with the EVP-based OAEP plumbing. LGTM.
src/lib/crypto/AsymmetricAlgorithm.cpp (1)
150-155: Wrap/unwrap whitelist updated for all OAEP variants.Recognizing RSA_PKCS_OAEP_SHA{1,224,256,384,512} here is necessary for wrap/unwrap paths. LGTM.
src/lib/crypto/test/RSATests.cpp (3)
622-627: Expanded padding matrix to all OAEP SHA variants.*Coverage looks complete (PKCS v1.5, OAEP SHA1/224/256/384/512, raw). LGTM.
648-649: PKCS#1 v1.5 max message length corrected to k−11.The -11 fix is accurate per PKCS#1. LGTM.
650-666: Per-hash OAEP max-plaintext calculations are correct.Using kBytes - (2*hLen + 2) aligns with PKCS#1 OAEP limits for each digest. LGTM.
src/lib/SoftHSM.cpp (2)
2376-2428: OAEP hash→mechanism mapping and MGF consistency check (encrypt) look correct.Mapping CKM_SHA_1/224/256/384/512 to the corresponding AsymMech values and enforcing MGF1 to match the selected hash is sound and consistent with the PSS handling elsewhere.
3131-3185: OAEP hash→mechanism mapping and MGF consistency check (decrypt) look correct.Same positives as in encrypt path; this keeps behavior symmetric across init paths.
| RSA_PKCS_OAEP_SHA1, | ||
| RSA_PKCS_OAEP_SHA224, | ||
| RSA_PKCS_OAEP_SHA256, | ||
| RSA_PKCS_OAEP_SHA384, | ||
| RSA_PKCS_OAEP_SHA512, | ||
| RSA_SHA1_PKCS, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enum split to per-hash OAEP variants. Verify no lingering uses of the old value.
Removing RSA_PKCS_OAEP in favor of explicit SHA* variants is fine. Please ensure no references to the removed enum remain (including Botan backend paths).
Run:
🏁 Script executed:
#!/bin/bash
# Find any stale references to the old enum and confirm both backends handle new ones.
rg -nP --type=cpp -C2 '\bRSA_PKCS_OAEP\b|RSA_PKCS_OAEP_SHA(1|224|256|384|512)' src/
# Sanity check OpenSSL and Botan RSA implementations for new cases.
fd -a 'OSSLRSA.cpp' src/ | xargs -I{} rg -n 'RSA_PKCS_OAEP_SHA' {}
fd -a 'BotanRSA.cpp' src/ | xargs -I{} rg -n 'RSA_PKCS_OAEP_SHA|OAEP' {}Length of output: 13233
Leftover AsymMech::RSA_PKCS_OAEP references remain — fix Botan and check MGF mappings.
Botan backend still uses AsymMech::RSA_PKCS_OAEP; change to AsymMech::RSA_PKCS_OAEP_SHA1 (Botan uses EME1(SHA-160)). See src/lib/crypto/BotanRSA.cpp:764 and src/lib/crypto/BotanRSA.cpp:843.
Also review src/lib/SoftHSM.cpp (approx. lines 6311–6335): several cases set expectedMgf = CKG_MGF1_SHA1 for non‑SHA1 hashes — confirm or correct.
| EVP_PKEY *pkey = EVP_PKEY_new(); | ||
| EVP_PKEY_set1_RSA(pkey, rsa); | ||
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | ||
| EVP_PKEY_encrypt_init(ctx); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add <openssl/evp.h> and harden EVP object creation/init (null/ret checks).
Without including evp.h this won’t compile; also check all EVP allocations and init calls to avoid NULL derefs.
Apply:
--- a/src/lib/crypto/OSSLRSA.cpp
+++ b/src/lib/crypto/OSSLRSA.cpp
@@
-#include <openssl/rsa.h>
+#include <openssl/rsa.h>
+#include <openssl/evp.h>
@@
- EVP_PKEY *pkey = EVP_PKEY_new();
- EVP_PKEY_set1_RSA(pkey, rsa);
- EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
- EVP_PKEY_encrypt_init(ctx);
+ EVP_PKEY *pkey = EVP_PKEY_new();
+ if (!pkey || EVP_PKEY_set1_RSA(pkey, rsa) <= 0)
+ {
+ ERROR_MSG("EVP_PKEY allocation/set1_RSA failed (0x%08lX)", ERR_get_error());
+ if (pkey) EVP_PKEY_free(pkey);
+ return false;
+ }
+ EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
+ if (!ctx)
+ {
+ ERROR_MSG("EVP_PKEY_CTX_new failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_free(pkey);
+ return false;
+ }
+ if (EVP_PKEY_encrypt_init(ctx) <= 0)
+ {
+ ERROR_MSG("EVP_PKEY_encrypt_init failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EVP_PKEY *pkey = EVP_PKEY_new(); | |
| EVP_PKEY_set1_RSA(pkey, rsa); | |
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | |
| EVP_PKEY_encrypt_init(ctx); | |
| EVP_PKEY *pkey = EVP_PKEY_new(); | |
| if (!pkey || EVP_PKEY_set1_RSA(pkey, rsa) <= 0) | |
| { | |
| ERROR_MSG("EVP_PKEY allocation/set1_RSA failed (0x%08lX)", ERR_get_error()); | |
| if (pkey) EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | |
| if (!ctx) | |
| { | |
| ERROR_MSG("EVP_PKEY_CTX_new failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| if (EVP_PKEY_encrypt_init(ctx) <= 0) | |
| { | |
| ERROR_MSG("EVP_PKEY_encrypt_init failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } |
| case AsymMech::RSA_PKCS: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | ||
| maxPad = 10 + 1; | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA1: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()); | ||
| maxPad = 2 * 160/8 + 2; | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA224: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()); | ||
| maxPad = 2 * 224/8 + 2; | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA256: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); | ||
| maxPad = 2 * 256/8 + 2; | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA384: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()); | ||
| maxPad = 2 * 384/8 + 2; | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA512: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()); | ||
| maxPad = 2 * 512/8 + 2; | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check return values of EVP_PKEY_CTX_set_*; don’t ignore configuration failures.
If any padding/hash setup fails, you proceed with a misconfigured ctx and get opaque failures later.
Example (apply similarly in each case):
- EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING);
+ if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING) <= 0) {
+ ERROR_MSG("EVP_PKEY_CTX_set_rsa_padding failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey);
+ return false;
+ }
@@
- EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING);
- EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256());
- EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256());
+ if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 ||
+ EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()) <= 0 ||
+ EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()) <= 0) {
+ ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey);
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case AsymMech::RSA_PKCS: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | |
| maxPad = 10 + 1; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA1: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()); | |
| maxPad = 2 * 160/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA224: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()); | |
| maxPad = 2 * 224/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA256: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); | |
| maxPad = 2 * 256/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA384: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()); | |
| maxPad = 2 * 384/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA512: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()); | |
| maxPad = 2 * 512/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING) <= 0) { | |
| ERROR_MSG("EVP_PKEY_CTX_set_rsa_padding failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 10 + 1; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA1: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 2 * 160/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA224: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 2 * 224/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA256: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 2 * 256/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA384: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 2 * 384/8 + 2; | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA512: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| maxPad = 2 * 512/8 + 2; | |
| break; |
| // The size of the input data cannot be more than the modulus | ||
| // length of the key - maxPad | ||
| if (data.size() > (size_t) (RSA_size(rsa) - maxPad)) | ||
| { | ||
| ERROR_MSG("Too much data supplied for encryption"); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
|
|
||
| osslPadding = RSA_NO_PADDING; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix underflow in input-length check when OAEP overhead ≥ modulus; return a clear error.
For 1024-bit keys with OAEP-SHA512, (RSA_size - maxPad) becomes negative and, after cast to size_t, the check is bypassed.
Apply:
- // The size of the input data cannot be more than the modulus
- // length of the key - maxPad
- if (data.size() > (size_t) (RSA_size(rsa) - maxPad))
+ // Guard against too-small modulus for selected padding/hash
+ if (maxPad && (size_t)RSA_size(rsa) <= maxPad)
+ {
+ ERROR_MSG("RSA modulus too small for selected OAEP mode (n=%d, overhead=%zu)", RSA_size(rsa), maxPad);
+ EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey);
+ return false;
+ }
+ // The size of the input data cannot exceed RSA_size - maxPad
+ if (data.size() > (size_t)RSA_size(rsa) - maxPad)
{
ERROR_MSG("Too much data supplied for encryption");
EVP_PKEY_CTX_free(ctx);
EVP_PKEY_free(pkey);
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The size of the input data cannot be more than the modulus | |
| // length of the key - maxPad | |
| if (data.size() > (size_t) (RSA_size(rsa) - maxPad)) | |
| { | |
| ERROR_MSG("Too much data supplied for encryption"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| osslPadding = RSA_NO_PADDING; | |
| return false; | |
| } | |
| // Guard against too-small modulus for selected padding/hash | |
| if (maxPad && (size_t)RSA_size(rsa) <= maxPad) | |
| { | |
| ERROR_MSG("RSA modulus too small for selected OAEP mode (n=%d, overhead=%zu)", RSA_size(rsa), maxPad); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| // The size of the input data cannot exceed RSA_size - maxPad | |
| if (data.size() > (size_t)RSA_size(rsa) - maxPad) | |
| { | |
| ERROR_MSG("Too much data supplied for encryption"); | |
| EVP_PKEY_CTX_free(ctx); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1276-1285, the length check casts
(RSA_size(rsa) - maxPad) to size_t which underflows when maxPad >= RSA_size(rsa)
(e.g. OAEP-SHA512 on 1024-bit keys); change the logic to compute the difference
in a signed type, explicitly detect non-positive available payload space (<= 0),
log/return a clear error about OAEP overhead exceeding modulus, free ctx and
pkey before returning, and only perform the size_t comparison if the available
byte count is positive.
| size_t outSize = 0; | ||
| int ret = EVP_PKEY_encrypt(ctx, NULL, &outSize, data.const_byte_str(), data.size()); | ||
| if (ret <= 0) { | ||
| ERROR_MSG("RSA public key encryption failed (0x%08X)", ret); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ERR_get_error() for diagnostics; also free ctx/pkey on success to avoid leaks and fix log text.
- Logging “ret” hides the actual OpenSSL error.
- The second error message says “private key encryption” in a public-key function.
- ctx and pkey are leaked on the success path.
Apply:
- int ret = EVP_PKEY_encrypt(ctx, NULL, &outSize, data.const_byte_str(), data.size());
- if (ret <= 0) {
- ERROR_MSG("RSA public key encryption failed (0x%08X)", ret);
+ int ret = EVP_PKEY_encrypt(ctx, NULL, &outSize, data.const_byte_str(), data.size());
+ 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;
}
@@
- ret = EVP_PKEY_encrypt(ctx, &encryptedData[0], &outSize, data.const_byte_str(), data.size());
- if (ret <= 0) {
- ERROR_MSG("RSA private key encryption failed (0x%08X)", ret);
+ ret = EVP_PKEY_encrypt(ctx, &encryptedData[0], &outSize, data.const_byte_str(), data.size());
+ 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;
}
-
- return true;
+ EVP_PKEY_CTX_free(ctx);
+ EVP_PKEY_free(pkey);
+ return true;Also applies to: 1298-1307
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1287-1295 (and similarly 1298-1307),
the error handling currently logs the EVP_PKEY_encrypt return value, mislabels
the operation as "private key encryption", and fails to free ctx and pkey on the
success path; replace the log to include OpenSSL diagnostics by calling
ERR_get_error() and converting it to a readable string (e.g. ERR_error_string or
ERR_error_string_n) in the error message, correct the text to "RSA public key
encryption failed", and ensure EVP_PKEY_CTX_free(ctx) and EVP_PKEY_free(pkey)
are called both on error and on the success path to avoid leaks (apply the same
fixes to the other block at 1298-1307).
| EVP_PKEY *pkey = EVP_PKEY_new(); | ||
| EVP_PKEY_set1_RSA(pkey, rsa); | ||
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | ||
| EVP_PKEY_decrypt_init(ctx); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror encrypt(): check EVP allocations/init in decrypt() too.
Prevent NULL derefs and surface errors earlier.
Apply:
- EVP_PKEY *pkey = EVP_PKEY_new();
- EVP_PKEY_set1_RSA(pkey, rsa);
- EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
- EVP_PKEY_decrypt_init(ctx);
+ EVP_PKEY *pkey = EVP_PKEY_new();
+ if (!pkey || EVP_PKEY_set1_RSA(pkey, rsa) <= 0)
+ {
+ ERROR_MSG("EVP_PKEY allocation/set1_RSA failed (0x%08lX)", ERR_get_error());
+ if (pkey) EVP_PKEY_free(pkey);
+ return false;
+ }
+ EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL);
+ if (!ctx)
+ {
+ ERROR_MSG("EVP_PKEY_CTX_new failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_free(pkey);
+ return false;
+ }
+ if (EVP_PKEY_decrypt_init(ctx) <= 0)
+ {
+ ERROR_MSG("EVP_PKEY_decrypt_init failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey);
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EVP_PKEY *pkey = EVP_PKEY_new(); | |
| EVP_PKEY_set1_RSA(pkey, rsa); | |
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | |
| EVP_PKEY_decrypt_init(ctx); | |
| EVP_PKEY *pkey = EVP_PKEY_new(); | |
| if (!pkey || EVP_PKEY_set1_RSA(pkey, rsa) <= 0) | |
| { | |
| ERROR_MSG("EVP_PKEY allocation/set1_RSA failed (0x%08lX)", ERR_get_error()); | |
| if (pkey) EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new(pkey, NULL); | |
| if (!ctx) | |
| { | |
| ERROR_MSG("EVP_PKEY_CTX_new failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| if (EVP_PKEY_decrypt_init(ctx) <= 0) | |
| { | |
| ERROR_MSG("EVP_PKEY_decrypt_init failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1334-1338, the decrypt() path creates
EVP objects without checking return values which can lead to NULL derefs and
leaks; add explicit checks after EVP_PKEY_new(), EVP_PKEY_set1_RSA(),
EVP_PKEY_CTX_new(), and EVP_PKEY_decrypt_init() and on any failure free any
previously allocated objects (EVP_PKEY_free, EVP_PKEY_CTX_free, etc.),
return/propagate a failure code or error, and log or surface the OpenSSL error
details; ensure the cleanup path mirrors the encrypt() checks so no resources
leak and errors are detected early.
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA1: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()); | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP: | ||
| osslPadding = RSA_PKCS1_OAEP_PADDING; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA224: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()); | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA256: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA384: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()); | ||
| break; | ||
| case AsymMech::RSA_PKCS_OAEP_SHA512: | ||
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | ||
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()); | ||
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check return values when configuring padding/hash for decrypt().
Same rationale as encrypt(): fail fast on misconfiguration.
Example:
- EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING);
- EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384());
- EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384());
+ if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 ||
+ EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()) <= 0 ||
+ EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()) <= 0) {
+ ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error());
+ EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey);
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA1: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP: | |
| osslPadding = RSA_PKCS1_OAEP_PADDING; | |
| case AsymMech::RSA_PKCS_OAEP_SHA224: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA256: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA384: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA512: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()); | |
| break; | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_PADDING); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA1: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha1()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha1()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA224: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha224()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha224()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA256: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA384: | |
| if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()) <= 0 || | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()) <= 0) { | |
| ERROR_MSG("EVP OAEP parameter setup failed (0x%08lX)", ERR_get_error()); | |
| EVP_PKEY_CTX_free(ctx); EVP_PKEY_free(pkey); | |
| return false; | |
| } | |
| - EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| - EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha384()); | |
| - EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha384()); | |
| break; | |
| case AsymMech::RSA_PKCS_OAEP_SHA512: | |
| EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING); | |
| EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha512()); | |
| EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha512()); | |
| break; |
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1342 to 1368, the EVP_PKEY_CTX_set_*
calls that configure RSA padding and OAEP/MGF1 digests for decrypt() are not
checking return values; update the code to check each
EVP_PKEY_CTX_set_rsa_padding, EVP_PKEY_CTX_set_rsa_oaep_md and
EVP_PKEY_CTX_set_rsa_mgf1_md call for success and on failure immediately return
an error (or false) from decrypt() (or throw the appropriate exception) so
misconfiguration fails fast and resources are cleaned up as in encrypt().
| size_t outSize = 0; | ||
| int ret = EVP_PKEY_decrypt(ctx, NULL, &outSize, encryptedData.const_byte_str(), encryptedData.size()); | ||
| if (ret <= 0) { | ||
| ERROR_MSG("RSA private key decryption failed (0x%08X)", ret); | ||
| EVP_PKEY_CTX_free(ctx); | ||
| EVP_PKEY_free(pkey); | ||
|
|
||
| int decSize = RSA_private_decrypt(encryptedData.size(), (unsigned char*) encryptedData.const_byte_str(), &data[0], rsa, osslPadding); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ERR_get_error() for decrypt() diagnostics.
Print the OpenSSL error queue instead of “ret”.
Apply:
- int ret = EVP_PKEY_decrypt(ctx, NULL, &outSize, encryptedData.const_byte_str(), encryptedData.size());
- if (ret <= 0) {
- ERROR_MSG("RSA private key decryption failed (0x%08X)", ret);
+ int ret = EVP_PKEY_decrypt(ctx, NULL, &outSize, encryptedData.const_byte_str(), encryptedData.size());
+ 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;
}
@@
- ret = EVP_PKEY_decrypt(ctx, &data[0], &outSize, encryptedData.const_byte_str(), encryptedData.size());
- if (ret <= 0) {
- ERROR_MSG("RSA private key decryption failed (0x%08X)", ret);
+ ret = EVP_PKEY_decrypt(ctx, &data[0], &outSize, encryptedData.const_byte_str(), encryptedData.size());
+ 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: 1391-1399
🤖 Prompt for AI Agents
In src/lib/crypto/OSSLRSA.cpp around lines 1380-1388 (and similarly 1391-1399),
the code logs only the integer return value from EVP_PKEY_decrypt on error;
replace this with OpenSSL error-queue diagnostics by calling ERR_get_error() (or
a loop to drain multiple errors) and formatting them with ERR_error_string_n (or
ERR_error_string) into a buffer, then pass that human-readable string into
ERROR_MSG; keep existing cleanup (EVP_PKEY_CTX_free/EVP_PKEY_free) and return
false as before.
7 participants
Using EVP interface, we can support different OAEP padding modes, such as OAEP-SHA224, OAEP-SHA256, OAEP-SHA384, OAEP-SHA512. This is my attempt at plumbing the support in - let me know if I'm doing it right. Fixes #474.
Summary by CodeRabbit
New Features
Improvements
Documentation