Refactor record padding handling to eliminate middle padding pattern

[julek-wolfssl]

Move padSz index advancement from individual message handlers to a
single location at the end of record processing in ProcessReply.
Previously, each handler (DoFinished, DoApplicationData, DoAlert,
DoCertificate, DoServerHello, etc.) advanced the index by padSz,
which then had to be corrected when processing multiple messages
in one record. This "middle padding" pattern was error-prone.

Now curSize is reduced by padSz after decryption/verification so
it reflects content size only. Message handlers advance the index
by content size only. padSz is added once when the record is fully
processed. The correction code for multi-message records is removed.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors TLS record padding handling so padSz is accounted for once per record (in ProcessReply/DoProcessReplyEx) instead of being applied and “corrected” within each individual message handler.

Changes:

• Removes per-message padSz index advancement across TLS 1.3 and (D)TLS message handlers.
• Adjusts DoProcessReplyEx to treat curSize as content-only by subtracting padSz, and adds padSz once when the record is fully processed.
• Updates multi-message record flow to eliminate the prior “middle padding” correction, and restricts buffer shrinking during multi-message processing.

Reviewed changes

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

File	Description
src/tls13.c	Removes per-handler padSz advancement so TLS 1.3 handlers advance by content length only.
src/internal.c	Centralizes padSz accounting in DoProcessReplyEx, updating message parsing logic and removing old padding correction paths.

Comments suppressed due to low confidence (4)

src/internal.c:1

• Subtracting padSz from ssl->curSize without first verifying curSize >= padSz can underflow on malformed/truncated records and corrupt subsequent parsing/state. Add a guard (e.g., if ssl->curSize < ssl->keys.padSz return a length/buffer error) before performing the subtraction.
  src/internal.c:1
• Advancing inputBuffer.idx by padSz has no bounds validation. If padding/tag bytes are missing (corrupted/truncated input), idx can jump beyond the actual buffered record data and leave the connection state inconsistent. Add a bounds check before incrementing (e.g., ensure idx + padSz does not exceed the available buffered data for the current record) and fail with a parse error if it would.
  src/internal.c:1
• inputLength is reduced by padSz without checking that inputLength >= padSz. When the remaining buffered bytes are smaller than padSz, this will underflow (likely to a very large word32) and can cause oversized copies/allocations or out-of-bounds reads later. Add an explicit check before subtracting and error out when insufficient bytes remain.
  src/internal.c:1
• The parameter size is used (if (size)), so (void)size; is redundant and can be removed to reduce confusion.

---

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

[julek-wolfssl]

Retest this please history lost.....

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/internal.c:1

• ssl->curSize is an unsigned type; subtracting padSz can underflow if padSz > curSize (e.g., malformed/short records or if padSz is unexpectedly large), which would inflate curSize and break subsequent bounds checks (including the new curStartIdx + curSize calculations). Make this subtraction conditional on ssl->curSize >= ssl->keys.padSz (return an error like BUFFER_E/LENGTH_ERROR otherwise), and avoid truncating padSz to word16 when comparing/subtracting.
  src/internal.c:1
• This DTLS 'drop record outside receiving window' path adds ssl->keys.padSz unconditionally, but curSize is only reduced by padSz when IsEncryptionOn(ssl, 0) is true. If encryption is off (or if padSz is stale from a prior encrypted record), this can skip too far and desynchronize parsing. Add padSz here only when the current record is encrypted (or ensure padSz is reset to 0 for plaintext records before this path).
  src/internal.c:1
• After advancing inputBuffer.idx by padSz, the code only checks >= length (and returns), but does not prevent idx from becoming > length. Leaving inputBuffer.idx past the buffer end can violate invariants for subsequent calls/paths. Add an explicit if (idx > length) return BUFFER_E; (or equivalent) immediately after the idx += padSz increment (and similarly in other padSz-advance early-return paths).

---

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

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 4 total — 3 posted, 2 skipped

Posted findings

• [Medium] Missing defensive guard before curSize -= padSz could allow word16 underflow — src/internal.c:23184-23185
• [Medium] Callers of message handlers still pass inputBuffer.length as totalSz, not content boundary — src/internal.c:23286-23289,23306-23309,23343-23346,23360-23362
• [Medium] No dedicated test for multi-message encrypted record processing after refactoring — src/internal.c:23654-23695

