PKCS #11 Refactor #2008

lundinc2 · 2020-05-13T23:00:55Z

PKCS #11 Refactor

Description

Fixed various MISRA violations.
Refactored last two remaining functions with a too high GNU Complexity.

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* Fixed various MISRA violations. * Refactored last two remaining functions with a too high GNU Complexity.

…mplexity of key generation attribute parsing.

qiutongs · 2020-05-15T19:29:51Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -899,13 +894,13 @@ static CK_RV prvEcKeyAttParse( CK_ATTRIBUTE_PTR pxAttribute,
    }

    /* private EC key attributes. */
-    if( ( xResult != CKR_OK ) && ( xIsPrivate == CK_TRUE ) )
+    if( ( xResult == CKR_ATTRIBUTE_VALUE_INVALID ) && ( xIsPrivate == CK_TRUE ) )


Are you intentionally making this logic change? I mean xResult could be some other error, like CKR_TEMPLATE_INCONSISTENT.

yes, the issue was it would override error messages from an earlier failure, this default case indicates that nothing was caught by the above switch statement

This approach seems risky from the start, though, because we're attempting to parse the data as a certain format without explicitly checking the attribute type. Safer would be to move these blocks into the switch/case above. That would also make this function easier to review and maintain, since it makes for a more typical structure.

As part of that, xResult could for example be initialized as CKR_OK. Then if you fall into the default switch/case, set it to ATTR_VALUE_INVALID.

I have refactored this switch statement to be more clear

qiutongs · 2020-05-15T19:31:03Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

    {
        xResult = prvEcPrivKeyAttParse( pxAttribute, pxMbedContext );
    }

    /* public EC key attributes. */
-    if( ( xResult != CKR_OK ) && ( xIsPrivate == CK_FALSE ) )
+    else if( ( xResult == CKR_ATTRIBUTE_VALUE_INVALID ) && ( xIsPrivate == CK_FALSE ) )


similar here.

