diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go index e0ab34d0d..17ecd6a14 100644 --- a/ssh/tailssh/incubator.go +++ b/ssh/tailssh/incubator.go @@ -235,6 +235,7 @@ func beIncubator(args []string) error { if err == nil && sessionCloser != nil { defer sessionCloser() } + var groupIDs []int for _, g := range strings.Split(ia.groups, ",") { gid, err := strconv.ParseInt(g, 10, 32) @@ -244,22 +245,10 @@ func beIncubator(args []string) error { groupIDs = append(groupIDs, int(gid)) } - if err := setGroups(groupIDs); err != nil { + if err := dropPrivileges(logf, int(ia.uid), ia.gid, groupIDs); err != nil { return err } - if egid := os.Getegid(); egid != ia.gid { - if err := syscall.Setgid(int(ia.gid)); err != nil { - logf(err.Error()) - os.Exit(1) - } - } - if euid != ia.uid { - // Switch users if required before starting the desired process. - if err := syscall.Setuid(int(ia.uid)); err != nil { - logf(err.Error()) - os.Exit(1) - } - } + if ia.isSFTP { logf("handling sftp") @@ -304,6 +293,108 @@ func beIncubator(args []string) error { return err } +// TODO(andrew-d): verify that this works in more configurations before +// enabling by default. +const assertDropPrivileges = false + +// dropPrivileges contains all the logic for dropping privileges to a different +// UID, GID, and set of supplementary groups. This function is +// security-sensitive and ordering-dependent; please be very cautious if/when +// refactoring. +// +// WARNING: if you change this function, you *MUST* run the TestDropPrivileges +// test in this package as root on at least Linux, FreeBSD and Darwin. This can +// be done by running: +// +// go test -c ./ssh/tailssh/ && sudo ./tailssh.test -test.v -test.run TestDropPrivileges +func dropPrivileges(logf logger.Logf, wantUid, wantGid int, supplementaryGroups []int) error { + fatalf := func(format string, args ...any) { + logf(format, args...) + os.Exit(1) + } + + euid := os.Geteuid() + egid := os.Getegid() + + if runtime.GOOS == "darwin" || runtime.GOOS == "freebsd" { + // On FreeBSD and Darwin, the first entry returned from the + // getgroups(2) syscall is the egid, and changing it with + // setgroups(2) changes the egid of the process. This is + // technically a violation of the POSIX standard; see the + // following article for more detail: + // https://www.usenix.org/system/files/login/articles/325-tsafrir.pdf + // + // In this case, we add an entry at the beginning of the + // groupIDs list containing the expected gid if it's not + // already there, which modifies the egid and additional groups + // as one unit. + if len(supplementaryGroups) == 0 || supplementaryGroups[0] != wantGid { + supplementaryGroups = append([]int{wantGid}, supplementaryGroups...) + } + } + + if err := setGroups(supplementaryGroups); err != nil { + return err + } + if egid != wantGid { + // On FreeBSD and Darwin, we may have already called the + // equivalent of setegid(wantGid) via the call to setGroups, + // above. However, per the manpage, setgid(getegid()) is an + // allowed operation regardless of privilege level. + // + // FreeBSD: + // The setgid() system call is permitted if the specified ID + // is equal to the real group ID or the effective group ID + // of the process, or if the effective user ID is that of + // the super user. + // + // Darwin: + // The setgid() function is permitted if the effective + // user ID is that of the super user, or if the specified + // group ID is the same as the effective group ID. If + // not, but the specified group ID is the same as the real + // group ID, setgid() will set the effective group ID to + // the real group ID. + if err := syscall.Setgid(wantGid); err != nil { + fatalf("Setgid(%d): %v", wantGid, err) + } + } + if euid != wantUid { + // Switch users if required before starting the desired process. + if err := syscall.Setuid(wantUid); err != nil { + fatalf("Setuid(%d): %v", wantUid, err) + } + } + + // If we changed either the UID or GID, defensively assert that we + // cannot reset the it back to our original values, and that the + // current egid/euid are the expected values after we change + // everything; if not, we exit the process. + if assertDropPrivileges { + if egid != wantGid { + if err := syscall.Setegid(egid); err == nil { + fatalf("unexpectedly able to set egid back to %d", egid) + } + } + if euid != wantUid { + if err := syscall.Seteuid(euid); err == nil { + fatalf("unexpectedly able to set euid back to %d", euid) + } + } + + if got := os.Getegid(); got != wantGid { + fatalf("got egid=%d, want %d", got, wantGid) + } + if got := os.Geteuid(); got != wantUid { + fatalf("got euid=%d, want %d", got, wantUid) + } + + // TODO(andrew-d): assert that our supplementary groups are correct + } + + return nil +} + // launchProcess launches an incubator process for the provided session. // It is responsible for configuring the process execution environment. // The caller can wait for the process to exit by calling cmd.Wait(). diff --git a/ssh/tailssh/privs_test.go b/ssh/tailssh/privs_test.go new file mode 100644 index 000000000..60cfe5e4f --- /dev/null +++ b/ssh/tailssh/privs_test.go @@ -0,0 +1,295 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build linux || darwin || freebsd || openbsd || netbsd || dragonfly + +package tailssh + +import ( + "encoding/json" + "errors" + "os" + "os/exec" + "os/user" + "path/filepath" + "reflect" + "regexp" + "runtime" + "strconv" + "syscall" + "testing" + + "golang.org/x/exp/slices" + "tailscale.com/types/logger" +) + +func TestDropPrivileges(t *testing.T) { + type SubprocInput struct { + UID int + GID int + AdditionalGroups []int + } + type SubprocOutput struct { + UID int + GID int + EUID int + EGID int + AdditionalGroups []int + } + + if v := os.Getenv("TS_TEST_DROP_PRIVILEGES_CHILD"); v != "" { + t.Logf("in child process") + + var input SubprocInput + if err := json.Unmarshal([]byte(v), &input); err != nil { + t.Fatal(err) + } + + // Get a handle to our provided JSON file before dropping privs. + f := os.NewFile(3, "out.json") + + // We're in our subprocess; actually drop privileges now. + dropPrivileges(t.Logf, input.UID, input.GID, input.AdditionalGroups) + + additional, _ := syscall.Getgroups() + + // Print our IDs + json.NewEncoder(f).Encode(SubprocOutput{ + UID: os.Getuid(), + GID: os.Getgid(), + EUID: os.Geteuid(), + EGID: os.Getegid(), + AdditionalGroups: additional, + }) + + // Close output file to ensure that it's flushed to disk before we exit + f.Close() + + // Always exit the process now that we have a different + // UID/GID/etc.; we don't want the Go test framework to try and + // clean anything up, since it might no longer have access. + os.Exit(0) + } + + if os.Getuid() != 0 { + t.Skip("test only works when run as root") + } + + rerunSelf := func(t *testing.T, input SubprocInput) []byte { + fpath := filepath.Join(t.TempDir(), "out.json") + outf, err := os.Create(fpath) + if err != nil { + t.Fatal(err) + } + + inputb, err := json.Marshal(input) + if err != nil { + t.Fatal(err) + } + + cmd := exec.Command(os.Args[0], "-test.v", "-test.run", "^"+regexp.QuoteMeta(t.Name())+"$") + cmd.Env = append(os.Environ(), "TS_TEST_DROP_PRIVILEGES_CHILD="+string(inputb)) + cmd.ExtraFiles = []*os.File{outf} + cmd.Stdout = logger.FuncWriter(logger.WithPrefix(t.Logf, "child: ")) + cmd.Stderr = logger.FuncWriter(logger.WithPrefix(t.Logf, "child: ")) + if err := cmd.Run(); err != nil { + t.Fatal(err) + } + outf.Close() + + jj, err := os.ReadFile(fpath) + if err != nil { + t.Fatal(err) + } + return jj + } + + // We want to ensure we're not colliding with existing users; find some + // unused UIDs and GIDs for the tests we run. + uid1 := findUnusedUID(t) + gid1 := findUnusedGID(t) + gid2 := findUnusedGID(t, gid1) + gid3 := findUnusedGID(t, gid1, gid2) + + // For some tests, we want a UID/GID pair with the same numerical + // value; this finds one. + uidgid1 := findUnusedUIDGID(t, uid1, gid1, gid2, gid3) + + t.Logf("uid1=%d gid1=%d gid2=%d gid3=%d uidgid1=%d", + uid1, gid1, gid2, gid3, uidgid1) + + testCases := []struct { + name string + uid int + gid int + additionalGroups []int + }{ + { + name: "all_different_values", + uid: uid1, + gid: gid1, + additionalGroups: []int{gid2, gid3}, + }, + { + name: "no_additional_groups", + uid: uid1, + gid: gid1, + additionalGroups: []int{}, + }, + // This is a regression test for the following bug, triggered + // on Darwin & FreeBSD: + // https://github.com/tailscale/tailscale/issues/7616 + { + name: "same_values", + uid: uidgid1, + gid: uidgid1, + additionalGroups: []int{uidgid1}, + }, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + subprocOut := rerunSelf(t, SubprocInput{ + UID: tt.uid, + GID: tt.gid, + AdditionalGroups: tt.additionalGroups, + }) + + var out SubprocOutput + if err := json.Unmarshal(subprocOut, &out); err != nil { + t.Logf("%s", subprocOut) + t.Fatal(err) + } + t.Logf("output: %+v", out) + + if out.UID != tt.uid { + t.Errorf("got uid %d; want %d", out.UID, tt.uid) + } + if out.GID != tt.gid { + t.Errorf("got gid %d; want %d", out.GID, tt.gid) + } + if out.EUID != tt.uid { + t.Errorf("got euid %d; want %d", out.EUID, tt.uid) + } + if out.EGID != tt.gid { + t.Errorf("got egid %d; want %d", out.EGID, tt.gid) + } + + // On FreeBSD and Darwin, the set of additional groups + // is prefixed with the egid; handle that case by + // modifying our expected set. + wantGroups := make(map[int]bool) + for _, id := range tt.additionalGroups { + wantGroups[id] = true + } + if runtime.GOOS == "darwin" || runtime.GOOS == "freebsd" { + wantGroups[tt.gid] = true + } + + gotGroups := make(map[int]bool) + for _, id := range out.AdditionalGroups { + gotGroups[id] = true + } + + if !reflect.DeepEqual(gotGroups, wantGroups) { + t.Errorf("got additional groups %+v; want %+v", gotGroups, wantGroups) + } + }) + } +} + +func findUnusedUID(t *testing.T, not ...int) int { + for i := 1000; i < 65535; i++ { + // Skip UIDs that might be valid + if maybeValidUID(i) { + continue + } + + // Skip UIDs that we're avoiding + if slices.Contains(not, i) { + continue + } + + // Not a valid UID, not one we're avoiding... all good! + return i + } + + t.Fatalf("unable to find an unused UID") + return -1 +} + +func findUnusedGID(t *testing.T, not ...int) int { + for i := 1000; i < 65535; i++ { + if maybeValidGID(i) { + continue + } + + // Skip GIDs that we're avoiding + if slices.Contains(not, i) { + continue + } + + // Not a valid GID, not one we're avoiding... all good! + return i + } + + t.Fatalf("unable to find an unused GID") + return -1 +} + +func findUnusedUIDGID(t *testing.T, not ...int) int { + for i := 1000; i < 65535; i++ { + if maybeValidUID(i) || maybeValidGID(i) { + continue + } + + // Skip IDs that we're avoiding + if slices.Contains(not, i) { + continue + } + + // Not a valid ID, not one we're avoiding... all good! + return i + } + + t.Fatalf("unable to find an unused UID/GID pair") + return -1 +} + +func maybeValidUID(id int) bool { + _, err := user.LookupId(strconv.Itoa(id)) + if err == nil { + return true + } + + var u1 user.UnknownUserIdError + if errors.As(err, &u1) { + return false + } + var u2 user.UnknownUserError + if errors.As(err, &u2) { + return false + } + + // Some other error; might be valid + return true +} + +func maybeValidGID(id int) bool { + _, err := user.LookupGroupId(strconv.Itoa(id)) + if err == nil { + return true + } + + var u1 user.UnknownGroupIdError + if errors.As(err, &u1) { + return false + } + var u2 user.UnknownGroupError + if errors.As(err, &u2) { + return false + } + + // Some other error; might be valid + return true +}