nftables: validate csumOffset bounds in payloadSet evaluate#13161
Open
ibondarenko1 wants to merge 1 commit into
Open
nftables: validate csumOffset bounds in payloadSet evaluate#13161ibondarenko1 wants to merge 1 commit into
ibondarenko1 wants to merge 1 commit into
Conversation
payloadSet.evaluate at pkg/tcpip/nftables/nft_payload_set.go:154
reads the existing checksum via binary.BigEndian.Uint16(payload[op.csumOffset:])
and writes the updated checksum via checksum.Put at line 160. Neither
call validates op.csumOffset against len(payload) before access.
op.csumOffset is sourced from the optional netlink attribute
NFTA_PAYLOAD_CSUM_OFFSET in initPayloadSet (line 227). The comment
at line 225 verbatim reads: "Optional attributes; validation is not
required." newPayloadSet does not validate it either.
When op.csumType == NFT_PAYLOAD_CSUM_INET (a valid type accepted
by validateChecksumType at line 63) and op.csumOffset + 2 > len(payload),
the indexed slice access panics:
payload[op.csumOffset:] with csumOffset > len: slice bounds out of range
binary.BigEndian.Uint16 internal b[0]/b[1]: index out of range
Within the same function, the data write at line 105 already validates
the analogous bound (len(payload) < offset+op.blen) and returns
NFT_BREAK on overflow. The checksum reads at lines 154/160 use the
same kind of attacker-controlled offset attribute but skip the
equivalent validation.
Linux kernel net/netfilter/nft_payload.c handles this via the
skb_copy_bits and skb_ensure_writable helpers inside
nft_payload_csum_inet, which return -1 when the requested offset+size
exceeds skb->len:
if (skb_copy_bits(skb, csum_offset, &sum, sizeof(sum)) < 0)
return -1;
...
if (skb_ensure_writable(skb, csum_offset + sizeof(sum)) ||
skb_store_bits(skb, csum_offset, &sum, sizeof(sum)) < 0)
return -1;
gVisor's Go port uses raw slice indexing which panics on
out-of-bounds instead of returning an error. Add an explicit length
check that gVisor's slice approach requires.
The check returns NFT_BREAK early when the requested csum location
is outside the packet payload, matching the existing NFT_BREAK
handling at line 105. No behavior change for well-formed rules.
Verified at runtime: a standalone Go reproducer of the line 154
code pattern panics with the Go runtime errors quoted above when
csumOffset equals, exceeds, or is one less than the payload length.
The safe case (csumOffset+2 <= len(payload)) is unchanged.
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
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.
Summary
payloadSet.evaluateinpkg/tcpip/nftables/nft_payload_set.goreads and writes the IP checksum field at
payload[op.csumOffset:]without validating
op.csumOffsetagainstlen(payload). When therule's
NFTA_PAYLOAD_CSUM_OFFSETnetlink attribute is set so thatthe checksum field falls outside the packet payload, the indexed
slice access panics at runtime with "slice bounds out of range" or
"index out of range".
Background
op.csumOffsetis sourced from the optional netlink attributeNFTA_PAYLOAD_CSUM_OFFSETininitPayloadSet(line 227). Thecomment at line 225 verbatim reads "Optional attributes; validation
is not required."
newPayloadSetdoes not validate it either.The data bounds check at line 105 already covers
op.offset+op.blen(the data being written), but the checksum reads at lines 154 and
160 use a separate attacker-controlled offset attribute that skips
the equivalent check.
Linux behavior
Linux kernel
net/netfilter/nft_payload.c::nft_payload_csum_inetperforms the equivalent bounds check via the
skb_copy_bitsandskb_ensure_writablehelpers, which return -1 when the requestedoffset+size exceeds the skb length:
The -1 return propagates to
nft_payload_set_evalwhich thengoto err. Linux gracefully fails the rule evaluation; gVisor'sGo port uses raw slice indexing which panics on out-of-bounds
instead of returning an error.
Reachability
Reachable from a process holding
CAP_NET_ADMINin the networknamespace. The nftables netlink handler gates rule installation
on
CAP_NET_ADMINatpkg/sentry/socket/netlink/netfilter/protocol.go:76:Once the rule is installed, the malformed
csumOffsetvalue plusa sufficiently short packet that hits the rule trigger the panic
at line 154. Severity is localized sandbox availability: the
sentry panic kills the sandbox. This is not CVE-grade given the
privilege requirement, but matches the hardening-class pattern of
recent panic fixes such as
1985b831b9,25a1144119,c6c0e528b9.Change
Add an early
NFT_BREAKreturn at the entry of theNFT_PAYLOAD_CSUM_INETbranch whenint(op.csumOffset)+2 > len(payload).Returns the same
NFT_BREAKverdict the existing offset+blen guarduses at line 105.
5 added lines, 0 deletions in
nft_payload_set.go. Two new test cases innftables_test.goexercise the boundary (csumOffset == lenandcsumOffset > len). No behavior change for well-formed rules.Test
Verified at runtime via a standalone Go reproducer of the line 154
pattern. Output:
Added two
TestEvaluatePayloadSetcases that exercise boundaryconditions (csumOffset equal to len, csumOffset > len). Pre-fix
these panic; post-fix they return
NFT_BREAK.