Skip to content

Support different OAEP modes #818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LawrenceHunter wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Support different OAEP modes #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
566ccaa
Plumb in different hash options for RSA_PKCS_OAEP
borancar Oct 29, 2020
1d89a73
Implement OSSLRSA ops using EVP interface
borancar Oct 29, 2020
70f7eba
Bump OpenSSL version
borancar Oct 30, 2020
5a18cb3
Explain else branch for openssl
borancar Jan 25, 2021
4308e25
Update botan {en,de}crypt with OAEP SHA-2
LawrenceHunter Sep 12, 2025
e12fe72
fix allow mgf-hash mismatch
LawrenceHunter Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ SoftHSM depends on a cryptographic library, Botan or OpenSSL.
Minimum required versions:

- Botan 2.0.0
- OpenSSL 1.0.0
- OpenSSL 1.0.2

If you are using Botan, use at least version 2.6.0. This will improve
the performance when doing public key operations.
Expand Down
6 changes: 4 additions & 2 deletions m4/acx_crypto_backend.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,11 @@ AC_DEFUN([ACX_CRYPTO_BACKEND],[
AC_MSG_RESULT(OpenSSL)

if test "x${enable_fips}" = "xyes"; then
ACX_OPENSSL(1,0,1)
# needed for FIPS compliance, so change only when FIPS requirements change
ACX_OPENSSL(1,0,2)
else
ACX_OPENSSL(1,0,0)
# increase this as features from newer versions needed
ACX_OPENSSL(1,0,2)
fi

CRYPTO_INCLUDES=$OPENSSL_INCLUDES
Expand Down
131 changes: 113 additions & 18 deletions src/lib/SoftHSM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2478,7 +2478,27 @@ CK_RV SoftHSM::AsymEncryptInit(CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMec
if (rv != CKR_OK)
return rv;

mechanism = AsymMech::RSA_PKCS_OAEP;
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;
}

isRSA = true;
break;
default:
Expand Down Expand Up @@ -3214,18 +3234,38 @@ CK_RV SoftHSM::AsymDecryptInit(CK_SESSION_HANDLE hSession, CK_MECHANISM_PTR pMec
DEBUG_MSG("pParameter must be of type CK_RSA_PKCS_OAEP_PARAMS");
return CKR_ARGUMENTS_BAD;
}
if (CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg != CKM_SHA_1)
{
DEBUG_MSG("hashAlg must be CKM_SHA_1");
return CKR_ARGUMENTS_BAD;

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;
Comment on lines +3238 to 3266
Copy link

@coderabbitai coderabbitai bot Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Contributor

@bukka bukka Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good idea with minor thing that pMechanism->pParameter does not need to be checked either as they are checked in MechParamCheckRSAPKCSOAEP

Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

mechanism = AsymMech::RSA_PKCS_OAEP;
isRSA = true;
break;
default:
Expand Down Expand Up @@ -6474,11 +6514,40 @@ CK_RV SoftHSM::WrapKeyAsym
break;

case CKM_RSA_PKCS_OAEP:
mech = AsymMech::RSA_PKCS_OAEP;
// SHA-1 is the only supported option
// PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen
if (keydata.size() > modulus_length - 2 - 2 * 160 / 8)
return CKR_KEY_SIZE_RANGE;
switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
case CKM_SHA_1:
mech = AsymMech::RSA_PKCS_OAEP_SHA1;
// PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen
if (keydata.size() > modulus_length - 2 - 2 * 160 / 8)
return CKR_KEY_SIZE_RANGE;
break;
case CKM_SHA224:
mech = AsymMech::RSA_PKCS_OAEP_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;
// PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen
if (keydata.size() > modulus_length - 2 - 2 * 256 / 8)
return CKR_KEY_SIZE_RANGE;
break;
case CKM_SHA384:
mech = AsymMech::RSA_PKCS_OAEP_SHA384;
// PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen
if (keydata.size() > modulus_length - 2 - 2 * 384 / 8)
return CKR_KEY_SIZE_RANGE;
break;
case CKM_SHA512:
mech = AsymMech::RSA_PKCS_OAEP_SHA512;
// PKCS#11 2.40 draft 2 section 2.1.8: input length <= k-2-2hashLen
if (keydata.size() > modulus_length - 2 - 2 * 512 / 8)
return CKR_KEY_SIZE_RANGE;
break;
default:
return CKR_MECHANISM_INVALID;
}
break;

default:
Expand Down Expand Up @@ -7044,7 +7113,25 @@ CK_RV SoftHSM::UnwrapKeyAsym

case CKM_RSA_PKCS_OAEP:
algo = AsymAlgo::RSA;
mode = AsymMech::RSA_PKCS_OAEP;
switch(CK_RSA_PKCS_OAEP_PARAMS_PTR(pMechanism->pParameter)->hashAlg) {
case CKM_SHA_1:
mode = AsymMech::RSA_PKCS_OAEP_SHA1;
break;
case CKM_SHA224:
mode = AsymMech::RSA_PKCS_OAEP_SHA224;
break;
case CKM_SHA256:
mode = AsymMech::RSA_PKCS_OAEP_SHA256;
break;
case CKM_SHA384:
mode = AsymMech::RSA_PKCS_OAEP_SHA384;
break;
case CKM_SHA512:
mode = AsymMech::RSA_PKCS_OAEP_SHA512;
break;
default:
return CKR_MECHANISM_INVALID;
}
break;

default:
Expand Down Expand Up @@ -13175,14 +13262,22 @@ CK_RV SoftHSM::MechParamCheckRSAPKCSOAEP(CK_MECHANISM_PTR pMechanism)
}

CK_RSA_PKCS_OAEP_PARAMS_PTR params = (CK_RSA_PKCS_OAEP_PARAMS_PTR)pMechanism->pParameter;
if (params->hashAlg != CKM_SHA_1)
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;
}
Comment on lines +13265 to 13282
Copy link

