[client] Restrict debug bundle log path and upload destinations (#6975)

client/internal/ipcauth/ownedfile.go

@@ -0,0 +1,63 @@
+package ipcauth
+
+import (
+	"fmt"
+	"os"
+)
+
+// OpenOwnedFile opens path for reading on behalf of the IPC caller identified by
+// id, and fails unless the opened file is a regular file that id owns.
+//
+// It exists for the paths a local caller hands to the daemon over the IPC. The
+// daemon runs as root, so opening such a path unchecked lets any local user read
+// any file through it. Ownership is the invariant that keeps the daemon from
+// reading, with its own privileges, a file the caller could not read itself: a
+// symlink or hard link planted at the path resolves to a file someone else owns
+// and is refused.
+//
+// The check is made against the open descriptor rather than the path, so
+// swapping the path between the check and the read cannot change the answer.
+//
+// A privileged caller is exempt: it can read the file directly, so refusing it
+// here would protect nothing. The regular-file requirement still applies to
+// everyone, since a fifo or device planted at the path is never a log file.
+func OpenOwnedFile(id Identity, path string) (*os.File, error) {
+	f, err := openForRead(path)
+	if err != nil {
+		return nil, err
+	}
+
+	if err := checkOwnership(id, f); err != nil {
+		if cerr := f.Close(); cerr != nil {
+			return nil, fmt.Errorf("%w (close: %v)", err, cerr)
+		}
+		return nil, err
+	}
+
+	return f, nil
+}
+
+func checkOwnership(id Identity, f *os.File) error {
+	info, err := f.Stat()
+	if err != nil {
+		return fmt.Errorf("stat %s: %w", f.Name(), err)
+	}
+
+	if !info.Mode().IsRegular() {
+		return fmt.Errorf("%s is not a regular file", f.Name())
+	}
+
+	if IsPrivilegedCaller(id) {
+		return nil
+	}
+
+	owned, err := fileOwnedBy(id, f)
+	if err != nil {
+		return fmt.Errorf("read owner of %s: %w", f.Name(), err)
+	}
+	if !owned {
+		return fmt.Errorf("%s is not owned by the caller (%s)", f.Name(), id)
+	}
+
+	return nil
+}

client/internal/ipcauth/ownedfile_test.go

@@ -0,0 +1,64 @@
+package ipcauth
+
+import (
+	"io"
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// otherIdentity is an unprivileged caller that owns nothing the test creates.
+func otherIdentity(t *testing.T) Identity {
+	t.Helper()
+	if runtime.GOOS == "windows" {
+		return Identity{SID: "S-1-5-21-1-2-3-1001"}
+	}
+	return Identity{UID: uint32(os.Geteuid() + 1), GID: uint32(os.Getegid() + 1)}
+}
+
+func TestOpenOwnedFileReadsFileOwnedByCaller(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "gui-client.log")
+	require.NoError(t, os.WriteFile(path, []byte("hello"), 0600))
+
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	f, err := OpenOwnedFile(id, path)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = f.Close() })
+
+	content, err := io.ReadAll(f)
+	require.NoError(t, err)
+	require.Equal(t, "hello", string(content))
+}
+
+func TestOpenOwnedFileRefusesFileOwnedByAnother(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "gui-client.log")
+	require.NoError(t, os.WriteFile(path, []byte("secret"), 0600))
+
+	_, err := OpenOwnedFile(otherIdentity(t), path)
+	require.ErrorContains(t, err, "not owned by the caller")
+}
+
+func TestOpenOwnedFileRefusesNonRegularFile(t *testing.T) {
+	dir := t.TempDir()
+
+	// The caller owns the directory, so this is the regular-file requirement
+	// talking, not the ownership check.
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	_, err = OpenOwnedFile(id, dir)
+	require.ErrorContains(t, err, "not a regular file")
+}
+
+func TestOpenOwnedFileRefusesMissingFile(t *testing.T) {
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	_, err = OpenOwnedFile(id, filepath.Join(t.TempDir(), "absent.log"))
+	require.Error(t, err)
+}

