Skip to content
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.

Sign up for GitHub

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

add key management commands for caching, exporting #16

Merged
merged 13 commits into from
Apr 10, 2024
Merged

add key management commands for caching, exporting #16

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0497302
add key management commands for caching, exporting
jpbland1 Apr 2, 2024
e016abe
add nvm key functions commit and erase
jpbland1 Apr 5, 2024
90ee2b2
add set key function
jpbland1 Apr 5, 2024
5482af4
update based on PR comments
jpbland1 Apr 5, 2024
afa643d
remove old header
jpbland1 Apr 5, 2024
e183ad3
stop using wolfSSL errors
jpbland1 Apr 8, 2024
2cfc28b
add read size to wh_Client_KeyExportResponse so clients
jpbland1 Apr 8, 2024
d4525d0
add labelSz check
jpbland1 Apr 8, 2024
c3261d3
update based on PR comments
jpbland1 Apr 8, 2024
2749ab9
fix label size check
jpbland1 Apr 9, 2024
6d423d1
update hsmReadKey to support NULL metadata
jpbland1 Apr 9, 2024
6e60871
update hsmReadKey to make meta and data optional
jpbland1 Apr 9, 2024
f2a6785
fix meta read out and commit test
jpbland1 Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
207 changes: 206 additions & 1 deletion src/wh_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@

/* wolfCrypt */
#include "wolfssl/wolfcrypt/settings.h"
#include "wolfssl/wolfcrypt/error-crypt.h"
#include "wolfssl/wolfcrypt/wc_port.h"
#include "wolfssl/wolfcrypt/cryptocb.h"
#include "wolfssl/wolfcrypt/curve25519.h"

/* Common WolfHSM types and defines shared with the server */
#include "wolfhsm/wh_common.h"
Expand All @@ -25,6 +27,7 @@
#include "wolfhsm/wh_message_comm.h"
#include "wolfhsm/wh_message_customcb.h"

#include "wolfhsm/wh_packet.h"
#include "wolfhsm/wh_client.h"

int wh_Client_Init(whClientContext* c, const whClientConfig* config)
Expand Down Expand Up @@ -308,4 +311,206 @@ int wh_Client_CustomCbCheckRegistered(whClientContext* c, uint16_t id, int* resp
}

return rc;
}
}

int wh_Client_KeyCacheRequest_ex(whClientContext* c, uint32_t flags,
billphipps marked this conversation as resolved.
Show resolved Hide resolved
uint8_t* label, uint32_t labelSz, uint8_t* in, uint32_t inSz,
uint16_t keyId)
{
uint8_t rawPacket[WH_COMM_MTU] = {0};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CommClient context already has an MTU-sized packet precisely for construction. The SendRequest helper was put together to simplify calls, but you are welcome to directly use this buffer (at the data offset) and update the SendRequest to not perform the memcpy when the src of the data is the same as the destination. We'll add this to the todo list so we can provide an accessor and avoid extra copies and allocations.

Oh, and this size should be WH_COMM_DATA_LEN, not MTU.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So change it back to using a single packet instead of taking data as a parameter? The way I had it in the old design?

whPacket* packet = (whPacket*)rawPacket;
uint8_t* packIn = (uint8_t*)(&packet->keyCacheReq + 1);
if (c == NULL || label == NULL || in == NULL || inSz == 0 ||
labelSz > WOLFHSM_NVM_LABEL_LEN) {
return WH_ERROR_BADARGS;
}
/* set flags */
packet->keyCacheReq.flags = flags;
/* set inSz */
packet->keyCacheReq.sz = inSz;
/* set labelLen */
packet->keyCacheReq.labelSz = labelSz;
/* set id */
packet->keyCacheReq.id = keyId;
/* set label */
XMEMCPY(packet->keyCacheReq.label, label, labelSz);
/* write in */
XMEMCPY(packIn, in, inSz);
/* write request */
return wh_Client_SendRequest(c, WH_MESSAGE_GROUP_KEY, WH_KEY_CACHE,
WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->keyCacheReq) + inSz,
(uint8_t*)packet);
}

int wh_Client_KeyCacheRequest(whClientContext* c, uint32_t flags,
uint8_t* label, uint32_t labelSz, uint8_t* in, uint32_t inSz)
{
return wh_Client_KeyCacheRequest_ex(c, flags, label, labelSz, in, inSz,
WOLFHSM_ID_ERASED);
}

