diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 29174fd26..b24eeeb4f 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -362,7 +362,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de 💣 tailscale.com/util/osdiag from tailscale.com/cmd/tailscaled+ W 💣 tailscale.com/util/osdiag/internal/wsc from tailscale.com/util/osdiag tailscale.com/util/osshare from tailscale.com/ipn/ipnlocal+ - LD tailscale.com/util/osuser from tailscale.com/ssh/tailssh + tailscale.com/util/osuser from tailscale.com/ssh/tailssh+ tailscale.com/util/race from tailscale.com/net/dns/resolver tailscale.com/util/racebuild from tailscale.com/logpolicy tailscale.com/util/rands from tailscale.com/ipn/ipnlocal+ diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 5a6507090..90b5cfd65 100644 --- a/ipn/ipnserver/server.go +++ b/ipn/ipnserver/server.go @@ -11,9 +11,7 @@ "io" "net" "net/http" - "os/exec" "os/user" - "runtime" "strconv" "strings" "sync" @@ -32,7 +30,6 @@ "tailscale.com/util/mak" "tailscale.com/util/set" "tailscale.com/util/systemd" - "tailscale.com/version" ) // Server is an IPN backend and its set of 0 or more active localhost @@ -205,7 +202,7 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) { lah := localapi.NewHandler(lb, s.logf, s.netMon, s.backendLogID) lah.PermitRead, lah.PermitWrite = s.localAPIPermissions(ci) lah.PermitCert = s.connCanFetchCerts(ci) - lah.CallerIsLocalAdmin = s.connIsLocalAdmin(ci) + lah.ConnIdentity = ci lah.ServeHTTP(w, r) return } @@ -367,61 +364,6 @@ func (s *Server) connCanFetchCerts(ci *ipnauth.ConnIdentity) bool { return false } -// connIsLocalAdmin reports whether ci has administrative access to the local -// machine, for whatever that means with respect to the current OS. -// -// This returns true only on Windows machines when the client user is elevated. -// This is useful because, on Windows, tailscaled itself always runs with -// elevated rights: we want to avoid privilege escalation for certain mutative operations. -func (s *Server) connIsLocalAdmin(ci *ipnauth.ConnIdentity) bool { - switch runtime.GOOS { - case "windows": - tok, err := ci.WindowsToken() - if err != nil { - if !errors.Is(err, ipnauth.ErrNotImplemented) { - s.logf("ipnauth.ConnIdentity.WindowsToken() error: %v", err) - } - return false - } - defer tok.Close() - - return tok.IsElevated() - - case "darwin": - if version.IsSandboxedMacOS() { - return false - } - // This is a standalone tailscaled setup, use the same logic as on - // Linux. - fallthrough - case "linux": - uid, ok := ci.Creds().UserID() - if !ok { - return false - } - // root is always admin. - if uid == "0" { - return true - } - // if non-root, must be operator AND able to execute "sudo tailscale". - operatorUID := s.mustBackend().OperatorUserID() - if operatorUID != "" && uid != operatorUID { - return false - } - u, err := user.LookupId(uid) - if err != nil { - return false - } - if err := exec.Command("sudo", "--other-user="+u.Name, "--list", "tailscale").Run(); err != nil { - return false - } - return true - - default: - return false - } -} - // addActiveHTTPRequest adds c to the server's list of active HTTP requests. // // If the returned error may be of type inUseOtherUserError. diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index a0066f17c..aeb5cd02c 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -18,6 +18,7 @@ "net/http/httputil" "net/netip" "net/url" + "os/exec" "runtime" "slices" "strconv" @@ -30,6 +31,7 @@ "tailscale.com/health" "tailscale.com/hostinfo" "tailscale.com/ipn" + "tailscale.com/ipn/ipnauth" "tailscale.com/ipn/ipnlocal" "tailscale.com/ipn/ipnstate" "tailscale.com/logtail" @@ -50,6 +52,7 @@ "tailscale.com/util/httpm" "tailscale.com/util/mak" "tailscale.com/util/osdiag" + "tailscale.com/util/osuser" "tailscale.com/util/rands" "tailscale.com/version" "tailscale.com/wgengine/magicsock" @@ -159,16 +162,12 @@ type Handler struct { // cert fetching access. PermitCert bool - // CallerIsLocalAdmin is whether the this handler is being invoked as a - // result of a LocalAPI call from a user who is a local admin of the current - // machine. - // - // As of 2023-10-26 it is only populated on Windows. - // - // It can be used to to restrict some LocalAPI operations which should only - // be run by an admin and not unprivileged users in a computing environment - // managed by IT admins. - CallerIsLocalAdmin bool + // ConnIdentity is the identity of the client connected to the Handler. + ConnIdentity *ipnauth.ConnIdentity + + // Test-only override for connIsLocalAdmin method. If non-nil, + // connIsLocalAdmin returns this value. + testConnIsLocalAdmin *bool b *ipnlocal.LocalBackend logf logger.Logf @@ -942,16 +941,22 @@ func (h *Handler) serveServeConfig(w http.ResponseWriter, r *http.Request) { } func authorizeServeConfigForGOOSAndUserContext(goos string, configIn *ipn.ServeConfig, h *Handler) error { - if !slices.Contains([]string{"windows", "linux", "darwin"}, goos) { + switch goos { + case "windows", "linux", "darwin": + default: return nil } - if goos == "darwin" && !version.IsSandboxedMacOS() { + // Only check for local admin on tailscaled-on-mac (based on "sudo" + // permissions). On sandboxed variants (MacSys and AppStore), tailscaled + // cannot serve files outside of the sandbox and this check is not + // relevant. + if goos == "darwin" && version.IsSandboxedMacOS() { return nil } if !configIn.HasPathHandler() { return nil } - if h.CallerIsLocalAdmin { + if h.connIsLocalAdmin() { return nil } switch goos { @@ -967,6 +972,76 @@ func authorizeServeConfigForGOOSAndUserContext(goos string, configIn *ipn.ServeC } +// connIsLocalAdmin reports whether the connected client has administrative +// access to the local machine, for whatever that means with respect to the +// current OS. +// +// This is useful because tailscaled itself always runs with elevated rights: +// we want to avoid privilege escalation for certain mutative operations. +func (h *Handler) connIsLocalAdmin() bool { + if h.testConnIsLocalAdmin != nil { + return *h.testConnIsLocalAdmin + } + if h.ConnIdentity == nil { + h.logf("[unexpected] missing ConnIdentity in LocalAPI Handler") + return false + } + switch runtime.GOOS { + case "windows": + tok, err := h.ConnIdentity.WindowsToken() + if err != nil { + if !errors.Is(err, ipnauth.ErrNotImplemented) { + h.logf("ipnauth.ConnIdentity.WindowsToken() error: %v", err) + } + return false + } + defer tok.Close() + + return tok.IsElevated() + + case "darwin": + // Unknown, or at least unchecked on sandboxed macOS variants. Err on + // the side of less permissions. + // + // authorizeServeConfigForGOOSAndUserContext should not call + // connIsLocalAdmin on sandboxed variants anyway. + if version.IsSandboxedMacOS() { + return false + } + // This is a standalone tailscaled setup, use the same logic as on + // Linux. + fallthrough + case "linux": + uid, ok := h.ConnIdentity.Creds().UserID() + if !ok { + return false + } + // root is always admin. + if uid == "0" { + return true + } + // if non-root, must be operator AND able to execute "sudo tailscale". + operatorUID := h.b.OperatorUserID() + if operatorUID != "" && uid != operatorUID { + return false + } + u, err := osuser.LookupByUID(uid) + if err != nil { + return false + } + // Short timeout just in case sudo hands for some reason. + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + if err := exec.CommandContext(ctx, "sudo", "--other-user="+u.Name, "--list", "tailscale").Run(); err != nil { + return false + } + return true + + default: + return false + } +} + func (h *Handler) serveCheckIPForwarding(w http.ResponseWriter, r *http.Request) { if !h.PermitRead { http.Error(w, "IP forwarding check access denied", http.StatusForbidden) diff --git a/ipn/localapi/localapi_test.go b/ipn/localapi/localapi_test.go index 2988e9604..cbaf8aa63 100644 --- a/ipn/localapi/localapi_test.go +++ b/ipn/localapi/localapi_test.go @@ -156,49 +156,17 @@ func TestWhoIsJustIP(t *testing.T) { } func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) { + newHandler := func(connIsLocalAdmin bool) *Handler { + return &Handler{testConnIsLocalAdmin: &connIsLocalAdmin} + } tests := []struct { name string - goos string configIn *ipn.ServeConfig h *Handler wantErr bool }{ { - name: "linux", - goos: "linux", - configIn: &ipn.ServeConfig{}, - h: &Handler{CallerIsLocalAdmin: false}, - wantErr: false, - }, - { - name: "linux-path-handler-admin", - goos: "linux", - configIn: &ipn.ServeConfig{ - Web: map[ipn.HostPort]*ipn.WebServerConfig{ - "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ - "/": {Path: "/tmp"}, - }}, - }, - }, - h: &Handler{CallerIsLocalAdmin: true}, - wantErr: false, - }, - { - name: "linux-path-handler-not-admin", - goos: "linux", - configIn: &ipn.ServeConfig{ - Web: map[ipn.HostPort]*ipn.WebServerConfig{ - "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ - "/": {Path: "/tmp"}, - }}, - }, - }, - h: &Handler{CallerIsLocalAdmin: false}, - wantErr: true, - }, - { - name: "windows-not-path-handler", - goos: "windows", + name: "not-path-handler", configIn: &ipn.ServeConfig{ Web: map[ipn.HostPort]*ipn.WebServerConfig{ "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ @@ -206,12 +174,11 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) { }}, }, }, - h: &Handler{CallerIsLocalAdmin: false}, + h: newHandler(false), wantErr: false, }, { - name: "windows-path-handler-admin", - goos: "windows", + name: "path-handler-admin", configIn: &ipn.ServeConfig{ Web: map[ipn.HostPort]*ipn.WebServerConfig{ "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ @@ -219,12 +186,11 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) { }}, }, }, - h: &Handler{CallerIsLocalAdmin: true}, + h: newHandler(true), wantErr: false, }, { - name: "windows-path-handler-not-admin", - goos: "windows", + name: "path-handler-not-admin", configIn: &ipn.ServeConfig{ Web: map[ipn.HostPort]*ipn.WebServerConfig{ "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ @@ -232,20 +198,36 @@ func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) { }}, }, }, - h: &Handler{CallerIsLocalAdmin: false}, + h: newHandler(false), wantErr: true, }, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := authorizeServeConfigForGOOSAndUserContext(tt.goos, tt.configIn, tt.h) - gotErr := err != nil - if gotErr != tt.wantErr { - t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want error %v", err, tt.wantErr) - } - }) + for _, goos := range []string{"linux", "windows", "darwin"} { + t.Run(goos+"-"+tt.name, func(t *testing.T) { + err := authorizeServeConfigForGOOSAndUserContext(goos, tt.configIn, tt.h) + gotErr := err != nil + if gotErr != tt.wantErr { + t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want error %v", err, tt.wantErr) + } + }) + } } + t.Run("other-goos", func(t *testing.T) { + configIn := &ipn.ServeConfig{ + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + "foo.test.ts.net:443": {Handlers: map[string]*ipn.HTTPHandler{ + "/": {Path: "/tmp"}, + }}, + }, + } + h := newHandler(false) + err := authorizeServeConfigForGOOSAndUserContext("dos", configIn, h) + if err != nil { + t.Errorf("authorizeServeConfigForGOOSAndUserContext() got error = %v, want nil", err) + } + }) } func TestServeWatchIPNBus(t *testing.T) {