She extension update #27

jpbland1 · 2024-05-02T07:43:33Z

added she functions, tested working with test vectors and the custom uid. decided to use the normal nvm functions to set she keys as it is simple. I think this should be the default way of programming the master ecu key, secret key etc. currently doesn't use the crypto struct for aes and cmac because those PRs are not merged yet. missing the load plain key command and test for export ram key, the load plain key command from my first draft needs to be re written due to how the new keyId system works.

load key and rnd functions with test vectors.

jpbland1 · 2024-05-02T07:53:12Z

the asan test is failing due to a stack-buffer-overflow in the nvm code. I changed the metadata len to 36 after I added the count parameter which is a uint32_t, 4 bytes. does something else need to be updated when that const changes that I'm not aware of?

bigbrett

Overall it looks great, though I haven't dug into the SHE spec to verify adherence. A few nits, including adding a few more negative tests to ensure things fail when they should fail. But yeah, first pass LGTM

src/wh_server_she.c

test/wh_test_she.c

bigbrett · 2024-05-02T21:22:40Z

test/wh_test_she.c

+        goto exit;
+    }
+    /* verify bootloader */
+    if ((ret = wh_Client_SheSecureBoot(client, bootloader, bootloaderSz)) != 0) {


can we add a negative test to know that the validation works and wont validate everything?

test/wh_test_she.c

setting the manufacturers keys, test export ram key, fix commited key check

src/wh_client_she.c

bigbrett · 2024-05-06T19:20:55Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Do we want to check that it is a multiple of the block/key size here?

I still think there are cases here where we need to validate the size. The size the user passes is directly passed to memcpy without any upper bound checking, so while it might be considered abuse of the function, it is still a potential buffer overflow. Yes it is on the client side and so not any real security implications, but still, I don't think we should allow the client to overflow a buffer in one of our public APIs if we can avoid it. We should look at all the size arguments passed to these APIs and ensure they aren't used directly without validation.

That's not how it's done in any of our crypto for aes, the ecb encrypt and decrypt functions don't even check minimum length:

for (i = 0; i < sz; i += AES_BLOCK_SIZE) { ret = wc_AesEncryptDirect(aes, out, in); if (ret != 0) break; in += AES_BLOCK_SIZE; out += AES_BLOCK_SIZE; }

when the data is longer than the packet buffer, assuming we're not using shared memory, we'll need a different scheme that lets the client send multiple packets for a single command, and that's not in the scope of this change

bigbrett · 2024-05-06T19:27:00Z

src/wh_client_she.c

+int wh_Client_SheEncEcbResponse(whClientContext* c, uint8_t* out, uint32_t sz)
+{
+    int ret;
+    uint16_t group;
+    uint16_t action;
+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)
+        return WH_ERROR_BADARGS;


same comment as above re: size checking against WOLFHSM_SHE_KEY_SZ

bigbrett · 2024-05-06T19:28:01Z

src/wh_client_she.c

+    if (ret == 0) {
+        if (packet->rc != WOLFHSM_SHE_ERC_NO_ERROR)
+            ret = packet->rc;
+        else if (sz < packet->sheEncEcbRes.sz)


equality check? If we are copying out fewer bytes than expected to the user (e.g. if sz > packet->sheEncEcbRes.sz) then we need to indicate that via output argument. But this is probably an error?

I just based it on how we do aes in wolfCrypt, see https://www.wolfssl.com/doxygen/group__AES.html#gaed1e38cd30d917165183fc68dd4b218b, usually the same buffer is used and overwritten for the output anyways.

bigbrett · 2024-05-06T19:28:30Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || iv == NULL ||


same comment as above re: size checking against WOLFHSM_SHE_KEY_SZ

bigbrett · 2024-05-06T19:29:40Z

src/wh_client_she.c

+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)


same comment as above re: size checking against WOLFHSM_SHE_KEY_SZ

src/wh_server_she.c

issues with key metadata not being set correctly

all ske keys through the load key command

matches the test vector

bigbrett

As always, would like to see some more expanded testing for these features. At least once negative test case would be good to ensure things fail when we need them to fail. Would go a long way towards being more confident that further code modifications don't introduce regerssions

src/wh_she_common.c

bigbrett · 2024-05-14T18:18:27Z

test/Makefile

@@ -108,6 +108,7 @@ ifeq ($(SHE),1)
 SRC_C += \
            $(WOLFHSM_DIR)/src/wh_client_she.c \
            $(WOLFHSM_DIR)/src/wh_server_she.c \
+            $(WOLFHSM_DIR)/src/wh_she_common.c \


trailing slash, while okay now, might lead to a cryptic Make error later on

src/wh_she_common.c

bigbrett · 2024-05-14T18:22:24Z

test/wh_test_she.c

    uint8_t bootloader[512];
-    uint8_t keyAndDigest[32] = {0};
+    uint8_t bootMacDigest[16] = {0};


do we have macros for these array sizes?

bigbrett

sorry, prior approval was premature - only blocker is removing arpa/inet.h. Can't merge that in

bigbrett

Good on my end. @billphipps please review

billphipps

Looks good overall. A few things we need to get correct, but I'll merge it now if you want to.

AES CBC and CMAC sizes larger than a single block must be supported. recommend to use DMA for these and simply pass a pointer when available. Otherwise, add a client-side loop to perform the iterative process.
Collect SHE state into a single struct and associate that with a server context. The same state can be used by multiple contexts, or not. It's not necessarily global.
Separate SHE stuff into different files/headers to minimize logic spread into common functions. SHE should appear to be an add-on or helper to use the base HSM library. I expect to code up the PKCS11 as a layer on top, hopefully with no internal logic aside from ID segregation.

A few nits and questions throughout. I still don't see a consistent whKeyId vs whNvmId use yet, but we'll get there. Excellent job on the test code! We may want to have a fixed known answer test in there before you generate everything randomly.

billphipps · 2024-05-06T20:57:41Z

wolfhsm/wh_common.h

+#define WOLFHSM_SHE_M1_SZ 16
+#define WOLFHSM_SHE_M2_SZ 32
+#define WOLFHSM_SHE_M3_SZ WOLFHSM_SHE_M1_SZ
+#define WOLFHSM_SHE_M4_SZ WOLFHSM_SHE_M2_SZ
+#define WOLFHSM_SHE_M5_SZ WOLFHSM_SHE_M1_SZ


Duplicated above.

wolfhsm/wh_common.h

billphipps · 2024-05-17T23:12:42Z

wolfhsm/wh_common.h

+int wh_SheGenerateLoadableKey(uint8_t keyId,
+    uint8_t authKeyId, uint32_t count, uint32_t flags, uint8_t* uid,
+    uint8_t* key, uint8_t* authKey, uint8_t* messageOne, uint8_t* messageTwo,
+    uint8_t* messageThree, uint8_t* messageFour, uint8_t* messageFive);


Is this necessary in common? Recommend to move all of the SHE defines to wh_she.h so that we can keep the common files clean. Not urgent.

billphipps · 2024-05-17T23:16:52Z

wolfhsm/wh_message.h

+/* SHE actions */
+enum {
+    WH_SHE_SET_UID,
+    WH_SHE_SECURE_BOOT_INIT,
+    WH_SHE_SECURE_BOOT_UPDATE,
+    WH_SHE_SECURE_BOOT_FINISH,
+    WH_SHE_GET_STATUS,
+    WH_SHE_LOAD_KEY,
+    WH_SHE_LOAD_PLAIN_KEY,
+    WH_SHE_EXPORT_RAM_KEY,
+    WH_SHE_INIT_RND,
+    WH_SHE_RND,
+    WH_SHE_EXTEND_SEED,
+    WH_SHE_ENC_ECB,
+    WH_SHE_ENC_CBC,
+    WH_SHE_DEC_ECB,
+    WH_SHE_DEC_CBC,
+    WH_SHE_GEN_MAC,
+    WH_SHE_VERIFY_MAC,
+};


Move these to wh_message_she.h so they can be shared with the server. Recommend to update naming to WH_MESSAGE_SHE_ACTION_xxx so it's clear these are message actions. Consider replacing the WOLFHSM_SHE_SUBTYPE with these as they appear to be a one-to-one match.

billphipps · 2024-05-17T23:17:18Z

wolfhsm/wh_server_she.h

+enum WOLFHSM_SHE_SUBTYPE {
+    WOLFHSM_SHE_SECURE_BOOT_INIT,
+    WOLFHSM_SHE_SECURE_BOOT_UPDATE,
+    WOLFHSM_SHE_SECURE_BOOT_FINISH,
+    WOLFHSM_SHE_GET_STATUS,
+    WOLFHSM_SHE_LOAD_KEY,
+    WOLFHSM_SHE_EXPORT_RAM_KEY,
+    WOLFHSM_SHE_INIT_RNG,
+    WOLFHSM_SHE_RND,
+    WOLFHSM_SHE_EXTEND_SEED,
+};


Can these be replaced by the ACTION enum?

yes these are redundant/wrong

billphipps · 2024-05-18T00:10:06Z

src/wh_server_she.c

+static uint8_t hsmShePrngState[WOLFHSM_SHE_KEY_SZ];
+static uint8_t hsmSheSbState = WOLFHSM_SHE_SB_INIT;
+static uint8_t hsmSheCmacKeyFound = 0;
+static uint8_t hsmSheRamKeyPlain = 0;
+static uint8_t hsmSheUidSet = 0;
+static uint32_t hsmSheBlSize = 0;
+static uint32_t hsmSheBlSizeReceived = 0;
+static uint32_t hsmSheRndInited = 0;
 /* cmac is global since the bootloader update can be called multiple times */
 Cmac sheCmac[1];
+Aes sheAes[1];


Collect all required she server state into a single struct that is allocated/provided at the same time as the server. Note that this state is per server instance and NOT global, but the server configs could use the same SHE instance to provide the same state, if desired.

billphipps · 2024-05-18T00:10:41Z

src/wh_server_she.c

 #include "wolfhsm/wh_packet.h"
+#include "wolfhsm/wh_error.h"
+#ifdef WOLFHSM_SHE_EXTENSION


Shouldn't this entire file be protected with this macro?

billphipps · 2024-05-18T00:12:28Z

src/wh_server_keystore.c

-    if (server == NULL || (keyId & WOLFHSM_KEYID_MASK) == WOLFHSM_KEYID_ERASED
-        || outSz == NULL) {
+    if (server == NULL || ((keyId & WOLFHSM_KEYID_MASK) == WOLFHSM_KEYID_ERASED
+        && (keyId & WOLFHSM_KEYTYPE_MASK) != WOLFHSM_KEYTYPE_SHE) ||


Does this need to be protected by the compile time ifdef for she_extension?

I don't think so, the she keyId stuff is not compile guarded since it's reserved in case they update to use she key later but still want the same keystore

billphipps · 2024-05-18T00:13:35Z

src/wh_server_keystore.c

 #ifdef WOLFHSM_SHE_EXTENSION
-    /* use empty string if we couldn't find the master ecu key */
-    if (ret != 0 && keyId == WOLFHSM_SHE_MASTER_ECU_KEY_ID) {
+    /* use empty key of zeros if we couldn't find the master ecu key */
+    if (ret == WH_ERROR_NOTFOUND &&
+        (keyId & WOLFHSM_KEYTYPE_MASK) == WOLFHSM_KEYTYPE_SHE &&
+        (keyId & WOLFHSM_KEYID_MASK) == WOLFHSM_SHE_MASTER_ECU_KEY_ID) {
        XMEMSET(out, 0, WOLFHSM_SHE_KEY_SZ);
-        meta->len = WOLFHSM_SHE_KEY_SZ;
-        meta->id = searchId;
+        *outSz = WOLFHSM_SHE_KEY_SZ;
+        if (outMeta != NULL) {
+            /* need empty flags and corect lenth and id */
+            XMEMSET(outMeta, 0, sizeof(meta));
+            meta->len = WOLFHSM_SHE_KEY_SZ;
+            meta->id = keyId;
+        }
        ret = 0;
    }
 #endif


Shouldn't this be handled in the SHE version of read key and not in the generic one?

multiple places can read the unset ecu key, they don't call their own she read function they make a she keyId and then call the normal read function, putting it here means no duplicated code

src/wh_client_she.c

jpbland1 · 2024-05-21T04:51:19Z

Adressed most of bills comments, reorganized the structs and changed the way the "non-loadable" she keys are loaded into the hsm

billphipps

A few suggestions to handle overrunning the packet size. Looks good enough for now.

billphipps · 2024-05-21T19:01:24Z

src/wh_client_she.c

+        sizeof(packet->sheExtendSeedReq.entropy));
+    /* send init rng req */
+    ret = wh_Client_SendRequest(c, WH_MESSAGE_GROUP_SHE, WH_SHE_EXTEND_SEED,
+        WOLFHSM_PACKET_STUB_SIZE, (uint8_t*)packet);


Suggested change

WOLFHSM_PACKET_STUB_SIZE, (uint8_t*)packet);

WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->sheExtendSeedReq), (uint8_t*)packet);

