diff --git a/internal/plugin/cli_binary.go b/internal/plugin/cli_binary.go index 8fba8542d..43a9d214d 100644 --- a/internal/plugin/cli_binary.go +++ b/internal/plugin/cli_binary.go @@ -37,7 +37,7 @@ import ( // PluginBinDir returns the directory the command proxy searches for plugin // binaries. Exported so the installer and the router cannot drift apart on // where that is. -func PluginBinDir() string { return pluginBinDir() } +func PluginBinDir() (string, error) { return pluginBinDir() } // PublishedBinaryPath returns where a plugin's command binary is published, // including the platform's executable suffix. @@ -46,7 +46,11 @@ func PluginBinDir() string { return pluginBinDir() } // the test compute the name itself is how the Windows job went red: the // installer appended .exe and the test did not. func PublishedBinaryPath(binName string) string { - dst := filepath.Join(pluginBinDir(), binName) + binDir, err := pluginBinDir() + if err != nil { + return "" + } + dst := filepath.Join(binDir, binName) if runtime.GOOS == "windows" { dst += ".exe" } @@ -133,7 +137,10 @@ func linkCLIBinary(destDir, name string, m *PluginManifest) error { return err } - binDir := pluginBinDir() + binDir, err := pluginBinDir() + if err != nil { + return err + } if err := os.MkdirAll(binDir, 0o755); err != nil { return fmt.Errorf("creating plugin bin dir: %w", err) } @@ -168,7 +175,11 @@ func linkCLIBinary(destDir, name string, m *PluginManifest) error { // unlinkCLIBinary removes a plugin's published binary. A missing file is not an // error — the plugin may never have shipped one. func unlinkCLIBinary(name string, m *PluginManifest) error { - return unlinkCLIBinaryIn(filepath.Dir(pluginBinDir()), name, m) + binDir, err := pluginBinDir() + if err != nil { + return err + } + return unlinkCLIBinaryIn(filepath.Dir(binDir), name, m) } // unlinkCLIBinaryIn is unlinkCLIBinary with the plugin directory whose installed diff --git a/internal/plugin/cli_binary_test.go b/internal/plugin/cli_binary_test.go index 3e76f0092..9a2820d63 100644 --- a/internal/plugin/cli_binary_test.go +++ b/internal/plugin/cli_binary_test.go @@ -123,7 +123,10 @@ func TestUnlinkCLIBinary_RemovesPublishedCommand(t *testing.T) { t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) - binDir := PluginBinDir() + binDir, err := PluginBinDir() + if err != nil { + t.Fatal(err) + } if err := os.MkdirAll(binDir, 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } @@ -168,7 +171,10 @@ func TestIsCommandInstalled_TracksThePublishedBinary(t *testing.T) { t.Fatal("reported installed before anything was published") } - binDir := PluginBinDir() + binDir, err := PluginBinDir() + if err != nil { + t.Fatal(err) + } if err := os.MkdirAll(binDir, 0o755); err != nil { t.Fatalf("MkdirAll: %v", err) } diff --git a/internal/plugin/cli_collision_test.go b/internal/plugin/cli_collision_test.go index 4b779df67..17931a58b 100644 --- a/internal/plugin/cli_collision_test.go +++ b/internal/plugin/cli_collision_test.go @@ -87,7 +87,11 @@ func TestCheckCLIBinaries_ReinstallAndOrphan(t *testing.T) { t.Fatalf("reinstall refused: %v", err) } - if err := os.MkdirAll(PluginBinDir(), 0o755); err != nil { + binDir, err := PluginBinDir() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(binDir, 0o755); err != nil { t.Fatal(err) } if err := os.WriteFile(PublishedBinaryPath("nself-orphan"), []byte("old"), 0o755); err != nil { diff --git a/internal/plugin/name_validate.go b/internal/plugin/name_validate.go index e5b46bf88..b8e633ad3 100644 --- a/internal/plugin/name_validate.go +++ b/internal/plugin/name_validate.go @@ -78,7 +78,11 @@ func safePublishedBinaryPath(bin string) (string, error) { return "", err } dst := PublishedBinaryPath(bin) - rel, err := filepath.Rel(pluginBinDir(), dst) + binDir, err := pluginBinDir() + if err != nil { + return "", err + } + rel, err := filepath.Rel(binDir, dst) if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { return "", errs.Newf("E401", "plugin binary %q resolves outside the plugin bin directory", bin) } diff --git a/internal/plugin/name_validate_round2_test.go b/internal/plugin/name_validate_round2_test.go index 06c1eaf84..d990b1d11 100644 --- a/internal/plugin/name_validate_round2_test.go +++ b/internal/plugin/name_validate_round2_test.go @@ -41,7 +41,10 @@ func binFixture(t *testing.T) (binDir, victim string) { home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) - binDir = pluginBinDir() + binDir, err := pluginBinDir() + if err != nil { + t.Fatal(err) + } if err := os.MkdirAll(binDir, 0o755); err != nil { t.Fatal(err) } @@ -91,7 +94,8 @@ func TestLinkCLIBinaryRefusesTraversal(t *testing.T) { func TestSafePublishedBinaryPathContainment(t *testing.T) { binFixture(t) p, err := safePublishedBinaryPath("nself-foo") - if err != nil || filepath.Dir(p) != pluginBinDir() { + binDir, pbdErr := pluginBinDir() + if err != nil || pbdErr != nil || filepath.Dir(p) != binDir { t.Fatalf("nself-foo -> %q, %v", p, err) } if _, err := safePublishedBinaryPath("../x"); err == nil { diff --git a/internal/plugin/router.go b/internal/plugin/router.go index 2d9fb9ee6..0fd01ab42 100644 --- a/internal/plugin/router.go +++ b/internal/plugin/router.go @@ -6,11 +6,12 @@ import ( "os/exec" "path/filepath" "runtime" - "strings" "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/errs" "github.com/nself-org/cli/internal/plugin/catalog" + "github.com/nself-org/cli/internal/pluginname" + "github.com/nself-org/cli/internal/pluginperm" ) // ExitCodeError is returned when a plugin process exits with a non-zero code. @@ -31,14 +32,15 @@ func (e *ExitCodeError) ExitCode() int { return e.Code } // inherited stdout/stderr and has already reported the failure itself. func (e *ExitCodeError) Silent() bool { return true } -// pluginBinDir returns the directory where plugin binaries are installed. -// This is ~/.nself/plugins/bin/ (or /tmp/.nself/plugins/bin/ as fallback). -func pluginBinDir() string { +// pluginBinDir returns ~/.nself/plugins/bin/. With HOME unset it refuses rather +// than fall back to a shared directory such as /tmp, where any local user could +// plant a binary that cron or systemd would run with the caller's secrets (D-1396). +func pluginBinDir() (string, error) { home, err := os.UserHomeDir() if err != nil { - return filepath.Join("/tmp", ".nself", "plugins", "bin") + return "", errs.Newf("E406", "HOME is not set; refusing to resolve plugin binaries in a shared directory") } - return filepath.Join(home, ".nself", "plugins", "bin") + return filepath.Join(home, ".nself", "plugins", "bin"), nil } // ProxyCommand checks if a plugin binary exists and executes it. @@ -68,7 +70,10 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err pluginBinary := fmt.Sprintf("nself-%s", cmdName) // S-002: Only look in the plugin bin directory, never the full PATH. - binDir := pluginBinDir() + binDir, err := pluginBinDir() + if err != nil { + return err + } candidate := filepath.Join(binDir, pluginBinary) if runtime.GOOS == "windows" { candidate += ".exe" @@ -118,8 +123,11 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err // The mount validates the binary at discovery time (mount.Discover, E406); // a binary that vanishes between discovery and exec fails with E406 here. func ProxyBinary(binName, slug string, args []string) error { - binDir := pluginBinDir() - if !safeBinaryName(binName) { + binDir, err := pluginBinDir() + if err != nil { + return err + } + if !pluginname.IsCommandBinary(binName) { return errs.Newf("E406", "plugin %q binary %q has an invalid name", slug, binName) } fileName := binName @@ -133,40 +141,12 @@ func ProxyBinary(binName, slug string, args []string) error { // The plugins directory owner can replace the file between check and exec; // this accepted window does not cross a different trust boundary. func ProxyBinaryAt(candidate, rootDir, slug string, args []string) error { - resolvedRoot, err := filepath.EvalSymlinks(rootDir) - if err != nil { - return errs.Newf("E406", "plugin %q binary directory is unavailable", slug) - } - resolved, err := filepath.EvalSymlinks(candidate) - if err != nil { - return errs.Newf("E406", "plugin %q binary is unavailable", slug) - } - rel, err := filepath.Rel(resolvedRoot, resolved) - if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return errs.Newf("E406", "plugin %q binary resolves outside the plugins directory", slug) - } - info, err := os.Stat(resolved) - if err != nil || !info.Mode().IsRegular() || (runtime.GOOS != "windows" && info.Mode().Perm()&0111 == 0) { - return errs.Newf("E406", "plugin %q binary is not executable", slug) + if _, err := pluginperm.Resolve(candidate, rootDir, slug); err != nil { + return errs.Newf("E406", "%v", err) } return execPluginBinary(candidate, slug, args) } -func safeBinaryName(name string) bool { - if !strings.HasPrefix(name, "nself-") || len(name) <= len("nself-") { - return false - } - if name[len("nself-")] < 'a' || name[len("nself-")] > 'z' { - return false - } - for _, r := range name[len("nself-"):] { - if (r < 'a' || r > 'z') && (r < '0' || r > '9') && r != '-' { - return false - } - } - return true -} - // execPluginBinary is the shared run of a plugin binary: inherited stdio, the // project settings the plugin's manifest declares, and the plugin's exit // status surfaced as ExitCodeError (silent: the child already reported). @@ -213,7 +193,11 @@ func pluginEnvForCommand(cmdName string) []string { } // Installed plugins sit alongside the bin directory the proxy reads, so // deriving the path from pluginBinDir keeps the two from drifting apart. - installed := filepath.Join(filepath.Dir(pluginBinDir()), cmdName) + binDir, err := pluginBinDir() + if err != nil { + return nil + } + installed := filepath.Join(filepath.Dir(binDir), cmdName) m := readPluginManifest(installed) if m == nil { return nil diff --git a/internal/plugin/router_test.go b/internal/plugin/router_test.go index 1560759fd..543993bce 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" @@ -82,3 +83,47 @@ func TestRouterKeepsExistingHint(t *testing.T) { t.Fatalf("no cache should keep the generic hint: %v", err) } } + +// TestPluginBinDirRefusesWithoutHOME proves D-1396: when HOME is unset the +// router must not fall back to a shared directory like /tmp/.nself/plugins/bin, +// because any local user could plant a binary there that cron or systemd would +// execute with the caller's secrets. +func TestPluginBinDirRefusesWithoutHOME(t *testing.T) { + t.Setenv("HOME", "") + t.Setenv("USERPROFILE", "") + _, err := pluginBinDir() + if err == nil { + t.Fatal("pluginBinDir succeeded without HOME; expected refusal") + } + if !strings.Contains(err.Error(), "HOME is not set") { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestProxyBinaryAtRefusesUnsafePath wires the ownership check through +// ProxyBinaryAt so that a binary in an unsafe directory is rejected even +// when the binary itself is well-formed. Removing the call to +// pluginperm.Verify in ProxyBinaryAt would make this test pass. +func TestProxyBinaryAtRefusesUnsafePath(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("ownership check is a no-op on Windows") + } + root := t.TempDir() + binDir := filepath.Join(root, "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + bin := filepath.Join(binDir, "nself-testplug") + if err := os.WriteFile(bin, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatal(err) + } + + // Make the bin directory world-writable -> ProxyBinaryAt must refuse. + if err := os.Chmod(binDir, 0o757); err != nil { + t.Fatal(err) + } + err := ProxyBinaryAt(bin, root, "testplug", nil) + if err == nil || !strings.Contains(err.Error(), "world-writable") { + t.Fatalf("ProxyBinaryAt did not refuse world-writable dir: %v", err) + } +} diff --git a/internal/pluginname/name.go b/internal/pluginname/name.go index 502123161..3576780b3 100644 --- a/internal/pluginname/name.go +++ b/internal/pluginname/name.go @@ -103,6 +103,12 @@ func SlugReason(name string) string { // element). It is wider than a plugin slug only for '.', '_' and capitals. var BinaryRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) +// IsCommandBinary reports whether name is "nself-" plus a plugin slug, the shape +// of every command binary the router will exec. +func IsCommandBinary(name string) bool { + return strings.HasPrefix(name, "nself-") && SlugRe.MatchString(name[len("nself-"):]) +} + // BinaryReason returns why bin is not a single plain path element that is safe // to join to the plugin bin directory, or "". Same checks as a plugin name (no // separator, no dot reference, no control character, Base unchanged) with the diff --git a/internal/pluginname/name_test.go b/internal/pluginname/name_test.go index 31d4485bc..3bc68b7ab 100644 --- a/internal/pluginname/name_test.go +++ b/internal/pluginname/name_test.go @@ -41,3 +41,16 @@ func TestBinaryReason(t *testing.T) { } } } + +func TestIsCommandBinary(t *testing.T) { + for _, ok := range []string{"nself-a", "nself-foo-2"} { + if !IsCommandBinary(ok) { + t.Errorf("IsCommandBinary(%q) = false, want true", ok) + } + } + for _, bad := range []string{"", "nself-", "nself-2x", "nself-Foo", "nself-a_b", "nself-a.b", "nself-a\n", "foo-bar", "nself-../x"} { + if IsCommandBinary(bad) { + t.Errorf("IsCommandBinary(%q) = true, want false", bad) + } + } +} diff --git a/internal/pluginperm/perm_unix.go b/internal/pluginperm/perm_unix.go new file mode 100644 index 000000000..2c04a8be3 --- /dev/null +++ b/internal/pluginperm/perm_unix.go @@ -0,0 +1,49 @@ +//go:build !windows + +package pluginperm + +import ( + "fmt" + "os" + "path/filepath" + "syscall" +) + +// Verify checks that every component of path up to and including root is +// safe for plugin execution: owned by the current uid and not writable by group +// or world, whatever the group (a shared group such as macOS staff would let +// other members replace plugin binaries). A missing or unstat-able component +// fails closed. +func Verify(path, root string) error { + uid := uint32(syscall.Getuid()) + cur := path + for { + info, err := os.Lstat(cur) + if err != nil { + return fmt.Errorf("plugin binary path component %q is not accessible", cur) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("plugin binary path component %q has no stat info", cur) + } + if stat.Uid != uid { + return fmt.Errorf("plugin binary path component %q is owned by uid %d, not %d", cur, stat.Uid, uid) + } + mode := info.Mode().Perm() + if mode&0o002 != 0 { + return fmt.Errorf("plugin binary path component %q is world-writable", cur) + } + if mode&0o020 != 0 { + return fmt.Errorf("plugin binary path component %q is group-writable (run chmod g-w %q to fix)", cur, cur) + } + if cur == root || cur == "/" || cur == "." { + break + } + parent := filepath.Dir(cur) + if parent == cur { + break + } + cur = parent + } + return nil +} diff --git a/internal/pluginperm/perm_unix_test.go b/internal/pluginperm/perm_unix_test.go new file mode 100644 index 000000000..a897ba58e --- /dev/null +++ b/internal/pluginperm/perm_unix_test.go @@ -0,0 +1,107 @@ +//go:build !windows + +package pluginperm + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestVerifyRefusesUnsafe proves that Verify +// rejects world-writable, group-writable-by-foreign-gid, and foreign-owned path +// components on Unix. Removing the check would make these tests pass when they +// must fail. Windows skips the check by design (see perm_windows.go). +func TestVerifyRefusesUnsafe(t *testing.T) { + root := t.TempDir() + sub := filepath.Join(root, "sub") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(sub, "bin") + if err := os.WriteFile(target, []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + + // Baseline: owned by us, no unsafe bits -> passes. + if err := Verify(target, root); err != nil { + t.Fatalf("baseline failed: %v", err) + } + + // World-writable component -> refused. + if err := os.Chmod(sub, 0o757); err != nil { + t.Fatal(err) + } + if err := Verify(target, root); err == nil || !strings.Contains(err.Error(), "world-writable") { + t.Fatalf("world-writable not refused: %v", err) + } + if err := os.Chmod(sub, 0o755); err != nil { + t.Fatal(err) + } + + // Group-writable -> refused even when the directory's gid matches the + // caller's primary gid. Shared groups such as macOS `staff` would + // otherwise let other members replace plugin binaries. Regression test + // for review finding: matching the primary GID does not establish a + // private group. + if err := os.Chmod(sub, 0o775); err != nil { + t.Fatal(err) + } + if err := Verify(target, root); err == nil || !strings.Contains(err.Error(), "group-writable") { + t.Fatalf("same-gid group-writable not refused: %v", err) + } + if err := os.Chmod(sub, 0o755); err != nil { + t.Fatal(err) + } + + // Foreign gid with group-write must still be refused (defense in depth). + foreignGID := findForeignGID(t) + if foreignGID != 0 { + if err := os.Chown(sub, -1, int(foreignGID)); err != nil { + t.Skipf("cannot chown to foreign gid %d: %v", foreignGID, err) + } + if err := os.Chmod(sub, 0o775); err != nil { + t.Fatal(err) + } + if err := Verify(target, root); err == nil || !strings.Contains(err.Error(), "group-writable") { + t.Fatalf("foreign group-writable not refused: %v", err) + } + // Restore for subsequent cases. + if err := os.Chmod(sub, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chown(sub, -1, os.Getgid()); err != nil { + t.Logf("warning: could not restore gid: %v", err) + } + } + + // Foreign owner -> refused. Only possible when running as root. + if os.Getuid() == 0 { + foreignUID := 65534 // nobody on most Linux/macOS systems + if err := os.Chown(sub, foreignUID, -1); err != nil { + t.Skipf("cannot chown to foreign uid %d: %v", foreignUID, err) + } + if err := Verify(target, root); err == nil || !strings.Contains(err.Error(), "owned by uid") { + t.Fatalf("foreign owner not refused: %v", err) + } + } +} + +// findForeignGID returns a gid that isn't the current effective gid, or 0 if +// none is available. It scans /etc/group via os/user when possible; falling +// back to 0 keeps the caller's skip logic honest. +func findForeignGID(t *testing.T) uint32 { + t.Helper() + myGID := uint32(os.Getgid()) + // Try a few well-known gids that exist on most Unix systems and aren't + // the user's primary group. This avoids importing os/user just for the + // test and keeps the dependency footprint small. + candidates := []uint32{1, 2, 99, 65534} + for _, g := range candidates { + if g != myGID { + return g + } + } + return 0 +} diff --git a/internal/pluginperm/perm_windows.go b/internal/pluginperm/perm_windows.go new file mode 100644 index 000000000..d0d8dafdc --- /dev/null +++ b/internal/pluginperm/perm_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package pluginperm + +// Verify is a no-op on Windows: NT ACLs do not +// map to uid/gid bits, and plugin runtime on Windows is unverified (see +// runtime_windows.go). The security model here relies on the per-user +// profile directory being protected by the OS. +func Verify(_, _ string) error { return nil } diff --git a/internal/pluginperm/pluginperm.go b/internal/pluginperm/pluginperm.go new file mode 100644 index 000000000..d9906706f --- /dev/null +++ b/internal/pluginperm/pluginperm.go @@ -0,0 +1,39 @@ +// Package pluginperm checks that a plugin binary path cannot be swapped by +// another local user before the CLI executes it (D-1396). It imports nothing +// from the module, so it adds no importer to internal/errs (fan-in ratchet). +package pluginperm + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "strings" +) + +// Resolve is the check a plugin binary passes before exec: candidate resolves +// (through symlinks) to a path inside rootDir, every component up to rootDir is +// safe (see Verify), and the target is a regular executable file. It returns +// the resolved path. Errors name slug and carry no code; the caller wraps them. +func Resolve(candidate, rootDir, slug string) (string, error) { + resolvedRoot, err := filepath.EvalSymlinks(rootDir) + if err != nil { + return "", fmt.Errorf("plugin %q binary directory is unavailable", slug) + } + resolved, err := filepath.EvalSymlinks(candidate) + if err != nil { + return "", fmt.Errorf("plugin %q binary is unavailable", slug) + } + rel, err := filepath.Rel(resolvedRoot, resolved) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("plugin %q binary resolves outside the plugins directory", slug) + } + if err := Verify(resolved, resolvedRoot); err != nil { + return "", err + } + info, err := os.Stat(resolved) + if err != nil || !info.Mode().IsRegular() || (runtime.GOOS != "windows" && info.Mode().Perm()&0111 == 0) { + return "", fmt.Errorf("plugin %q binary is not executable", slug) + } + return resolved, nil +}