int wh_Client_KeyCacheResponse(whClientContext* c, uint16_t* keyId)
{
uint16_t group;
uint16_t action;
uint16_t size;
int ret;
whPacket packet[1] = {0};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of unions, but some of these messages are really small. Consider using the smaller structs when possible to avoid a big allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it really saved a lot of space in the old design to only have 1 packet for everything huh?

if (c == NULL || keyId == NULL)
return WH_ERROR_BADARGS;
ret = wh_Client_RecvResponse(c, &group, &action, &size, (uint8_t*)packet);
if (ret == 0) {
if (packet->rc != 0)
ret = packet->rc;
billphipps marked this conversation as resolved.
Show resolved Hide resolved
else
*keyId = packet->keyCacheRes.id;
}
return ret;
}

int wh_Client_KeyEvictRequest(whClientContext* c, uint16_t keyId)
{
whPacket packet[1] = {0};
if (c == NULL || keyId == WOLFHSM_ID_ERASED)
return WH_ERROR_BADARGS;
/* set the keyId */
packet->keyEvictReq.id = keyId;
/* write request */
return wh_Client_SendRequest(c, WH_MESSAGE_GROUP_KEY, WH_KEY_EVICT,
WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->keyEvictReq),
(uint8_t*)packet);
}

int wh_Client_KeyEvictResponse(whClientContext* c)
{
uint16_t group;
uint16_t action;
uint16_t size;
int ret;
whPacket packet[1] = {0};
if (c == NULL)
return WH_ERROR_BADARGS;
ret = wh_Client_RecvResponse(c, &group, &action, &size, (uint8_t*)packet);
if (ret == 0) {
if (packet->rc != 0)
ret = packet->rc;
}
return ret;
}

int wh_Client_KeyExportRequest(whClientContext* c, uint16_t keyId)
{
whPacket packet[1] = {0};
if (c == NULL || keyId == WOLFHSM_ID_ERASED)
return WH_ERROR_BADARGS;
/* set keyId */
packet->keyExportReq.id = keyId;
/* write request */
return wh_Client_SendRequest(c, WH_MESSAGE_GROUP_KEY, WH_KEY_EXPORT,
WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->keyExportReq),
(uint8_t*)packet);
}

int wh_Client_KeyExportResponse(whClientContext* c, uint8_t* label,
uint32_t labelSz, uint8_t* out, uint32_t* outSz)
{
uint16_t group;
uint16_t action;
uint16_t size;
int ret;
uint8_t rawPacket[WH_COMM_MTU] = {0};
whPacket* packet = (whPacket*)rawPacket;
uint8_t* packOut = (uint8_t*)(&packet->keyExportRes + 1);
if (c == NULL || labelSz > WOLFHSM_NVM_LABEL_LEN || outSz == NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why any labelSz could cause an error. If the user provides a smaller label storage (even 0), then I think that should be valid. Simply labelSz = min(sizeof(metadata.label), labelSz);

outSz == NULL is valid as long as out == NULL. Probably just a test case, but still consistent.
Should outSz be set to the size of out before the function is called?

Copy link
Contributor Author

@jpbland1 jpbland1 Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

labelSz == 1000000000000 would cause out of bounds read, we can't accept any labelSz and outSz can't be NULL because when out is NULL it means the user is expecting outSz to be populated so they know how big their buffer needs to be for a subsequent call

Copy link
Contributor Author

@jpbland1 jpbland1 Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought more about this, we definitely need to validate labelSz because an incorrect value of 25 doesn't guarantee the buffer can actually handle sizeof(metadata.label) which i think is 24. if we have an unknown length we should bail out, going to modify it to only err if label is non-NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that, actually makes total sense for the buffer to be larger than max if they're reusing some generic buffer, my bad

return WH_ERROR_BADARGS;
ret = wh_Client_RecvResponse(c, &group, &action, &size, rawPacket);
if (ret == 0) {
if (packet->rc != 0)
ret = packet->rc;
else {
if (out == NULL) {
*outSz = packet->keyExportRes.len;
}
else if (*outSz < packet->keyExportRes.len) {
ret = WH_ERROR_ABORTED;
billphipps marked this conversation as resolved.
Show resolved Hide resolved
}
else {
XMEMCPY(out, packOut, packet->keyExportRes.len);
*outSz = packet->keyExportRes.len;
}
if (label != NULL)
XMEMCPY(label, packet->keyExportRes.label, labelSz);
}
}
return ret;
}

int wh_Client_KeyCommitRequest(whClientContext* c, whNvmId keyId)
billphipps marked this conversation as resolved.
Show resolved Hide resolved
{
whPacket packet[1] = {0};
if (c == NULL || keyId == WOLFHSM_ID_ERASED)
return WH_ERROR_BADARGS;
/* set keyId */
packet->keyCommitReq.id = keyId;
/* write request */
return wh_Client_SendRequest(c, WH_MESSAGE_GROUP_KEY, WH_KEY_COMMIT,
WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->keyCommitReq),
(uint8_t*)packet);
}