@coderabbitai coderabbitai bot Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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;
}

Copy link
Contributor

@bukka bukka Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense

Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if (params->source != CKZ_DATA_SPECIFIED)
Expand Down
6 changes: 5 additions & 1 deletion src/lib/crypto/AsymmetricAlgorithm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,11 @@ bool AsymmetricAlgorithm::isWrappingMech(AsymMech::Type padding)
{
case AsymMech::RSA:
case AsymMech::RSA_PKCS:
case AsymMech::RSA_PKCS_OAEP:
case AsymMech::RSA_PKCS_OAEP_SHA1:
case AsymMech::RSA_PKCS_OAEP_SHA224:
case AsymMech::RSA_PKCS_OAEP_SHA256:
case AsymMech::RSA_PKCS_OAEP_SHA384:
case AsymMech::RSA_PKCS_OAEP_SHA512:
return true;

default:
Expand Down
6 changes: 5 additions & 1 deletion src/lib/crypto/AsymmetricAlgorithm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ struct AsymMech
RSA,
RSA_MD5_PKCS,
RSA_PKCS,
RSA_PKCS_OAEP,
RSA_PKCS_OAEP_SHA1,
RSA_PKCS_OAEP_SHA224,
RSA_PKCS_OAEP_SHA256,
RSA_PKCS_OAEP_SHA384,
RSA_PKCS_OAEP_SHA512,
RSA_SHA1_PKCS,
RSA_SHA224_PKCS,
RSA_SHA256_PKCS,
Expand Down
32 changes: 28 additions & 4 deletions src/lib/crypto/BotanRSA.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,8 +761,20 @@ bool BotanRSA::encrypt(PublicKey* publicKey, const ByteString& data,
case AsymMech::RSA_PKCS:
eme = "PKCS1v15";
break;
case AsymMech::RSA_PKCS_OAEP:
eme = "EME1(SHA-160)";
case AsymMech::RSA_PKCS_OAEP_SHA1:
eme = "OAEP(SHA-160)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA224:
eme = "OAEP(SHA-224)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA256:
eme = "OAEP(SHA-256)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA384:
eme = "OAEP(SHA-384)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA512:
eme = "OAEP(SHA-512)";
break;
case AsymMech::RSA:
eme = "Raw";
Expand Down Expand Up @@ -840,8 +852,20 @@ bool BotanRSA::decrypt(PrivateKey* privateKey, const ByteString& encryptedData,
case AsymMech::RSA_PKCS:
eme = "PKCS1v15";
break;
case AsymMech::RSA_PKCS_OAEP:
eme = "EME1(SHA-160)";
case AsymMech::RSA_PKCS_OAEP_SHA1:
eme = "OAEP(SHA-160)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA224:
eme = "OAEP(SHA-224)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA256:
eme = "OAEP(SHA-256)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA384:
eme = "OAEP(SHA-384)";
break;
case AsymMech::RSA_PKCS_OAEP_SHA512:
eme = "OAEP(SHA-512)";
break;
case AsymMech::RSA:
eme = "Raw";
Expand Down
Loading
Loading