[feature] [sal] add PKCS#11 provider layer (sal/pkcs11)#97
Conversation
nils-cercariolo-st
left a comment
nils-cercariolo-st
left a comment
There was a problem hiding this comment.
Hello Jerome ! I tried to dig a bit into PKCS#11 and review the code. All my comments are subject to what I could find online from PKCS#11 but as I don't have a deep understanding of it, every remark is to take with a grain of salt. I tried to be quite thorough so there are quite a few remarks (it's also quite a big PR). Let me know if you have any questions !
Thanks,
| /* handles[0..found-1] are the private key handles */ | ||
| ``` | ||
|
|
||
| ### 3. Generate a key pair in slot 0 |
There was a problem hiding this comment.
Since slot 0 is usually already provisioned with a key that we can't reprovision, maybe better to make this example with slot 1 ?
There was a problem hiding this comment.
Yes indeed , I have tested on a test part with slot 0 open for reprovisioning . I will update
| */ | ||
| *phKey = STSE_PKCS11_PRIVKEY_HANDLE_BASE + 0x300UL + (CK_ULONG)slot_num; | ||
|
|
||
| (void)shared_secret; /* The result is available but storage is out-of-scope here */ |
There was a problem hiding this comment.
shared secret seems completely discarded, I'm not sure I understand correctly how this function can be useful in this state.
| CK_RV rv; | ||
| CK_ECDH1_DERIVE_PARAMS *pParams; | ||
|
|
||
| (void)pTemplate; (void)ulAttributeCount; |
There was a problem hiding this comment.
Can we leave the template empty ? Don't we have to describe more the key that gets generated ?
| * C_DeriveKey — ECDH1-Derive: compute shared secret using STSAFE-A ECDH engine. | ||
| * | ||
| * The derived secret is returned as a CKO_SECRET_KEY object. | ||
| * Only CKM_ECDH1_DERIVE with CKD_NULL is supported. |
There was a problem hiding this comment.
We say this but we don't check it in code I believe
| case CKM_ECDSA: | ||
| pInfo->ulMinKeySize = 256UL; | ||
| pInfo->ulMaxKeySize = 521UL; | ||
| pInfo->flags = CKF_HW | CKF_SIGN | CKF_VERIFY; |
There was a problem hiding this comment.
We say CKF_VERIFY is supported but every C_Verify functions mention CKR_FUNCTION_NOT_SUPPORTED
There was a problem hiding this comment.
Good catch , it is an unwanted omission on my side . I have tested with an OpenSSL engine (verification done on the Host platform) . I will update this .
|
|
||
| case CKA_ID: | ||
| case CKA_LABEL: | ||
| /* Match by ID: we use the slot/zone number as a single-byte ID */ |
There was a problem hiding this comment.
Mixing CKA_ID and CKA_LABEL. Works with current implementation since CKA_LABEL is 1 byte of the slot number but it's fragile and if we change CKA_LABEL to be more explicit, this case wouldn't work properly.
| break; | ||
|
|
||
| default: | ||
| /* Unknown attributes in the template are ignored */ |
There was a problem hiding this comment.
Should probably return CK_FALSE for unknown attribute
| CK_ULONG stse_pkcs11_ec_point_size(uint16_t coord_size) | ||
| { | ||
| /* DER OCTET STRING: 0x04 (tag) + 1 byte length + 0x04 (point prefix) + X + Y */ | ||
| return (CK_ULONG)(3U + 2U * (CK_ULONG)coord_size); |
There was a problem hiding this comment.
I believe it doesn't work for NIST-P521 as the length is bigger than 127 bytes (DER standard), so DER encoding needs 1 byte more.
Could implement something like that instead :
CK_ULONG stse_pkcs11_ec_point_size(uint16_t coord_size)
{
CK_ULONG point_len =
1UL + 2UL * (CK_ULONG)coord_size;
if (point_len < 0x80UL) {
/* Tag + one-byte length + point */
return 2UL + point_len;
}
if (point_len <= 0xFFUL) {
/* Tag + 0x81 + one-byte length + point */
return 3UL + point_len;
}
return 0UL;
}
| } | ||
|
|
||
| /* If the inner content starts with 0x04 it is the X9.62 point prefix */ | ||
| if (inner_offset < point_len && pEc_point[inner_offset] == 0x04U) { |
There was a problem hiding this comment.
If we have a raw point but with X[1] = 0x04 then this parser believes it's following DER format where it is not and you not work properly. Quite an edge case but could be a headache to debug later.
| stse_pkcs11_object_t obj = { | ||
| .obj_class = CKO_CERTIFICATE, | ||
| .handle = STSE_PKCS11_CERT_HANDLE(i), | ||
| .slot_or_zone = pLib->config.cert_zone_indices[i], |
There was a problem hiding this comment.
Seems like we're assuming cert zone = cert index which works well for just the first one but no guarantee for other certificates/memory zone in stsafe. This behaviour is also present in other places.
2 participants
IMPORTANT INFORMATION
Contributor License Agreement (CLA)