Harden chain depth bounds and parser input validation

[ColtonWilley]

Summary

• Enforce MAX_CHAIN_DEPTH limits in OCSP chain processing (SendCertificateStatus, ProcessChainOCSPRequest), certificate loading (ProcessUserChain), and TLS 1.3 certificate sending (SendTls13Certificate). Add idx bounds checks to chain accessors in ssl.c.
• Harden SNI extension parser (TLSX_SNI_GetFromBuffer) with length validation preventing buffer overreads on malformed ClientHello. Require exact listLen == extLen - OPAQUE16_LEN match to prevent extension boundary misalignment.
• Fix off-by-one in TLSX_CSR_Free (<= to <) — csr->requests is a count, and a full-depth chain with leaf OCSP could push the free loop one past the request.ocsp[] array.
• Add remaining-buffer bounds checks to PKCS7 decoders: DecodeEnvelopedData and DecodeAuthEnvelopedData (encryptedContentSz, authTagSz), DecodeEncryptedData (two sites), SignedData null signature tag, and PwriKek_KeyUnWrap cekLen off-by-4.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m0plus

• FLASH: .text +68 B (+0.1%, 63,231 B / 262,144 B, total: 24% used)

gcc-arm-cortex-m3

• FLASH: .text +356 B (+0.3%, 120,929 B / 262,144 B, total: 46% used)

gcc-arm-cortex-m4