billphipps · 2024-05-21T19:05:32Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Better error out if it can't be placed in a single packet.

Suggested change

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncEcbReq))

billphipps · 2024-05-21T19:07:58Z

src/wh_client_she.c

+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Suggested change

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ|| sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncEcbRes))

billphipps · 2024-05-21T19:08:57Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || iv == NULL ||


Suggested change

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || iv == NULL ||

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncCbcReq || iv == NULL ||

billphipps · 2024-05-21T19:09:29Z

src/wh_client_she.c

+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Suggested change

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncCbcRes))

billphipps · 2024-05-21T19:40:27Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Suggested change

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheDecEcbReq))

billphipps · 2024-05-21T19:40:39Z

src/wh_client_she.c

+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Suggested change

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheDecEcbRes))

src/wh_client_she.c

billphipps · 2024-05-21T19:41:54Z

src/wh_client_she.c

+    int ret;
+    uint8_t* packIn;
+    whPacket* packet;
+    if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || iv == NULL ||


Suggested change

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || iv == NULL ||

if (c == NULL || in == NULL || sz < WOLFHSM_SHE_KEY_SZ || || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheDecCbcReq) || iv == NULL ||

billphipps · 2024-05-21T19:44:04Z

src/wh_client_she.c

+    uint16_t dataSz;
+    uint8_t* packOut;
+    whPacket* packet;
+    if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)


