-
Notifications
You must be signed in to change notification settings - Fork 24
More static code analysis fixes #173
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
47feff0
1648653
2f61030
48884e6
3dce681
0c894a8
052e15b
e61e5da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -7864,10 +7864,19 @@ CK_RV C_DeriveKey(CK_SESSION_HANDLE hSession, | |||||||
| FindAttributeType(pTemplate, ulAttributeCount, CKA_VALUE_LEN, | ||||||||
| &lenAttr); | ||||||||
| if (kdfParams->bExpand) { | ||||||||
| if (!lenAttr) { | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| CK_ULONG reqLen; | ||||||||
| if (!lenAttr || !lenAttr->pValue) { | ||||||||
| return CKR_ATTRIBUTE_VALUE_INVALID; | ||||||||
| } | ||||||||
| if (lenAttr->ulValueLen != sizeof(CK_ULONG)) { | ||||||||
| return CKR_ATTRIBUTE_VALUE_INVALID; | ||||||||
| } | ||||||||
| keyLen = *(word32*)lenAttr->pValue; | ||||||||
| XMEMCPY(&reqLen, lenAttr->pValue, sizeof(CK_ULONG)); | ||||||||
| /* On 64-bit, CK_ULONG may exceed word32 range */ | ||||||||
| if (reqLen == 0 || reqLen > (CK_ULONG)0xFFFFFFFF) { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| return CKR_ATTRIBUTE_VALUE_INVALID; | ||||||||
| } | ||||||||
| keyLen = (word32)reqLen; | ||||||||
| } | ||||||||
LinuxJedi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| else { | ||||||||
| keyLen = WC_MAX_DIGEST_SIZE; | ||||||||
|
|
@@ -7944,14 +7953,34 @@ CK_RV C_DeriveKey(CK_SESSION_HANDLE hSession, | |||||||
| if (tlsParams->pReturnedKeyMaterial == NULL) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
|
|
||||||||
| keyLen = (word32)(2 * tlsParams->ulMacSizeInBits) + | ||||||||
| (word32)(2 * tlsParams->ulKeySizeInBits) + | ||||||||
| (word32)(2 * tlsParams->ulIVSizeInBits); | ||||||||
| if (keyLen == 0) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| if ((keyLen % 8) != 0) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| keyLen /= 8; | ||||||||
| { | ||||||||
| CK_ULONG totalBits; | ||||||||
| CK_ULONG a = 2 * tlsParams->ulMacSizeInBits; | ||||||||
| CK_ULONG b = 2 * tlsParams->ulKeySizeInBits; | ||||||||
| CK_ULONG c = 2 * tlsParams->ulIVSizeInBits; | ||||||||
| /* Validate individual fields won't overflow when doubled */ | ||||||||
| if (tlsParams->ulMacSizeInBits > ((CK_ULONG)0xFFFFFFFF / 2) || | ||||||||
| tlsParams->ulKeySizeInBits > ((CK_ULONG)0xFFFFFFFF / 2) || | ||||||||
| tlsParams->ulIVSizeInBits > ((CK_ULONG)0xFFFFFFFF / 2)) { | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| } | ||||||||
| /* Check sum won't overflow on 32-bit */ | ||||||||
| if (a > (CK_ULONG)0xFFFFFFFF - b) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| totalBits = a + b; | ||||||||
| if (totalBits > (CK_ULONG)0xFFFFFFFF - c) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| totalBits += c; | ||||||||
| if (totalBits == 0) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| if ((totalBits % 8) != 0) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| totalBits /= 8; | ||||||||
| /* On 64-bit, CK_ULONG may exceed word32 range */ | ||||||||
| if (totalBits > (CK_ULONG)0xFFFFFFFF) | ||||||||
| return CKR_MECHANISM_PARAM_INVALID; | ||||||||
| keyLen = (word32)totalBits; | ||||||||
| } | ||||||||
LinuxJedi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| derivedKey = (byte*)XMALLOC(keyLen, NULL, DYNAMIC_TYPE_TMP_BUFFER); | ||||||||
| if (derivedKey == NULL) | ||||||||
|
|
@@ -8081,7 +8110,7 @@ CK_RV C_DeriveKey(CK_SESSION_HANDLE hSession, | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| if (rv == CKR_OK) { | ||||||||
| if ((rv == CKR_OK) && (derivedKey != NULL)) { | ||||||||
| rv = SetInitialStates(obj); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14827,6 +14827,112 @@ static CK_RV test_hkdf_gen_key(void* args) | |||||
|
|
||||||
| return ret; | ||||||
| } | ||||||
|
|
||||||
| /* Regression test: HKDF expand with NULL pValue in CKA_VALUE_LEN | ||||||
| * previously crashed (Issue #1315: lenAttr->pValue was dereferenced | ||||||
| * without NULL check). | ||||||
| */ | ||||||
| static CK_RV test_hkdf_derive_expand_null_value_len(void* args) | ||||||
| { | ||||||
| CK_SESSION_HANDLE session = *(CK_SESSION_HANDLE*)args; | ||||||
| CK_RV ret; | ||||||
| CK_OBJECT_HANDLE hBaseKey = CK_INVALID_HANDLE; | ||||||
| CK_OBJECT_HANDLE hPrk = CK_INVALID_HANDLE; | ||||||
| CK_OBJECT_HANDLE hExpandKey = CK_INVALID_HANDLE; | ||||||
|
|
||||||
| CK_BYTE ikm[] = { | ||||||
| 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, | ||||||
| 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, | ||||||
| 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b | ||||||
| }; | ||||||
| CK_ULONG ikm_len = sizeof(ikm); | ||||||
| CK_BYTE salt[] = { | ||||||
| 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, | ||||||
| 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f | ||||||
| }; | ||||||
| CK_BYTE info[] = { 0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, | ||||||
| 0xf8, 0xf9 }; | ||||||
| CK_ULONG prk_len = 32; | ||||||
|
|
||||||
| /* Create IKM as a secret key object */ | ||||||
| CK_ATTRIBUTE templateSecret[] = { | ||||||
| {CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass)}, | ||||||
| {CKA_KEY_TYPE, &genericKeyType, sizeof(genericKeyType)}, | ||||||
| {CKA_TOKEN, &ckFalse, sizeof(ckFalse)}, | ||||||
| {CKA_PRIVATE, &ckTrue, sizeof(ckTrue)}, | ||||||
| {CKA_SENSITIVE, &ckFalse, sizeof(ckFalse)}, | ||||||
| {CKA_EXTRACTABLE, &ckTrue, sizeof(ckTrue)}, | ||||||
| {CKA_VALUE, ikm, ikm_len}, | ||||||
| {CKA_VALUE_LEN, &ikm_len, sizeof(ikm_len)}, | ||||||
| {CKA_DERIVE, &ckTrue, sizeof(ckTrue)} | ||||||
| }; | ||||||
| CK_ULONG templateSecretCount = | ||||||
| sizeof(templateSecret) / sizeof(templateSecret[0]); | ||||||
|
|
||||||
| /* Extract params */ | ||||||
| CK_HKDF_PARAMS paramsExtract = { | ||||||
| CK_TRUE, CK_FALSE, CKM_SHA256_HMAC, | ||||||
| CKF_HKDF_SALT_DATA, salt, sizeof(salt), | ||||||
| CK_INVALID_HANDLE, NULL_PTR, 0 | ||||||
| }; | ||||||
| CK_MECHANISM mechExtract = | ||||||
| { CKM_HKDF_DERIVE, ¶msExtract, sizeof(paramsExtract) }; | ||||||
| CK_ATTRIBUTE templateExtract[] = { | ||||||
| {CKA_CLASS, &secretKeyClass, sizeof(secretKeyClass)}, | ||||||
| {CKA_KEY_TYPE, &genericKeyType, sizeof(genericKeyType)}, | ||||||
| {CKA_SENSITIVE, &ckFalse, sizeof(ckFalse)}, | ||||||
| {CKA_EXTRACTABLE, &ckTrue, sizeof(ckTrue)}, | ||||||
| {CKA_VALUE_LEN, &prk_len, sizeof(prk_len)} | ||||||
| }; | ||||||
| CK_ULONG templateExtractCount = | ||||||
| sizeof(templateExtract) / sizeof(templateExtract[0]); | ||||||
|
|
||||||
| /* Expand params - expand only */ | ||||||
| CK_HKDF_PARAMS paramsExpand = { | ||||||
| CK_FALSE, CK_TRUE, CKM_SHA256_HMAC, | ||||||
| CKF_HKDF_SALT_NULL, NULL_PTR, 0, | ||||||
| CK_INVALID_HANDLE, info, sizeof(info) | ||||||
| }; | ||||||
| CK_MECHANISM mechExpand = | ||||||
| { CKM_HKDF_DERIVE, ¶msExpand, sizeof(paramsExpand) }; | ||||||
|
|
||||||
| /* Expand template with NULL pValue for CKA_VALUE_LEN. | ||||||
| * This triggers the NULL dereference at crypto.c:7870. | ||||||
|
||||||
| * This triggers the NULL dereference at crypto.c:7870. | |
| * This exercises the previously failing NULL CKA_VALUE_LEN pValue case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When HKDF expand requires CKA_VALUE_LEN, this code returns CKR_ATTRIBUTE_VALUE_INVALID both when the attribute is missing and when it has an invalid value. Elsewhere in crypto.c, missing required template attributes return CKR_TEMPLATE_INCOMPLETE (and CKR_ATTRIBUTE_VALUE_INVALID is used for NULL pValue/wrong ulValueLen). Consider returning CKR_TEMPLATE_INCOMPLETE when lenAttr is NULL, and reserve CKR_ATTRIBUTE_VALUE_INVALID for NULL pValue / wrong size / out-of-range values.