Add support for wrapping/unwrapping using CKM_AES_GCM mechanism#880
Open
fpl-switstack wants to merge 1 commit into
Open
Add support for wrapping/unwrapping using CKM_AES_GCM mechanism#880fpl-switstack wants to merge 1 commit into
fpl-switstack wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds AES-GCM key wrapping and unwrapping support to SoftHSM by extending the mechanism reporting, symmetric encryption/decryption, and key wrap/unwrap functions. Includes validation of GCM parameters (IV, AAD, tag bits), test infrastructure updates for GCM metadata, and comprehensive test coverage with new GCM-wrapped test vectors. ChangesAES-GCM Wrapping and Unwrapping Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
fpl-switstack
force-pushed
the
feature/aes-gcm-wrap-unwrap
branch
from
f52ec1f to
e5560b8
Compare
fpl-switstack
force-pushed
the
feature/aes-gcm-wrap-unwrap
branch
from
e5560b8 to
113a842
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/SoftHSM.cpp (3)
1218-1223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't advertise wrap/unwrap on
CKM_AES_ECBandCKM_AES_CTR.This shared AES flag assignment now gives
CKF_WRAP | CKF_UNWRAPtoCKM_AES_ECBandCKM_AES_CTRas well asCKM_AES_GCM, butC_WrapKey/C_UnwrapKeystill reject ECB/CTR.C_GetMechanismInfowill report capabilities the module does not actually implement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/SoftHSM.cpp` around lines 1218 - 1223, The code is incorrectly setting CKF_WRAP|CKF_UNWRAP for CKM_AES_ECB and CKM_AES_CTR; update the switch in SoftHSM.cpp handling CKM_AES_ECB/CKM_AES_CTR/CKM_AES_GCM so that pInfo->flags |= CKF_WRAP | CKF_UNWRAP is only applied for CKM_AES_GCM (or otherwise gated by mech == CKM_AES_GCM), while keeping pInfo->ulMinKeySize/ulMaxKeySize and CKF_ENCRYPT|CKF_DECRYPT unchanged for ECB and CTR; adjust the case grouping or add a separate case for CKM_AES_GCM to prevent advertising unsupported wrap/unwrap capabilities in C_GetMechanismInfo.
2429-2439:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate nested GCM pointers before copying them.
These paths only validate that
pParameteris aCK_GCM_PARAMS, then copypIv/pAADwithout checking the nested pointers. That leaves reachable UB on malformed input:ulIvLen == 0still takes&iv[0], andulAADLen > 0withpAAD == NULL_PTRwill dereference a null pointer instead of returning a PKCS#11 error.Also applies to: 3184-3194, 6699-6710, 7450-7460
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/SoftHSM.cpp` around lines 2429 - 2439, The code reads CK_GCM_PARAMS via CK_GCM_PARAMS_PTR(pMechanism->pParameter) and unconditionally memcpy's into iv/aad which can dereference NULL or use &iv[0] when length==0; update the block in SoftHSM.cpp around pMechanism handling so you first extract CK_GCM_PARAMS* params = CK_GCM_PARAMS_PTR(pMechanism->pParameter), then validate params->pIv is non-NULL when params->ulIvLen > 0 (and return CKR_ARGUMENTS_BAD if not), only resize iv and memcpy when ulIvLen > 0, and likewise validate params->pAAD is non-NULL when params->ulAADLen > 0 and only resize+memcpy aad when >0; apply the same pattern to the other occurrences noted (around lines 3184-3194, 6699-6710, 7450-7460) referencing pMechanism, CK_GCM_PARAMS_PTR, iv, aad, and return CKR_ARGUMENTS_BAD on invalid nested pointers.
7034-7039:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CKM_AES_CBCfell out of the AES key-type gate.
C_WrapKeyandC_UnwrapKeystill acceptCKM_AES_CBC, but the refactored mechanism-keyed validation only constrainsCKM_AES_CBC_PADandCKM_AES_GCMtoCKK_AES. A non-AES secret key with 16/24/32 bytes can now reach the AES-CBC wrap/unwrap path and be interpreted as AES material instead of failing withCKR_*_KEY_TYPE_INCONSISTENT.Also applies to: 7071-7079, 7786-7792, 7846-7855
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/SoftHSM.cpp` around lines 7034 - 7039, The AES key-type gate omitted CKM_AES_CBC, allowing non-AES secret keys to be treated as AES when wrapping/unwrapping; update the mechanism-to-key-type validation (the branches that currently restrict CKM_AES_CBC_PAD and CKM_AES_GCM to CKK_AES) to include CKM_AES_CBC as well so C_WrapKey and C_UnwrapKey will enforce CKK_AES for CKM_AES_CBC (and similarly update the identical checks around the other occurrences mentioned) and return CKR_KEY_TYPE_INCONSISTENT when the key type is not CKK_AES.
🧹 Nitpick comments (1)
src/lib/test/SymmetricAlgorithmTests.cpp (1)
1260-1274: 💤 Low valueConsider adding explicit CKM_AES_GCM case for clarity.
The
wrapOverheadlambda correctly returns 16 for GCM (via theelsebranch), but the comment says "CKM_AES_CBC_PAD adds padding" which is misleading when GCM also falls through here. For better maintainability, consider making the GCM case explicit.♻️ Optional clarification
auto wrapOverhead = [mechanismType]() { if (mechanismType == CKM_AES_KEY_WRAP || mechanismType == CKM_AES_KEY_WRAP_PAD) return 8; else if (mechanismType == CKM_AES_CBC) return 0; // No padding overhead for CKM_AES_CBC + else if (mechanismType == CKM_AES_GCM) + return 16; // GCM authentication tag (128-bit) else return 16; // CKM_AES_CBC_PAD adds padding };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/test/SymmetricAlgorithmTests.cpp` around lines 1260 - 1274, The wrapOverhead lambda currently treats CKM_AES_GCM via the else branch which is labeled only for CKM_AES_CBC_PAD; update the lambda (referencing mechanismType and wrapOverhead) to add an explicit CKM_AES_GCM case returning 16 and adjust the comments so the fallback branch clearly documents which mechanisms map to which overheads (CKM_AES_KEY_WRAP, CKM_AES_KEY_WRAP_PAD => 8; CKM_AES_CBC => 0; CKM_AES_CBC_PAD and CKM_AES_GCM => 16) to improve clarity and maintainability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 7801-7813: The CKM_AES_GCM precheck incorrectly assumes a fixed
12-byte tag (28-byte minimum) and rejects valid wrapped lengths; update the
check in the CKM_AES_GCM case to compute tagBytes from ulTagBits (ensure
ulTagBits is a valid multiple of 8 within the accepted range) and then validate
ulWrappedKeyLen against (16 + tagBytes) minimum and allow any remainder bytes
for ciphertext length rather than the hard-coded modulo-16 remainder rule;
replace the current condition that returns CKR_WRAPPED_KEY_LEN_RANGE with one
that uses tagBytes and the existing tag-bit constraints already used elsewhere
to determine validity.
- Around line 7090-7095: The conditional rejecting DES2/DES3 wrapping keys uses
|| which makes it always true; update the check in the
CKM_DES3_CBC/CKM_DES3_CBC_PAD case so the two key-type comparisons use && (i.e.,
require the key type to be neither CKK_DES2 nor CKK_DES3 before returning
CKR_WRAPPING_KEY_TYPE_INCONSISTENT) — adjust the same pattern found later around
the other case (the occurrence at the block covering lines ~7866-7871) to use &&
as well; locate the comparisons using
wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) and replace the
logical operator accordingly.
In `@src/lib/test/SymmetricAlgorithmTests.h`:
- Around line 172-178: The designated-initializer usage for m_gcm_params is
non-portable for pre-C++20 builds; replace the brace-designation with explicit
member assignments instead: locate where m_gcm_params is set (the code block
initializing m_gcm_params using .pIv, .ulIvLen, .ulIvBits, .pAAD, .ulAADLen,
.ulTagBits) and assign each field individually (e.g., m_gcm_params.pIv =
m_data[1].data(); m_gcm_params.ulIvLen = m_data[1].size(); etc.), using m_data
and m_tagbits as in the original initializer to preserve semantics.
---
Outside diff comments:
In `@src/lib/SoftHSM.cpp`:
- Around line 1218-1223: The code is incorrectly setting CKF_WRAP|CKF_UNWRAP for
CKM_AES_ECB and CKM_AES_CTR; update the switch in SoftHSM.cpp handling
CKM_AES_ECB/CKM_AES_CTR/CKM_AES_GCM so that pInfo->flags |= CKF_WRAP |
CKF_UNWRAP is only applied for CKM_AES_GCM (or otherwise gated by mech ==
CKM_AES_GCM), while keeping pInfo->ulMinKeySize/ulMaxKeySize and
CKF_ENCRYPT|CKF_DECRYPT unchanged for ECB and CTR; adjust the case grouping or
add a separate case for CKM_AES_GCM to prevent advertising unsupported
wrap/unwrap capabilities in C_GetMechanismInfo.
- Around line 2429-2439: The code reads CK_GCM_PARAMS via
CK_GCM_PARAMS_PTR(pMechanism->pParameter) and unconditionally memcpy's into
iv/aad which can dereference NULL or use &iv[0] when length==0; update the block
in SoftHSM.cpp around pMechanism handling so you first extract CK_GCM_PARAMS*
params = CK_GCM_PARAMS_PTR(pMechanism->pParameter), then validate params->pIv is
non-NULL when params->ulIvLen > 0 (and return CKR_ARGUMENTS_BAD if not), only
resize iv and memcpy when ulIvLen > 0, and likewise validate params->pAAD is
non-NULL when params->ulAADLen > 0 and only resize+memcpy aad when >0; apply the
same pattern to the other occurrences noted (around lines 3184-3194, 6699-6710,
7450-7460) referencing pMechanism, CK_GCM_PARAMS_PTR, iv, aad, and return
CKR_ARGUMENTS_BAD on invalid nested pointers.
- Around line 7034-7039: The AES key-type gate omitted CKM_AES_CBC, allowing
non-AES secret keys to be treated as AES when wrapping/unwrapping; update the
mechanism-to-key-type validation (the branches that currently restrict
CKM_AES_CBC_PAD and CKM_AES_GCM to CKK_AES) to include CKM_AES_CBC as well so
C_WrapKey and C_UnwrapKey will enforce CKK_AES for CKM_AES_CBC (and similarly
update the identical checks around the other occurrences mentioned) and return
CKR_KEY_TYPE_INCONSISTENT when the key type is not CKK_AES.
---
Nitpick comments:
In `@src/lib/test/SymmetricAlgorithmTests.cpp`:
- Around line 1260-1274: The wrapOverhead lambda currently treats CKM_AES_GCM
via the else branch which is labeled only for CKM_AES_CBC_PAD; update the lambda
(referencing mechanismType and wrapOverhead) to add an explicit CKM_AES_GCM case
returning 16 and adjust the comments so the fallback branch clearly documents
which mechanisms map to which overheads (CKM_AES_KEY_WRAP, CKM_AES_KEY_WRAP_PAD
=> 8; CKM_AES_CBC => 0; CKM_AES_CBC_PAD and CKM_AES_GCM => 16) to improve
clarity and maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bc2cfe7-885a-4b4b-a16b-5c7f14226019
📒 Files selected for processing (3)
src/lib/SoftHSM.cppsrc/lib/test/SymmetricAlgorithmTests.cppsrc/lib/test/SymmetricAlgorithmTests.h
Comment on lines
+7090
to
+7095
| case CKM_DES3_CBC: | ||
| case CKM_DES3_CBC_PAD: | ||
| if (wrapKey->getUnsignedLongValue(CKA_CLASS, CKO_VENDOR_DEFINED) != CKO_SECRET_KEY || | ||
| 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; |
There was a problem hiding this comment.
Use && in the DES2/DES3 check.
keyType != CKK_DES2 || keyType != CKK_DES3 is always true, so every DES2/DES3 wrapping or unwrapping key is rejected here.
Suggested fix
- if (wrapKey->getUnsignedLongValue(CKA_CLASS, CKO_VENDOR_DEFINED) != CKO_SECRET_KEY ||
- wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 ||
- wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)
+ if (wrapKey->getUnsignedLongValue(CKA_CLASS, CKO_VENDOR_DEFINED) != CKO_SECRET_KEY ||
+ (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 (unwrapKey->getUnsignedLongValue(CKA_CLASS, CKO_VENDOR_DEFINED) != CKO_SECRET_KEY ||
- unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES2 ||
- unwrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) != CKK_DES3)
+ if (unwrapKey->getUnsignedLongValue(CKA_CLASS, CKO_VENDOR_DEFINED) != CKO_SECRET_KEY ||
+ (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;Also applies to: 7866-7871
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/SoftHSM.cpp` around lines 7090 - 7095, The conditional rejecting
DES2/DES3 wrapping keys uses || which makes it always true; update the check in
the CKM_DES3_CBC/CKM_DES3_CBC_PAD case so the two key-type comparisons use &&
(i.e., require the key type to be neither CKK_DES2 nor CKK_DES3 before returning
CKR_WRAPPING_KEY_TYPE_INCONSISTENT) — adjust the same pattern found later around
the other case (the occurrence at the block covering lines ~7866-7871) to use &&
as well; locate the comparisons using
wrapKey->getUnsignedLongValue(CKA_KEY_TYPE, CKK_VENDOR_DEFINED) and replace the
logical operator accordingly.
Comment on lines
+7801
to
+7813
| case CKM_AES_GCM: | ||
| // for GCM, the shortest block is 16 bytes + authentication tag | ||
| // in this impementation, 96 bits are requested at least, in multiples of 8 bits | ||
| // possibles values are therefore: | ||
| // - 96 bits (12 bytes) | ||
| // - 104 bits (13 bytes) | ||
| // - 112 bits (14 bytes) | ||
| // - 120 bits (15 bytes) | ||
| // - 128 bits (16 bytes) | ||
| // minimum length of the overall message in bytes is therefore 16 + 12 = 28 bytes | ||
| // remainder lengths (after modulo 16) between 1 and 11 are forbidden | ||
| if ((ulWrappedKeyLen < 28) || ( (ulWrappedKeyLen % 16) && (ulWrappedKeyLen % 16) < 12) ) | ||
| return CKR_WRAPPED_KEY_LEN_RANGE; |
There was a problem hiding this comment.
The AES-GCM wrapped-length precheck rejects valid tag sizes.
Elsewhere this patch accepts ulTagBits from 64 to 128 in 8-bit steps, but this guard hard-codes a 28-byte minimum and forbids remainders below 12. That rejects valid ciphertexts such as a 16-byte AES key wrapped with a 64-bit tag (24 bytes total).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/SoftHSM.cpp` around lines 7801 - 7813, The CKM_AES_GCM precheck
incorrectly assumes a fixed 12-byte tag (28-byte minimum) and rejects valid
wrapped lengths; update the check in the CKM_AES_GCM case to compute tagBytes
from ulTagBits (ensure ulTagBits is a valid multiple of 8 within the accepted
range) and then validate ulWrappedKeyLen against (16 + tagBytes) minimum and
allow any remainder bytes for ciphertext length rather than the hard-coded
modulo-16 remainder rule; replace the current condition that returns
CKR_WRAPPED_KEY_LEN_RANGE with one that uses tagBytes and the existing tag-bit
constraints already used elsewhere to determine validity.
Comment on lines
+172
to
+178
| m_gcm_params = { | ||
| .pIv = m_data[1].data(), | ||
| .ulIvLen = m_data[1].size(), | ||
| .ulIvBits = m_data[1].size()<<3, | ||
| .pAAD = m_data[2].data(), | ||
| .ulAADLen = m_data[2].size(), | ||
| .ulTagBits = m_tagbits }; |
There was a problem hiding this comment.
Designated initializers require C++20 or compiler extensions.
The designated initializer syntax (.pIv = ..., .ulIvLen = ...) is not standard C++ until C++20. While GCC and Clang support this as an extension in earlier standards, it may cause portability issues with strict C++11/14/17 mode. Based on learnings, SoftHSMv2 targets C++11 compatibility.
Consider using member-by-member assignment for portable C++11 code:
🔧 Proposed fix for C++11 compatibility
CK_MECHANISM &mechanism() {
- // adjust mechanism to point to data provided
- // we do it here and not in the constructor,
- // has data() can move around as we push to the vectors
- m_mechanism.pParameter = m_data[1].data();
- m_mechanism.ulParameterLen = m_data[1].size();
if( m_mechanism.mechanism == CKM_AES_GCM ) {
- m_gcm_params = {
- .pIv = m_data[1].data(),
- .ulIvLen = m_data[1].size(),
- .ulIvBits = m_data[1].size()<<3,
- .pAAD = m_data[2].data(),
- .ulAADLen = m_data[2].size(),
- .ulTagBits = m_tagbits };
-
+ m_gcm_params.pIv = m_data[1].data();
+ m_gcm_params.ulIvLen = m_data[1].size();
+ m_gcm_params.ulIvBits = m_data[1].size() << 3;
+ m_gcm_params.pAAD = m_data[2].data();
+ m_gcm_params.ulAADLen = m_data[2].size();
+ m_gcm_params.ulTagBits = m_tagbits;
+
m_mechanism.pParameter = &m_gcm_params;
m_mechanism.ulParameterLen = sizeof(CK_GCM_PARAMS);
} else {
- // other cases: pParameter points to the IV
+ // Other cases: pParameter points to the IV
m_mechanism.pParameter = m_data[1].data();
m_mechanism.ulParameterLen = m_data[1].size();
}
return m_mechanism;
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/test/SymmetricAlgorithmTests.h` around lines 172 - 178, The
designated-initializer usage for m_gcm_params is non-portable for pre-C++20
builds; replace the brace-designation with explicit member assignments instead:
locate where m_gcm_params is set (the code block initializing m_gcm_params using
.pIv, .ulIvLen, .ulIvBits, .pAAD, .ulAADLen, .ulTagBits) and assign each field
individually (e.g., m_gcm_params.pIv = m_data[1].data(); m_gcm_params.ulIvLen =
m_data[1].size(); etc.), using m_data and m_tagbits as in the original
initializer to preserve semantics.
Source: Learnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Just another trial on adding support for wrapping/unwrapping using CKM_AES_GCM mechanism.
Heavily based on the work from @keldonin in #641
Summary by CodeRabbit
New Features
Tests