int wh_Client_KeyCommitResponse(whClientContext* c)
{
uint16_t group;
uint16_t action;
uint16_t size;
int ret;
whPacket packet[1] = {0};
if (c == NULL)
return WH_ERROR_BADARGS;
ret = wh_Client_RecvResponse(c, &group, &action, &size, (uint8_t*)packet);
if (ret == 0) {
if (packet->rc != 0)
ret = packet->rc;
}
return ret;
}

int wh_Client_KeyEraseRequest(whClientContext* c, whNvmId keyId)
billphipps marked this conversation as resolved.
Show resolved Hide resolved
{
whPacket packet[1] = {0};
if (c == NULL || keyId == WOLFHSM_ID_ERASED)
return WH_ERROR_BADARGS;
/* set keyId */
packet->keyEraseReq.id = keyId;
/* write request */
return wh_Client_SendRequest(c, WH_MESSAGE_GROUP_KEY, WH_KEY_ERASE,
WOLFHSM_PACKET_STUB_SIZE + sizeof(packet->keyEraseReq),
(uint8_t*)packet);
}

int wh_Client_KeyEraseResponse(whClientContext* c)
{
uint16_t group;
uint16_t action;
uint16_t size;
int ret;
whPacket packet[1] = {0};
if (c == NULL)
return WH_ERROR_BADARGS;
ret = wh_Client_RecvResponse(c, &group, &action, &size, (uint8_t*)packet);
if (ret == 0) {
if (packet->rc != 0)
ret = packet->rc;
}
return ret;
}

void wh_Client_SetKeyCurve25519(curve25519_key* key, whNvmId keyId)
billphipps marked this conversation as resolved.
Show resolved Hide resolved
{
XMEMCPY(key->devCtx, (void*)&keyId, sizeof(keyId));
}

#if 0
int wh_Client_KeyCommit(whClientContext* c, uint16_t keyId);
int wh_Client_KeyErase(whClientContext* c, uint16_t keyId);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the simple busy-retry versions of the above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no those were leftover from before I realized it had to be asynchronous, removed

30 changes: 3 additions & 27 deletions src/wh_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,13 @@
#include "wolfhsm/wh_server.h"
#include "wolfhsm/wh_server_nvm.h"
#include "wolfhsm/wh_server_crypto.h"
#include "wolfhsm/wh_server_keystore.h"
#if defined(WOLFHSM_SHE_EXTENSION)
#include "wolfhsm/wh_server_she.h"
#endif

/** Forward declarations. */
/* TODO: Move these out to separate C files */
static int _wh_Server_HandleCommRequest(whServerContext* server,
uint16_t magic, uint16_t action, uint16_t seq,
uint16_t req_size, const void* req_packet,
uint16_t *out_resp_size, void* resp_packet);
static int _wh_Server_HandleKeyRequest(whServerContext* server,
uint16_t magic, uint16_t action, uint16_t seq,
uint16_t req_size, const void* req_packet,
uint16_t *out_resp_size, void* resp_packet);
static int _wh_Server_HandlePkcs11Request(whServerContext* server,
uint16_t magic, uint16_t action, uint16_t seq,
uint16_t req_size, const void* req_packet,
Expand Down Expand Up @@ -119,22 +112,6 @@ static int _wh_Server_HandleCommRequest(whServerContext* server,
return rc;
}

static int _wh_Server_HandleKeyRequest(whServerContext* server,
uint16_t magic, uint16_t action, uint16_t seq,
uint16_t req_size, const void* req_packet,
uint16_t *out_resp_size, void* resp_packet)
{
(void)server;
(void)magic;
(void)action;
(void)seq;
(void)req_size;
(void)req_packet;
(void)out_resp_size;
(void)resp_packet;
return 0;
}

static int _wh_Server_HandlePkcs11Request(whServerContext* server,
uint16_t magic, uint16_t action, uint16_t seq,
uint16_t req_size, const void* req_packet,
Expand Down Expand Up @@ -202,8 +179,8 @@ int wh_Server_HandleRequestMessage(whServerContext* server)
break;

case WH_MESSAGE_GROUP_KEY:
rc = _wh_Server_HandleKeyRequest(server, magic, action, seq,
size, data, &size, data);
rc = wh_Server_HandleKeyRequest(server, magic, action, seq,
data, &size);
break;

case WH_MESSAGE_GROUP_CRYPTO:
Expand Down Expand Up @@ -243,4 +220,3 @@ int wh_Server_HandleRequestMessage(whServerContext* server)
}
return rc;
}

Loading