util/osuser: reject leading dashes in usernames

Reject leading dashes in usernames and add double dash to getent call
on linux to prevent values sent as usernames being interpreted as
command options.

Fixes https://github.com/tailscale/corp/issues/44813

Signed-off-by: Mario Minardi <mario@tailscale.com>

util/osuser/user.go

@@ -8,11 +8,14 @@ package osuser
 import (
 	"context"
 	"errors"
+	"fmt"
 	"log"
 	"os/exec"
 	"os/user"
+	"reflect"
 	"runtime"
 	"strings"
+	"sync"
 	"time"
 	"unicode/utf8"
 
@@ -50,6 +53,12 @@ func LookupByUsername(username string) (*user.User, error) {
 // lookupStd is either user.Lookup or user.LookupId.
 type lookupStd func(string) (*user.User, error)
 
+var execGetent = func(ctx context.Context, args ...string) ([]byte, error) {
+	return exec.CommandContext(ctx, "getent", args...).Output()
+}
+
+var getentDoubleDashSupported = sync.OnceValue(probeGetentDoubleDashSupport)
+
 func lookup(usernameOrUID string, std lookupStd, wantShell bool) (*user.User, string, error) {
 	// Skip getent entirely on Non-Unix platforms that won't ever have it.
 	// (Using HasPrefix for "wasip1", anticipating that WASI support will
@@ -118,6 +127,12 @@ func checkGetentInput(usernameOrUID string) bool {
 	if len(usernameOrUID) > maxUid || len(usernameOrUID) == 0 {
 		return false
 	}
+
+	// Leading dashes aren't valid for usernames.
+	if strings.HasPrefix(usernameOrUID, "-") {
+		return false
+	}
+
 	for _, r := range usernameOrUID {
 		if r < ' ' || r == 0x7f || r == utf8.RuneError { // TODO(bradfitz): more?
 			return false
@@ -139,23 +154,77 @@ func userLookupGetent(usernameOrUID string, std lookupStd) (*user.User, string,
 	}
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
-	out, err := exec.CommandContext(ctx, "getent", "passwd", usernameOrUID).Output()
+
+	args := []string{"passwd"}
+	// Append "--" only if the local getent accepts it, to prevent a username or
+	// UID from being interpreted as an option without breaking getent variants
+	// that do not support end-of-options parsing here.
+	if getentDoubleDashSupported() {
+		args = append(args, "--")
+	}
+	args = append(args, usernameOrUID)
+
+	out, err := execGetent(ctx, args...)
 	if err != nil {
 		log.Printf("error calling getent for user %q: %v", usernameOrUID, err)
 		u, err := std(usernameOrUID)
 		return u, "", err
 	}
+	u, shell, err := parseGetentUser(out)
+	if err != nil {
+		log.Printf("getent for user %q returned invalid output %q: %v", usernameOrUID, out, err)
+		u, err := std(usernameOrUID)
+		return u, "", err
+	}
+	return u, shell, nil
+}
+
+func probeGetentDoubleDashSupport() bool {
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	out, err := execGetent(ctx, "passwd", "root")
+	if err != nil {
+		return false
+	}
+
+	outDoubleDash, err := execGetent(ctx, "passwd", "--", "root")
+	if err != nil {
+		return false
+	}
+
+	u, shell, err := parseGetentUser(out)
+	if err != nil {
+		return false
+	}
+
+	uDoubleDash, shellDoubleDash, err := parseGetentUser(outDoubleDash)
+	if err != nil {
+		return false
+	}
+
+	// Short-circuit if the user is obviously incorrect
+	if uDoubleDash.Name != "root" || uDoubleDash.Uid != "0" || uDoubleDash.Gid != "0" {
+		return false
+	}
+
+	if !reflect.DeepEqual(u, uDoubleDash) || shell != shellDoubleDash {
+		return false
+	}
+
+	return true
+}
+
+func parseGetentUser(out []byte) (*user.User, string, error) {
 	// output is "alice:x:1001:1001:Alice Smith,,,:/home/alice:/bin/bash"
 	f := strings.SplitN(strings.TrimSpace(string(out)), ":", 10)
 	for len(f) < 7 {
 		f = append(f, "")
 	}
-	var mandatoryFields = []int{0, 2, 3, 5}
-	for _, v := range mandatoryFields {
-		if f[v] == "" {
-			log.Printf("getent for user %q returned invalid output: %q", usernameOrUID, out)
-			u, err := std(usernameOrUID)
-			return u, "", err
+	var mandatoryFields = map[int]string{0: "Username", 2: "Uid", 3: "Gid", 5: "HomeDir"}
+	for k, v := range mandatoryFields {
+		if f[k] == "" {
+			return nil, "", fmt.Errorf("missing mandatory field %q", v)
 		}
 	}
 	return &user.User{

util/osuser/user_test.go

@@ -0,0 +1,103 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package osuser
+
+import (
+	"context"
+	"errors"
+	"os/user"
+	"reflect"
+	"sync"
+	"testing"
+)
+
+func TestProbeGetentDoubleDashSupport(t *testing.T) {
+	origExecGetent := execGetent
+	t.Cleanup(func() {
+		execGetent = origExecGetent
+	})
+
+	tests := []struct {
+		name string
+		out  []byte
+		err  error
+		want bool
+	}{
+		{
+			name: "supported",
+			out:  []byte("root:x:0:0:root:/root:/bin/sh\n"),
+			want: true,
+		},
+		{
+			name: "error",
+			err:  errors.New("unsupported"),
+			want: false,
+		},
+		{
+			name: "wrong-user",
+			out:  []byte("daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin\n"),
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			execGetent = func(_ context.Context, args ...string) ([]byte, error) {
+				return tt.out, tt.err
+			}
+			if got := probeGetentDoubleDashSupport(); got != tt.want {
+				t.Fatalf("probeGetentDashDashSupport() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestUserLookupGetentUsesProbeResult(t *testing.T) {
+	origExecGetent := execGetent
+	origProbe := getentDoubleDashSupported
+	t.Cleanup(func() {
+		execGetent = origExecGetent
+		getentDoubleDashSupported = origProbe
+	})
+
+	tests := []struct {
+		name      string
+		supported bool
+		wantArgs  []string
+	}{
+		{
+			name:      "supported",
+			supported: true,
+			wantArgs:  []string{"passwd", "--", "alice"},
+		},
+		{
+			name:      "unsupported",
+			supported: false,
+			wantArgs:  []string{"passwd", "alice"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			getentDoubleDashSupported = sync.OnceValue(func() bool { return tt.supported })
+			execGetent = func(_ context.Context, args ...string) ([]byte, error) {
+				if !reflect.DeepEqual(args, tt.wantArgs) {
+					t.Fatalf("args = %q, want %q", args, tt.wantArgs)
+				}
+				return []byte("alice:x:1001:1001:Alice:/home/alice:/bin/sh\n"), nil
+			}
+			std := func(string) (*user.User, error) {
+				t.Fatal("std lookup should not be called")
+				return nil, nil
+			}
+			u, shell, err := userLookupGetent("alice", std)
+			if err != nil {
+				t.Fatalf("userLookupGetent error: %v", err)
+			}
+			if u.Username != "alice" || shell != "/bin/sh" {
+				t.Fatalf("got user=%+v shell=%q", u, shell)
+			}
+		})
+	}
+}