Fix decoder panics on malformed frame headers and lengths (v4 backport) - #300
Open
plorenz wants to merge 3 commits into
Open
Fix decoder panics on malformed frame headers and lengths (v4 backport)#300plorenz wants to merge 3 commits into
plorenz wants to merge 3 commits into
Conversation
Backport of #301 to the v4 release line. - assigns the cached replyFor value on every path through cacheReplyFor, so ReplyFor, IsReply and IsReplyingTo cannot dereference a nil pointer when the ReplyFor header is present but not four bytes wide - rejects a wrong-width ReplyFor header in unmarshalHeaders, so a malformed frame is dropped rather than silently handled as a non-reply - adds coverage for malformed, well-formed and absent ReplyFor headers, and for the unmarshal rejection
plorenz
force-pushed
the
fix/v4-reply-for-nil-deref
branch
from
August 24, 2026 20:04
9b2e1b8 to
36a1b2e
Compare
Backport of #303 to the v4 release line. - compares the header end offset as uint64 in unmarshalHeaders, so a declared length above MaxInt32 is rejected rather than converted to a negative int on a 32-bit platform and then panicking on the slice - sums the header and body lengths as uint64 in unmarshalV2 and rejects a total that will not fit an int32, so the uint32 sum can no longer wrap to a value that satisfies every subsequent check - adds regression coverage for declared lengths that overflow an int and for sums that wrap or exceed MaxInt32 The int32 bound is a representability limit so the total is exact on 32-bit platforms, not a policy limit on message size.
plorenz
changed the title
- rewrites the added regression-test comments to describe the malformed input and the required outcome, leaving what the code used to do to git history - keeps the GOARCH=386 caveat, which is a live limit on what the test exercises rather than a note about a past change
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #302 and #304. Backport of #299 to the v4 release line, so ziti 1.6.x and
2.0.x can pick it up.
Three defects in the V2 frame decoder, all of which let a peer panic the rx
goroutine, which has no recover above it.
ReplyFor cached as a nil pointer (#302). A
ReplyForheader present but notfour bytes wide left the cached
replyForunset, because the fallback thatdefaults it to -1 tested the header's byte slice rather than the pointer field
it was defaulting.
ReplyFor,IsReplyandIsReplyingTothen dereferencednil, and
IsReplyruns on every inbound message.Header length truncated to int (#304).
unmarshalHeaderscomparedi + 8 + int(length)against the buffer length. Whereintis 32 bits,int(length)is negative above MaxInt32, so the check passed and the slicepanicked. Now compared as
uint64.Header and body lengths summed as uint32 (#304).
unmarshalV2allocatedmake([]byte, headersLength+bodyLength), and the wrapped sum satisfied everycheck that followed. Now summed as
uint64and rejected if it will not fit anint32.The ReplyFor and uint32-wrap defects are reachable only by authenticated peers,
behind the bind handler. The int-truncation defect is reachable through
rxHello, before any authentication, on 32-bit builds only.Same changes as #299, adapted to this branch's logging and header constants.
Verified on amd64 and under
GOARCH=386.