Skip to content

Commit

Permalink
pkg/rpcserver: pkg/flatrpc: executor: add handshake stage 0
Browse files Browse the repository at this point in the history
As we figured out in #5805, syz-manager treats random incoming RPC
connections as trusted, and will crash if a non-executor client sends
an invalid packet to it.

To address this issue, we introduce another stage of handshake, which
includes a cookie exchange:
 - upon connection from an executor, the manager sends a ConnectHello RPC
   message to it, which contains a random 64-bit cookie;
 - the executor calculates a hash of that cookie and includes it into
   its ConnectRequest together with the other information;
 - before checking the validity of ConnectRequest, the manager ensures
   client sanity (passed ID didn't change, hashed cookie has the expected
   value)

We deliberately pick a random cookie instead of a magic number: if the
fuzzer somehow learns to send packets to the manager, we don't want it to
crash multiple managers on the same machine.
  • Loading branch information
ramosian-glider committed Feb 20, 2025
1 parent 5066879 commit a3b37db
Show file tree
Hide file tree
Showing 8 changed files with 327 additions and 45 deletions.
16 changes: 16 additions & 0 deletions executor/executor_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -629,9 +629,24 @@ class Runner
failmsg("bad restarting", "restarting=%d", restarting_);
}

// Implementation must match that in pkg/rpcserver/rpcserver.go.
uint64 HashAuthCookie(uint64 cookie)
{
const uint64_t prime1 = 73856093;
const uint64_t prime2 = 83492791;

return (cookie * prime1) ^ prime2;
}

int Handshake()
{
// Handshake stage 0: get a cookie from the manager.
rpc::ConnectHelloRawT conn_hello;
conn_.Recv(conn_hello);

// Handshake stage 1: share basic information about the client.
rpc::ConnectRequestRawT conn_req;
conn_req.cookie = HashAuthCookie(conn_hello.cookie);
conn_req.id = vm_index_;
conn_req.arch = GOARCH;
conn_req.git_revision = GIT_REVISION;
Expand All @@ -656,6 +671,7 @@ class Runner
if (conn_reply.cover)
max_signal_.emplace();

// Handshake stage 2: share information requested by the manager.
rpc::InfoRequestRawT info_req;
info_req.files = ReadFiles(conn_reply.files);

Expand Down
29 changes: 27 additions & 2 deletions pkg/flatrpc/conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ import (
)

func TestConn(t *testing.T) {
connectHello := &ConnectHello{
Cookie: 1,
}
connectReq := &ConnectRequest{
Cookie: 73856093,
Id: 1,
Arch: "arch",
GitRevision: "rev1",
Expand Down Expand Up @@ -52,6 +56,9 @@ func TestConn(t *testing.T) {
go func() {
done <- serv.Serve(context.Background(),
func(_ context.Context, c *Conn) error {
if err := Send(c, connectHello); err != nil {
return err
}
connectReqGot, err := Recv[*ConnectRequestRaw](c)
if err != nil {
return err
Expand Down Expand Up @@ -79,6 +86,12 @@ func TestConn(t *testing.T) {
c := dial(t, serv.Addr.String())
defer c.Close()

connectHelloGot, err := Recv[*ConnectHelloRaw](c)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, connectHello, connectHelloGot)

if err := Send(c, connectReq); err != nil {
t.Fatal(err)
}
Expand All @@ -102,7 +115,11 @@ func TestConn(t *testing.T) {
}

func BenchmarkConn(b *testing.B) {
connectHello := &ConnectHello{
Cookie: 1,
}
connectReq := &ConnectRequest{
Cookie: 73856093,
Id: 1,
Arch: "arch",
GitRevision: "rev1",
Expand All @@ -125,7 +142,11 @@ func BenchmarkConn(b *testing.B) {
done <- serv.Serve(context.Background(),
func(_ context.Context, c *Conn) error {
for i := 0; i < b.N; i++ {
_, err := Recv[*ConnectRequestRaw](c)
if err := Send(c, connectHello); err != nil {
return err
}

_, err = Recv[*ConnectRequestRaw](c)
if err != nil {
return err
}
Expand All @@ -143,10 +164,14 @@ func BenchmarkConn(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, err := Recv[*ConnectHelloRaw](c)
if err != nil {
b.Fatal(err)
}
if err := Send(c, connectReq); err != nil {
b.Fatal(err)
}
_, err := Recv[*ConnectReplyRaw](c)
_, err = Recv[*ConnectReplyRaw](c)
if err != nil {
b.Fatal(err)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/flatrpc/flatrpc.fbs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ enum Feature : uint64 (bit_flags) {
BinFmtMisc,
Swap,
}

table ConnectHelloRaw {
cookie :uint64;
}

table ConnectRequestRaw {
cookie :uint64;
id :int64;
arch :string;
git_revision :string;
Expand Down
113 changes: 103 additions & 10 deletions pkg/flatrpc/flatrpc.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit a3b37db

Please sign in to comment.