• FLASH: .text +320 B (+0.2%, 198,254 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m4-baremetal

• FLASH: .text +64 B (+0.1%, 65,803 B / 262,144 B, total: 25% used)

gcc-arm-cortex-m4-crypto-only

• FLASH: .text +256 B (+0.1%, 173,294 B / 262,144 B, total: 66% used)

gcc-arm-cortex-m4-dtls13

• FLASH: .text +768 B (+0.4%, 179,352 B / 1,048,576 B, total: 17% used)

gcc-arm-cortex-m4-min-ecc

• FLASH: .text +64 B (+0.1%, 60,781 B / 262,144 B, total: 23% used)

gcc-arm-cortex-m4-openssl-compat

• FLASH: .rodata +24 B, .text +1,408 B (+0.2%, 765,740 B / 1,048,576 B, total: 73% used)

gcc-arm-cortex-m4-pkcs7

• FLASH: .text +256 B (+0.1%, 210,673 B / 262,144 B, total: 80% used)

gcc-arm-cortex-m4-pq

• FLASH: .text +832 B (+0.3%, 276,952 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m4-rsa-only

• FLASH: .text +576 B (+0.2%, 322,168 B / 1,048,576 B, total: 31% used)

gcc-arm-cortex-m4-sp-math

• FLASH: .text +64 B (+0.1%, 60,781 B / 262,144 B, total: 23% used)

gcc-arm-cortex-m4-tls12

• FLASH: .text +384 B (+0.3%, 121,677 B / 262,144 B, total: 46% used)

gcc-arm-cortex-m4-tls13

• FLASH: .text +768 B (+0.3%, 234,208 B / 262,144 B, total: 89% used)

gcc-arm-cortex-m7

• FLASH: .text +320 B (+0.2%, 198,254 B / 262,144 B, total: 76% used)

gcc-arm-cortex-m7-pq

• FLASH: .text +832 B (+0.3%, 277,528 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m7-tls13

• FLASH: .text +832 B (+0.4%, 234,272 B / 262,144 B, total: 89% used)

linuxkm-standard

• Data: __patchable_function_entries -8 B (-0.0%, 45,872 B)

stm32-sim-stm32h753

• FLASH: .text +64 B (+0.0%, 181,492 B / 2,097,152 B, total: 9% used)
  No memory changes detected for:
• linuxkm-pie

[github-actions]

retest this please

[Copilot]

Pull request overview

This PR hardens certificate-chain depth handling and improves parser input validation to prevent out-of-bounds access and buffer overreads, with accompanying API/regression tests.

Changes:

• Enforces MAX_CHAIN_DEPTH in certificate chain loading/sending paths and adds index bounds checks to chain accessors.
• Hardens SNI extension parsing with stricter length validation and fixes an off-by-one in OCSP CSR cleanup.
• Adds PKCS7 AuthEnvelopedData truncation regression coverage and updates API tests; removes a DTLS oversized-chain test.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
wolfcrypt/src/pkcs7.c	Adds remaining-buffer bounds checks during AuthEnvelopedData parsing.
src/tls.c	Tightens SNI extension length validation; fixes OCSP CSR free-loop off-by-one.
src/ssl.c	Adds idx bounds checks for peer-chain accessors (length/cert/X509).
src/ssl_load.c	Enforces MAX_CHAIN_DEPTH during user chain parsing/loading.
src/internal.c	Bounds OCSP chain processing loops by MAX_CHAIN_DEPTH.
src/tls13.c	Rejects TLS 1.3 certificate sends when chain count exceeds MAX_CHAIN_DEPTH.
tests/api/test_pkcs7.h	Declares/registers new PKCS7 truncated AuthEnvelopedData test.
tests/api/test_pkcs7.c	Adds new truncation regression test for AuthEnvelopedData decode.
tests/api/test_dtls.h	Removes declaration/registration of oversized-chain DTLS 1.3 test.
tests/api/test_dtls.c	Removes implementation of oversized-chain DTLS 1.3 test.
tests/api.c	Adds tests for chain accessor idx bounds and max-depth chain buffer rejection.

---

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

[julek-wolfssl]

Copilot has some valid points

[ColtonWilley]

Copilot has some valid points

Agreed, I have pushed updates to address the issues.

[ColtonWilley]

Jenkins retest this please

Fixed

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] MAX_CHAIN_DEPTH check placement can reject a valid maximum-depth chain that has trailing data — src/ssl_load.c:328-333
• [Low] Max-depth load test overshoots the limit and omits the exact-boundary success case — tests/api.c:3866-3915
• [Info] PR description lists pkcs7.c changes that are not present in the diff — wolfcrypt/src/pkcs7.c:14971-14975,15239-15243
• [Info] SendTls13Certificate depth guard is now defense-in-depth only and its direct test path was removed — src/tls13.c:9523-9526

Review generated by Skoll

[dgarske]

🟠 [Medium] MAX_CHAIN_DEPTH check placement can reject a valid maximum-depth chain that has trailing data

The new depth check if (cnt >= MAX_CHAIN_DEPTH) is placed at the TOP of the parse loop, before DataToDerBuffer. The loop is commonly entered one extra time after the last real certificate to consume trailing data (trailing newline/whitespace), and that extra entry is normally handled gracefully by the ret == ASN_NO_PEM_HEADER && gotOne branch further down. For a chain of EXACTLY MAX_CHAIN_DEPTH (default 9) valid certificates followed by any unconsumed trailing bytes, cnt equals MAX_CHAIN_DEPTH when the loop re-enters, so the new check fires FIRST and returns MAX_CHAIN_ERROR instead of letting the existing ASN_NO_PEM_HEADER path terminate cleanly with ret = 0. This regresses a valid maximum-length chain to a load failure. Note certs/server-cert.pem in this repo is itself a 2-cert PEM, so multi-cert PEM with trailing bytes is the norm; the boundary is real, just narrow (requires hitting exactly the max depth). The PR's own new test uses MAX_CHAIN_DEPTH + 1 copies (overshoots) and never exercises the exact boundary.

Fix: Move the cnt >= MAX_CHAIN_DEPTH check so it only rejects when an additional real certificate is actually present (i.e., after DataToDerBuffer and after the ASN_NO_PEM_HEADER && gotOne trailing-data handling). Add a test that loads exactly MAX_CHAIN_DEPTH certificates (with a trailing newline) and asserts success, in addition to the over-limit case.

[dgarske]

🔵 [Low] Max-depth load test overshoots the limit and omits the exact-boundary success case

The test sets nCerts = MAX_CHAIN_DEPTH + 1 copies of svrCertFile, but server-cert.pem contains 2 certificates, so the buffer actually holds ~2*(MAX_CHAIN_DEPTH+1) certs. It therefore only proves that a grossly-oversized chain is rejected and does not pin the boundary. It is missing the complementary positive case: a chain of exactly MAX_CHAIN_DEPTH certificates must still load successfully. That gap is exactly what would let the placement issue in ProcessUserChain (separate finding) slip through. The const int nCerts = MAX_CHAIN_DEPTH + 1; naming/comment is also misleading given each file is multi-cert.

Fix: Add a positive boundary test (exactly MAX_CHAIN_DEPTH certs -> success) and clarify the per-file cert count in the comment.

[dgarske]

⚪ [Info] PR description lists pkcs7.c changes that are not present in the diff

The PR body claims bounds checks were added to DecodeEnvelopedData, DecodeEncryptedData (two sites), SignedData null signature tag, and PwriKek_KeyUnWrap (cekLen off-by-4). The actual pkcs7.c diff only contains the two wc_PKCS7_DecodeAuthEnvelopedData checks (encryptedContentSz and authTagSz, both under #ifdef NO_PKCS7_STREAM). Either the commit is incomplete or the description is stale. The two checks that ARE present are correct: idx is advanced only past validated length bytes so pkiMsgSz - idx cannot underflow, and the authTagSz check is a genuine fix (the existing authTagSz > sizeof(authTag) check only guards the destination, not the pkiMsg source read at the subsequent XMEMCPY).

Fix: Confirm whether the other PKCS7 decoder hardening described in the PR body was intended for this commit. If not, trim the description to match the diff so reviewers/changelog stay accurate.

[dgarske]

⚪ [Info] SendTls13Certificate depth guard is now defense-in-depth only and its direct test path was removed

With the new ProcessUserChain limit, an oversized chain can no longer be loaded (it fails with MAX_CHAIN_ERROR at load time), so the new ssl->buffers.certChainCnt > MAX_CHAIN_DEPTH guard inside SendTls13Certificate is effectively unreachable via the normal load path -- it is a reasonable belt-and-suspenders check, but untested. The PR simultaneously deletes test_dtls13_oversized_cert_chain, which was the only test that drove an oversized chain through the actual TLS1.3 certificate-send path. The deletion itself is justified (that test expected WOLFSSL_SUCCESS on load, which is no longer possible), and the test_dtls13.h macro list trailing-comma update is correct. Net effect is reduced direct coverage of the send-side guard.

Fix: Optional: keep the guard (cheap and defensive) but note in a comment that it is unreachable under normal loading, or add a unit test that sets ssl->buffers.certChainCnt directly to exercise the MAX_CHAIN_ERROR return.