Review follow-up on the two preceding commits.
Only skip the padding when the record content is actually consumed. The
post-handshake auth advance ran on any pending return that left processReply
at doProcessInit, which assumes the certificate_request was the last message
in its record. Two states break that: a fragmented certificate_request makes
DoTls13HandShakeMsg() rewind inOutIdx so the fragment can be reprocessed, and
RFC 8446 section 5.1 lets a peer coalesce several handshake messages into one
record, leaving the index inside the record. Adding padSz in either case
points the index at record content. Both states are already mishandled
without this series, since post-handshake auth forces processReply to
doProcessInit regardless, so this is a narrower guard rather than a
regression, and there is no memory safety consequence either way: the largest
possible index is the end of the record. The new test mirrors the end of
record check that follows the message handlers.
Also restore buildArgsSet across the record size probe. The probe borrows
buildMsgState and buildArgsSet from the suspended build; the previous commit
put back only the first. FreeBuildMsgArgs() clears buildArgsSet
unconditionally on the way out, and the resumed call skips the block that
would set it again, so the flag stays clear for the rest of the record. That
is currently harmless, because the only thing it guards is freeing a
dynamically allocated IV and no cipher in the tree needs one, but it leaves
the fix one field short of its own premise.
Add test_record_size_preserves_build_msg_state, which parks a connection at
BUILD_MSG_ENCRYPT with the arguments flag set, runs the probe, and requires
both fields to survive. It fails without the restore and passes with it.
Guard the post-handshake auth block on WOLFSSL_POST_HANDSHAKE_AUTH as well.
The only code that leaves processReply at doProcessInit from inside
DoTls13HandShakeMsg() is the post-handshake auth branch, which is itself
behind that macro, so the check cannot fire without it.
Say that the oversized record is rejected rather than that it overflows the
buffer. BuildMessage() catches it and returns BUFFER_E; the old wording read
like memory corruption.
Verified with --enable-all --enable-asynccrypt-sw and with plain --enable-all,
both warning free under the os-check CFLAGS. The two reproducers from the
preceding commits still complete.
Tobias Frauenschläger
1c5382f169