Fix uint16 overflow in consumeSingleTURNFrame

[GerardGao]

SUMMARY

A malformed ChannelData frame with a length field in [0xFFFC, 0xFFFF]
caused the frame-size computation in consumeSingleTURNFrame to
overflow uint16, wrapping to 0. STUNConn.ReadFrom then reported a
zero-size frame with a nil error and never consumed the buffered
data, leaving the server read loop spinning at 100% CPU. A single
4-byte packet over TCP triggers this, allowing a remote DoS.

The frame size is now computed with uint32 arithmetic, and the
padding math was rewritten in an equivalent overflow-safe form.
The same overflow class in the STUN branch is covered by the fix.

TEST

• TestOverflowRepro_ConsumeFrame / TestOverflowRepro_ReadFromDoesNotConsume
  fail before the fix (n=0, err=nil) and pass after.
• TestPaddingFormulaEquivalence exhaustively proves the new padding
  formula matches the old one for all 65536 length values.
• Boundary scans cover 0xFFF8-0xFFFF, STUN-length overflow, max frame
  size (65540), zero-length and non-multiple padding.
• golangci-lint v2.10.1: 0 issues.
• go test -race on internal/proto, internal/client, internal/server: pass.

CONTEXT

Found while reviewing the codebase for potential protocol-handling
bugs. The fix is limited to internal/proto and changes no public API.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.52%. Comparing base (874faf9) to head (ca3db96).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   82.42%   82.52%   +0.10%     
==========================================
  Files          46       46              
  Lines        3305     3307       +2     
==========================================
+ Hits         2724     2729       +5     
+ Misses        377      374       -3     
  Partials      204      204              

Flag	Coverage Δ
go	82.52% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[GerardGao]

@fallenmi Thanks for the thorough review. Both points are addressed in the latest push (ca3db96):

1. ReadFrom no longer reports more bytes than copied: when a complete frame is larger than the caller's buffer, ReadFrom consumes the entire frame from the stream (keeping framing aligned) but returns only the bytes actually copied, per the net.PacketConn contract. Client.Listen no longer panics on a 65,536-byte ChannelData frame.
2. Added end-to-end boundary test: TestClientListenWithOversizedChannelData reproduces the exact "slice bounds out of range [:65536] with capacity 65535" panic against the previous head and passes now. Also added TestReadFromOversizedFrameReportsCopiedBytes and TestReadFromContinuesAfterOversizedFrame covering the return-value contract and stream alignment.

Verified locally: proto and root package -race suites pass, golangci-lint reports 0 issues, and the existing overflow/boundary tests are unchanged.

[JoTurk]

The 65536-byte client-crash is unrealistic, Turn server buffer is limited to 1600-bytes and pion/webrtc has a 1200 mtu.

this PR should be only limited to safe frame-length handling, and making sure we don't overflow, and we should reject frames whose declared size exceeds len(payload)

[GerardGao]

@JoTurk agreed on both points — the PR is now narrowed to exactly what you described. Pushed in 0b539fa.

Dropped the client-crash framing. You're right that a 65,536-byte frame is not reachable through the pion stack (server inboundMTU, webrtc's 1200 MTU). The oversized-frame handling is no longer justified as a DoS fix, so I removed the truncate-and-deliver path and the Client.Listen end-to-end test built around it.

What remains, per your direction:

1. Don't overflow. Frame size is computed in uint32. On main, a ChannelData length of 0xFFFE/0xFFFF wraps and consumeSingleTURNFrame returns n=4, err=nil — a "valid" 4-byte frame for a declared 65 KB one, so ReadFrom reports a frame it never consumed and the read loop spins. Verified RED on main / GREEN on this head.

2. Reject frames whose declared size exceeds len(payload).

if n > len(payload) {
    s.buff = s.buff[n:]
    return 0, nil, errTURNFrameTooLarge
}

The frame is consumed so stream framing stays aligned and the next frame still parses; no caller ever observes n > len(payload).

Tests: folded into the existing internal/proto/stun_conn_test.go — no separate regression files. Three additions: the 0xFFF8..0xFFFF / STUN-length overflow scan, an exhaustive proof that the rewritten padding formula matches the original for all 65,536 length values, and a ReadFrom case asserting rejection plus clean parsing of the following frame.

Diff is now 2 files, +112/-11, confined to internal/proto, no public API change.

Verified: go build ./..., go vet ./..., go test -race ./internal/proto/... pass; golangci-lint run reports zero issues in the changed files. (TestCreateTCPConnectionInvalid and TestConnectRequest fail identically on unmodified main in my environment — pre-existing, unrelated.)

[GerardGao]

Rebased the three commits to use a consistent author identity (GerardGao <213731635+GerardGao@users.noreply.github.com> for all three — one had a corporate email). No code change at all; the diff is identical to the previous push.

To summarise what the current head contains, per your direction:

1. Overflow fix only. datagramSize is now uint32; the old uint16 arithmetic wrapped on dataLen >= 0xFFFC, producing a near-zero frame size that made ReadFrom return (0, nil) without consuming the buffer and spin forever.
2. Reject frames whose declared size exceeds len(payload). When n > len(payload), ReadFrom advances the buffer by the full frame size (stream framing stays aligned) and returns errTURNFrameTooLarge. No truncate-and-deliver path.
3. No client-crash test. The Client.Listen end-to-end test and client_oversized_frame_test.go are gone. Tests are limited to internal/proto: an overflow scan for 0xFFF8..0xFFFF, an exhaustive padding-formula equivalence proof, and a ReadFrom boundary test with a 1600-byte payload (matching the realistic server buffer).

Could someone approve the workflow run? CI has not triggered yet — I believe it needs a maintainer to approve since this is my first contribution here.

[GerardGao]

Hi @JoTurk, I've addressed all your review feedback:

• Removed the unrealistic client-crash test scenario (large frame exceeding 64 KB buffer)
• Kept the fix scope to two things as you requested: safe frame-length handling via uint32 (preventing uint16 overflow) and rejecting frames whose declared size exceeds the available payload
• Frames that are too large are now consumed in full to maintain stream alignment before returning errTURNFrameTooLarge

Could you please take another look and, if everything looks good, approve the CI workflow run? (First-contribution restriction requires maintainer approval to trigger GitHub Actions.)

Thank you!