Validate client buffer registration ownership - #111
Merged
Conversation
A connected client could send raw RegisterClientBuffer/UnregisterClientBuffer requests for a channel it has no publisher on. The server only checked that the session matched the current server session (which is shared by every client), so any client could: - Inject a bogus or fd-less buffer registration for another publisher's channel. With "first registration wins" this could pre-empt the real publisher's registration, and on Android/memfd would make subscribers map the wrong or a missing backing fd. - Erase another publisher's entire buffer group via UnregisterClientBuffer, which takes no ownership token and simply drops the (session, buffer_index) entry. Require that the requesting connection owns a publisher backed by the target channel's storage before it may register or unregister client buffers. Ownership is derived from the server-side ClientHandler that already tracks each connection's publishers, so it cannot be spoofed and needs no protocol, client or Rust changes. Crucially this still allows the legitimate multi-publisher shared-buffer model: several publishers on a channel (or on virtual channels of a mux) all pass the check and converge on the shared "first registration wins" buffer group. Multiplexer channels consult their virtual channels, where mux-backed publishers actually live. Also reject fd-backed registrations whose fd_index is missing/out of range instead of silently registering the buffer without its backing fd. Shadow recovery registers buffers directly on the channel and is unaffected. Adds server-level regression tests for owner-succeeds, foreign register/ unregister rejection, and invalid fd index rejection. Supersedes cursor bot PR #83, whose single-publisher_id ownership model predated and conflicted with the anonymous-memfd shared-buffer model.
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
Properly fixes the security issue cursor bot PR #83 targeted, using an approach
that is compatible with the anonymous-memfd shared-buffer model that PR #83
conflicted with (which is why it was excluded from the earlier combined PR).
A connected client could send raw
RegisterClientBuffer/UnregisterClientBufferrequests for a channel it has no publisher on. The server only validated that
the request's session matched the current server session — but that session id
is shared by every client. So any client could:
channel. With the "first registration wins" dedup this could pre-empt the
real publisher's registration, and on Android/memfd would make subscribers
map the wrong or a missing backing fd (crashes / corrupted reads).
UnregisterClientBuffercarried no ownership token and simply dropped the
(session, buffer_index)entry.
Fix
Require that the requesting connection owns a publisher backed by the target
channel's storage before it may register or unregister client buffers:
ServerChannel::HasPublisherOwnedBy(ClientHandler*)checks the channel'susers for a publisher owned by the requesting connection.
ChannelMultiplexeroverrides it to also consult its virtual channels,since publishers backed by a mux's storage live on the vchans, not the mux.
fd_indexis missing/out of rangeinstead of silently registering the buffer without its backing fd.
Ownership is derived from the server-side
ClientHandlerthat already trackseach connection's publishers, so it cannot be spoofed and requires no
protocol, client or Rust changes.
Why this differs from PR #83
PR #83 added a
publisher_idtoken to the register/unregister protocol andstored a single owning publisher per buffer group, rejecting cross-owner
access. That single-owner model predated and conflicted with the current
anonymous-memfd shared-buffer model, where multiple publishers on a channel
(or on virtual channels of a mux) legitimately register for the same
(session, buffer_index, slot)and converge on one shared buffer via "firstregistration wins". This connection-ownership approach preserves that model:
every legitimate publisher passes the check, while unrelated clients are
rejected. Shadow recovery registers buffers directly on the channel and is
unaffected.
Test plan
New server-level regression tests (
//server:server_test):RegisterClientBufferOwnerSucceedsRegisterClientBufferForeignRejectedUnregisterClientBufferForeignRejectedRegisterClientBufferInvalidFdIndexRejectedValidated on macOS arm64 (POSIX) and, via a Linux container, on both the
default
/dev/shmbackend and--config=linux_memfd(the fd-backedregistration path this guards):
//...on macOS arm64 (25/25)//...on Linux default (25/25) and memfd//... -//rust_client/...(23/23)client_test/c_client:client_teston macOS + Linux + memfdbridge_test(exercises the real registration path end to end)