Apply checking of req_size in handlers

[padelsbach]

Several functions in wh_server_she.c ignored the received payload length with (void)req_size;. This change adds size checking and associated unit tests.

[bigbrett]

I believe this leaks a non-SHE error code. This module can only return SHE errors. We have an error code translation function _TranslateSheReturnCode(). Check the rest of the code for similar issues plz.

[padelsbach]

Good catch. Line 220 sets resp.rc = ret unlike the other functions. This is a pre-existing bug. Will fix

[padelsbach]

This was also a pre-existing issue. out_resp_size was not set by this handler.

This is finding 253

[padelsbach]

This change ensures the response is sent, even if a handler found an issue. Credit to Claude for this.

Edit: this is finding 770

[bigbrett]

Great work. A few more issues I noticed, but getting closer.

[bigbrett]

now that this is guarded by ret==0, the resp structure needs to be either initialized, or this field needs to be set unconditionally. otherwise the case where ret==error means a garbage stack value could be sent back to the caller.

Note that multiple handlers now have this issue - before the response fields were set unconditionally, but now they are gated on success return code, which means failure case could echo garbage back to the caller. Here are some of the cases and what ends up being leaked back to client via uninitialized stack-allocated response structure

  1. _SecureBootInit — status
  2. _SecureBootUpdate — status
  3. _SecureBootFinish — status
  4. _GetStatus — sreg
  5. _InitRnd — status
  6. _ExtendSeed — status
  7. _Rnd — rnd[16] (16 bytes of stack)
  8. _ExportRamKey — 112 bytes of stack data
  9. _GenerateMac — mac[16]
  10. _VerifyMac — status

Pretty sure most of the client APIs are smart about checking resp.rc before copying out but we shouldn't rely on this

[padelsbach]

Agreed, could leak stack bytes through the uninitialized response. Will init these to 0.

[bigbrett]

should probably check that resp.rc is sane based on the input data too, right? Should have a failure value? Same comment for all these test cases.

[padelsbach]

done

[bigbrett]

remove void cast now that it is used

[padelsbach]

done

[bigbrett]

In the SHE spec, ERC_KEY_NOT_AVAILABLE means the requested key slot is empty or has not been provisioned. So this is not appropriate if _AesMp16() were to fail. This particular fail case should just map to ERC_GENERAL_ERROR, either set directly, or preserve wolfCrypt error code and have it caught at the end via fallthrough to _TranslateSheReturnCode()

[padelsbach]

Fixed

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #299

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
Findings: 4

Medium (3)

Missing WH_SHE_ERC_KEY_NOT_AVAILABLE error translation in _LoadKey

File: src/wh_server_she.c:499-515
Function: _LoadKey
Category: Incorrect error handling

The refactoring of _LoadKey removed the else { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; } branch that previously translated a wh_Server_KeystoreReadKey failure into the SHE-spec-mandated WH_SHE_ERC_KEY_NOT_AVAILABLE error code. Now when the auth key read fails, the raw keystore error code propagates through instead of being mapped to the correct SHE error. This is inconsistent with all other handlers in this file (_EncEcb, _EncCbc, _DecEcb, _DecCbc, _GenerateMac, _VerifyMac, _ExportRamKey) which all preserve the WH_SHE_ERC_KEY_NOT_AVAILABLE translation after a wh_Server_KeystoreReadKey failure. The raw error code will be passed to _TranslateSheReturnCode(ret) for the response, which may not map it to KEY_NOT_AVAILABLE, and it is also returned directly from the function.

if (ret == 0) {
        keySz = sizeof(kdfInput);
        ret = wh_Server_KeystoreReadKey(server,
                                        WH_MAKE_KEYID(WH_KEYTYPE_SHE,
                                                      server->comm->client_id,
                                                      _PopAuthId(req.messageOne)),
                                        NULL, kdfInput, &keySz);
    }
    /* make K2 using AES-MP(authKey | WH_SHE_KEY_UPDATE_MAC_C) */
    if (ret == 0) {
        memcpy(kdfInput + keySz, _SHE_KEY_UPDATE_MAC_C,
               sizeof(_SHE_KEY_UPDATE_MAC_C));
        ret = _AesMp16(server, kdfInput, keySz + sizeof(_SHE_KEY_UPDATE_MAC_C),
                       tmpKey);
        if (ret != 0) {
            ret = WH_SHE_ERC_GENERAL_ERROR;
        }
    }

