From 5611f290eb118eddc256560eaaa69f509347b4de Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 15 Jan 2021 08:43:23 -0800 Subject: [PATCH] ipn, ipnserver: only require sudo on Linux for mutable CLI actions This partially reverts d6e9fb1df0fd6, which modified the permissions on the tailscaled Unix socket and thus required "sudo tailscale" even for "tailscale status". Instead, open the permissions back up (on Linux only) but have the server look at the peer creds and only permit read-only actions unless you're root. In the future we'll also have a group that can do mutable actions. On OpenBSD and FreeBSD, the permissions on the socket remain locked down to 0600 from d6e9fb1df0fd6. Signed-off-by: Brad Fitzpatrick --- ipn/ipnserver/conn_linux.go | 49 +++++++++++++++++++++++ ipn/ipnserver/conn_no_ucred.go | 27 +++++++++++++ ipn/ipnserver/server.go | 12 ++++-- ipn/message.go | 72 ++++++++++++++++++++++++---------- ipn/message_test.go | 3 +- safesocket/unixsocket.go | 25 +++++++++++- 6 files changed, 161 insertions(+), 27 deletions(-) create mode 100644 ipn/ipnserver/conn_linux.go create mode 100644 ipn/ipnserver/conn_no_ucred.go diff --git a/ipn/ipnserver/conn_linux.go b/ipn/ipnserver/conn_linux.go new file mode 100644 index 000000000..1aca57e26 --- /dev/null +++ b/ipn/ipnserver/conn_linux.go @@ -0,0 +1,49 @@ +// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build linux + +package ipnserver + +import ( + "net" + + "golang.org/x/sys/unix" + "tailscale.com/types/logger" +) + +func isReadonlyConn(c net.Conn, logf logger.Logf) (ro bool) { + ro = true // conservative default for naked returns below + uc, ok := c.(*net.UnixConn) + if !ok { + logf("unexpected connection type %T", c) + return + } + raw, err := uc.SyscallConn() + if err != nil { + logf("SyscallConn: %v", err) + return + } + + var cred *unix.Ucred + cerr := raw.Control(func(fd uintptr) { + cred, err = unix.GetsockoptUcred(int(fd), + unix.SOL_SOCKET, + unix.SO_PEERCRED) + }) + if cerr != nil { + logf("raw.Control: %v", err) + return + } + if err != nil { + logf("raw.Control: %v", err) + return + } + if cred.Uid == 0 { + // root is not read-only. + return false + } + logf("non-root connection from %v (read-only)", cred.Uid) + return true +} diff --git a/ipn/ipnserver/conn_no_ucred.go b/ipn/ipnserver/conn_no_ucred.go new file mode 100644 index 000000000..c50e4778d --- /dev/null +++ b/ipn/ipnserver/conn_no_ucred.go @@ -0,0 +1,27 @@ +// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build !linux + +package ipnserver + +import ( + "net" + + "tailscale.com/types/logger" +) + +func isReadonlyConn(c net.Conn, logf logger.Logf) bool { + // Windows doesn't need/use this mechanism, at least yet. It + // has a different last-user-wins auth model. + + // And on Darwin, we're not using it yet, as the Darwin + // tailscaled port isn't yet done, and unix.Ucred and + // unix.GetsockoptUcred aren't in x/sys/unix. + + // TODO(bradfitz): OpenBSD and FreeBSD should implement this too. + // But their x/sys/unix package is different than Linux, so + // I didn't include it for now. + return false +} diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 745f72a08..c4fd95a2b 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -268,6 +268,10 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { defer s.removeAndCloseConn(c) logf("[v1] incoming control connection") + if isReadonlyConn(c, logf) { + ctx = ipn.ReadonlyContextOf(ctx) + } + for ctx.Err() == nil { msg, err := ipn.ReadMsg(br) if err != nil { @@ -279,7 +283,7 @@ func (s *server) serveConn(ctx context.Context, c net.Conn, logf logger.Logf) { return } s.bsMu.Lock() - if err := s.bs.GotCommandMsg(msg); err != nil { + if err := s.bs.GotCommandMsg(ctx, msg); err != nil { logf("GotCommandMsg: %v", err) } gotQuit := s.bs.GotQuit @@ -355,7 +359,7 @@ func (s *server) addConn(c net.Conn, isHTTP bool) (ci connIdentity, err error) { if doReset { s.logf("identity changed; resetting server") s.bsMu.Lock() - s.bs.Reset() + s.bs.Reset(context.TODO()) s.bsMu.Unlock() } }() @@ -407,7 +411,7 @@ func (s *server) removeAndCloseConn(c net.Conn) { } else { s.logf("client disconnected; stopping server") s.bsMu.Lock() - s.bs.Reset() + s.bs.Reset(context.TODO()) s.bsMu.Unlock() } } @@ -581,7 +585,7 @@ func Run(ctx context.Context, logf logger.Logf, logid string, getEngine func() ( server.bs = ipn.NewBackendServer(logf, b, server.writeToClients) if opts.AutostartStateKey != "" { - server.bs.GotCommand(&ipn.Command{ + server.bs.GotCommand(context.TODO(), &ipn.Command{ Version: version.Long, Start: &ipn.StartArgs{ Opts: ipn.Options{ diff --git a/ipn/message.go b/ipn/message.go index b8ef74a4f..a9106dac7 100644 --- a/ipn/message.go +++ b/ipn/message.go @@ -6,6 +6,7 @@ import ( "bytes" + "context" "encoding/binary" "encoding/json" "errors" @@ -20,6 +21,24 @@ "tailscale.com/version" ) +type readOnlyContextKey struct{} + +// IsReadonlyContext reports whether ctx is a read-only context, as currently used +// by Unix non-root users running the "tailscale" CLI command. They can run "status", +// but not much else. +func IsReadonlyContext(ctx context.Context) bool { + return ctx.Value(readOnlyContextKey{}) != nil +} + +// ReadonlyContextOf returns ctx wrapped with a context value that +// will make IsReadonlyContext reports true. +func ReadonlyContextOf(ctx context.Context) context.Context { + if IsReadonlyContext(ctx) { + return ctx + } + return context.WithValue(ctx, readOnlyContextKey{}, readOnlyContextKey{}) +} + var jsonEscapedZero = []byte(`\u0000`) type NoArgs struct{} @@ -111,7 +130,7 @@ func (bs *BackendServer) SendInUseOtherUserErrorMessage(msg string) { // GotCommandMsg parses the incoming message b as a JSON Command and // calls GotCommand with it. -func (bs *BackendServer) GotCommandMsg(b []byte) error { +func (bs *BackendServer) GotCommandMsg(ctx context.Context, b []byte) error { cmd := &Command{} if len(b) == 0 { return nil @@ -119,15 +138,15 @@ func (bs *BackendServer) GotCommandMsg(b []byte) error { if err := json.Unmarshal(b, cmd); err != nil { return err } - return bs.GotCommand(cmd) + return bs.GotCommand(ctx, cmd) } -func (bs *BackendServer) GotFakeCommand(cmd *Command) error { +func (bs *BackendServer) GotFakeCommand(ctx context.Context, cmd *Command) error { cmd.Version = version.Long - return bs.GotCommand(cmd) + return bs.GotCommand(ctx, cmd) } -func (bs *BackendServer) GotCommand(cmd *Command) error { +func (bs *BackendServer) GotCommand(ctx context.Context, cmd *Command) error { if cmd.Version != version.Long && !cmd.AllowVersionSkew { vs := fmt.Sprintf("GotCommand: Version mismatch! frontend=%#v backend=%#v", cmd.Version, version.Long) @@ -141,12 +160,33 @@ func (bs *BackendServer) GotCommand(cmd *Command) error { }) return nil } + + // TODO(bradfitz): finish plumbing context down to all the methods below; + // currently we just check for read-only contexts in this method and + // then never use contexts again. + + // Actions permitted with a read-only context: + if c := cmd.RequestEngineStatus; c != nil { + bs.b.RequestEngineStatus() + return nil + } else if c := cmd.RequestStatus; c != nil { + bs.b.RequestStatus() + return nil + } else if c := cmd.Ping; c != nil { + bs.b.Ping(c.IP) + return nil + } + + if IsReadonlyContext(ctx) { + msg := "permission denied" + bs.send(Notify{ErrMessage: &msg}) + return nil + } + if cmd.Quit != nil { bs.GotQuit = true return errors.New("Quit command received") - } - - if c := cmd.Start; c != nil { + } else if c := cmd.Start; c != nil { opts := c.Opts opts.Notify = bs.send return bs.b.Start(opts) @@ -165,27 +205,17 @@ func (bs *BackendServer) GotCommand(cmd *Command) error { } else if c := cmd.SetWantRunning; c != nil { bs.b.SetWantRunning(*c) return nil - } else if c := cmd.RequestEngineStatus; c != nil { - bs.b.RequestEngineStatus() - return nil - } else if c := cmd.RequestStatus; c != nil { - bs.b.RequestStatus() - return nil } else if c := cmd.FakeExpireAfter; c != nil { bs.b.FakeExpireAfter(c.Duration) return nil - } else if c := cmd.Ping; c != nil { - bs.b.Ping(c.IP) - return nil - } else { - return fmt.Errorf("BackendServer.Do: no command specified") } + return fmt.Errorf("BackendServer.Do: no command specified") } -func (bs *BackendServer) Reset() error { +func (bs *BackendServer) Reset(ctx context.Context) error { // Tell the backend we got a Logout command, which will cause it // to forget all its authentication information. - return bs.GotFakeCommand(&Command{Logout: &NoArgs{}}) + return bs.GotFakeCommand(ctx, &Command{Logout: &NoArgs{}}) } type BackendClient struct { diff --git a/ipn/message_test.go b/ipn/message_test.go index 1a657bc2f..9cd6a887e 100644 --- a/ipn/message_test.go +++ b/ipn/message_test.go @@ -6,6 +6,7 @@ import ( "bytes" + "context" "testing" "time" @@ -81,7 +82,7 @@ func TestClientServer(t *testing.T) { serverToClientCh <- append([]byte{}, b...) } clientToServer := func(b []byte) { - bs.GotCommandMsg(b) + bs.GotCommandMsg(context.TODO(), b) } slogf := func(fmt string, args ...interface{}) { t.Logf("s: "+fmt, args...) diff --git a/safesocket/unixsocket.go b/safesocket/unixsocket.go index ad96ac7be..996a274b9 100644 --- a/safesocket/unixsocket.go +++ b/safesocket/unixsocket.go @@ -64,10 +64,33 @@ func listen(path string, port uint16) (ln net.Listener, _ uint16, err error) { if err != nil { return nil, 0, err } - os.Chmod(path, 0600) + os.Chmod(path, socketPermissionsForOS()) return pipe, 0, err } +// socketPermissionsForOS returns the permissions to use for the +// tailscaled.sock. +func socketPermissionsForOS() os.FileMode { + if runtime.GOOS == "linux" { + // On Linux, the ipn/ipnserver package looks at the Unix peer creds + // and only permits read-only actions from non-root users, so we want + // this opened up wider. + // + // TODO(bradfitz): unify this all one in place probably, moving some + // of ipnserver (which does much of the "safe" bits) here. Maybe + // instead of net.Listener, we should return a type that returns + // an identity in addition to a net.Conn? (returning a wrapped net.Conn + // would surprise downstream callers probably) + // + // TODO(bradfitz): if OpenBSD and FreeBSD do the equivalent peercreds + // stuff that's in ipn/ipnserver/conn_ucred.go, they should also + // return 0666 here. + return 0666 + } + // Otherwise, root only. + return 0600 +} + // connectMacOSAppSandbox connects to the Tailscale Network Extension, // which is necessarily running within the macOS App Sandbox. Our // little dance to connect a regular user binary to the sandboxed