client/internal/ipcauth/ownedfile_unix.go

@@ -0,0 +1,35 @@
+//go:build !windows
+
+package ipcauth
+
+import (
+	"fmt"
+	"os"
+	"syscall"
+)
+
+// openForRead opens a caller-supplied path without following a symlink at its
+// final component and without blocking: a fifo planted at the path would
+// otherwise stall the open until a writer appears, and the daemon holds a lock
+// while it collects the file.
+func openForRead(path string) (*os.File, error) {
+	f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, fmt.Errorf("open %s: %w", path, err)
+	}
+	return f, nil
+}
+
+func fileOwnedBy(id Identity, f *os.File) (bool, error) {
+	info, err := f.Stat()
+	if err != nil {
+		return false, err
+	}
+
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		return false, fmt.Errorf("no owner information in %T", info.Sys())
+	}
+
+	return stat.Uid == id.UID, nil
+}

client/internal/ipcauth/ownedfile_unix_test.go

@@ -0,0 +1,57 @@
+//go:build !windows
+
+package ipcauth
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// A symlink is the shape the arbitrary-read attempt takes: the caller owns the
+// link, the file it points at belongs to someone else.
+func TestOpenOwnedFileRefusesSymlink(t *testing.T) {
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target.log")
+	require.NoError(t, os.WriteFile(target, []byte("secret"), 0600))
+
+	link := filepath.Join(dir, "gui-client.log")
+	require.NoError(t, os.Symlink(target, link))
+
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	_, err = OpenOwnedFile(id, link)
+	// O_NOFOLLOW on a symlink reports ELOOP on Linux/Darwin and EMLINK on FreeBSD.
+	if !errors.Is(err, syscall.ELOOP) && !errors.Is(err, syscall.EMLINK) {
+		t.Fatalf("symlink open: got %v, want ELOOP or EMLINK", err)
+	}
+}
+
+// A fifo would block the open until a writer showed up, stalling the daemon
+// while it holds its lock.
+func TestOpenOwnedFileRefusesFifoWithoutBlocking(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "gui-client.log")
+	require.NoError(t, syscall.Mkfifo(path, 0600))
+
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	done := make(chan error, 1)
+	go func() {
+		_, err := OpenOwnedFile(id, path)
+		done <- err
+	}()
+
+	select {
+	case err := <-done:
+		require.ErrorContains(t, err, "not a regular file")
+	case <-time.After(5 * time.Second):
+		t.Fatal("opening a fifo blocked")
+	}
+}

client/internal/ipcauth/ownedfile_windows.go

@@ -0,0 +1,59 @@
+//go:build windows
+
+package ipcauth
+
+import (
+	"fmt"
+	"os"
+
+	"golang.org/x/sys/windows"
+)
+
+// openForRead opens a caller-supplied path without following a reparse point at
+// it. FILE_FLAG_OPEN_REPARSE_POINT is the Windows analogue of O_NOFOLLOW: it
+// opens a symlink/junction itself rather than its target, so the regular-file
+// check in checkOwnership refuses a link the caller planted to redirect the
+// read. FILE_FLAG_BACKUP_SEMANTICS lets a directory open too (as os.Open does),
+// so a directory planted at the path is refused as non-regular rather than
+// erroring here. The share mode matches os.Open so a log being written stays
+// openable.
+func openForRead(path string) (*os.File, error) {
+	p, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return nil, fmt.Errorf("convert path %s: %w", path, err)
+	}
+
+	handle, err := windows.CreateFile(
+		p,
+		windows.GENERIC_READ,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		nil,
+		windows.OPEN_EXISTING,
+		windows.FILE_FLAG_OPEN_REPARSE_POINT|windows.FILE_FLAG_BACKUP_SEMANTICS,
+		0,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("open %s: %w", path, err)
+	}
+
+	return os.NewFile(uintptr(handle), path), nil
+}
+
+// fileOwnedBy compares the file's owner SID with the caller's. Files an elevated
+// process creates are owned by BUILTIN\Administrators rather than by the user,
+// but such a caller is privileged and never reaches this check.
+func fileOwnedBy(id Identity, f *os.File) (bool, error) {
+	// x/sys/windows GetSecurityInfo frees the OS buffer itself and returns a
+	// Go-heap copy, so there is nothing to LocalFree here.
+	sd, err := windows.GetSecurityInfo(windows.Handle(f.Fd()), windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION)
+	if err != nil {
+		return false, fmt.Errorf("read security info: %w", err)
+	}
+
+	owner, _, err := sd.Owner()
+	if err != nil {
+		return false, fmt.Errorf("read owner: %w", err)
+	}
+
+	return id.SID != "" && owner.String() == id.SID, nil
+}

