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 google#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, the executor sends a ConnectHelloRequest containing
   its ID assigned to it at startup;
 - the manager responds with a ConnectHelloResponse containing 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 7a2638b
Show file tree
Hide file tree
Showing 8 changed files with 506 additions and 18 deletions.
19 changes: 19 additions & 0 deletions executor/executor_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -629,9 +629,27 @@ class Runner
failmsg("bad restarting", "restarting=%d", restarting_);
}

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

return (cookie * prime1) ^ ((cookie >> 32) * prime2);
}

int Handshake()
{
// Handshake stage 0: request a cookie.
rpc::ConnectHelloRequestRawT conn_hello_req;
conn_hello_req.id = vm_index_;
conn_.Send(conn_hello_req);
rpc::ConnectHelloReplyRawT conn_hello_reply;
conn_.Recv(conn_hello_reply);

// Handshake stage 1: share basic information about the client.
rpc::ConnectRequestRawT conn_req;
conn_req.cookie = HashAuthCookie(conn_hello_reply.cookie);
conn_req.id = vm_index_;
conn_req.arch = GOARCH;
conn_req.git_revision = GIT_REVISION;
Expand All @@ -656,6 +674,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
50 changes: 48 additions & 2 deletions pkg/flatrpc/conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,14 @@ import (
)

func TestConn(t *testing.T) {
connectHelloReq := &ConnectHelloRequest{
Id: 1,
}
connectHelloReply := &ConnectHelloReply{
Cookie: 1,
}
connectReq := &ConnectRequest{
Cookie: 73856093,
Id: 1,
Arch: "arch",
GitRevision: "rev1",
Expand Down Expand Up @@ -52,6 +59,13 @@ func TestConn(t *testing.T) {
go func() {
done <- serv.Serve(context.Background(),
func(_ context.Context, c *Conn) error {
connectHelloReqGot, err := Recv[*ConnectHelloRequestRaw](c)
if err := Send(c, connectHelloReply); err != nil {
return err
}
if !reflect.DeepEqual(connectHelloReq, connectHelloReqGot) {
return fmt.Errorf("connectHelloReq != connectHelloReqGot")
}
connectReqGot, err := Recv[*ConnectRequestRaw](c)
if err != nil {
return err
Expand Down Expand Up @@ -79,6 +93,16 @@ func TestConn(t *testing.T) {
c := dial(t, serv.Addr.String())
defer c.Close()

if err := Send(c, connectHelloReq); err != nil {
t.Fatal(err)
}

connectHelloReplyGot, err := Recv[*ConnectHelloReplyRaw](c)
if err != nil {
t.Fatal(err)
}
assert.Equal(t, connectHelloReply, connectHelloReplyGot)

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

func BenchmarkConn(b *testing.B) {
connectHelloReq := &ConnectHelloRequest{
Id: 1,
}
connectHelloReply := &ConnectHelloReply{
Cookie: 1,
}
connectReq := &ConnectRequest{
Cookie: 73856093,
Id: 1,
Arch: "arch",
GitRevision: "rev1",
Expand All @@ -125,7 +156,15 @@ 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)
_, err := Recv[*ConnectHelloRequestRaw](c)
if err != nil {
return err
}
if err := Send(c, connectHelloReply); err != nil {
return err
}

_, err = Recv[*ConnectRequestRaw](c)
if err != nil {
return err
}
Expand All @@ -143,10 +182,17 @@ func BenchmarkConn(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
if err := Send(c, connectHelloReq); err != nil {
b.Fatal(err)
}
_, err := Recv[*ConnectHelloReplyRaw](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
9 changes: 9 additions & 0 deletions pkg/flatrpc/flatrpc.fbs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,17 @@ enum Feature : uint64 (bit_flags) {
BinFmtMisc,
Swap,
}

table ConnectHelloRequestRaw {
id :int64;
}

table ConnectHelloReplyRaw {
cookie :uint64;
}

table ConnectRequestRaw {
cookie :uint64;
id :int64;
arch :string;
git_revision :string;
Expand Down
Loading

0 comments on commit 7a2638b

Please sign in to comment.