Add missing input validation to VERIFY_ACERT and VERIFY cert handler

[jackctj117]

This pull request adds important input validation checks to the certificate verification request handlers in src/wh_server_cert.c. These changes improve the robustness and security of the server by ensuring that incoming requests are well-formed and do not cause buffer overflows or invalid memory accesses.

Input validation improvements:

• Added a check to ensure the request size is at least as large as the expected request structure before processing verify ACERT requests, returning an error if not (WH_ERROR_ABORTED).
• Added checks to both verify and verify ACERT request handlers to ensure the certificate data length does not exceed the remaining request size, returning an error (WH_ERROR_BADARGS) if the data would overflow. [1] [2]

[Copilot]

Pull request overview

Adds request-boundary validation to certificate verification handlers to prevent out-of-bounds reads and malformed request processing.

Changes:

• Added bounds checks to ensure cert_len does not exceed remaining request payload for VERIFY and VERIFY_ACERT.
• Added minimum request-size validation for VERIFY_ACERT before translating/parsing the request.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #317

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

High (2)

Missing minimum request size validation in Verify case allows unsigned underflow

File: src/wh_server_cert.c:517-524
Function: wh_Server_HandleCertRequest
Category: Incorrect error handling

The Verify case adds a bounds check req.cert_len > req_size - sizeof(req) but does not first validate that req_size >= sizeof(req). Since req_size is a uint16_t (unsigned), if req_size < sizeof(req), the subtraction req_size - sizeof(req) wraps around to a very large value, causing the check to always pass and defeating the intended validation. The VerifyAcert case (added in the same PR at line ~713) correctly includes both checks — first req_size < sizeof(req), then the cert_len check — indicating this is an oversight/omission in the Verify case.

/* Validate certificate data fits within request */
if (req.cert_len > req_size - sizeof(req)) {
    resp.rc = WH_ERROR_BADARGS;
    wh_MessageCert_TranslateVerifyResponse(
        magic, &resp,
        (whMessageCert_VerifyResponse*)resp_packet);
    *out_resp_size = sizeof(resp);
    break;
}

Recommendation: Add a minimum request size check before the cert_len validation, matching the pattern used in the VerifyAcert case:

/* Validate minimum request size */
if (req_size < sizeof(req)) {
    resp.rc = WH_ERROR_ABORTED;
    wh_MessageCert_TranslateVerifyResponse(
        magic, &resp,
        (whMessageCert_VerifyResponse*)resp_packet);
    *out_resp_size = sizeof(resp);
    break;
}

This should be placed before the TranslateVerifyRequest call (before the existing cert_len check).

---

Integer underflow in cert_len validation when req_size < sizeof(req)

File: src/wh_server_cert.c:518
Function: wh_Server_HandleCertRequest
Category: Message parsing flaws

The new validation check req.cert_len > req_size - sizeof(req) performs unsigned subtraction without first verifying that req_size >= sizeof(req). If req_size is smaller than sizeof(req), the subtraction wraps around to a very large unsigned value, causing the bounds check to always pass. This effectively bypasses the newly added certificate length validation, allowing cert_data to be read out of bounds. The PR author correctly added the missing minimum size check (if (req_size < sizeof(req))) in the second hunk for the VerifyAcert case (around line 716), but omitted the equivalent guard in the Verify case. The inconsistency between the two code paths strongly suggests this is an oversight.

wh_MessageCert_TranslateVerifyRequest(
                    magic, (whMessageCert_VerifyRequest*)req_packet, &req);

                /* Validate certificate data fits within request */
                if (req.cert_len > req_size - sizeof(req)) {
                    resp.rc = WH_ERROR_BADARGS;
                    ...
                    break;
                }

Recommendation: Add a minimum request size check before the cert_len validation, matching the pattern used in the VerifyAcert case:

/* Validate minimum request size */
if (req_size < sizeof(req)) {
    resp.rc = WH_ERROR_ABORTED;
    wh_MessageCert_TranslateVerifyResponse(
        magic, &resp,
        (whMessageCert_VerifyResponse*)resp_packet);
    *out_resp_size = sizeof(resp);
    break;
}

This should be placed before both the TranslateVerifyRequest call and the cert_len check.

---

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

[jackctj117]

Response to bot feedback:

The VERIFY case already has the minimum request size check at line 509 (if (req_size < sizeof(req))).
The cert_len validation on line 519 is inside the else branch, so req_size >= sizeof(req) is guaranteed
— no unsigned underflow is possible. The structure is slightly different from the VERIFY_ACERT case
(if/else vs early break) but the logic is equivalent.