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]

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

[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.