CVE-2023-28436 (Go, published 2023-03-23T20:15Z)
Description: value: Tailscale is software for using Wireguard and multi-factor authentication (MFA). A vulnerability identified in the implementation of Tailscale SSH starting in version 1.34.0 and prior to prior to 1.38.2 in FreeBSD allows commands to be run with a higher privilege group ID than that specified in Tailscale SSH access rules. A difference in the behavior of the FreeBSD `setgroups` system call from POSIX meant that the Tailscale client running on a FreeBSD-based operating system did not appropriately restrict groups on the host when using Tailscale SSH. When accessing a FreeBSD host over Tailscale SSH, the egid of the tailscaled process was used instead of that of the user specified in Tailscale SSH access rules.

Tailscale SSH commands may have been run with a higher privilege group ID than that specified in Tailscale SSH access rules if they met all of the following criteria: the destination node was a FreeBSD device with Tailscale SSH enabled; Tailscale SSH access rules permitted access for non-root users; and a non-interactive SSH session was used.

Affected users should upgrade to version 1.38.2 to remediate the issue.


Severity: nan (CVSS v3 base score: 8)
Weakness: CWE-269 \u2014 Improper Privilege Management
The product does not properly assign, modify, track, or check privileges for an actor, creating an unintended sphere of control for that actor.
Repository: https://github.com/tailscale/tailscale
Files changed:
ssh/tailssh/incubator.go
ssh/tailssh/privs_test.go
Commit message: ssh/tailssh: fix privilege dropping on FreeBSD; add tests

On FreeBSD and Darwin, changing a process's supplementary groups with
setgroups(2) will also change the egid of the process, setting it to the
first entry in the provided list. This is distinct from the behaviour on
other platforms (and possibly a violation of the POSIX standard).

Because of this, on FreeBSD with no TTY, our incubator code would
previously not change the process's gid, because it would read the
newly-changed egid, compare it against the expected egid, and since they
matched, not change the gid. Because we didn't use the 'login' program
on FreeBSD without a TTY, this would propagate to a child process.

This could be observed by running "id -p" in two contexts. The expected
output, and the output returned when running from a SSH shell, is:

    andrew@freebsd:~ $ id -p
    uid         andrew
    groups      andrew

However, when run via "ssh andrew@freebsd id -p", the output would be:

    $ ssh andrew@freebsd id -p
    login       root
    uid         andrew
    rgid        wheel
    groups      andrew

(this could also be observed via "id -g -r" to print just the gid)

We fix this by pulling the details of privilege dropping out into their
own function and prepending the expected gid to the start of the list on
Darwin and FreeBSD.

Finally, we add some tests that run a child process, drop privileges,
and assert that the final UID/GID/additional groups are what we expect.

More information can be found in the following article:
    https://www.usenix.org/system/files/login/articles/325-tsafrir.pdf

Updates #7616
Alternative to #7609

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I0e6513c31b121108b50fe561c89e5816d84a45b9
(cherry picked from commit ccace1f7df0e4155e00258a0122663228473a2cc)


Vulnerable code:
	if err := setGroups(groupIDs); err != nil {
	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)
		}
	}

Fixed code:

	if err := dropPrivileges(logf, int(ia.uid), ia.gid, groupIDs); err != nil {

// 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
}

// 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
}