From f80dc128959322c42be63f93ea9d8c18a257067f Mon Sep 17 00:00:00 2001 From: Vincent Landgraf Date: Fri, 29 Oct 2021 14:57:02 +0200 Subject: [PATCH 1/8] adds support for key wrap based on des3 https://github.com/opendnssec/SoftHSMv2/issues/405 (cherry picked from commit 40856c7d148d94245991cf24016cd65e7222eb1e) --- src/lib/SoftHSM.cpp | 7 +++++-- src/lib/crypto/BotanDES.cpp | 3 +++ src/lib/crypto/OSSLDES.cpp | 19 +++++++++++++++++-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 62c695b99..9c13b4f19 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6612,6 +6612,9 @@ CK_RV SoftHSM::C_WrapKey pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; + case CKM_DES3_CBC: + case CKM_DES3_CBC_PAD: + break; default: return CKR_MECHANISM_INVALID; } @@ -6652,8 +6655,8 @@ CK_RV SoftHSM::C_WrapKey return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - if (pMechanism->mechanism == CKM_DES3_CBC && (wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + if ((pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_CBC_PAD) && ((wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3))) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; // Check if the wrapping key can be used for wrapping diff --git a/src/lib/crypto/BotanDES.cpp b/src/lib/crypto/BotanDES.cpp index 393cf4dc9..9d3ebaa2d 100644 --- a/src/lib/crypto/BotanDES.cpp +++ b/src/lib/crypto/BotanDES.cpp @@ -61,12 +61,15 @@ std::string BotanDES::getCipher() const switch (currentKey->getBitLen()) { case 56: + case 64: // People shouldn't really be using 56-bit DES keys, generate a warning DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); algo = "DES"; break; case 112: + case 128: case 168: + case 192: algo = "TripleDES"; break; default: diff --git a/src/lib/crypto/OSSLDES.cpp b/src/lib/crypto/OSSLDES.cpp index 4fb56b5eb..4b6acf792 100644 --- a/src/lib/crypto/OSSLDES.cpp +++ b/src/lib/crypto/OSSLDES.cpp @@ -57,9 +57,12 @@ const EVP_CIPHER* OSSLDES::getCipher() const if ( #ifndef WITH_FIPS (currentKey->getBitLen() != 56) && + (currentKey->getBitLen() != 64) && #endif (currentKey->getBitLen() != 112) && - (currentKey->getBitLen() != 168)) + (currentKey->getBitLen() != 128) && + (currentKey->getBitLen() != 168) && + (currentKey->getBitLen() != 192)) { ERROR_MSG("Invalid DES currentKey length (%d bits)", currentKey->getBitLen()); @@ -67,7 +70,7 @@ const EVP_CIPHER* OSSLDES::getCipher() const } // People shouldn't really be using 56-bit DES keys, generate a warning - if (currentKey->getBitLen() == 56) + if (currentKey->getBitLen() == 56 || currentKey->getBitLen() == 64) { DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); } @@ -78,10 +81,13 @@ const EVP_CIPHER* OSSLDES::getCipher() const switch(currentKey->getBitLen()) { case 56: + case 64: return EVP_des_cbc(); case 112: + case 128: return EVP_des_ede_cbc(); case 168: + case 192: return EVP_des_ede3_cbc(); }; } @@ -90,10 +96,13 @@ const EVP_CIPHER* OSSLDES::getCipher() const switch(currentKey->getBitLen()) { case 56: + case 64: return EVP_des_ecb(); case 112: + case 128: return EVP_des_ede_ecb(); case 168: + case 192: return EVP_des_ede3_ecb(); }; } @@ -102,10 +111,13 @@ const EVP_CIPHER* OSSLDES::getCipher() const switch(currentKey->getBitLen()) { case 56: + case 64: return EVP_des_ofb(); case 112: + case 128: return EVP_des_ede_ofb(); case 168: + case 192: return EVP_des_ede3_ofb(); }; } @@ -114,10 +126,13 @@ const EVP_CIPHER* OSSLDES::getCipher() const switch(currentKey->getBitLen()) { case 56: + case 64: return EVP_des_cfb(); case 112: + case 128: return EVP_des_ede_cfb(); case 168: + case 192: return EVP_des_ede3_cfb(); }; } From 7efd531709e15f7491a54b464b86a4375b8dabd3 Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Mon, 25 Aug 2025 10:08:40 +0300 Subject: [PATCH 2/8] Added support for CKM_DES3_ECB wrap/unwrap functionality. Enabled the use of CKA_MODIFIABLE and CKA_VALUE_LEN in the unwrap template. Based on PR #657 https://github.com/softhsm/SoftHSMv2/pull/657 --- src/lib/P11Attributes.cpp | 2 +- src/lib/SoftHSM.cpp | 176 ++++++++++++++++++++++++-------------- 2 files changed, 115 insertions(+), 63 deletions(-) diff --git a/src/lib/P11Attributes.cpp b/src/lib/P11Attributes.cpp index fc9ab0041..1ab832820 100644 --- a/src/lib/P11Attributes.cpp +++ b/src/lib/P11Attributes.cpp @@ -411,7 +411,7 @@ CK_RV P11Attribute::update(Token* token, bool isPrivate, CK_VOID_PTR pValue, CK_ // given non-Cryptoki attribute is read-only is obviously outside the scope of Cryptoki. // Attributes cannot be changed if CKA_MODIFIABLE is set to false - if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE) { + if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE && op != OBJECT_OP_UNWRAP) { ERROR_MSG("An object is with CKA_MODIFIABLE set to false is not modifiable"); return CKR_ATTRIBUTE_READ_ONLY; } diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 9c13b4f19..558882f77 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6269,6 +6269,7 @@ CK_RV SoftHSM::WrapKeySym // Get the symmetric algorithm matching the mechanism SymAlgo::Type algo = SymAlgo::Unknown; SymWrap::Type mode = SymWrap::Unknown; + SymMode::Type sym_mode = SymMode::Unknown; size_t bb = 8; size_t blocksize = 0; auto wrappedlen = keydata.size(); @@ -6291,22 +6292,33 @@ CK_RV SoftHSM::WrapKeySym #endif case CKM_AES_CBC: algo = SymAlgo::AES; + sym_mode = SymMode::CBC; break; case CKM_AES_CBC_PAD: blocksize = 16; wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::AES; - break; - - case CKM_DES3_CBC: - algo = SymAlgo::DES3; + sym_mode = SymMode::CBC; break; case CKM_DES3_CBC_PAD: - blocksize = 8; + blocksize = 8; wrappedlen = RFC5652Pad(keydata, blocksize); + algo = SymAlgo::DES3; + sym_mode = SymMode::CBC; + break; + + case CKM_DES3_CBC: + blocksize = 8; + algo = SymAlgo::DES3; + sym_mode = SymMode::CBC; + break; + + case CKM_DES3_ECB: + blocksize = 0; algo = SymAlgo::DES3; + sym_mode = SymMode::ECB; break; default: @@ -6333,13 +6345,14 @@ CK_RV SoftHSM::WrapKeySym switch(pMechanism->mechanism) { case CKM_AES_CBC: - case CKM_AES_CBC_PAD: + case CKM_AES_CBC_PAD: case CKM_DES3_CBC: - case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC_PAD: + case CKM_DES3_ECB: iv.resize(blocksize); memcpy(&iv[0], pMechanism->pParameter, blocksize); - if (!cipher->encryptInit(wrappingkey, SymMode::CBC, iv, false)) + if (!cipher->encryptInit(wrappingkey, sym_mode, iv, false)) { cipher->recycleKey(wrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); @@ -6612,6 +6625,10 @@ CK_RV SoftHSM::C_WrapKey pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; + case CKM_DES3_ECB: + if (pMechanism->ulParameterLen != 0) + return CKR_ARGUMENTS_BAD; // no IV for ECB; allow NULL or non-NULL pointer when length==0 + break; case CKM_DES3_CBC: case CKM_DES3_CBC_PAD: break; @@ -6655,9 +6672,11 @@ CK_RV SoftHSM::C_WrapKey return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - if ((pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_CBC_PAD) && ((wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && - wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3))) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + if ((pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_CBC_PAD || pMechanism->mechanism == CKM_DES3_ECB) && + ((wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3))) { + return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; + } // Check if the wrapping key can be used for wrapping if (wrapKey->getBooleanValue(CKA_WRAP, false) == false) @@ -6868,6 +6887,7 @@ CK_RV SoftHSM::UnwrapKeySym // Get the symmetric algorithm matching the mechanism SymAlgo::Type algo = SymAlgo::Unknown; SymWrap::Type mode = SymWrap::Unknown; + SymMode::Type sym_mode = SymMode::Unknown; size_t bb = 8; size_t blocksize = 0; @@ -6884,16 +6904,25 @@ CK_RV SoftHSM::UnwrapKeySym mode = SymWrap::AES_KEYWRAP_PAD; break; #endif - case CKM_AES_CBC_PAD: + case CKM_AES_CBC_PAD: algo = SymAlgo::AES; + sym_mode = SymMode::CBC; blocksize = 16; break; - - case CKM_DES3_CBC_PAD: + + case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC: algo = SymAlgo::DES3; + sym_mode = SymMode::CBC; blocksize = 8; - break; - + break; + + case CKM_DES3_ECB: + algo = SymAlgo::DES3; + sym_mode = SymMode::ECB; + blocksize = 0; + break; + default: return CKR_MECHANISM_INVALID; } @@ -6919,39 +6948,39 @@ CK_RV SoftHSM::UnwrapKeySym switch(pMechanism->mechanism) { - case CKM_AES_CBC_PAD: - case CKM_DES3_CBC_PAD: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); - - if (!cipher->decryptInit(unwrappingkey, SymMode::CBC, iv, false)) - { - cipher->recycleKey(unwrappingkey); - CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_MECHANISM_INVALID; - } - if (!cipher->decryptUpdate(wrapped, keydata)) - { - cipher->recycleKey(unwrappingkey); - CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; - } - // Finalize encryption - if (!cipher->decryptFinal(decryptedFinal)) - { - cipher->recycleKey(unwrappingkey); - CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; - } - keydata += decryptedFinal; + case CKM_AES_CBC_PAD: + case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC: + case CKM_DES3_ECB: + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize); - if(!RFC5652Unpad(keydata,blocksize)) - { - cipher->recycleKey(unwrappingkey); - CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; // TODO should be another error - } - break; + if (!cipher->decryptInit(unwrappingkey, SymMode::CBC, iv, false)) { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_MECHANISM_INVALID; + } + if (!cipher->decryptUpdate(wrapped, keydata)) { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_GENERAL_ERROR; + } + // Finalize encryption + if (!cipher->decryptFinal(decryptedFinal)) { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_GENERAL_ERROR; + } + keydata += decryptedFinal; + + if (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) { + if (!RFC5652Unpad(keydata, blocksize)) { + cipher->recycleKey(unwrappingkey); + CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return CKR_GENERAL_ERROR; // TODO should be another error + } + } + break; default: // Unwrap the key @@ -7230,20 +7259,23 @@ CK_RV SoftHSM::C_UnwrapKey return rv; break; - case CKM_AES_CBC_PAD: + case CKM_AES_CBC_PAD: // TODO check block length - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != 16) + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; - case CKM_DES3_CBC_PAD: + case CKM_DES3_CBC_PAD: // TODO check block length - if (pMechanism->pParameter == NULL_PTR || - pMechanism->ulParameterLen != 8) + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; break; - + + case CKM_DES3_ECB: + if (pMechanism->ulParameterLen != 0) + return CKR_ARGUMENTS_BAD; // ECB takes no IV; allow NULL or non-NULL pointer when length==0 + break; + default: return CKR_MECHANISM_INVALID; } @@ -7282,12 +7314,14 @@ CK_RV SoftHSM::C_UnwrapKey if ((pMechanism->mechanism == CKM_RSA_PKCS || pMechanism->mechanism == CKM_RSA_PKCS_OAEP || pMechanism->mechanism == CKM_RSA_AES_KEY_WRAP) && unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_RSA) return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT; - if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - if (pMechanism->mechanism == CKM_DES3_CBC && (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 || - unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) - return CKR_WRAPPING_KEY_TYPE_INCONSISTENT; - + if ((pMechanism->mechanism == CKM_AES_CBC || pMechanism->mechanism == CKM_AES_CBC_PAD) && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_AES) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT; + if ((pMechanism->mechanism == CKM_DES3_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC || pMechanism->mechanism == CKM_DES3_ECB) && + (unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 && + unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)) + return CKR_UNWRAPPING_KEY_TYPE_INCONSISTENT; + // Check if the unwrapping key can be used for unwrapping if (unwrapKey->getBooleanValue(CKA_UNWRAP, false) == false) return CKR_KEY_FUNCTION_NOT_PERMITTED; @@ -7337,7 +7371,11 @@ CK_RV SoftHSM::C_UnwrapKey }; CK_ULONG secretAttribsCount = 4; - // Add the additional + // Capture optional CKA_VALUE_LEN from the template (for compatibility) + CK_ULONG requestedValueLen = 0; + bool haveRequestedValueLen = false; + + // Add the additional attributes, skipping read-only for unwrap and capturing CKA_VALUE_LEN if (ulCount > (maxAttribs - secretAttribsCount)) return CKR_TEMPLATE_INCONSISTENT; for (CK_ULONG i = 0; i < ulCount; ++i) @@ -7349,6 +7387,14 @@ CK_RV SoftHSM::C_UnwrapKey case CKA_PRIVATE: case CKA_KEY_TYPE: continue; + case CKA_VALUE_LEN: + // Accept a caller-provided length for compatibility, but do not set it here; we will verify after unwrap + if (pTemplate[i].ulValueLen != sizeof(CK_ULONG)) { + return CKR_TEMPLATE_INCONSISTENT; + } + requestedValueLen = *(CK_ULONG *) pTemplate[i].pValue; + haveRequestedValueLen = true; + continue; default: secretAttribs[secretAttribsCount++] = pTemplate[i]; } @@ -7423,6 +7469,12 @@ CK_RV SoftHSM::C_UnwrapKey return rv; } + // If the caller supplied CKA_VALUE_LEN, ensure it matches the unwrapped key length + if (haveRequestedValueLen && keydata.size() != requestedValueLen) + { + return CKR_TEMPLATE_INCONSISTENT; + } + // Create the secret object using C_CreateObject rv = this->CreateObject(hSession, secretAttribs, secretAttribsCount, hKey, OBJECT_OP_UNWRAP); From 5d712e3729dbe8b9ba1d70170498c52dce73fdc0 Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Mon, 25 Aug 2025 10:47:56 +0300 Subject: [PATCH 3/8] Addressed the CodeRabbit bot issues. --- src/lib/SoftHSM.cpp | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 558882f77..07679c6e8 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6349,9 +6349,13 @@ CK_RV SoftHSM::WrapKeySym case CKM_DES3_CBC: case CKM_DES3_CBC_PAD: case CKM_DES3_ECB: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); - + if (blocksize > 0) { + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize); + } else { + iv.wipe(); + } + if (!cipher->encryptInit(wrappingkey, sym_mode, iv, false)) { cipher->recycleKey(wrappingkey); @@ -6631,6 +6635,8 @@ CK_RV SoftHSM::C_WrapKey break; case CKM_DES3_CBC: case CKM_DES3_CBC_PAD: + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; break; default: return CKR_MECHANISM_INVALID; @@ -6952,10 +6958,14 @@ CK_RV SoftHSM::UnwrapKeySym case CKM_DES3_CBC_PAD: case CKM_DES3_CBC: case CKM_DES3_ECB: - iv.resize(blocksize); - memcpy(&iv[0], pMechanism->pParameter, blocksize); + if (blocksize > 0) { + iv.resize(blocksize); + memcpy(&iv[0], pMechanism->pParameter, blocksize); + } else { + iv.wipe(); + } - if (!cipher->decryptInit(unwrappingkey, SymMode::CBC, iv, false)) { + if (!cipher->decryptInit(unwrappingkey, sym_mode, iv, false)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); return CKR_MECHANISM_INVALID; @@ -6977,7 +6987,7 @@ CK_RV SoftHSM::UnwrapKeySym if (!RFC5652Unpad(keydata, blocksize)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return CKR_GENERAL_ERROR; // TODO should be another error + return CKR_WRAPPED_KEY_INVALID; } } break; @@ -7271,6 +7281,11 @@ CK_RV SoftHSM::C_UnwrapKey return CKR_ARGUMENTS_BAD; break; + case CKM_DES3_CBC: + if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) + return CKR_ARGUMENTS_BAD; + break; + case CKM_DES3_ECB: if (pMechanism->ulParameterLen != 0) return CKR_ARGUMENTS_BAD; // ECB takes no IV; allow NULL or non-NULL pointer when length==0 From 5cd5a551454dc9ffc6acb466500bba6b715cef5d Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Mon, 25 Aug 2025 12:09:35 +0300 Subject: [PATCH 4/8] Addressed more CodeRabbit bot issues. --- src/lib/P11Attributes.cpp | 5 +++-- src/lib/SoftHSM.cpp | 11 +++++++++-- src/lib/crypto/BotanDES.cpp | 4 ++-- src/lib/crypto/OSSLDES.cpp | 9 ++++++--- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/lib/P11Attributes.cpp b/src/lib/P11Attributes.cpp index 1ab832820..1c3640a82 100644 --- a/src/lib/P11Attributes.cpp +++ b/src/lib/P11Attributes.cpp @@ -410,9 +410,10 @@ CK_RV P11Attribute::update(Token* token, bool isPrivate, CK_VOID_PTR pValue, CK_ // combinations of other attributes) for a particular library and token. Whether or not a // given non-Cryptoki attribute is read-only is obviously outside the scope of Cryptoki. - // Attributes cannot be changed if CKA_MODIFIABLE is set to false + // Attributes cannot be changed if CKA_MODIFIABLE is false. + // Note: During CREATE/GENERATE/UNWRAP the object is in creation; allow setting attributes in the same op. if (!isModifiable() && op != OBJECT_OP_GENERATE && op != OBJECT_OP_CREATE && op != OBJECT_OP_UNWRAP) { - ERROR_MSG("An object is with CKA_MODIFIABLE set to false is not modifiable"); + ERROR_MSG("An object with CKA_MODIFIABLE set to false is not modifiable"); return CKR_ATTRIBUTE_READ_ONLY; } diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 07679c6e8..f3005eee4 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6307,18 +6307,22 @@ CK_RV SoftHSM::WrapKeySym wrappedlen = RFC5652Pad(keydata, blocksize); algo = SymAlgo::DES3; sym_mode = SymMode::CBC; + bb = 7; break; case CKM_DES3_CBC: blocksize = 8; algo = SymAlgo::DES3; sym_mode = SymMode::CBC; + bb = 7; break; case CKM_DES3_ECB: + // ECB mode has no IV; keep blocksize=0 so iv remains empty blocksize = 0; algo = SymAlgo::DES3; sym_mode = SymMode::ECB; + bb = 7; break; default: @@ -6921,12 +6925,15 @@ CK_RV SoftHSM::UnwrapKeySym algo = SymAlgo::DES3; sym_mode = SymMode::CBC; blocksize = 8; + bb = 7; break; case CKM_DES3_ECB: algo = SymAlgo::DES3; sym_mode = SymMode::ECB; + // ECB mode has no IV; keep blocksize=0 so iv remains empty blocksize = 0; + bb = 7; break; default: @@ -7484,8 +7491,8 @@ CK_RV SoftHSM::C_UnwrapKey return rv; } - // If the caller supplied CKA_VALUE_LEN, ensure it matches the unwrapped key length - if (haveRequestedValueLen && keydata.size() != requestedValueLen) + // If provided, enforce CKA_VALUE_LEN only for secret keys + if (objClass == CKO_SECRET_KEY && haveRequestedValueLen && keydata.size() != requestedValueLen) { return CKR_TEMPLATE_INCONSISTENT; } diff --git a/src/lib/crypto/BotanDES.cpp b/src/lib/crypto/BotanDES.cpp index 9d3ebaa2d..0fc31353b 100644 --- a/src/lib/crypto/BotanDES.cpp +++ b/src/lib/crypto/BotanDES.cpp @@ -62,8 +62,8 @@ std::string BotanDES::getCipher() const { case 56: case 64: - // People shouldn't really be using 56-bit DES keys, generate a warning - DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); + // Single-DES (effective 56-bit) is weak; warn irrespective of representation + DEBUG_MSG("CAUTION: use of single-DES keys (effective 56-bit) is not recommended!"); algo = "DES"; break; case 112: diff --git a/src/lib/crypto/OSSLDES.cpp b/src/lib/crypto/OSSLDES.cpp index 4b6acf792..053c87854 100644 --- a/src/lib/crypto/OSSLDES.cpp +++ b/src/lib/crypto/OSSLDES.cpp @@ -53,7 +53,10 @@ const EVP_CIPHER* OSSLDES::getCipher() const { if (currentKey == NULL) return NULL; - // Check currentKey bit length; 3DES only supports 56-bit, 112-bit or 168-bit keys + // Accept both effective (without parity) and parity-included lengths: + // DES: 56 or 64 bits (64 includes parity) + // 2-key 3DES: 112 or 128 bits (128 includes parity) + // 3-key 3DES: 168 or 192 bits (192 includes parity) if ( #ifndef WITH_FIPS (currentKey->getBitLen() != 56) && @@ -69,10 +72,10 @@ const EVP_CIPHER* OSSLDES::getCipher() const return NULL; } - // People shouldn't really be using 56-bit DES keys, generate a warning + // Single-DES (effective 56-bit) is weak; warn irrespective of representation if (currentKey->getBitLen() == 56 || currentKey->getBitLen() == 64) { - DEBUG_MSG("CAUTION: use of 56-bit DES keys is not recommended!"); + DEBUG_MSG("CAUTION: use of single-DES keys (effective 56-bit) is not recommended!"); } // Determine the cipher mode From 2245eb17650f57fcebbcbb38b3fbb3127a076f43 Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Mon, 25 Aug 2025 12:30:28 +0300 Subject: [PATCH 5/8] Addressed more CodeRabbit bot issues. --- src/lib/SoftHSM.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index f3005eee4..997f7cf36 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -1138,13 +1138,12 @@ CK_RV SoftHSM::C_GetMechanismInfo(CK_SLOT_ID slotID, CK_MECHANISM_TYPE type, CK_ /* FALLTHROUGH */ #endif case CKM_DES3_CBC: - pInfo->flags |= CKF_WRAP; /* FALLTHROUGH */ case CKM_DES3_ECB: // Key size is not in use pInfo->ulMinKeySize = 0; pInfo->ulMaxKeySize = 0; - pInfo->flags |= CKF_ENCRYPT | CKF_DECRYPT; + pInfo->flags |= CKF_ENCRYPT | CKF_DECRYPT | CKF_WRAP | CKF_UNWRAP; break; case CKM_DES3_CMAC: // Key size is not in use @@ -6293,6 +6292,7 @@ CK_RV SoftHSM::WrapKeySym case CKM_AES_CBC: algo = SymAlgo::AES; sym_mode = SymMode::CBC; + blocksize = 16; break; case CKM_AES_CBC_PAD: @@ -6982,10 +6982,13 @@ CK_RV SoftHSM::UnwrapKeySym CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); return CKR_GENERAL_ERROR; } - // Finalize encryption + // Finalize decryption if (!cipher->decryptFinal(decryptedFinal)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); + return (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) + ? CKR_WRAPPED_KEY_INVALID + : CKR_GENERAL_ERROR; return CKR_GENERAL_ERROR; } keydata += decryptedFinal; From 045d4fb5f4fedf90191c9a50a8e1b7f737da611d Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Mon, 25 Aug 2025 12:45:47 +0300 Subject: [PATCH 6/8] Addressed more CodeRabbit bot issues. --- src/lib/SoftHSM.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index 997f7cf36..b8c761a0d 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6989,7 +6989,6 @@ CK_RV SoftHSM::UnwrapKeySym return (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) ? CKR_WRAPPED_KEY_INVALID : CKR_GENERAL_ERROR; - return CKR_GENERAL_ERROR; } keydata += decryptedFinal; @@ -7280,13 +7279,11 @@ CK_RV SoftHSM::C_UnwrapKey break; case CKM_AES_CBC_PAD: - // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; break; case CKM_DES3_CBC_PAD: - // TODO check block length if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; break; @@ -12124,7 +12121,7 @@ CK_RV SoftHSM::deriveSymmetric // attributes set to CK_TRUE bool bNeverExtractable = baseKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false) && otherKey->getBooleanValue(CKA_NEVER_EXTRACTABLE, false); - bOK = bOK && osobject->setAttribute(CKA_ALWAYS_SENSITIVE, bNeverExtractable); + bOK = bOK && osobject->setAttribute(CKA_NEVER_EXTRACTABLE, bNeverExtractable); } else if (pMechanism->mechanism == CKM_CONCATENATE_BASE_AND_DATA || pMechanism->mechanism == CKM_CONCATENATE_DATA_AND_BASE) From 00c91b3c5ae5d75ba2797a028e362bde7446da2c Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Tue, 26 Aug 2025 13:55:03 +0300 Subject: [PATCH 7/8] Added key length checks. --- src/lib/SoftHSM.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index b8c761a0d..de5024d99 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6311,6 +6311,8 @@ CK_RV SoftHSM::WrapKeySym break; case CKM_DES3_CBC: + if (wrappedlen % 8) != 0) + return CKR_KEY_SIZE_RANGE; blocksize = 8; algo = SymAlgo::DES3; sym_mode = SymMode::CBC; @@ -6319,6 +6321,8 @@ CK_RV SoftHSM::WrapKeySym case CKM_DES3_ECB: // ECB mode has no IV; keep blocksize=0 so iv remains empty + if (wrappedlen % 8) != 0) + return CKR_KEY_SIZE_RANGE; blocksize = 0; algo = SymAlgo::DES3; sym_mode = SymMode::ECB; @@ -6986,9 +6990,7 @@ CK_RV SoftHSM::UnwrapKeySym if (!cipher->decryptFinal(decryptedFinal)) { cipher->recycleKey(unwrappingkey); CryptoFactory::i()->recycleSymmetricAlgorithm(cipher); - return (pMechanism->mechanism == CKM_AES_CBC_PAD || pMechanism->mechanism == CKM_DES3_CBC_PAD) - ? CKR_WRAPPED_KEY_INVALID - : CKR_GENERAL_ERROR; + return CKR_WRAPPED_KEY_INVALID; } keydata += decryptedFinal; @@ -7286,16 +7288,22 @@ CK_RV SoftHSM::C_UnwrapKey case CKM_DES3_CBC_PAD: if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; + if (ulWrappedKeyLen == 0 || (ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_CBC: if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 8) return CKR_ARGUMENTS_BAD; + if ((ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_ECB: if (pMechanism->ulParameterLen != 0) return CKR_ARGUMENTS_BAD; // ECB takes no IV; allow NULL or non-NULL pointer when length==0 + if ((ulWrappedKeyLen % 8) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; default: From e86110b6e45aafd2955b880940d3481e84228453 Mon Sep 17 00:00:00 2001 From: Jukka Kangas Date: Tue, 26 Aug 2025 14:12:17 +0300 Subject: [PATCH 8/8] Fixed len checks. --- src/lib/SoftHSM.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/lib/SoftHSM.cpp b/src/lib/SoftHSM.cpp index de5024d99..28dcfc90e 100644 --- a/src/lib/SoftHSM.cpp +++ b/src/lib/SoftHSM.cpp @@ -6293,6 +6293,8 @@ CK_RV SoftHSM::WrapKeySym algo = SymAlgo::AES; sym_mode = SymMode::CBC; blocksize = 16; + if ((wrappedlen % 16) != 0) + return CKR_KEY_SIZE_RANGE; break; case CKM_AES_CBC_PAD: @@ -6311,7 +6313,7 @@ CK_RV SoftHSM::WrapKeySym break; case CKM_DES3_CBC: - if (wrappedlen % 8) != 0) + if ((wrappedlen % 8) != 0) return CKR_KEY_SIZE_RANGE; blocksize = 8; algo = SymAlgo::DES3; @@ -6321,7 +6323,7 @@ CK_RV SoftHSM::WrapKeySym case CKM_DES3_ECB: // ECB mode has no IV; keep blocksize=0 so iv remains empty - if (wrappedlen % 8) != 0) + if ((wrappedlen % 8) != 0) return CKR_KEY_SIZE_RANGE; blocksize = 0; algo = SymAlgo::DES3; @@ -7283,6 +7285,8 @@ CK_RV SoftHSM::C_UnwrapKey case CKM_AES_CBC_PAD: if (pMechanism->pParameter == NULL_PTR || pMechanism->ulParameterLen != 16) return CKR_ARGUMENTS_BAD; + if (ulWrappedKeyLen == 0 || (ulWrappedKeyLen % 16) != 0) + return CKR_WRAPPED_KEY_LEN_RANGE; break; case CKM_DES3_CBC_PAD: