Bound the first client frame before allocating for it - #137
Conversation
An unidentified client picks the size of the buffer TryReadFirstFrameAsync allocates for the first frame by writing four header bytes, before any of the declared payload has arrived; the only existing check refused declared lengths above 256 MiB, which still let a peer force a large allocation and a wait out to QueryTimeoutMs for nothing. Real first frames (the status query, LoginTokenQuery, Identification) are a few hundred bytes at most, so bound the length to 64 KiB before allocating and reuse the existing InvalidDataException path, which already ends the session without dialing a backend or writing a reply.
Client.RemoteEndPoint cannot be null once the header read above has already succeeded on the same stream, so the null-coalescing fallback was dead weight that only left an unreachable branch for coverage to complain about.
…dress The bound comment claimed Identification carries a mod list. It does not: the 1.22.6 Packet_ClientIdentification has nine fields, short strings and two ints, and mod lists travel server to client, never the other way. The comment now names what the packet actually carries instead. The over-bound log line also read client.Client.RemoteEndPoint directly, which a concurrent teardown can dispose out from under it. ClientSessionRunner already has the session in scope by the time it reads the first frame, and ProxySession captures the client address once at construction for exactly this reason, so the log now reads session.ClientRemote instead.
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Please update this branch against current indev before the code review proceeds. The PR targets aaa5a58, while indev is at d4163ff, so the histories are three commits apart in both directions. The current Build & ServerMod tests and Sonar checks pass, but review and test evidence should cover the current integration branch. After updating the PR, please request review again. I found no line-specific code defect on this stale head; this request is for branch freshness.
|
Branch updated with a merge of current indev (head cc5627c), no code change; unit suites green (716/717, 312/312, 115/115, 97/97, only known local-only ProxyBootTests.ARegistryThatCannotBind_ExitsTwoRatherThanCrashingOnTheWayOut failing) and the 30 ServerMod scenarios green; review re-requested from Zaldaryon. |
|
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Approving on head cc5627c. The branch is synced with current indev, all CI checks pass, and local verification confirms the fix:
FirstFrameBoundTestspasses 3/3, and restoring the old 256 MiB ceiling failsClientThatOnlyDeclaresAHugeLength_IsDisconnectedWithoutDialingABackendas expected.- Engine comparison confirms
Packet_ClientIdentificationSerializerwrites only the nine declared fields and no mod list, so 64 KiB is a safe ceiling. - The 7-day review window for behavior changes is satisfied.
Two non-blocking follow-up notes:
Nimbus.Proxy/Core/ClientSessionRunner.cs:221: the trace line prefixes with[first-frame]instead of[s{session.Id}]used across the rest of the class. Folding the session id into the prefix helps log filtering.Nimbus.Proxy/Core/ClientSessionRunner.cs:97: throwing into the general session catch-all produces aWarnlog for "session crashed" on an unauthenticated 4-byte write. HandlingInvalidDataExceptionexplicitly or logging atTracewould prevent warning noise from port probes.
2 participants
TryReadFirstFrameAsync reads a client's opening frame before that client has identified: four header bytes, a declared length, and then an allocation of that many bytes ahead of receiving any of them, followed by a wait of up to QueryTimeoutMs (1500ms by default) for the rest to arrive. The only guard on the declared length refused anything above 256 MiB, so anything under that let an unauthenticated peer choose the size of an allocation and hold the proxy waiting on it at the cost of a four-byte write, with no upper bound worth calling small.
A real first frame is nowhere near that size. The status query and LoginTokenQuery this proxy handles are a handful of bytes, and Identification, read straight out of the 1.22.6 assembly's Packet_ClientIdentification, is nine fields: MdProtocolVersion, Playername, MpToken, ServerPassword, PlayerUID, NetworkVersion, ShortGameVersion, ViewDistance and RenderMetaBlocks, all short strings or ints and no mod list (that travels server to client, not the other way). This change adds MaxFirstFrameSize, 64 KiB, checked in ClientSessionRunner.TryReadFirstFrameAsync before the allocation runs, well above anything a real client sends and far below the old 256 MiB ceiling. A declared length past the bound throws the same InvalidDataException the old oversized case already threw, which the caller already handles by logging and closing the socket without dialing a backend or writing a reply, so the rejected session ends through the path that was already there rather than a new one. VsWire.MaxFrameSize and the FrameSniffer are untouched, since mid-session frames after identification are a separate, legitimately larger case.
Three tests were added in FirstFrameBoundTests.cs, built on the existing SessionHarness and RecordingBackend used by the other socket-level session tests. One drives a client that declares 200 MiB and sends nothing else, and asserts the session closes and no backend connection is ever opened; against the unfixed code this same scenario allocates the 200 MiB, waits out the full query timeout, and then still proceeds to dial the backend, which the test catches. Another sends exactly MaxFirstFrameSize bytes and confirms that frame is still accepted and forwarded as before. The third sends one byte over the bound and confirms it is refused. The existing first-frame tests (Identification first, status query first, a quiet client, sticky reconnect routing) were run unchanged and still pass.