Skipped findings

• [Low] Truncation cast of word32 padSz to word16 suppresses potential warnings
• [Medium] Callers of message handlers still pass inputBuffer.length as totalSz, not content boundary

Review generated by Skoll via openclaw

[dgarske]

🟡 [Medium] Missing defensive guard before curSize -= padSz could allow word16 underflow
💡 SUGGEST bug

The new centralized padding subtraction ssl->curSize -= (word16)ssl->keys.padSz performs unsigned word16 arithmetic. If padSz exceeds curSize due to a malformed record or bug in MAC verification, this underflows to a very large value (e.g., curSize=50, padSz=100 → curSize=65486).

For TLS 1.3, removeMsgInnerPadding guarantees padSz <= curSize by construction (scan-based). For non-TLS-1.3, VerifyMac should guarantee this, but there is no explicit check at the subtraction site.

Importantly, the MAX_PLAINTEXT_SZ check at lines 23245/23263 catches the underflow for pre-TLS-1.3 (ETM and non-ETM paths). However, for TLS 1.3 these checks are skipped (comment at line 23261: 'TLS13 plaintext limit is checked earlier before decryption'), so if the invariant were ever violated in TLS 1.3, the underflowed curSize would propagate to message handlers unchecked.

Suggestion:

Suggested change

	if (IsEncryptionOn(ssl, 0))
	/* Reduce curSize to content only, excluding padding/MAC overhead.
	* padSz is accounted for once at the end of record processing. */
	if (IsEncryptionOn(ssl, 0)) {
	if (ssl->keys.padSz > ssl->curSize)
	return BUFFER_E;
	ssl->curSize -= (word16)ssl->keys.padSz;
	}

[julek-wolfssl]

Added.

[dgarske]

🟡 [Medium] No dedicated test for multi-message encrypted record processing after refactoring
💡 SUGGEST test

The core change in this PR is eliminating the 'middle padding' correction pattern where padding was added per-handler and then corrected at record end. The existing test suite provides indirect coverage through standard TLS/DTLS handshakes (e.g., test_records_span_network_boundaries, test_tls13_middlebox_compat_empty_session_id), but there is no dedicated test that:

1. Constructs a single TLS 1.2 encrypted record containing multiple handshake messages
2. Verifies all messages are processed correctly
3. Verifies the index advances by exactly content_size + padSz at the end

This refactoring removes ~15 separate *inOutIdx += ssl->keys.padSz call sites and replaces them with a single centralized addition. A regression in any handler's index advancement would cause subtle protocol-level failures that might not be caught by existing tests.

Recommendation: Add a targeted test that sends a TLS 1.2 encrypted record with multiple handshake messages (e.g., Certificate + ServerKeyExchange in a single record) and verifies correct processing. This would directly exercise the refactored multi-message record loop and catch any regression in the centralized padding handling.

[julek-wolfssl]

Added a test in scripts/multi-msg-record.test.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[julek-wolfssl]

Failures unrelated to PR.

[JacobBarthelmeh]

Retest this please Jenkins

[JacobBarthelmeh]

This appears to be a good refactor. I need help on closer review of DTLS 1.3 changes though. @rizlik could you provide an additional review?

[douzzer]

retest this please
(results lost)

[dgarske]

Check for non ASCII here:

[check-source-text] [5 of 41] [3caf4b9da9]
    configure...   real 0m8.506s  user 0m5.226s  sys 0m4.103s
8 bit:
./scripts/multi-msg-record.test:29:# Locate python3 ¶ skip (exit 77) if unavailable.

[julek-wolfssl]

Fixed.

[rizlik]

Very nice!
Can you add a github action that install python script dependencies and run the test? My fear is that this test might be skipped because of the missing dependency and the multimessage is left untested in our CI.

[julek-wolfssl]

@rizlik added python setup to relevant workflows.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4

• FLASH: .text -448 B (-0.2%, 197,569 B / 262,144 B, total: 75% used)

gcc-arm-cortex-m4-baremetal

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

gcc-arm-cortex-m4-tls12

• FLASH: .text -384 B (-0.3%, 121,053 B / 262,144 B, total: 46% used)
  No memory changes detected for:
• gcc-arm-cortex-m4-min-ecc