From 90e395cebce187955d4ce4464d89f27a4284b336 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Tue, 27 Oct 2026 06:50:56 -0400 Subject: [PATCH 1/5] fix(plugin): refuse /tmp fallback and verify path ownership before exec (D-1396) pluginBinDir now returns an error when HOME is unset instead of falling back to /tmp/.nself/plugins/bin, which any local user could populate with a malicious binary that cron or systemd would execute with the caller's secrets. ProxyBinaryAt additionally walks every component of the resolved plugin path up to the plugins root and verifies that each is owned by the current uid and has no group- or world-write bits set. All callers of pluginBinDir and PluginBinDir are updated for the new (string, error) signature. A regression test proves refusal without HOME. --- internal/plugin/cli_binary.go | 19 +++-- internal/plugin/cli_binary_test.go | 10 ++- internal/plugin/cli_collision_test.go | 6 +- internal/plugin/name_validate.go | 6 +- internal/plugin/name_validate_round2_test.go | 8 ++- internal/plugin/router.go | 76 ++++++++++++++++---- internal/plugin/router_test.go | 16 +++++ 7 files changed, 118 insertions(+), 23 deletions(-) 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..26f27f699 100644 --- a/internal/plugin/router.go +++ b/internal/plugin/router.go @@ -7,6 +7,7 @@ import ( "path/filepath" "runtime" "strings" + "syscall" "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/errs" @@ -31,14 +32,55 @@ 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 } +// errHomeUnset is returned when HOME cannot be resolved. The previous /tmp +// fallback let any local user plant a binary that cron or systemd would run +// with the caller's secrets (D-1396). +var errHomeUnset = errs.Newf("E406", "HOME is not set; refusing to resolve plugin binaries in a shared directory") + // pluginBinDir returns the directory where plugin binaries are installed. -// This is ~/.nself/plugins/bin/ (or /tmp/.nself/plugins/bin/ as fallback). -func pluginBinDir() string { +// It refuses to fall back to a shared directory when HOME is unset (D-1396): +// callers without HOME must set it explicitly rather than risk executing a +// binary planted by another local user. +func pluginBinDir() (string, error) { home, err := os.UserHomeDir() if err != nil { - return filepath.Join("/tmp", ".nself", "plugins", "bin") + return "", errHomeUnset } - return filepath.Join(home, ".nself", "plugins", "bin") + return filepath.Join(home, ".nself", "plugins", "bin"), nil +} + +// verifyPluginPathOwnership checks that every component of path up to and +// including root is owned by the current uid and has no group/world write. +// A missing or unstat-able component fails closed. Windows skips the check. +func verifyPluginPathOwnership(path, root string) error { + if runtime.GOOS == "windows" { + return nil + } + uid := uint32(syscall.Getuid()) + cur := path + for { + info, err := os.Lstat(cur) + if err != nil { + return errs.Newf("E406", "plugin binary path component %q is not accessible", cur) + } + if stat, ok := info.Sys().(*syscall.Stat_t); ok { + if stat.Uid != uid { + return errs.Newf("E406", "plugin binary path component %q is owned by uid %d, not %d", cur, stat.Uid, uid) + } + } + if info.Mode().Perm()&0022 != 0 { + return errs.Newf("E406", "plugin binary path component %q is group- or world-writable", cur) + } + if cur == root || cur == "/" || cur == "." { + break + } + parent := filepath.Dir(cur) + if parent == cur { + break + } + cur = parent + } + return nil } // ProxyCommand checks if a plugin binary exists and executes it. @@ -66,14 +108,15 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err return 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" } - if _, err := os.Stat(candidate); err != nil { // CLI-R19: an unknown command is the moment a user most needs to be told // how to get it, so the actionable message — `nself install X` — goes in @@ -100,10 +143,9 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err if installHint == "" { return fmt.Errorf("unknown command %q, and no plugin named %q is installed", cmdName, cmdName) } - return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\n\nIf this is an nSelf plugin, install it with:\n %s", + return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\nIf this is an nSelf plugin, install it with:\n%s", cmdName, cmdName, installHint) } - return ProxyBinaryAt(candidate, filepath.Dir(binDir), cmdName, args) } @@ -118,7 +160,10 @@ 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() + binDir, err := pluginBinDir() + if err != nil { + return err + } if !safeBinaryName(binName) { return errs.Newf("E406", "plugin %q binary %q has an invalid name", slug, binName) } @@ -145,6 +190,9 @@ func ProxyBinaryAt(candidate, rootDir, slug string, args []string) error { if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return errs.Newf("E406", "plugin %q binary resolves outside the plugins directory", slug) } + if err := verifyPluginPathOwnership(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 errs.Newf("E406", "plugin %q binary is not executable", slug) @@ -189,7 +237,6 @@ func runPluginCommand(cmd *exec.Cmd) error { cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - // Run process if err := cmd.Run(); err != nil { if exitError, ok := err.(*exec.ExitError); ok { @@ -197,7 +244,6 @@ func runPluginCommand(cmd *exec.Cmd) error { } return fmt.Errorf("failed to execute plugin: %w", err) } - return nil } @@ -213,7 +259,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..b71f55d76 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -82,3 +82,19 @@ 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) + } +} From 5fab4ccb36dc540370fc5f56429fd41423b6eafa Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Tue, 27 Oct 2026 07:06:11 -0400 Subject: [PATCH 2/5] fix(plugin): split ownership check by platform, restore hint formatting, allow private-group write - Move verifyPluginPathOwnership uid/stat logic into router_unix.go and router_windows.go with build tags so GOOS=windows builds succeed. - Restore the blank line before the install hint and the two-space indent plus double newline in the hint text to match origin/main. - Allow group-write when the directory's gid matches the user's primary group (umask 002 / user-private-group installs); refuse foreign-gid group-write with a message suggesting chmod g-w. - Add tests that refuse world-writable and foreign-gid group-writable path components and wire the check through ProxyBinaryAt. --- internal/plugin/router.go | 47 +++------ internal/plugin/router_test.go | 154 +++++++++++++++++++++++++++--- internal/plugin/router_unix.go | 51 ++++++++++ internal/plugin/router_windows.go | 10 ++ 4 files changed, 213 insertions(+), 49 deletions(-) create mode 100644 internal/plugin/router_unix.go create mode 100644 internal/plugin/router_windows.go diff --git a/internal/plugin/router.go b/internal/plugin/router.go index 26f27f699..a27d6e3fd 100644 --- a/internal/plugin/router.go +++ b/internal/plugin/router.go @@ -7,7 +7,6 @@ import ( "path/filepath" "runtime" "strings" - "syscall" "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/errs" @@ -50,37 +49,12 @@ func pluginBinDir() (string, error) { } // verifyPluginPathOwnership checks that every component of path up to and -// including root is owned by the current uid and has no group/world write. +// including root is safe for plugin execution. On Unix this means owned by +// the current uid and not writable by anyone else (group-write is allowed +// only when the group is the user's own private group, matching umask 002). // A missing or unstat-able component fails closed. Windows skips the check. func verifyPluginPathOwnership(path, root string) error { - if runtime.GOOS == "windows" { - return nil - } - uid := uint32(syscall.Getuid()) - cur := path - for { - info, err := os.Lstat(cur) - if err != nil { - return errs.Newf("E406", "plugin binary path component %q is not accessible", cur) - } - if stat, ok := info.Sys().(*syscall.Stat_t); ok { - if stat.Uid != uid { - return errs.Newf("E406", "plugin binary path component %q is owned by uid %d, not %d", cur, stat.Uid, uid) - } - } - if info.Mode().Perm()&0022 != 0 { - return errs.Newf("E406", "plugin binary path component %q is group- or world-writable", cur) - } - if cur == root || cur == "/" || cur == "." { - break - } - parent := filepath.Dir(cur) - if parent == cur { - break - } - cur = parent - } - return nil + return verifyPluginPathOwnershipPlatform(path, root) } // ProxyCommand checks if a plugin binary exists and executes it. @@ -108,6 +82,7 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err return err } pluginBinary := fmt.Sprintf("nself-%s", cmdName) + // S-002: Only look in the plugin bin directory, never the full PATH. binDir, err := pluginBinDir() if err != nil { @@ -117,6 +92,7 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err if runtime.GOOS == "windows" { candidate += ".exe" } + if _, err := os.Stat(candidate); err != nil { // CLI-R19: an unknown command is the moment a user most needs to be told // how to get it, so the actionable message — `nself install X` — goes in @@ -141,11 +117,12 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err } } if installHint == "" { - return fmt.Errorf("unknown command %q, and no plugin named %q is installed", cmdName, cmdName) - } - return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\nIf this is an nSelf plugin, install it with:\n%s", - cmdName, cmdName, installHint) + return fmt.Errorf("unknown command %q, and no plugin named %q is installed", cmdName, cmdName) } + return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\n\nIf this is an nSelf plugin, install it with:\n %s", + cmdName, cmdName, installHint) +} + return ProxyBinaryAt(candidate, filepath.Dir(binDir), cmdName, args) } @@ -237,6 +214,7 @@ func runPluginCommand(cmd *exec.Cmd) error { cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + // Run process if err := cmd.Run(); err != nil { if exitError, ok := err.(*exec.ExitError); ok { @@ -244,6 +222,7 @@ func runPluginCommand(cmd *exec.Cmd) error { } return fmt.Errorf("failed to execute plugin: %w", err) } + return nil } diff --git a/internal/plugin/router_test.go b/internal/plugin/router_test.go index b71f55d76..3d7e9ee0c 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -1,14 +1,15 @@ package plugin import ( - "errors" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/nself-org/cli/internal/compat/compattest" - "github.com/nself-org/cli/internal/errs" +"errors" +"os" +"path/filepath" + "runtime" +"strings" +"testing" + +"github.com/nself-org/cli/internal/compat/compattest" +"github.com/nself-org/cli/internal/errs" ) // catalogCacheFixture isolates HOME and the catalog cache, and (when body is @@ -88,13 +89,136 @@ func TestRouterKeepsExistingHint(t *testing.T) { // 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") +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) +} +} + +// TestVerifyPluginPathOwnershipRefusesUnsafe proves that verifyPluginPathOwnership +// 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 router_windows.go). +func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("ownership check is a no-op on Windows") + } + 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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(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 by a foreign gid -> refused. We can't easily create a + // directory owned by another uid in a unit test, but we can flip the gid + // to one that isn't ours when running as root, or rely on the fact that + // most CI runners have at least one supplementary group. When we can't + // arrange a foreign gid we skip rather than false-pass. + 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 := verifyPluginPathOwnership(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) + } } - if !strings.Contains(err.Error(), "HOME is not set") { - t.Fatalf("unexpected error: %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 := verifyPluginPathOwnership(target, root); err == nil || !strings.Contains(err.Error(), "owned by uid") { + t.Fatalf("foreign owner not refused: %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 +// verifyPluginPathOwnership 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) + } + + // Baseline: safe directory -> accepted (exec will fail because it's a + // shell script, but ownership passes). + if err := verifyPluginPathOwnership(bin, root); err != nil { + t.Fatalf("baseline ownership failed: %v", 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) + } +} + +// 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/plugin/router_unix.go b/internal/plugin/router_unix.go new file mode 100644 index 000000000..bd4b21644 --- /dev/null +++ b/internal/plugin/router_unix.go @@ -0,0 +1,51 @@ +//go:build !windows + +package plugin + +import ( + "os" + "path/filepath" + "syscall" + + "github.com/nself-org/cli/internal/errs" +) + +// verifyPluginPathOwnershipPlatform is the Unix implementation of +// verifyPluginPathOwnership. It walks every component from path up to root +// and refuses the chain when any component is owned by another uid or is +// writable by group/world outside the user's private group (umask 002). +func verifyPluginPathOwnershipPlatform(path, root string) error { + uid := uint32(syscall.Getuid()) + gid := uint32(syscall.Getgid()) + cur := path + for { + info, err := os.Lstat(cur) + if err != nil { + return errs.Newf("E406", "plugin binary path component %q is not accessible", cur) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return errs.Newf("E406", "plugin binary path component %q has no stat info", cur) + } + if stat.Uid != uid { + return errs.Newf("E406", "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 errs.Newf("E406", "plugin binary path component %q is world-writable", cur) + } + if mode&0o020 != 0 && stat.Gid != gid { + return errs.Newf("E406", "plugin binary path component %q is group-writable by foreign gid %d (run chmod g-w %q to fix)", cur, stat.Gid, cur) + } + if cur == root || cur == "/" || cur == "." { + break + } + parent := filepath.Dir(cur) + if parent == cur { + break + } + cur = parent + } + return nil +} + diff --git a/internal/plugin/router_windows.go b/internal/plugin/router_windows.go new file mode 100644 index 000000000..9d0a57b87 --- /dev/null +++ b/internal/plugin/router_windows.go @@ -0,0 +1,10 @@ +//go:build windows + +package plugin + +// verifyPluginPathOwnershipPlatform 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 verifyPluginPathOwnershipPlatform(_, _ string) error { return nil } + From 01c2a0c077bfd164128fec018bd30d32622f1688 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 16:25:17 -0400 Subject: [PATCH 3/5] fix(plugin): reject group-writable paths regardless of GID --- internal/plugin/router_test.go | 53 ++++++++++++++++++++-------------- internal/plugin/router_unix.go | 5 ++-- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/internal/plugin/router_test.go b/internal/plugin/router_test.go index 3d7e9ee0c..f726cdc46 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -134,29 +134,40 @@ func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { t.Fatal(err) } - // Group-writable by a foreign gid -> refused. We can't easily create a - // directory owned by another uid in a unit test, but we can flip the gid - // to one that isn't ours when running as root, or rely on the fact that - // most CI runners have at least one supplementary group. When we can't - // arrange a foreign gid we skip rather than false-pass. + // 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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(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) - } + 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 := verifyPluginPathOwnership(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. diff --git a/internal/plugin/router_unix.go b/internal/plugin/router_unix.go index bd4b21644..c58698dbf 100644 --- a/internal/plugin/router_unix.go +++ b/internal/plugin/router_unix.go @@ -16,7 +16,6 @@ import ( // writable by group/world outside the user's private group (umask 002). func verifyPluginPathOwnershipPlatform(path, root string) error { uid := uint32(syscall.Getuid()) - gid := uint32(syscall.Getgid()) cur := path for { info, err := os.Lstat(cur) @@ -34,8 +33,8 @@ func verifyPluginPathOwnershipPlatform(path, root string) error { if mode&0o002 != 0 { return errs.Newf("E406", "plugin binary path component %q is world-writable", cur) } - if mode&0o020 != 0 && stat.Gid != gid { - return errs.Newf("E406", "plugin binary path component %q is group-writable by foreign gid %d (run chmod g-w %q to fix)", cur, stat.Gid, cur) + if mode&0o020 != 0 { + return errs.Newf("E406", "plugin binary path component %q is group-writable (run chmod g-w %q to fix)", cur, cur) } if cur == root || cur == "/" || cur == "." { break From d4090e885f1627399c6c51d65dd6132729c79792 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 17:21:43 -0400 Subject: [PATCH 4/5] style(plugin): gofmt router files (D-1396) --- internal/plugin/router.go | 8 ++-- internal/plugin/router_test.go | 78 +++++++++++++++---------------- internal/plugin/router_unix.go | 1 - internal/plugin/router_windows.go | 1 - 4 files changed, 43 insertions(+), 45 deletions(-) diff --git a/internal/plugin/router.go b/internal/plugin/router.go index a27d6e3fd..d75064cec 100644 --- a/internal/plugin/router.go +++ b/internal/plugin/router.go @@ -117,11 +117,11 @@ func ProxyCommandWithHint(cmdName string, args []string, installHint string) err } } if installHint == "" { - return fmt.Errorf("unknown command %q, and no plugin named %q is installed", cmdName, cmdName) + return fmt.Errorf("unknown command %q, and no plugin named %q is installed", cmdName, cmdName) + } + return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\n\nIf this is an nSelf plugin, install it with:\n %s", + cmdName, cmdName, installHint) } - return fmt.Errorf("unknown command %q, and no plugin named %q is installed.\n\nIf this is an nSelf plugin, install it with:\n %s", - cmdName, cmdName, installHint) -} return ProxyBinaryAt(candidate, filepath.Dir(binDir), cmdName, args) } diff --git a/internal/plugin/router_test.go b/internal/plugin/router_test.go index f726cdc46..3a4e4ae47 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -1,15 +1,15 @@ package plugin import ( -"errors" -"os" -"path/filepath" + "errors" + "os" + "path/filepath" "runtime" -"strings" -"testing" + "strings" + "testing" -"github.com/nself-org/cli/internal/compat/compattest" -"github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/compat/compattest" + "github.com/nself-org/cli/internal/errs" ) // catalogCacheFixture isolates HOME and the catalog cache, and (when body is @@ -89,15 +89,15 @@ func TestRouterKeepsExistingHint(t *testing.T) { // 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) -} + 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) + } } // TestVerifyPluginPathOwnershipRefusesUnsafe proves that verifyPluginPathOwnership @@ -106,32 +106,32 @@ t.Fatalf("unexpected error: %v", err) // must fail. Windows skips the check by design (see router_windows.go). func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { if runtime.GOOS == "windows" { - t.Skip("ownership check is a no-op on Windows") + t.Skip("ownership check is a no-op on Windows") } root := t.TempDir() sub := filepath.Join(root, "sub") if err := os.MkdirAll(sub, 0o755); err != nil { - t.Fatal(err) + t.Fatal(err) } target := filepath.Join(sub, "bin") if err := os.WriteFile(target, []byte("#!/bin/sh\n"), 0o755); err != nil { - t.Fatal(err) + t.Fatal(err) } // Baseline: owned by us, no unsafe bits -> passes. if err := verifyPluginPathOwnership(target, root); err != nil { - t.Fatalf("baseline failed: %v", err) + t.Fatalf("baseline failed: %v", err) } // World-writable component -> refused. if err := os.Chmod(sub, 0o757); err != nil { - t.Fatal(err) + t.Fatal(err) } if err := verifyPluginPathOwnership(target, root); err == nil || !strings.Contains(err.Error(), "world-writable") { - t.Fatalf("world-writable not refused: %v", err) + t.Fatalf("world-writable not refused: %v", err) } if err := os.Chmod(sub, 0o755); err != nil { - t.Fatal(err) + t.Fatal(err) } // Group-writable -> refused even when the directory's gid matches the @@ -172,13 +172,13 @@ func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { // 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 := verifyPluginPathOwnership(target, root); err == nil || !strings.Contains(err.Error(), "owned by uid") { - t.Fatalf("foreign owner not refused: %v", err) - } + 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 := verifyPluginPathOwnership(target, root); err == nil || !strings.Contains(err.Error(), "owned by uid") { + t.Fatalf("foreign owner not refused: %v", err) + } } } @@ -188,31 +188,31 @@ func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { // verifyPluginPathOwnership 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") + 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) + 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) + t.Fatal(err) } // Baseline: safe directory -> accepted (exec will fail because it's a // shell script, but ownership passes). if err := verifyPluginPathOwnership(bin, root); err != nil { - t.Fatalf("baseline ownership failed: %v", err) + t.Fatalf("baseline ownership failed: %v", err) } // Make the bin directory world-writable -> ProxyBinaryAt must refuse. if err := os.Chmod(binDir, 0o757); err != nil { - t.Fatal(err) + 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) + t.Fatalf("ProxyBinaryAt did not refuse world-writable dir: %v", err) } } @@ -227,9 +227,9 @@ func findForeignGID(t *testing.T) uint32 { // test and keeps the dependency footprint small. candidates := []uint32{1, 2, 99, 65534} for _, g := range candidates { - if g != myGID { - return g - } + if g != myGID { + return g + } } return 0 } diff --git a/internal/plugin/router_unix.go b/internal/plugin/router_unix.go index c58698dbf..11a02129a 100644 --- a/internal/plugin/router_unix.go +++ b/internal/plugin/router_unix.go @@ -47,4 +47,3 @@ func verifyPluginPathOwnershipPlatform(path, root string) error { } return nil } - diff --git a/internal/plugin/router_windows.go b/internal/plugin/router_windows.go index 9d0a57b87..4b4e9a9c0 100644 --- a/internal/plugin/router_windows.go +++ b/internal/plugin/router_windows.go @@ -7,4 +7,3 @@ package plugin // runtime_windows.go). The security model here relies on the per-user // profile directory being protected by the OS. func verifyPluginPathOwnershipPlatform(_, _ string) error { return nil } - From da86310d14faad27a01e91160f3f4e2f1a8bf3f3 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 17:45:46 -0400 Subject: [PATCH 5/5] refactor(plugin): move exec-path ownership check into internal/pluginperm (D-1396) internal/plugin was 102 lines over its package-size ceiling. The path resolution and ownership check now live in pluginperm (no errs import, so the errs fan-in ceiling holds) and the nself- binary check reuses the pluginname slug grammar. No ceiling is raised. --- internal/plugin/router.go | 63 ++-------- internal/plugin/router_test.go | 108 +----------------- internal/plugin/router_unix.go | 49 -------- internal/pluginname/name.go | 6 + internal/pluginname/name_test.go | 13 +++ internal/pluginperm/perm_unix.go | 49 ++++++++ internal/pluginperm/perm_unix_test.go | 107 +++++++++++++++++ .../perm_windows.go} | 6 +- internal/pluginperm/pluginperm.go | 39 +++++++ 9 files changed, 227 insertions(+), 213 deletions(-) delete mode 100644 internal/plugin/router_unix.go create mode 100644 internal/pluginperm/perm_unix.go create mode 100644 internal/pluginperm/perm_unix_test.go rename internal/{plugin/router_windows.go => pluginperm/perm_windows.go} (56%) create mode 100644 internal/pluginperm/pluginperm.go diff --git a/internal/plugin/router.go b/internal/plugin/router.go index d75064cec..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,32 +32,17 @@ 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 } -// errHomeUnset is returned when HOME cannot be resolved. The previous /tmp -// fallback let any local user plant a binary that cron or systemd would run -// with the caller's secrets (D-1396). -var errHomeUnset = errs.Newf("E406", "HOME is not set; refusing to resolve plugin binaries in a shared directory") - -// pluginBinDir returns the directory where plugin binaries are installed. -// It refuses to fall back to a shared directory when HOME is unset (D-1396): -// callers without HOME must set it explicitly rather than risk executing a -// binary planted by another local user. +// 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 "", errHomeUnset + return "", errs.Newf("E406", "HOME is not set; refusing to resolve plugin binaries in a shared directory") } return filepath.Join(home, ".nself", "plugins", "bin"), nil } -// verifyPluginPathOwnership checks that every component of path up to and -// including root is safe for plugin execution. On Unix this means owned by -// the current uid and not writable by anyone else (group-write is allowed -// only when the group is the user's own private group, matching umask 002). -// A missing or unstat-able component fails closed. Windows skips the check. -func verifyPluginPathOwnership(path, root string) error { - return verifyPluginPathOwnershipPlatform(path, root) -} - // ProxyCommand checks if a plugin binary exists and executes it. // If not, it instructs the user to install it. // @@ -141,7 +127,7 @@ func ProxyBinary(binName, slug string, args []string) error { if err != nil { return err } - if !safeBinaryName(binName) { + if !pluginname.IsCommandBinary(binName) { return errs.Newf("E406", "plugin %q binary %q has an invalid name", slug, binName) } fileName := binName @@ -155,43 +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) - } - if err := verifyPluginPathOwnership(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 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). diff --git a/internal/plugin/router_test.go b/internal/plugin/router_test.go index 3a4e4ae47..543993bce 100644 --- a/internal/plugin/router_test.go +++ b/internal/plugin/router_test.go @@ -100,92 +100,10 @@ func TestPluginBinDirRefusesWithoutHOME(t *testing.T) { } } -// TestVerifyPluginPathOwnershipRefusesUnsafe proves that verifyPluginPathOwnership -// 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 router_windows.go). -func TestVerifyPluginPathOwnershipRefusesUnsafe(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("ownership check is a no-op on Windows") - } - 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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(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 := verifyPluginPathOwnership(target, root); err == nil || !strings.Contains(err.Error(), "owned by uid") { - t.Fatalf("foreign owner not refused: %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 -// verifyPluginPathOwnership in ProxyBinaryAt would make this test pass. +// 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") @@ -200,12 +118,6 @@ func TestProxyBinaryAtRefusesUnsafePath(t *testing.T) { t.Fatal(err) } - // Baseline: safe directory -> accepted (exec will fail because it's a - // shell script, but ownership passes). - if err := verifyPluginPathOwnership(bin, root); err != nil { - t.Fatalf("baseline ownership failed: %v", err) - } - // Make the bin directory world-writable -> ProxyBinaryAt must refuse. if err := os.Chmod(binDir, 0o757); err != nil { t.Fatal(err) @@ -215,21 +127,3 @@ func TestProxyBinaryAtRefusesUnsafePath(t *testing.T) { t.Fatalf("ProxyBinaryAt did not refuse world-writable dir: %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/plugin/router_unix.go b/internal/plugin/router_unix.go deleted file mode 100644 index 11a02129a..000000000 --- a/internal/plugin/router_unix.go +++ /dev/null @@ -1,49 +0,0 @@ -//go:build !windows - -package plugin - -import ( - "os" - "path/filepath" - "syscall" - - "github.com/nself-org/cli/internal/errs" -) - -// verifyPluginPathOwnershipPlatform is the Unix implementation of -// verifyPluginPathOwnership. It walks every component from path up to root -// and refuses the chain when any component is owned by another uid or is -// writable by group/world outside the user's private group (umask 002). -func verifyPluginPathOwnershipPlatform(path, root string) error { - uid := uint32(syscall.Getuid()) - cur := path - for { - info, err := os.Lstat(cur) - if err != nil { - return errs.Newf("E406", "plugin binary path component %q is not accessible", cur) - } - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return errs.Newf("E406", "plugin binary path component %q has no stat info", cur) - } - if stat.Uid != uid { - return errs.Newf("E406", "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 errs.Newf("E406", "plugin binary path component %q is world-writable", cur) - } - if mode&0o020 != 0 { - return errs.Newf("E406", "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/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/plugin/router_windows.go b/internal/pluginperm/perm_windows.go similarity index 56% rename from internal/plugin/router_windows.go rename to internal/pluginperm/perm_windows.go index 4b4e9a9c0..d0d8dafdc 100644 --- a/internal/plugin/router_windows.go +++ b/internal/pluginperm/perm_windows.go @@ -1,9 +1,9 @@ //go:build windows -package plugin +package pluginperm -// verifyPluginPathOwnershipPlatform is a no-op on Windows: NT ACLs do not +// 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 verifyPluginPathOwnershipPlatform(_, _ string) error { return nil } +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 +}