Suggested change

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ)

if (c == NULL || out == NULL || sz < WOLFHSM_SHE_KEY_SZ || sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheDecCbcRes))

billphipps

Minor fixes for next pr

jpbland1 added 2 commits May 2, 2024 03:38

add she functions, tested working for secure boot,

085bd20

load key and rnd functions with test vectors.

add she test

0835adb

jpbland1 requested review from billphipps and bigbrett and removed request for billphipps May 2, 2024 07:50

jpbland1 self-assigned this May 2, 2024

jpbland1 requested a review from billphipps May 2, 2024 07:50

jpbland1 assigned billphipps and bigbrett May 2, 2024

jpbland1 added 3 commits May 2, 2024 13:42

make nvm metadata a multiple of WHFU_BYTES_PER_UNIT

002a8c4

remove DEBUG=1 from SHE workflow

c1243dd

try cleaning before each test run

ae764e7

bigbrett requested changes May 2, 2024

View reviewed changes

jpbland1 added 2 commits May 3, 2024 06:10

add load plain key, add convenience functions for

f3c4e63

setting the manufacturers keys, test export ram key, fix commited key check

consolidate headers

2eef2b7

jpbland1 requested a review from bigbrett May 3, 2024 10:29

bigbrett reviewed May 7, 2024

View reviewed changes