qiutongs · 2020-05-15T19:34:42Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

        {
            /* Do not write the last 6 bytes to key storage. */
            pxDerKey[ MAX_LENGTH_KEY - lDerKeyLength + 1 ] -= 6;
-            lActualKeyLength -= 6;


Is it a bug in the old code? There seems no update to lActualKeyLength after initialization.

this logic needs to be examined thoroughly, i didn't get a regression anywhere when i removed it, but we will need to dig deep into the mbedTLS APIs and how the EC keys are created / used.

Also i kept the logic here as it was originally, as i need to deep dive

In other words, by adding an empty public key to a "naked" private key, mbed is making it so that the resulting private key format is inconsistent with the PKCS #11 standard. We're therefore omitting the last six bytes in that case (i.e. because we're using mbed to encode the key, but we need our storage and I/O values to be conformant with P#11). However, this situation probably only arises when we're doing round-trip testing between keys generated by different modules.

qiutongs · 2020-05-15T19:36:23Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c


        if( xResult == CKR_OK )
        {
-            if( 0 == memcmp( xLabel.pValue, pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS, xLabelLength ) )
+            if( 0 == strcmp( xLabel.pValue, pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS ) )


Is it less safe if change to strcmp? Is it better to explicitly specify the length?

it should be OK, the second operator is a constant that is always NULL t terminated. MISRA complains when you use mem API with NULL terminated strings

I concern the case when the length of pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS is larger than xLabelLength. You might access invalid memory.

Let's use strncmp:

strncmp( xLabel.pValue, pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS, xLabelLength )

qiutongs · 2020-05-15T19:39:08Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

-        {
-            PKCS11_PAL_GetObjectValueCleanup( pxObject, ulObjectLength );
-        }
+        PKCS11_PAL_GetObjectValueCleanup( pxObject, ulObjectLength );


So it is safe to always call cleanup even PKCS11_PAL_GetObjectValue returns error in line 1219?

It should always be safe, as the handle will be invalid if the other calls were unsuccessful.

lundinc2 · 2020-05-15T20:01:13Z

/bot run checks

lundinc2 · 2020-05-15T23:44:08Z

/bot run checks

dan4thewin

Sorry, I didn't get very far.

dan4thewin · 2020-05-16T00:18:10Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

-                                                   NULL, 0,                                      /* Q */
-                                                   NULL, 0,                                      /* D */
-                                                   NULL, 0 );                                    /* E */
+            lMbedReturn = mbedtls_rsa_import_raw( pxRsaContext,


Can you describe the impact of these changes ( |= vs = )?

Sure! Originally all of these functions were called in a loop without checking their return value. The intention of the code was if one call failed, it wouldn't get overwritten from a call after it.

Now it is refactored to check the return value in each loop iteration. Also it seems a bit misleading to have the return value's get OR'd together, as it makes it seems the return value are related, but they are independent of each other.

Lastly, MISRA does not allow for bitwise operations on signed integers.

dcgaws · 2020-05-18T21:31:11Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -899,13 +894,13 @@ static CK_RV prvEcKeyAttParse( CK_ATTRIBUTE_PTR pxAttribute,
    }

    /* private EC key attributes. */
-    if( ( xResult != CKR_OK ) && ( xIsPrivate == CK_TRUE ) )
+    if( ( xResult == CKR_ATTRIBUTE_VALUE_INVALID ) && ( xIsPrivate == CK_TRUE ) )


This approach seems risky from the start, though, because we're attempting to parse the data as a certain format without explicitly checking the attribute type. Safer would be to move these blocks into the switch/case above. That would also make this function easier to review and maintain, since it makes for a more typical structure.

As part of that, xResult could for example be initialized as CKR_OK. Then if you fall into the default switch/case, set it to ATTR_VALUE_INVALID.

dcgaws · 2020-05-18T21:32:33Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -948,7 +943,7 @@ void prvFindObjectInListByLabel( uint8_t * pcLabel,
        if( 0 == memcmp( pcLabel, xP11Context.xObjectList.xObjects[ ucIndex ].xLabel, xLabelLength ) )
        {
            *pxPalHandle = xP11Context.xObjectList.xObjects[ ucIndex ].xHandle;
-            *pxAppHandle = ucIndex + 1; /* Zero is not a valid handle, so let's offset by 1. */
+            *pxAppHandle = ucIndex + 1UL; /* Zero is not a valid handle, so let's offset by 1. */


Converting our internal storage pointer to and from the app handle is something that I've typically seen handled by static helper functions. That makes it easier to maintain and modify down the road, if necessary.

Okay will follow up with a PR to move this to static helper function. Cutting a ticket to track this effort

dcgaws · 2020-05-18T21:44:50Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

        {
            /* Do not write the last 6 bytes to key storage. */
            pxDerKey[ MAX_LENGTH_KEY - lDerKeyLength + 1 ] -= 6;
-            lActualKeyLength -= 6;


In other words, by adding an empty public key to a "naked" private key, mbed is making it so that the resulting private key format is inconsistent with the PKCS #11 standard. We're therefore omitting the last six bytes in that case (i.e. because we're using mbed to encode the key, but we need our storage and I/O values to be conformant with P#11). However, this situation probably only arises when we're doing round-trip testing between keys generated by different modules.

dcgaws · 2020-05-18T21:46:49Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -1214,7 +1220,6 @@ static CK_RV prvSaveDerKeyToPal( mbedtls_pk_context * pxMbedContext,

            if( xResult == CKR_OK )
            {
-                xFreeMemory = CK_TRUE;


We're still unconditionally calling vPortFree below, though, even in the case where the malloc failed.

Alfred pointed out that free handles the case when it is passed a NULL pointer, so there is no reason to check if malloc failed when freeing

dcgaws · 2020-05-18T21:49:00Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c


        if( xResult == CKR_OK )
        {
-            if( 0 == memcmp( xLabel.pValue, pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS, xLabelLength ) )
+            if( 0 == strncmp( xLabel.pValue, pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS, xLabel.ulValueLen ) )


Isn't strncmp less efficient than memcmp in this case?

MISRA complains about NULL terminated strings being operated on by the mem family of functions

dcgaws · 2020-05-18T21:52:13Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

    {
        *pxPalHandle = PKCS11_PAL_FindObject( ( uint8_t * ) pkcs11configLABEL_DEVICE_PRIVATE_KEY_FOR_TLS, ( uint8_t ) pxLabel->ulValueLen );
        xIsPrivate = CK_FALSE;
    }
+    else
+    {
+        /* Unknown label passed to function */


Shouldn't an error condition be set here then?

The pxPalHandle is set to INVALID when initialized, so we will return an error

dcgaws · 2020-05-18T21:52:54Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

            {
                prvFindObjectInListByLabel( ( uint8_t * ) pkcs11configLABEL_DEVICE_PUBLIC_KEY_FOR_TLS, strlen( ( char * ) pkcs11configLABEL_DEVICE_PUBLIC_KEY_FOR_TLS ), &xPalHandle, &xAppHandle2 );
-


This is hard to review in GitHub with the wide columns. Is it over 80?

Yes, I'll make a follow up PR to condense all lines to 80 chars

dcgaws · 2020-05-18T22:20:22Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

+
+            if( lMbedTLSResult == 0 )
+            {
+                lMbedTLSResult = mbedtls_mpi_read_binary( &xS, &pucSignature[ 32 ], 32 );


It would be great to stop using the hard-coded key length values for ECDSA as part of this refactoring.

I have cut an issue to track this and will create a follow up PR.

* Fixed various MISRA violations. * Refactored last two remaining functions with a too high GNU Complexity.

lundinc2 added 6 commits May 13, 2020 15:53

PKCS aws#11 Refactor

47b17dd

* Fixed various MISRA violations. * Refactored last two remaining functions with a too high GNU Complexity.

Uncrustified.

64eb354

More MISRA fixes. Fixed MISRA regression when trying to reduce GNU co…

e4a0fa7

…mplexity of key generation attribute parsing.

Uncrustified.

c7ecd97

Uncrustified.

518ff4c

Fix MB test regression.

0b7a947

qiutongs reviewed May 15, 2020

View reviewed changes

lundinc2 added 2 commits May 15, 2020 14:01

Fix build.

586509e

Use strncmp

7d535d2

qiutongs previously approved these changes May 15, 2020

View reviewed changes

dan4thewin reviewed May 16, 2020

View reviewed changes

dcgaws suggested changes May 18, 2020

View reviewed changes

Refactored switch statement.

bc8ef82

lundinc2 dismissed qiutongs’s stale review via bc8ef82 May 18, 2020 23:30

Uncrustified.

99fb057

dcgaws approved these changes May 21, 2020

View reviewed changes

qiutongs approved these changes May 21, 2020

View reviewed changes

lundinc2 merged commit 896f879 into aws:master May 21, 2020

alfred2g pushed a commit to alfred2g/amazon-freertos that referenced this pull request May 21, 2020

PKCS aws#11 Refactor (aws#2008)

b9eb517

* Fixed various MISRA violations. * Refactored last two remaining functions with a too high GNU Complexity.

		{
		prvFindObjectInListByLabel( ( uint8_t * ) pkcs11configLABEL_DEVICE_PUBLIC_KEY_FOR_TLS, strlen( ( char * ) pkcs11configLABEL_DEVICE_PUBLIC_KEY_FOR_TLS ), &xPalHandle, &xAppHandle2 );

PKCS #11 Refactor #2008

PKCS #11 Refactor #2008

Conversation

lundinc2 commented May 13, 2020 • edited Loading

Description

Checklist:

qiutongs May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiutongs May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundinc2 commented May 15, 2020

lundinc2 commented May 15, 2020

dan4thewin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundinc2 May 18, 2020 • edited Loading

Choose a reason for hiding this comment

lundinc2 commented May 13, 2020 •

edited

Loading

qiutongs May 15, 2020 •

edited

Loading

qiutongs May 15, 2020 •

edited

Loading

lundinc2 May 18, 2020 •

edited

Loading