Recommendation: Add an else branch after wh_Server_KeystoreReadKey to translate the error, consistent with all other handlers:

    if (ret == 0) {
        keySz = sizeof(kdfInput);
        ret = wh_Server_KeystoreReadKey(server, ..., kdfInput, &keySz);
        if (ret != 0) {
            ret = WH_SHE_ERC_KEY_NOT_AVAILABLE;
        }
    }

---

Integer overflow in req_size validation can bypass bounds checks on 32-bit platforms

File: src/wh_server_she.c:1148-1157
Function: _EncEcb
Category: Integer overflows

The newly added size checks use expressions like req_size < (sizeof(req) + field) where field is a uint32_t derived from attacker-controlled req.sz. On 32-bit platforms (common for HSMs), sizeof(req) + field is computed in 32-bit size_t arithmetic and can wrap around to a small value when field is close to UINT32_MAX. If sizeof(req) and sizeof(resp) are both multiples of 16 (AES_BLOCK_SIZE), an attacker can craft req.sz such that both (sizeof(req) + field) and (sizeof(resp) + field) wrap to 0 or a small value, bypassing both the input and output buffer size checks. This would allow reading past req_packet and writing past resp_packet. The same pattern appears in _EncCbc, _DecEcb, and _DecCbc.

field = req.sz;
field -= (field % AES_BLOCK_SIZE);
if (req_size < (sizeof(req) + field)) {
    ret = WH_ERROR_BUFFER_SIZE;
}
else if ((sizeof(resp) + field) > WOLFHSM_CFG_COMM_DATA_LEN) {
    ret = WH_ERROR_BUFFER_SIZE;
}

Recommendation: Add an explicit upper-bound check on field (or req.sz) before the addition, e.g., if (field > WOLFHSM_CFG_COMM_DATA_LEN) as an early reject. Alternatively, cast operands to uint64_t or use a safe addition helper that detects overflow: if (field > SIZE_MAX - sizeof(req) || req_size < (sizeof(req) + field)).

---

Integer overflow in _VerifyMac second addition bypasses size check

File: src/wh_server_she.c:1509-1513
Function: _VerifyMac
Category: Integer overflows

The _VerifyMac function correctly checks for overflow in totalLen = req.messageLen + req.macLen with (totalLen < req.messageLen). However, the subsequent check req_size < (sizeof(req) + totalLen) has the same 32-bit integer overflow issue. When totalLen passes the first check (no wrap in the addition), it can still be close to UINT32_MAX (e.g., messageLen = 0x7FFFFFFF, macLen = 0x7FFFFFFF yields totalLen = 0xFFFFFFFE). On a 32-bit platform, sizeof(req) + 0xFFFFFFFE wraps to a small value, bypassing the bounds check. This could allow mac = message + req.messageLen to point past the request buffer, leading to an out-of-bounds read during CMAC verification.

totalLen = req.messageLen + req.macLen;
if ((totalLen < req.messageLen) ||
    (req_size < (sizeof(req) + totalLen))) {
    ret = WH_ERROR_BUFFER_SIZE;
}

Recommendation: Add an overflow check for the second addition as well: if ((totalLen > SIZE_MAX - sizeof(req)) || (req_size < (sizeof(req) + totalLen))). Alternatively, check totalLen > WOLFHSM_CFG_COMM_DATA_LEN before adding sizeof(req).

---

Low (1)

_LoadKey lost WH_SHE_ERC_KEY_NOT_AVAILABLE error mapping for KeystoreReadKey failure

File: src/wh_server_she.c:498-511
Function: _LoadKey
Category: SHE extension vulnerabilities

The old code explicitly mapped wh_Server_KeystoreReadKey failure to WH_SHE_ERC_KEY_NOT_AVAILABLE. The refactored code removes this mapping — when wh_Server_KeystoreReadKey fails, the raw internal error code now propagates through the function and is only translated at the end by _TranslateSheReturnCode(ret). If _TranslateSheReturnCode does not properly map all internal wolfHSM error codes to SHE-specific error codes, this could leak internal error code values to the client, providing information about the server's internal state. The old else { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; } block was removed without an equivalent replacement.

