diff --git a/.github/workflows/golang-test-linux.yml b/.github/workflows/golang-test-linux.yml index eea69eaa7..e4388bfe3 100644 --- a/.github/workflows/golang-test-linux.yml +++ b/.github/workflows/golang-test-linux.yml @@ -244,6 +244,14 @@ jobs: NB_TEST_PKCS11_DISPOSABLE: "1" run: CGO_ENABLED=0 go test -tags pkcs11 -timeout 5m ./client/internal/pkcs11/... ./client/internal/certproof/... + - name: Test SSH privilege drop + # purego marks the runtime as cgo without libc's setxid hooks, so the syscall + # package's setuid family panics in this build. Drop privileges as root to cover it. + run: | + CGO_ENABLED=0 go test -c -tags 'pkcs11 privileged' -o "$RUNNER_TEMP/ssh-server.test" ./client/ssh/server/ + sudo "$RUNNER_TEMP/ssh-server.test" -test.run '^TestDropPrivilegesAllThreads$' -test.v + CGO_ENABLED=0 GOARCH=arm64 go test -c -tags 'pkcs11 privileged' -o "$RUNNER_TEMP/ssh-server-arm64.test" ./client/ssh/server/ + test_client_on_docker: name: "Client (Docker) / Unit" needs: [build-cache] diff --git a/.golangci.yaml b/.golangci.yaml index e350b9de7..f52f822dd 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -24,6 +24,15 @@ linters: settings: errcheck: check-type-assertions: false + forbidigo: + analyze-types: true + forbid: + - pattern: ^(fmt\.Print(|f|ln)|print|println)$ + # Builds that link libc through purego mark the runtime as cgo without + # registering the libc setxid hooks, so these panic in AllThreadsSyscall. + - pattern: ^(syscall|unix)\.(Setgroups|Set(e|re|res)?[ug]id|AllThreadsSyscall6?)$ + pkg: ^(syscall|golang\.org/x/sys/unix)$ + msg: use the setgroups/setgid/setuid helpers in client/ssh/server, which go through libc in purego builds gocritic: disabled-checks: - commentFormatting @@ -86,6 +95,9 @@ linters: - linters: - forbidigo path: signal/cmd/root\.go + - linters: + - forbidigo + path: client/ssh/server/setid_unix\.go - linters: - unused path: sharedsock/filter\.go diff --git a/client/ssh/server/executor_drop_privileged_linux_test.go b/client/ssh/server/executor_drop_privileged_linux_test.go new file mode 100644 index 000000000..27cc1384b --- /dev/null +++ b/client/ssh/server/executor_drop_privileged_linux_test.go @@ -0,0 +1,102 @@ +//go:build linux && privileged + +package server + +import ( + "bufio" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const envDropPrivilegesChild = "NB_TEST_DROP_PRIVILEGES_CHILD" + +const ( + dropTargetUID = 65534 + dropTargetGID = 65534 + dropExtraGID = 65533 +) + +// TestDropPrivilegesAllThreads drops privileges in a child process, because the drop +// cannot be undone, and checks that every OS thread of that process carries the target +// credentials. A drop applied to the calling thread only would leave the others as root. +func TestDropPrivilegesAllThreads(t *testing.T) { + if os.Getenv(envDropPrivilegesChild) == "1" { + dropPrivilegesChild(t) + return + } + if os.Geteuid() != 0 { + t.Skip("requires root") + } + + cmd := exec.Command(os.Args[0], "-test.run=^TestDropPrivilegesAllThreads$", "-test.v") + cmd.Env = append(os.Environ(), envDropPrivilegesChild+"=1") + out, err := cmd.CombinedOutput() + t.Logf("child output:\n%s", out) + require.NoError(t, err, "child must drop privileges on all threads") + assert.Contains(t, string(out), "--- PASS: TestDropPrivilegesAllThreads", "child test must run and pass") +} + +func dropPrivilegesChild(t *testing.T) { + // Park goroutines on their own OS threads so the process has threads other than + // the one that performs the drop. + release := make(chan struct{}) + defer close(release) + ready := make(chan struct{}) + for range 4 { + go func() { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + ready <- struct{}{} + <-release + }() + <-ready + } + + pd := NewPrivilegeDropper() + require.NoError(t, pd.DropPrivileges(dropTargetUID, dropTargetGID, []uint32{dropTargetGID, dropExtraGID})) + + tasks, err := filepath.Glob("/proc/self/task/*/status") + require.NoError(t, err) + require.Greater(t, len(tasks), 4, "process must have several threads for the check to mean anything") + + want := map[string]string{ + "Uid": fmt.Sprintf("%d\t%d\t%d\t%d", dropTargetUID, dropTargetUID, dropTargetUID, dropTargetUID), + "Gid": fmt.Sprintf("%d\t%d\t%d\t%d", dropTargetGID, dropTargetGID, dropTargetGID, dropTargetGID), + "Groups": fmt.Sprintf("%d %d", dropExtraGID, dropTargetGID), + } + for _, task := range tasks { + got := readStatusFields(t, task) + for field, value := range want { + assert.Equal(t, value, got[field], "%s of %s", field, task) + } + } + + assert.Error(t, seteuid(0), "euid 0 must not be restorable after the drop") + assert.Error(t, setegid(0), "egid 0 must not be restorable after the drop") +} + +func readStatusFields(t *testing.T, path string) map[string]string { + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + + fields := make(map[string]string) + scanner := bufio.NewScanner(f) + for scanner.Scan() { + key, value, ok := strings.Cut(scanner.Text(), ":") + if !ok { + continue + } + fields[key] = strings.TrimSpace(value) + } + require.NoError(t, scanner.Err()) + return fields +} diff --git a/client/ssh/server/executor_unix.go b/client/ssh/server/executor_unix.go index ee0b0ff78..5b42190ab 100644 --- a/client/ssh/server/executor_unix.go +++ b/client/ssh/server/executor_unix.go @@ -11,7 +11,6 @@ import ( "path/filepath" "runtime" "strings" - "syscall" log "github.com/sirupsen/logrus" ) @@ -151,15 +150,15 @@ func (pd *PrivilegeDropper) setGroupsAndIDs(targetUID, targetGID uint32, supplem } } - if err := syscall.Setgroups(groups); err != nil { + if err := setgroups(groups); err != nil { return fmt.Errorf("setgroups to %v: %w", groups, err) } - if err := syscall.Setgid(int(targetGID)); err != nil { + if err := setgid(int(targetGID)); err != nil { return fmt.Errorf("setgid to %d: %w", targetGID, err) } - if err := syscall.Setuid(int(targetUID)); err != nil { + if err := setuid(int(targetUID)); err != nil { return fmt.Errorf("setuid to %d: %w", targetUID, err) } @@ -182,12 +181,12 @@ func (pd *PrivilegeDropper) validatePrivilegeDropSuccess(targetUID, targetGID ui // validatePrivilegeDropReversibility ensures privileges cannot be restored func (pd *PrivilegeDropper) validatePrivilegeDropReversibility(targetUID, targetGID uint32, originalUID, originalGID int) error { if originalGID != int(targetGID) { - if err := syscall.Setegid(originalGID); err == nil { + if err := setegid(originalGID); err == nil { return fmt.Errorf("privilege drop validation failed: able to restore original GID %d", originalGID) } } if originalUID != int(targetUID) { - if err := syscall.Seteuid(originalUID); err == nil { + if err := seteuid(originalUID); err == nil { return fmt.Errorf("privilege drop validation failed: able to restore original UID %d", originalUID) } } diff --git a/client/ssh/server/setid_libc_linux.go b/client/ssh/server/setid_libc_linux.go new file mode 100644 index 000000000..13a9f254c --- /dev/null +++ b/client/ssh/server/setid_libc_linux.go @@ -0,0 +1,108 @@ +//go:build pkcs11 && linux && !android && (amd64 || arm64) + +package server + +import ( + "fmt" + "runtime" + "sync" + "syscall" + + "github.com/ebitengine/purego" +) + +// This build links libc through purego, whose fakecgo runtime marks the binary as cgo +// without registering the libc hooks a real cgo build provides. syscall's setuid family +// then panics in AllThreadsSyscall, so the calls go to libc directly: glibc applies them +// to every thread, and all Go threads here are created through pthread_create. + +type libcSetID struct { + setgroups func(size uintptr, list *uint32) int32 + setgid func(gid uint32) int32 + setuid func(uid uint32) int32 + setegid func(egid uint32) int32 + seteuid func(euid uint32) int32 + errnoLocation func() *int32 +} + +var ( + libcSetIDOnce sync.Once + libcSetIDFns libcSetID + libcSetIDErr error +) + +func loadLibcSetID() (*libcSetID, error) { + libcSetIDOnce.Do(func() { + syms := []struct { + fn any + name string + }{ + {&libcSetIDFns.setgroups, "setgroups"}, + {&libcSetIDFns.setgid, "setgid"}, + {&libcSetIDFns.setuid, "setuid"}, + {&libcSetIDFns.setegid, "setegid"}, + {&libcSetIDFns.seteuid, "seteuid"}, + {&libcSetIDFns.errnoLocation, "__errno_location"}, + } + for _, s := range syms { + addr, err := purego.Dlsym(purego.RTLD_DEFAULT, s.name) + if err != nil { + libcSetIDErr = fmt.Errorf("resolve libc %s: %w", s.name, err) + return + } + purego.RegisterFunc(s.fn, addr) + } + }) + if libcSetIDErr != nil { + return nil, libcSetIDErr + } + return &libcSetIDFns, nil +} + +// libcCall runs call on a locked thread so errno is read from the thread that set it. +func libcCall(call func(*libcSetID) int32) error { + fns, err := loadLibcSetID() + if err != nil { + return err + } + + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + if call(fns) == 0 { + return nil + } + if errno := *fns.errnoLocation(); errno != 0 { + return syscall.Errno(errno) + } + return syscall.EINVAL +} + +func setgroups(gids []int) error { + list := make([]uint32, len(gids)) + for i, g := range gids { + list[i] = uint32(g) + } + return libcCall(func(f *libcSetID) int32 { + if len(list) == 0 { + return f.setgroups(0, nil) + } + return f.setgroups(uintptr(len(list)), &list[0]) + }) +} + +func setgid(gid int) error { + return libcCall(func(f *libcSetID) int32 { return f.setgid(uint32(gid)) }) +} + +func setuid(uid int) error { + return libcCall(func(f *libcSetID) int32 { return f.setuid(uint32(uid)) }) +} + +func setegid(egid int) error { + return libcCall(func(f *libcSetID) int32 { return f.setegid(uint32(egid)) }) +} + +func seteuid(euid int) error { + return libcCall(func(f *libcSetID) int32 { return f.seteuid(uint32(euid)) }) +} diff --git a/client/ssh/server/setid_unix.go b/client/ssh/server/setid_unix.go new file mode 100644 index 000000000..bd9fe3856 --- /dev/null +++ b/client/ssh/server/setid_unix.go @@ -0,0 +1,25 @@ +//go:build unix && !(pkcs11 && linux && !android && (amd64 || arm64)) + +package server + +import "syscall" + +func setgroups(gids []int) error { + return syscall.Setgroups(gids) +} + +func setgid(gid int) error { + return syscall.Setgid(gid) +} + +func setuid(uid int) error { + return syscall.Setuid(uid) +} + +func setegid(egid int) error { + return syscall.Setegid(egid) +} + +func seteuid(euid int) error { + return syscall.Seteuid(euid) +}