Skip to content

transport/comm: bound peer-controlled lengths on receive paths #410

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

Open
Frauschi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

transport/comm: bound peer-controlled lengths on receive paths #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions src/wh_comm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,12 @@ int wh_CommClient_RecvResponse(whCommClient* context,
#ifdef WOLFHSM_CFG_ENABLE_TIMEOUT
(void)wh_Timeout_Stop(&context->respTimeout);
#endif
if (size < sizeof(*context->hdr)) {
/* Size is too small - transport-level corruption; treat as fatal
* and clear pending since the caller must tear down anyway. */
if ((size < sizeof(*context->hdr)) ||
(size > sizeof(context->packet))) {
/* Size out of range - transport-level corruption (the transport
* clamps its copy, but reports the true peer-controlled length, so
* an oversized value here means a truncated/bogus message). Treat as
* fatal and clear pending since the caller must tear down anyway. */
context->pending = 0;
rc = WH_ERROR_ABORTED;
}
Expand Down Expand Up @@ -369,7 +372,10 @@ int wh_CommServer_RecvRequest(whCommServer* context,
&size,
context->packet);
if (rc == 0) {
if (size < sizeof(*context->hdr)) {
/* size is the true peer-controlled length; the transport clamps its own
* copy, so an out-of-range value here is transport-level corruption. */
if ((size < sizeof(*context->hdr)) ||
(size > sizeof(context->packet))) {
rc = WH_ERROR_ABORTED;
}
if (rc == 0) {
Expand Down
28 changes: 24 additions & 4 deletions src/wh_transport_mem.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,11 @@ int wh_TransportMem_RecvResponse(void* c, uint16_t* out_len, void* data)
whTransportMemCsr req;
whTransportMemCsr resp;

/* A destination without a capacity (out_len) can't be clamped, so reject
* it rather than copy a peer-controlled length unbounded. */
if ( (context == NULL) ||
(context->initialized == 0)) {
(context->initialized == 0) ||
((data != NULL) && (out_len == NULL))) {
return WH_ERROR_BADARGS;
}

Expand All @@ -170,7 +173,13 @@ int wh_TransportMem_RecvResponse(void* c, uint16_t* out_len, void* data)
}

if ((data != NULL) && (resp.s.len != 0)) {
wh_Utils_memcpy_invalidate(data, context->resp_data, resp.s.len);
/* Clamp to the destination capacity so a corrupt/oversized resp.s.len
* can't overflow `data`. The true length is still reported below. */
uint16_t copy_len = resp.s.len;
if (copy_len > *out_len) {
copy_len = *out_len;
}
wh_Utils_memcpy_invalidate(data, context->resp_data, copy_len);
}

if (out_len != NULL) {
Expand Down Expand Up @@ -234,8 +243,11 @@ int wh_TransportMem_RecvRequest(void* c, uint16_t* out_len, void* data)
whTransportMemCsr req;
whTransportMemCsr resp;

/* A destination without a capacity (out_len) can't be clamped, so reject
* it rather than copy a peer-controlled length unbounded. */
if ( (context == NULL) ||
(context->initialized == 0)) {
(context->initialized == 0) ||
((data != NULL) && (out_len == NULL))) {
return WH_ERROR_BADARGS;
}

Expand All @@ -254,7 +266,15 @@ int wh_TransportMem_RecvRequest(void* c, uint16_t* out_len, void* data)
}

if ((data != NULL) && (req.s.len != 0)) {
wh_Utils_memcpy_invalidate(data, context->req_data, req.s.len);
/* req.s.len is peer-controlled. Clamp to the destination capacity
* so an oversized len can't overflow `data`. The true length is still
* reported below, so the comm layer rejects an over-length request
* rather than acting on a truncation. */
uint16_t copy_len = req.s.len;
if (copy_len > *out_len) {
copy_len = *out_len;
}
wh_Utils_memcpy_invalidate(data, context->req_data, copy_len);
}
if (out_len != NULL) {
*out_len = req.s.len;
Expand Down
Loading