if (ret == 0) {
    keySz = sizeof(kdfInput);
    ret = wh_Server_KeystoreReadKey(server,
                                    WH_MAKE_KEYID(WH_KEYTYPE_SHE,
                                                  server->comm->client_id,
                                                  _PopAuthId(req.messageOne)),
                                    NULL, kdfInput, &keySz);
}
/* make K2 using AES-MP(authKey | WH_SHE_KEY_UPDATE_MAC_C) */
if (ret == 0) {
    ...

Recommendation: Either restore the explicit WH_SHE_ERC_KEY_NOT_AVAILABLE mapping after the wh_Server_KeystoreReadKey call (e.g., if (ret != 0) { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; }), or verify that _TranslateSheReturnCode correctly maps all possible internal error codes returned by wh_Server_KeystoreReadKey to appropriate SHE error codes.

---

This review was generated automatically by Fenrir. Findings are non-blocking.

[padelsbach]

Addressed the overflow possibility raised by @wolfSSL-Fenrir-bot. Will not fix the WH_SHE_ERC_KEY_NOT_AVAILABLE concerns based on comment above from Brett

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #299

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
Findings: 2

Medium (1)

_LoadKey lost WH_SHE_ERC_KEY_NOT_AVAILABLE error mapping for auth key read failure

File: src/wh_server_she.c:497-523
Function: _LoadKey
Category: Incorrect error handling

The refactoring of _LoadKey removed the else { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; } branch that mapped wh_Server_KeystoreReadKey failures to the SHE-specified error code. Previously, if the auth key could not be read, ret was explicitly set to WH_SHE_ERC_KEY_NOT_AVAILABLE. In the new code, the raw error code from wh_Server_KeystoreReadKey propagates, and _TranslateSheReturnCode maps it to WH_SHE_ERC_GENERAL_ERROR instead of the correct WH_SHE_ERC_KEY_NOT_AVAILABLE. All other handlers that call wh_Server_KeystoreReadKey (_EncEcb, _EncCbc, _DecEcb, _DecCbc, _GenerateMac, _VerifyMac) correctly preserved the else { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; } pattern, making this an inconsistent copy-paste omission.

if (ret == 0) {
        keySz = sizeof(kdfInput);
        ret = wh_Server_KeystoreReadKey(server,
                                        WH_MAKE_KEYID(WH_KEYTYPE_SHE,
                                                      server->comm->client_id,
                                                      _PopAuthId(req.messageOne)),
                                        NULL, kdfInput, &keySz);
    }
    /* make K2 using AES-MP(authKey | WH_SHE_KEY_UPDATE_MAC_C) */
    if (ret == 0) {
        ...
        ret = _AesMp16(server, kdfInput, keySz + sizeof(_SHE_KEY_UPDATE_MAC_C),
                       tmpKey);
        if (ret != 0) {
            ret = WH_SHE_ERC_GENERAL_ERROR;
        }
    }
    /* Missing: else { ret = WH_SHE_ERC_KEY_NOT_AVAILABLE; } */

Recommendation: Add an else branch after the wh_Server_KeystoreReadKey call (or between the two if (ret == 0) blocks) to map keystore read failures to WH_SHE_ERC_KEY_NOT_AVAILABLE, matching the pattern used in all other SHE handlers:

if (ret == 0) {
    keySz = sizeof(kdfInput);
    ret = wh_Server_KeystoreReadKey(server, ..., NULL, kdfInput, &keySz);
    if (ret != 0) {
        ret = WH_SHE_ERC_KEY_NOT_AVAILABLE;
    }
}

---

Low (1)

_SetUid mixes ret == 0 and ret == WH_SHE_ERC_NO_ERROR guard conditions

File: src/wh_server_she.c:193-223
Function: _SetUid
Category: Logic errors

The PR introduced ret == 0 checks for the new buffer-size and translate guards, but the pre-existing memcpy guard still uses ret == WH_SHE_ERC_NO_ERROR. While WH_SHE_ERC_NO_ERROR is almost certainly defined as 0, the function now operates across two error-code domains (wolfHSM generic errors like WH_ERROR_BUFFER_SIZE and SHE-specific errors like WH_SHE_ERC_SEQUENCE_ERROR) and checks them inconsistently. If WH_SHE_ERC_NO_ERROR were ever redefined to a non-zero sentinel value, the memcpy guard would silently break. No other handler in this file mixes these two comparison styles.

if (ret == 0) {
        ret = wh_MessageShe_TranslateSetUidRequest(
            magic, (whMessageShe_SetUidRequest*)req_packet, &req);
    }

    if ((ret == 0) && (server->she->uidSet == 1)) {
        ret = WH_SHE_ERC_SEQUENCE_ERROR;
    }

    if (ret == WH_SHE_ERC_NO_ERROR) {
        memcpy(server->she->uid, req.uid, sizeof(req.uid));
        server->she->uidSet = 1;
    }

Recommendation: Change if (ret == WH_SHE_ERC_NO_ERROR) to if (ret == 0) for consistency with the other guards in the same function, or change all the ret == 0 checks to ret == WH_SHE_ERC_NO_ERROR.

---

This review was generated automatically by Fenrir. Findings are non-blocking.