From e189fb68860820c1903dc12fe3c7670d9ceacde2 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Fri, 13 May 2022 12:23:44 -0700 Subject: [PATCH] Add version handshake before communication is stablished Details on how it works is in wire.Handshake. Updates #4805 PiperOrigin-RevId: 448552448 --- examples/seccheck/server.cc | 41 ++++++++++++++ pkg/sentry/seccheck/checkers/remote/BUILD | 3 +- pkg/sentry/seccheck/checkers/remote/remote.go | 49 +++++++++++++---- .../seccheck/checkers/remote/remote_test.go | 32 +++++++++++ .../seccheck/checkers/remote/test/BUILD | 3 +- .../seccheck/checkers/remote/test/server.go | 53 ++++++++++++++++--- .../checkers/remote/{header => wire}/BUILD | 10 ++-- .../remote/{header/header.go => wire/wire.go} | 7 ++- .../header_test.go => wire/wire_test.go} | 2 +- pkg/sentry/seccheck/points/common.proto | 48 +++++++++++++++++ 10 files changed, 221 insertions(+), 27 deletions(-) rename pkg/sentry/seccheck/checkers/remote/{header => wire}/BUILD (58%) rename pkg/sentry/seccheck/checkers/remote/{header/header.go => wire/wire.go} (90%) rename pkg/sentry/seccheck/checkers/remote/{header/header_test.go => wire/wire_test.go} (98%) diff --git a/examples/seccheck/server.cc b/examples/seccheck/server.cc index 025280dc2..d8567d7fe 100644 --- a/examples/seccheck/server.cc +++ b/examples/seccheck/server.cc @@ -28,6 +28,7 @@ #include "absl/cleanup/cleanup.h" #include "absl/strings/string_view.h" +#include "pkg/sentry/seccheck/points/common.pb.h" #include "pkg/sentry/seccheck/points/container.pb.h" #include "pkg/sentry/seccheck/points/sentry.pb.h" #include "pkg/sentry/seccheck/points/syscall.pb.h" @@ -163,6 +164,41 @@ void startPollThread(int poll_fd) { pthread_detach(thread); } +// handshake performs version exchange with client. See common.proto for details +// about the protocol. +bool handshake(int client_fd) { + std::vector buf(10240); + int bytes = read(client_fd, buf.data(), buf.size()); + if (bytes < 0) { + printf("Error receiving handshake message: %d\n", errno); + return false; + } else if (bytes == buf.size()) { + // Protect against the handshake becoming larger than the buffer allocated + // for it. + printf("handshake message too big\n"); + return false; + } + ::gvisor::common::Handshake in = {}; + if (!in.ParseFromArray(buf.data(), bytes)) { + printf("Error parsing handshake message\n"); + return false; + } + + constexpr uint32_t minSupportedVersion = 1; + if (in.version() < minSupportedVersion) { + printf("Client has unsupported version %u\n", in.version()); + return false; + } + + ::gvisor::common::Handshake out; + out.set_version(1); + if (!out.SerializeToFileDescriptor(client_fd)) { + printf("Error sending handshake message: %d\n", errno); + return false; + } + return true; +} + extern "C" int main(int argc, char** argv) { for (int c = 0; (c = getopt(argc, argv, "q")) != -1;) { switch (c) { @@ -221,6 +257,11 @@ extern "C" int main(int argc, char** argv) { } printf("Connection accepted\n"); + if (!handshake(client)) { + close(client); + continue; + } + struct epoll_event evt; evt.data.fd = client; evt.events = EPOLLIN; diff --git a/pkg/sentry/seccheck/checkers/remote/BUILD b/pkg/sentry/seccheck/checkers/remote/BUILD index fcf9ca788..9a86ba389 100644 --- a/pkg/sentry/seccheck/checkers/remote/BUILD +++ b/pkg/sentry/seccheck/checkers/remote/BUILD @@ -12,7 +12,7 @@ go_library( "//pkg/fd", "//pkg/log", "//pkg/sentry/seccheck", - "//pkg/sentry/seccheck/checkers/remote/header", + "//pkg/sentry/seccheck/checkers/remote/wire", "//pkg/sentry/seccheck/points:points_go_proto", "@org_golang_google_protobuf//proto:go_default_library", "@org_golang_x_sys//unix:go_default_library", @@ -31,6 +31,7 @@ go_test( "//pkg/fd", "//pkg/sentry/seccheck", "//pkg/sentry/seccheck/checkers/remote/test", + "//pkg/sentry/seccheck/checkers/remote/wire", "//pkg/sentry/seccheck/points:points_go_proto", "//pkg/test/testutil", "@com_github_cenkalti_backoff//:go_default_library", diff --git a/pkg/sentry/seccheck/checkers/remote/remote.go b/pkg/sentry/seccheck/checkers/remote/remote.go index 65ecaf688..a12cee250 100644 --- a/pkg/sentry/seccheck/checkers/remote/remote.go +++ b/pkg/sentry/seccheck/checkers/remote/remote.go @@ -17,7 +17,9 @@ package remote import ( + "errors" "fmt" + "io" "os" "golang.org/x/sys/unix" @@ -27,7 +29,7 @@ import ( "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/seccheck" - "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/header" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" ) @@ -81,6 +83,38 @@ func setup(path string) (*os.File, error) { if err := unix.Connect(int(f.Fd()), &addr); err != nil { return nil, fmt.Errorf("connect(%q): %w", path, err) } + + // Perform handshake. See common.proto for details about the protocol. + hsOut := pb.Handshake{Version: wire.CurrentVersion} + out, err := proto.Marshal(&hsOut) + if err != nil { + return nil, fmt.Errorf("marshalling handshake message: %w", err) + } + if _, err := f.Write(out); err != nil { + return nil, fmt.Errorf("sending handshake message: %w", err) + } + + in := make([]byte, 10240) + read, err := f.Read(in) + if err != nil && !errors.Is(err, io.EOF) { + return nil, fmt.Errorf("reading handshake message: %w", err) + } + // Protect against the handshake becoming larger than the buffer allocated + // for it. + if read == len(in) { + return nil, fmt.Errorf("handshake message too big") + } + hsIn := pb.Handshake{} + if err := proto.Unmarshal(in[:read], &hsIn); err != nil { + return nil, fmt.Errorf("unmarshalling handshake message: %w", err) + } + + // Check that remote version can be supported. + const minSupportedVersion = 1 + if hsIn.Version < minSupportedVersion { + return nil, fmt.Errorf("remote version (%d) is smaller than minimum supported (%d)", hsIn.Version, minSupportedVersion) + } + cu.Release() return f, nil } @@ -90,13 +124,6 @@ func New(_ map[string]interface{}, endpoint *fd.FD) (seccheck.Checker, error) { if endpoint == nil { return nil, fmt.Errorf("remote sink requires an endpoint") } - // TODO(gvisor.dev/issue/4805): perform version handshake with remote: - // 1. sentry and remote exchange versions - // 2. sentry continues if remote >= min(sentry) - // 3. remote continues if sentry >= min(remote). - // min() being the minimal supported version. Let's say current sentry - // supports batching but remote doesn't, sentry can chose to not batch or - // refuse the connection. return &Remote{endpoint: endpoint}, nil } @@ -115,11 +142,11 @@ func (r *Remote) write(msg proto.Message, msgType pb.MessageType) { log.Debugf("Marshal(%+v): %v", msg, err) return } - hdr := header.Header{ - HeaderSize: uint16(header.HeaderStructSize), + hdr := wire.Header{ + HeaderSize: uint16(wire.HeaderStructSize), MessageType: uint16(msgType), } - var hdrOut [header.HeaderStructSize]byte + var hdrOut [wire.HeaderStructSize]byte hdr.MarshalUnsafe(hdrOut[:]) // TODO(gvisor.dev/issue/4805): Change to non-blocking write. Count as dropped diff --git a/pkg/sentry/seccheck/checkers/remote/remote_test.go b/pkg/sentry/seccheck/checkers/remote/remote_test.go index 1b6f80a9a..61ae35605 100644 --- a/pkg/sentry/seccheck/checkers/remote/remote_test.go +++ b/pkg/sentry/seccheck/checkers/remote/remote_test.go @@ -31,6 +31,7 @@ import ( "gvisor.dev/gvisor/pkg/fd" "gvisor.dev/gvisor/pkg/sentry/seccheck" "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/test" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" "gvisor.dev/gvisor/pkg/test/testutil" ) @@ -153,6 +154,37 @@ func TestBasic(t *testing.T) { } } +func TestVersionUnsupported(t *testing.T) { + server, err := test.NewServer() + if err != nil { + t.Fatalf("newServer(): %v", err) + } + defer server.Close() + + server.SetVersion(0) + + _, err = setup(server.Path) + if err == nil || !strings.Contains(err.Error(), "remote version") { + t.Fatalf("Wrong error: %v", err) + } +} + +func TestVersionNewer(t *testing.T) { + server, err := test.NewServer() + if err != nil { + t.Fatalf("newServer(): %v", err) + } + defer server.Close() + + server.SetVersion(wire.CurrentVersion + 10) + + endpoint, err := setup(server.Path) + if err != nil { + t.Fatalf("setup(): %v", err) + } + _ = endpoint.Close() +} + // Test that the example C++ server works. It's easier to test from here and // also changes that can break it will likely originate here. func TestExample(t *testing.T) { diff --git a/pkg/sentry/seccheck/checkers/remote/test/BUILD b/pkg/sentry/seccheck/checkers/remote/test/BUILD index 5017de973..3e4cca046 100644 --- a/pkg/sentry/seccheck/checkers/remote/test/BUILD +++ b/pkg/sentry/seccheck/checkers/remote/test/BUILD @@ -10,11 +10,12 @@ go_library( deps = [ "//pkg/cleanup", "//pkg/log", - "//pkg/sentry/seccheck/checkers/remote/header", + "//pkg/sentry/seccheck/checkers/remote/wire", "//pkg/sentry/seccheck/points:points_go_proto", "//pkg/sync", "//pkg/test/testutil", "//pkg/unet", + "@org_golang_google_protobuf//proto:go_default_library", "@org_golang_x_sys//unix:go_default_library", ], ) diff --git a/pkg/sentry/seccheck/checkers/remote/test/server.go b/pkg/sentry/seccheck/checkers/remote/test/server.go index 48835c95d..7bf93d845 100644 --- a/pkg/sentry/seccheck/checkers/remote/test/server.go +++ b/pkg/sentry/seccheck/checkers/remote/test/server.go @@ -24,9 +24,10 @@ import ( "time" "golang.org/x/sys/unix" + "google.golang.org/protobuf/proto" "gvisor.dev/gvisor/pkg/cleanup" "gvisor.dev/gvisor/pkg/log" - "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/header" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/test/testutil" @@ -46,6 +47,8 @@ type Server struct { // +checklocks:mu points []Message + + version uint32 } // Message corresponds to a single message sent from checkers.Remote. @@ -97,8 +100,9 @@ func newServerPath(path string) (*Server, error) { } server := &Server{ - Path: path, - socket: ss, + Path: path, + socket: ss, + version: wire.CurrentVersion, } go server.run() cu.Release() @@ -115,6 +119,11 @@ func (s *Server) run() { } return } + if err := s.handshake(client); err != nil { + log.Warningf(err.Error()) + _ = client.Close() + continue + } s.mu.Lock() s.clients = append(s.clients, client) s.mu.Unlock() @@ -122,6 +131,33 @@ func (s *Server) run() { } } +// handshake performs version exchange with client. See common.proto for details +// about the protocol. +func (s *Server) handshake(client *unet.Socket) error { + var in [1024]byte + read, err := client.Read(in[:]) + if err != nil { + return fmt.Errorf("reading handshake message: %w", err) + } + hsIn := pb.Handshake{} + if err := proto.Unmarshal(in[:read], &hsIn); err != nil { + return fmt.Errorf("unmarshalling handshake message: %w", err) + } + if hsIn.Version != wire.CurrentVersion { + return fmt.Errorf("wrong version number, want: %d, got, %d", wire.CurrentVersion, hsIn.Version) + } + + hsOut := pb.Handshake{Version: s.version} + out, err := proto.Marshal(&hsOut) + if err != nil { + return fmt.Errorf("marshalling handshake message: %w", err) + } + if _, err := client.Write(out); err != nil { + return fmt.Errorf("sending handshake message: %w", err) + } + return nil +} + func (s *Server) handleClient(client *unet.Socket) { defer func() { s.mu.Lock() @@ -144,11 +180,11 @@ func (s *Server) handleClient(client *unet.Socket) { if read == 0 { return } - if read < header.HeaderStructSize { + if read < wire.HeaderStructSize { panic("invalid message") } - hdr := header.Header{} - hdr.UnmarshalUnsafe(buf[0:header.HeaderStructSize]) + hdr := wire.Header{} + hdr.UnmarshalUnsafe(buf[0:wire.HeaderStructSize]) if read < int(hdr.HeaderSize) { panic(fmt.Sprintf("message truncated, header size: %d, readL %d", hdr.HeaderSize, read)) } @@ -209,3 +245,8 @@ func (s *Server) WaitForCount(count int) error { return nil }, 5*time.Second) } + +// SetVersion sets the version to be used in handshake. +func (s *Server) SetVersion(newVersion uint32) { + s.version = newVersion +} diff --git a/pkg/sentry/seccheck/checkers/remote/header/BUILD b/pkg/sentry/seccheck/checkers/remote/wire/BUILD similarity index 58% rename from pkg/sentry/seccheck/checkers/remote/header/BUILD rename to pkg/sentry/seccheck/checkers/remote/wire/BUILD index a7e8d2b8b..b0c1ad5d5 100644 --- a/pkg/sentry/seccheck/checkers/remote/header/BUILD +++ b/pkg/sentry/seccheck/checkers/remote/wire/BUILD @@ -3,15 +3,15 @@ load("//tools:defs.bzl", "go_library", "go_test") package(licenses = ["notice"]) go_library( - name = "header", - srcs = ["header.go"], + name = "wire", + srcs = ["wire.go"], marshal = True, visibility = ["//:sandbox"], ) go_test( - name = "header_test", + name = "wire_test", size = "small", - srcs = ["header_test.go"], - library = ":header", + srcs = ["wire_test.go"], + library = ":wire", ) diff --git a/pkg/sentry/seccheck/checkers/remote/header/header.go b/pkg/sentry/seccheck/checkers/remote/wire/wire.go similarity index 90% rename from pkg/sentry/seccheck/checkers/remote/header/header.go rename to pkg/sentry/seccheck/checkers/remote/wire/wire.go index b5e455f3d..da68cba0f 100644 --- a/pkg/sentry/seccheck/checkers/remote/header/header.go +++ b/pkg/sentry/seccheck/checkers/remote/wire/wire.go @@ -12,8 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package header contains the message header used in the remote checker. -package header +// Package wire defines structs used in the wire format for the remote checker. +package wire + +// CurrentVersion is the current wire and protocol version. +const CurrentVersion = 1 // HeaderStructSize size of header struct in bytes. const HeaderStructSize = 8 diff --git a/pkg/sentry/seccheck/checkers/remote/header/header_test.go b/pkg/sentry/seccheck/checkers/remote/wire/wire_test.go similarity index 98% rename from pkg/sentry/seccheck/checkers/remote/header/header_test.go rename to pkg/sentry/seccheck/checkers/remote/wire/wire_test.go index 22b92a800..817250287 100644 --- a/pkg/sentry/seccheck/checkers/remote/header/header_test.go +++ b/pkg/sentry/seccheck/checkers/remote/wire/wire_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package header +package wire import "testing" diff --git a/pkg/sentry/seccheck/points/common.proto b/pkg/sentry/seccheck/points/common.proto index 11aabfc42..880f8096d 100644 --- a/pkg/sentry/seccheck/points/common.proto +++ b/pkg/sentry/seccheck/points/common.proto @@ -16,6 +16,54 @@ syntax = "proto3"; package gvisor.common; +// Handshake message is used when establishing a connection. Version information +// is exchanged to determine if the communication can proceed. Each side reports +// a single version of the protocol that it supports. If they can't support the +// version reported by the peer, they must close the connection. If the peer +// version is higher (newer), it should continue to communicate, making the peer +// responsible to send messages that are compatible with your version. If the +// peer can't support it, the peer should close the connection. +// +// In short: +// 1. sentry and remote exchange versions +// 2. sentry continues if remote >= min(sentry) +// 3. remote continues if sentry >= min(remote) +// +// Suppose that peer A is at version 1 and peer B is at 2. Peer A sees that B is +// at a newer version and continues with communication. Peer B will see that A +// is at version 1 (older) and will check if it can send messages that are +// compatible with version 1. If yes, then the communication can continue. If +// not, A should close the connection. +// +// Here are 2 practical examples: +// 1. New field added to the header: this requires a change in protocol +// version (e.g. 1 => 2). However, if not essential to communication, the +// new field can be ignored by a peer that is still using version 1. +// Sentry version 1, remote version 2: remote doesn't get the new field, +// but can still receive messages. +// Sentry version 2, remote version 1: remote gets the new field, but +// ignores it since it's not aware the field exists yet. Note that remote +// must rely on header length to determine where the payload is. +// +// 2. Change in message format for batching: this requires a change in +// protocol version (2 => 3). Batching can only be used if both sides can +// handle it. +// Sentry version 2, remote version 3: remote gets a message at a time. If +// it still can do that, remote can accept that sentry is in version 2. +// Sentry version 3, remote version 2: remote is not able to process +// batched messages. If the sentry can still produce one message at a time +// the communication can continue, otherwise the sentry should close the +// connection. +// +// Note that addition of new message types do not require version changes. +// Server implementations should gracefully handle messages that it doesn't +// understand. Similarly, payload for message can change following protobuf +// rules for compatibilty. For example, adding new fields to a protobuf type +// doesn't require version bump. +message Handshake { + uint32 version = 1; +} + message Credentials { uint32 real_uid = 1; uint32 effective_uid = 2;