jpbland1 added 4 commits May 8, 2024 19:20

break she handlers into separate functions, fix

e103f0d

issues with key metadata not being set correctly

use label to store she specific metadata

4f2f86e

make she key message 1 through 5 generator and load

1e2d535

all ske keys through the load key command

verify that the output of wh_SheGenerateLoadableKey

7057fc8

matches the test vector

jpbland1 requested a review from bigbrett May 13, 2024 16:42

add cmac she functions

1faaaea

bigbrett approved these changes May 14, 2024

View reviewed changes

bigbrett requested changes May 14, 2024

View reviewed changes

jpbland1 added 2 commits May 15, 2024 13:31

implement local versions of htonl and ntohl

7afe89c

add NULL checks to loadable key generator

53eff1b

jpbland1 requested a review from bigbrett May 15, 2024 20:38

bigbrett previously approved these changes May 17, 2024

View reviewed changes

billphipps reviewed May 18, 2024

View reviewed changes

jpbland1 added 2 commits May 20, 2024 18:01

reorganize she globals into a sub struct of the server context

9e15120

use wc_AesCmacGenerate_ex where possible

9b6de43

jpbland1 dismissed bigbrett’s stale review via 9b6de43 May 20, 2024 22:35

jpbland1 added 3 commits May 20, 2024 18:48

check that keyIds are valid for wh_SheGenerateLoadableKey

1849402

use pre load to load keys that are supposed to be read only

605a7e3

remove outdated enum, move header

1eceb24

jpbland1 requested review from billphipps and bigbrett May 21, 2024 04:50

billphipps previously approved these changes May 21, 2024

View reviewed changes

fix bad size argument for extending she PRNG

b9e3019

jpbland1 dismissed billphipps’s stale review via b9e3019 May 21, 2024 20:47

billphipps approved these changes May 21, 2024

View reviewed changes

billphipps merged commit cc1527e into wolfSSL:main May 21, 2024
1 check passed

	WOLFHSM_PACKET_STUB_SIZE, (uint8_t*)packet);
	WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->sheExtendSeedReq), (uint8_t*)packet);

	if (c == NULL \|\| in == NULL \|\| sz < WOLFHSM_SHE_KEY_SZ)
	if (c == NULL \|\| in == NULL \|\| sz < WOLFHSM_SHE_KEY_SZ \|\| sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncEcbReq))

	if (c == NULL \|\| out == NULL \|\| sz < WOLFHSM_SHE_KEY_SZ)
	if (c == NULL \|\| out == NULL \|\| sz < WOLFHSM_SHE_KEY_SZ\|\| sz > (WH_COMM_DATA_LEN - WOLFHSM_PACKET_STUB_SIZE - sizeof(packet->sheEncEcbRes))

She extension update #27

She extension update #27

Conversation

jpbland1 commented May 2, 2024

jpbland1 commented May 2, 2024

bigbrett 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

bigbrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

bigbrett left a comment

Choose a reason for hiding this comment

billphipps 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

jpbland1 commented May 21, 2024

billphipps 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

billphipps left a comment

Choose a reason for hiding this comment