Fixes: for GH #10068

[sebastian-carpenter]

Description

Various fixes related to GH #10068.

• Increased client hello lengths in WOLFSSL_ECH struct to be word32. This wasn't necessary but seems to follow our convention better
• The error from GetMsgHash was trampled. Fixed the checks. Added a check for when it returns 0 too (not sure if this was intentionally not checked)
• Added NULL check. Not sure how important this is but it's good defense.
• Mirrored arguments over to wc_HpkeContextOpenBase since *SealBase had them.
• ech->type is changed to EHC_TYPE_INNER but on error may not be changed back to ECH_TYPE_OUTER. May cause propagating issues so I restored the value in each return.
• Optimization: verify key length is exactly equal when setting the key so it doesn't fail later.

Testing

Broke api negative testing (with NULL parameters) into a standalone function. So far I have only written negative tests for what was in hpke_test_single but at some point all the other public hpke api's should be added too.

Checklist

• [ X] added tests
• updated/added doxygen
• updated appropriate READMEs
• Updated manual and documentation

[Copilot]

Pull request overview

Targets multiple ECH-related fixes for GH #10068, focusing on correcting length handling, error propagation, and defensive validation to prevent ECH/HPKE misuse and state inconsistencies.

Changes:

• Widened ECH length fields (aadLen, innerClientHelloLen) to word32 and updated related parsing/expansion logic.
• Fixed GetMsgHash() error handling (including 0 return) and added ECH NULL checks and state restoration on error paths.
• Hardened HPKE/ECH config validation (e.g., wc_HpkeContextOpenBase() NULL checks; enforce KEM-specific public key length in ECH config parsing).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
wolfssl/internal.h	Updates ECH struct field widths to avoid truncation issues.
wolfcrypt/src/hpke.c	Adds stricter argument validation to wc_HpkeContextOpenBase().
src/tls13.c	Fixes ECH acceptance hashing error propagation; restores ECH type on error; adds ECH extension NULL checks.
src/tls.c	Adjusts ECH write/parse paths for widened lengths and safer parsing.
src/ssl_ech.c	Validates ECH config HPKE public key length against KEM-specific expected length.

Comments suppressed due to low confidence (1)

src/tls.c:13682

• ech->innerClientHelloLen is now word32, but it is serialized into the ECH extension using a forced cast to word16. If innerClientHelloLen exceeds 65535 (the case this PR is addressing), the value written on the wire will be truncated while the code still writes innerClientHelloLen bytes of payload (and TLSX_ECH_GetSize() also sizes using the full word32). Add an explicit bounds check and fail (or clamp consistently) when innerClientHelloLen > 0xFFFF before calling c16toa(), rather than truncating.

            /* innerClientHelloLen */
            c16toa((word16)ech->innerClientHelloLen, writeBuf_p);
            writeBuf_p += 2;
            /* set payload offset for when we finalize */
            ech->outerClientPayload = writeBuf_p;
            /* write zeros for payload */
            XMEMSET(writeBuf_p, 0, ech->innerClientHelloLen);
            writeBuf_p += ech->innerClientHelloLen;

---

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

[sebastian-carpenter]

Jenkins retest this please.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

wolfcrypt/test/test.c:1

• The new negative-API coverage is valuable, but the repeated pattern (call, compare to BAD_FUNC_ARG, then translate/reset ret) is duplicated many times in hpke_test_api. To make it easier to extend coverage to more HPKE APIs (as noted in the PR description) and reduce copy/paste risk, consider factoring this into a small helper (e.g., a local function or macro like EXPECT_BAD_FUNC_ARG(expr) that sets ret appropriately).

---

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

[sebastian-carpenter]

Jenkins retest this please.

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 5 total — 5 posted, 0 skipped

Posted findings

• [Medium] Incomplete fix: ech->type not restored on TLSX_GetRequestSize error return — src/tls13.c:4757-4763
• [Low] Incomplete fix: type = 0 not updated to ECH_TYPE_OUTER for consistency — src/tls13.c:4766
• [Medium] No truncation guard on c16toa after widening innerClientHelloLen to word32 — src/tls.c:13699
• [Medium] Missing negative tests for changed HPKE context functions — wolfcrypt/test/test.c:32182
• [Medium] SetEchConfigsEx: wc_HpkeKemGetEncLen returns 0 for unsupported KEM, redundant with hpkePubkeyLen == 0 check — src/ssl_ech.c:551-553

Review generated by Skoll via openclaw

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/tls13.c:5720

• acceptOffset is derived from ((WOLFSSL_ECH*)args->echX->data)->confBuf - input without validating that confBuf is non-NULL and actually points within the current input buffer. Since EchCheckAcceptance() uses input + acceptOffset directly, an invalid/stale confBuf can lead to out-of-bounds reads during hashing/compare. Add bounds checks (e.g., ensure confBuf >= input and confBuf + ECH_ACCEPT_CONFIRMATION_SZ <= input + helloSz + headerSz) and return a clean error when the offset is invalid.

        /* account for hrr extension instead of server random */
        if (args->extMsgType == hello_retry_request) {
            args->acceptOffset =
                (word32)(((WOLFSSL_ECH*)args->echX->data)->confBuf - input);
            args->acceptLabel = (byte*)echHrrAcceptConfirmationLabel;

---

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

[sebastian-carpenter]

Jenkins retest this please.