client/internal/ipcauth/ownedfile_windows_test.go

@@ -0,0 +1,78 @@
+//go:build windows
+
+package ipcauth
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
+)
+
+// fileOwnerSID reads the owner SID of path the same way OpenOwnedFile does, so
+// the test can construct an Identity that matches (or deliberately does not).
+func fileOwnerSID(t *testing.T, path string) string {
+	t.Helper()
+	f, err := os.Open(path)
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = f.Close() })
+
+	sd, err := windows.GetSecurityInfo(windows.Handle(f.Fd()), windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION)
+	require.NoError(t, err)
+	owner, _, err := sd.Owner()
+	require.NoError(t, err)
+	return owner.String()
+}
+
+// The allow branch of fileOwnedBy is the SID-equality path the legitimate GUI
+// flow depends on. Running elevated, a created file is owned by
+// BUILTIN\Administrators; an Identity carrying that SID with Elevated=false and
+// no groups is unprivileged by IsPrivileged (which reads the token, not the
+// SID's RID), so this exercises the real GetSecurityInfo equality rather than
+// the privileged-caller shortcut.
+func TestOpenOwnedFileWindowsOwnerMatchAllows(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "gui-client.log")
+	require.NoError(t, os.WriteFile(path, []byte("hello"), 0600))
+
+	ownerSID := fileOwnerSID(t, path)
+	id := Identity{SID: ownerSID}
+	require.False(t, id.IsPrivileged(), "identity built from the owner SID must be unprivileged for this to test the match path")
+
+	f, err := OpenOwnedFile(id, path)
+	require.NoError(t, err)
+	_ = f.Close()
+}
+
+func TestOpenOwnedFileWindowsOwnerMismatchRefuses(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "gui-client.log")
+	require.NoError(t, os.WriteFile(path, []byte("secret"), 0600))
+
+	other := Identity{SID: "S-1-5-21-9-9-9-9999"}
+	require.False(t, other.IsPrivileged())
+
+	_, err := OpenOwnedFile(other, path)
+	require.ErrorContains(t, err, "not owned by the caller")
+}
+
+// FILE_FLAG_OPEN_REPARSE_POINT must make OpenOwnedFile refuse a symlink the same
+// way O_NOFOLLOW does on Unix, so a planted link can't redirect the read to
+// another file. Creating a symlink needs a privilege the runner may lack, so the
+// test skips rather than fails when it can't.
+func TestOpenOwnedFileWindowsRefusesSymlink(t *testing.T) {
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target.log")
+	require.NoError(t, os.WriteFile(target, []byte("secret"), 0600))
+
+	link := filepath.Join(dir, "gui-client.log")
+	if err := os.Symlink(target, link); err != nil {
+		t.Skipf("cannot create symlink (privilege not held?): %v", err)
+	}
+
+	id, err := CurrentProcessIdentity()
+	require.NoError(t, err)
+
+	_, err = OpenOwnedFile(id, link)
+	require.Error(t, err, "a symlink must be refused")
+}