libckteec: fix null pointer dereference on template serialization #392
Conversation
libckteec/src/serialize_ck.c
Outdated
| @@ -461,6 +464,9 @@ static CK_RV serialize_mecha_aes_iv(struct serializer *obj, | |||
| uint32_t iv_size = mecha->ulParameterLen; | |||
| CK_RV rv = CKR_GENERAL_ERROR; | |||
|
|
|||
| if (!mecha->pParameter) | |||
There was a problem hiding this comment.
The mechanism currently using serialize_mecha_aes_iv() expect an IV of 16byte. But I think this function would be more flexible if it supports any IV size (for example here we don't enforce iv_size is 16), including 0:
if (iv_size && !mecha->pParameter)
return CKR_MECHANISM_PARAM_INVALID;There was a problem hiding this comment.
Can we use mecha->ulParameterLen instead of iv_size? Since mecha->ulParameterLen is already used when serialize_buffer() is called.
There was a problem hiding this comment.
If you think using iv_size seems more appropriate, feel free to resolve this conversation since it’s already reflected in the commit.
There was a problem hiding this comment.
Testing iv_size or mecha->ulParameterLen is equivalent. At your will.
Fix potential null-pointer dereference on serialize_mecha_XXX() by checking every pointer value in under CK_MECHANISM_PTR. Link: OP-TEE#388 Signed-off-by: Hoyong Jin <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>
hoyong2007
force-pushed
the
add_nullcheck
branch
from
622c7c8
to
d1cc179
Compare
There was a problem hiding this comment.
Sorry, I think I answered too quickly on these changes. For all cases where a buffer pointer is passed together with its buffer size, we should not have to strictly check the buffer is not NULL when the size is not 0. Note that we cannot verify that the passed pointer is really well sized regarding the passed size. That said, your changes add no regression and will have negligible impact on the API function execution performances so I'm fine we consider them.
By the way, I see that few of the functions in serialize_ck.c already return a error when mecha->ulParameterLen != sizeof(*params) but not all function do. For consistency, I think all these functions should. But that would need dedicated commit since your commit initially addresses potential NULL buffer references. Would you mind adding such a commit to your P-R?
By the way, I've not posted a Review-by explicit comment for this P-R so you should not add my R-b tag in the commit message.
| @@ -420,13 +423,13 @@ static CK_RV serialize_mecha_aes_gcm(struct serializer *obj, | |||
| CK_RV rv = CKR_GENERAL_ERROR; | |||
| CK_ULONG aad_len = 0; | |||
|
|
|||
| if (!param || !param->pIv) | |||
There was a problem hiding this comment.
I think test on !param->pIv was not really useful, testing !param should be enough. That said, for consistency, could you test !param || (param->ulIvLen && !param->pIv)
There was a problem hiding this comment.
Looking again at PKCS#11 specifications, on one hand v2.40-errata01 says both AES-GCM IV can be NULL and must be between 1 and 256 byte. On the other hand v2.30 says AES-GCM IV must be between 1 and 2^32-1 bytes. In all one, I think the legacy implementation here in optee_client is enough, so OK for your proposal if (!param || !param->pIv).
|
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
There was a problem hiding this comment.
@hoyong2007, sorry for this very late feedback.
Changes look good to me, just an indentation to fix.
| (params->ulPublicDataLen && !params->pPublicData)) | ||
| return CKR_MECHANISM_PARAM_INVALID; | ||
|
|
||
| params_size = 3 * sizeof(uint32_t) + params->ulSharedDataLen + |
There was a problem hiding this comment.
Could you fix indentation at line below?
| @@ -420,13 +423,13 @@ static CK_RV serialize_mecha_aes_gcm(struct serializer *obj, | |||
| CK_RV rv = CKR_GENERAL_ERROR; | |||
| CK_ULONG aad_len = 0; | |||
|
|
|||
| if (!param || !param->pIv) | |||
There was a problem hiding this comment.
Looking again at PKCS#11 specifications, on one hand v2.40-errata01 says both AES-GCM IV can be NULL and must be between 1 and 256 byte. On the other hand v2.30 says AES-GCM IV must be between 1 and 2^32-1 bytes. In all one, I think the legacy implementation here in optee_client is enough, so OK for your proposal if (!param || !param->pIv).
Fix potential null-pointer dereference on serialize_mecha_XXX() by checking every pointer value in under CK_MECHANISM_PTR.
Link: #388