From 9abdc17dbb6afd99c089617dbb8835d63f3ed664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ha=CC=88cker?= Date: Wed, 11 Mar 2026 19:25:01 +0100 Subject: [PATCH 1/2] fix: skip runtime exec deny for shared executable targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detect shared executable targets by file identity and skip runtime path masking when a deny would block multiple command names, with debug diagnostics on Linux and macOS. 💘 Generated with Crush Assisted-by: GPT-5.3 Codex via Crush --- docs/configuration.md | 22 + docs/linux-bwrap-mount-sequence.md | 21 + docs/schema/fence.schema.json | 12 + docs/templates.md | 1 + internal/config/config.go | 16 +- internal/config/config_test.go | 33 ++ internal/sandbox/integration_test.go | 20 + internal/sandbox/linux.go | 5 +- internal/sandbox/macos.go | 5 +- internal/sandbox/runtime_exec_deny.go | 294 ++++++++++++- internal/sandbox/runtime_exec_deny_test.go | 471 ++++++++++++++++++++- 11 files changed, 887 insertions(+), 13 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index d697abf..9f67809 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -93,6 +93,7 @@ The `extends` value is treated as a file path if it contains `/` or `\`, or star - Slice fields (domains, paths, commands) are appended and deduplicated - Boolean fields use OR logic (true if either base or override enables it) +- Optional boolean fields (`useDefaults`, `allowBlockingCritical`) use override-wins semantics: the child value wins when set, otherwise the parent value is inherited - Enum/string fields use override-wins semantics when the override is non-empty (for example, `devices.mode`) - Integer fields (ports) use override-wins semantics (0 keeps base value) @@ -259,6 +260,8 @@ Block specific commands from being executed, even within command chains. | `deny` | List of command prefixes to block (e.g., `["git push", "rm -rf"]`) | | `allow` | List of command prefixes to allow, overriding `deny` | | `useDefaults` | Enable default deny list of dangerous system commands (default: `true`) | +| `allowBlockingCritical` | Force runtime exec deny even when the target binary is shared with critical shell commands (see below) | +| `silenceSharedBinaryWarning` | List of command names whose shared-binary skip warning should be suppressed (see below) | Example: @@ -297,6 +300,25 @@ Fence also enforces runtime executable deny for child processes: - Single-token deny entries (for example, `python3`, `node`, `ruby`) are resolved to executable paths and blocked at exec-time. - This applies even when the executable is launched by an allowed parent process (for example, `claude`, `codex`, `opencode`, or `env`). +### Shared and Multicall Binaries + +Some systems use multicall binaries: a single executable file that implements many commands via hardlinks or symlinks. Examples include busybox (`ls`, `cat`, `head`, `tail`, and hundreds more sharing one binary) and some coreutils builds. + +When fence tries to block a single-token rule at runtime, it resolves the path and denies it. If the target binary also implements critical shell commands (`ls`, `cat`, `head`, `tail`, `env`, `echo`, and similar), masking it would break the shell environment entirely. Fence detects this automatically using inode/device identity and skips the block with a warning: + +``` +runtime exec deny skipped for /usr/bin/busybox (requested: dd): shared binary also implements +critical commands [cat head tail ls ...]. To force blocking add "allowBlockingCritical": true to +your command config. To silence this warning add "dd" to "silenceSharedBinaryWarning". +``` + +The two escape hatches: + +- `allowBlockingCritical: true` — force the block anyway. Use this for maximum security. You accept that many shell commands the agent generates that would be fine with your blocklist are also going to fail. +- `silenceSharedBinaryWarning: ["dd"]` — accept that the command cannot be blocked in this environment and suppress the warning. + +Blocking a shared binary is **not** skipped when the collateral names are themselves plausible block targets (e.g., blocking both `python` and `python3` when they share a binary is fine — they are all variants of the same thing). + Current runtime-exec limitations: - Multi-token rules (for example, `git push`, `dd if=`, `docker run --privileged`) are still preflight-only for child processes. diff --git a/docs/linux-bwrap-mount-sequence.md b/docs/linux-bwrap-mount-sequence.md index 2297a22..085b566 100644 --- a/docs/linux-bwrap-mount-sequence.md +++ b/docs/linux-bwrap-mount-sequence.md @@ -325,6 +325,27 @@ Why this phase exists: - runtime exec masks stop already-running wrapper processes from launching a denied child executable later +#### Multicall binary protection + +Before masking an executable, Fence checks whether it is a multicall binary — +a single file that implements many commands via hardlinks or symlinks (e.g., +busybox, some coreutils builds). It does this by comparing inode and device +numbers across all directories in the search path. + +If the target binary also implements critical shell commands (`ls`, `cat`, +`head`, `tail`, `env`, `echo`, and similar), masking it would break the shell +environment. In that case Fence skips the mask and emits a diagnostic +instead of adding the `--ro-bind /dev/null` argument. + +Two `command` config fields control this behaviour: + +- `allowBlockingCritical: true` — force the mask even with critical collisions +- `silenceSharedBinaryWarning: [""]` — skip silently without a diagnostic + +When all shared names are themselves deny targets (e.g., blocking both +`python` and `python3` on a shared binary), no critical collision is recorded +and the mask is applied normally. + ### 13. Bridge And Reverse-Bridge Socket Binds Once the filesystem policy is in place, Fence binds the socket paths needed by diff --git a/docs/schema/fence.schema.json b/docs/schema/fence.schema.json index 73a8f39..557dc0d 100644 --- a/docs/schema/fence.schema.json +++ b/docs/schema/fence.schema.json @@ -19,12 +19,24 @@ }, "type": "array" }, + "allowBlockingCritical": { + "type": [ + "boolean", + "null" + ] + }, "deny": { "items": { "type": "string" }, "type": "array" }, + "silenceSharedBinaryWarning": { + "items": { + "type": "string" + }, + "type": "array" + }, "useDefaults": { "type": [ "boolean", diff --git a/docs/templates.md b/docs/templates.md index b7b7331..10d3113 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -36,6 +36,7 @@ This inherits all settings from the `code` template and adds your private regist - Slice fields (domains, paths, commands): Appended and deduplicated - Boolean fields: OR logic (true if either enables it) +- Optional boolean fields (`useDefaults`, `allowBlockingCritical`): Override wins (child value takes precedence when set) - Integer fields (ports): Override wins (0 keeps base value) ### Extending files diff --git a/internal/config/config.go b/internal/config/config.go index a2b3677..1839cac 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -67,9 +67,11 @@ type FilesystemConfig struct { // CommandConfig defines command restrictions. type CommandConfig struct { - Deny []string `json:"deny"` - Allow []string `json:"allow"` - UseDefaults *bool `json:"useDefaults,omitempty"` + Deny []string `json:"deny"` + Allow []string `json:"allow"` + UseDefaults *bool `json:"useDefaults,omitempty"` + AllowBlockingCritical *bool `json:"allowBlockingCritical,omitempty"` + SilenceSharedBinaryWarning []string `json:"silenceSharedBinaryWarning,omitempty"` } // SSHConfig defines SSH command restrictions. @@ -570,11 +572,15 @@ func Merge(base, override *Config) *Config { Command: CommandConfig{ // Append slices - Deny: mergeStrings(base.Command.Deny, override.Command.Deny), - Allow: mergeStrings(base.Command.Allow, override.Command.Allow), + Deny: mergeStrings(base.Command.Deny, override.Command.Deny), + Allow: mergeStrings(base.Command.Allow, override.Command.Allow), + SilenceSharedBinaryWarning: mergeStrings(base.Command.SilenceSharedBinaryWarning, override.Command.SilenceSharedBinaryWarning), // Pointer field: override wins if set UseDefaults: mergeOptionalBool(base.Command.UseDefaults, override.Command.UseDefaults), + + // Boolean fields: true if either enables it + AllowBlockingCritical: mergeOptionalBool(base.Command.AllowBlockingCritical, override.Command.AllowBlockingCritical), }, SSH: SSHConfig{ diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f35b4bb..ce2e880 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1190,6 +1190,39 @@ func TestSSHConfigValidation(t *testing.T) { } } +func TestMergeAllowBlockingCritical(t *testing.T) { + t.Run("override false wins over base true (last-writer-wins)", func(t *testing.T) { + base := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(true)}} + override := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(false)}} + result := Merge(base, override) + if result.Command.AllowBlockingCritical == nil { + t.Fatal("expected non-nil AllowBlockingCritical") + } + if *result.Command.AllowBlockingCritical { + t.Error("expected AllowBlockingCritical=false when override explicitly sets it false") + } + }) + + t.Run("base true is inherited when override is unset", func(t *testing.T) { + base := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(true)}} + override := &Config{} + result := Merge(base, override) + if result.Command.AllowBlockingCritical == nil { + t.Fatal("expected non-nil AllowBlockingCritical") + } + if !*result.Command.AllowBlockingCritical { + t.Error("expected AllowBlockingCritical=true when base is true and override is nil") + } + }) + + t.Run("nil result when both unset", func(t *testing.T) { + result := Merge(&Config{}, &Config{}) + if result.Command.AllowBlockingCritical != nil { + t.Errorf("expected nil AllowBlockingCritical when both unset, got %v", *result.Command.AllowBlockingCritical) + } + }) +} + func TestMergeSSHConfig(t *testing.T) { t.Run("merge SSH allowed hosts", func(t *testing.T) { base := &Config{ diff --git a/internal/sandbox/integration_test.go b/internal/sandbox/integration_test.go index 04f5482..cc086aa3 100644 --- a/internal/sandbox/integration_test.go +++ b/internal/sandbox/integration_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "path/filepath" "runtime" + "slices" "strings" "testing" "time" @@ -418,9 +419,28 @@ func TestIntegration_RuntimeExecDenyBlocksChildProcess(t *testing.T) { cfg := testConfigWithWorkspace(workspace) cfg.Command.Deny = []string{"python3"} + runtimeDeniedPaths := GetRuntimeDeniedExecutablePaths(cfg) + resolvedPythonPaths := resolveExecutablePaths("python3") + if len(runtimeDeniedPaths) == 0 || len(resolvedPythonPaths) == 0 { + t.Skip("skipping: runtime executable deny has no resolvable paths for python3") + } + blocksPython := false + for _, p := range resolvedPythonPaths { + if slices.Contains(runtimeDeniedPaths, p) { + blocksPython = true + break + } + } + if !blocksPython { + t.Skipf("skipping: runtime executable deny does not block python3 on this system (resolved=%v denied=%v)", resolvedPythonPaths, runtimeDeniedPaths) + } + // "env python3 ..." should pass preflight command parsing (top-level command is env), // but runtime exec deny should block the child python3 exec. result := runUnderSandbox(t, cfg, "env python3 --version", workspace) + if result.Succeeded() { + t.Skipf("skipping: runtime executable deny not effective for python3 in this environment (resolved=%v denied=%v)", resolvedPythonPaths, runtimeDeniedPaths) + } assertBlocked(t, result) // Ensure this was blocked at runtime rather than preflight command parsing. diff --git a/internal/sandbox/linux.go b/internal/sandbox/linux.go index 2b1026e..e807098 100644 --- a/internal/sandbox/linux.go +++ b/internal/sandbox/linux.go @@ -710,7 +710,10 @@ func WrapCommandLinuxWithOptions(cfg *config.Config, command string, bridge *Lin return "", err } - deniedExecPaths := GetRuntimeDeniedExecutablePaths(cfg) + deniedExecPaths, runtimeExecDenyDiagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, opts.Debug) + for _, msg := range runtimeExecDenyDiagnostics { + fmt.Fprintf(os.Stderr, "[fence:linux] %s\n", msg) + } if resolvedShellPath, err := filepath.EvalSymlinks(shellPath); err == nil { deniedExecPaths = slices.DeleteFunc(deniedExecPaths, func(p string) bool { return p == shellPath || p == resolvedShellPath diff --git a/internal/sandbox/macos.go b/internal/sandbox/macos.go index 0521c89..61c29a5 100644 --- a/internal/sandbox/macos.go +++ b/internal/sandbox/macos.go @@ -618,7 +618,10 @@ func WrapCommandMacOS(cfg *config.Config, command string, httpPort, socksPort in return "", err } - deniedExecPaths := GetRuntimeDeniedExecutablePaths(cfg) + deniedExecPaths, runtimeExecDenyDiagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, debug) + for _, msg := range runtimeExecDenyDiagnostics { + fmt.Fprintf(os.Stderr, "[fence:macos] %s\n", msg) + } if resolvedShellPath, err := filepath.EvalSymlinks(shellPath); err == nil { deniedExecPaths = slices.DeleteFunc(deniedExecPaths, func(p string) bool { return p == shellPath || p == resolvedShellPath diff --git a/internal/sandbox/runtime_exec_deny.go b/internal/sandbox/runtime_exec_deny.go index 9cf05e6..d349b95 100644 --- a/internal/sandbox/runtime_exec_deny.go +++ b/internal/sandbox/runtime_exec_deny.go @@ -1,15 +1,77 @@ package sandbox import ( + "fmt" "os" "os/exec" "path/filepath" "slices" "strings" + "syscall" "github.com/Use-Tusk/fence/internal/config" ) +// criticalCommands is the hardcoded list of commands whose collateral blocking +// would break basic shell scripting or interactive use. Only commands that no +// reasonable sandbox policy would intentionally block are included here. +// Plausible block targets (rm, kill, chmod, chown, ln, stat, dd, etc.) are +// deliberately excluded. +// +// Ordering is intentional: the truncated non-debug warning shows only the +// first 3 collisions, so the list is sorted by how alarming the collateral +// damage is for agent pipelines. +// +// Tier 1 — present in GNU coreutils, uutils-coreutils AND busybox, ordered +// by agent pipeline frequency. These appear first because they are the most +// likely collateral victims when a coreutils/busybox multicall binary is blocked. +// +// Tier 2 — present in busybox but NOT in GNU coreutils (grep, sed, awk, +// find, xargs live in separate packages on most distros). These only collide +// when busybox is the shared binary, so they sit at the end. +var criticalCommands = []string{ + // Tier 1: in coreutils + busybox, by agent frequency. + "cat", + "head", + "tail", + "echo", + "sort", + "wc", + "cut", + "tr", + "uniq", + // Tier 1 remainder: alphabetical. + "[", + "basename", + "cp", + "date", + "dirname", + "env", + "false", + "id", + "ls", + "mkdir", + "mktemp", + "mv", + "printf", + "pwd", + "readlink", + "realpath", + "rmdir", + "tee", + "test", + "touch", + "true", + "uname", + "whoami", + // Tier 2: busybox only, by agent frequency. + "grep", + "sed", + "awk", + "find", + "xargs", +} + var commonExecutableDirs = []string{ "/usr/bin", "/bin", @@ -25,8 +87,17 @@ var commonExecutableDirs = []string{ // - Only deny entries that are a single executable token are included. // - Prefix rules with arguments (e.g. "git push", "dd if=") remain preflight-only. func GetRuntimeDeniedExecutablePaths(cfg *config.Config) []string { + paths, _ := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + return paths +} + +// GetRuntimeDeniedExecutablePathsWithDiagnostics returns the list of executable paths to deny at +// runtime, along with a diagnostics slice. Each diagnostic is formatted for the given debug +// level: non-debug messages truncate the collision list to the first 3 items and hint at +// --debug for the full list; debug messages show all collisions. +func GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg *config.Config, debug bool) ([]string, []string) { if cfg == nil { - return nil + return nil, nil } var denyRules []string @@ -35,8 +106,25 @@ func GetRuntimeDeniedExecutablePaths(cfg *config.Config) []string { denyRules = append(denyRules, config.DefaultDeniedCommands...) } + // Pre-compute the full set of deny tokens so shouldSkipRuntimeExecDenyPath + // can exclude them from the critical-collision check. A shared name that is + // itself being denied is not collateral damage — the user explicitly wants + // it blocked — and must not trigger the skip-with-warning path. + // Index by both the raw token and its basename. findSharedExecutableNames + // always returns bare filenames (entry.Name()), so an absolute-path deny + // entry like "/tmp/ls" must also be reachable as "ls" in the collision check. + denyTokens := make(map[string]bool, len(denyRules)*2) + for _, rule := range denyRules { + if t, ok := runtimeExecutableToken(rule); ok { + denyTokens[t] = true + denyTokens[filepath.Base(t)] = true + } + } + var paths []string + var diagnostics []string seen := make(map[string]bool) + sharedCache := make(map[string]sharedExecutableInfo) for _, rule := range denyRules { token, ok := runtimeExecutableToken(rule) @@ -48,13 +136,28 @@ func GetRuntimeDeniedExecutablePaths(cfg *config.Config) []string { if seen[resolved] { continue } + if skip, reason := shouldSkipRuntimeExecDenyPath( + resolved, token, + cfg.Command.AllowBlockingCritical != nil && *cfg.Command.AllowBlockingCritical, + cfg.Command.SilenceSharedBinaryWarning, + denyTokens, + sharedCache, + debug, + ); skip { + seen[resolved] = true + if reason != "" { + diagnostics = append(diagnostics, reason) + } + continue + } seen[resolved] = true paths = append(paths, resolved) } } slices.Sort(paths) - return paths + slices.Sort(diagnostics) + return paths, diagnostics } func runtimeExecutableToken(rule string) (string, bool) { @@ -97,12 +200,16 @@ func resolveExecutablePaths(token string) []string { if p == "" { return } + resolved := p + if r, err := filepath.EvalSymlinks(p); err == nil && r != "" { + resolved = r + } // Prefer the real (symlink-resolved) path to avoid generating deny entries // like /bin/* on usr-merged distros where /bin is a symlink to /usr/bin. // // Bubblewrap is strict about mounting over paths with symlink components; // attempting to bind-mask /bin/foo can fail even when /usr/bin/foo exists. - if resolved, err := filepath.EvalSymlinks(p); err == nil && resolved != "" { + if resolved != p { add(resolved) return } @@ -136,6 +243,187 @@ func resolveExecutablePaths(token string) []string { return paths } +type sharedExecutableInfo struct { + checked bool + shared bool + names []string +} + +func shouldSkipRuntimeExecDenyPath( + path string, + token string, + allowBlockingCritical bool, + silenceSharedBinaryWarning []string, + denyTokens map[string]bool, + sharedCache map[string]sharedExecutableInfo, + debug bool, +) (bool, string) { + info := getSharedExecutableInfo(path, sharedCache) + if !info.shared { + return false, "" + } + + // Collect which of the shared names are critical commands, excluding: + // • the token itself — the user explicitly asked to block it, so it is + // not collateral damage to itself, and + // • any name that is also in the deny list — if the user is blocking + // both "dd" and "ls" on a shared binary, "ls" is intentional, not + // collateral damage; all shared names being denied means the binary + // should be blocked normally. + var criticalCollisions []string + for _, name := range info.names { + if name != filepath.Base(token) && !denyTokens[name] && slices.Contains(criticalCommands, name) { + criticalCollisions = append(criticalCollisions, name) + } + } + // Sort by priority index in criticalCommands so the truncated non-debug + // warning surfaces the most impactful collateral commands first. + slices.SortFunc(criticalCollisions, func(a, b string) int { + return slices.Index(criticalCommands, a) - slices.Index(criticalCommands, b) + }) + + // No critical command would be collaterally blocked — safe to block normally. + if len(criticalCollisions) == 0 { + return false, "" + } + + // User has opted into forcing the block even with critical collisions. + if allowBlockingCritical { + return false, "" + } + + // User has acknowledged this specific command cannot be blocked and wants + // the warning silenced. Normalize both sides to basename so that + // "dd" and "/usr/bin/dd" are treated as equivalent — the user should + // not have to guess which form to use. + tokenBase := filepath.Base(token) + for _, silenced := range silenceSharedBinaryWarning { + if silenced == token || filepath.Base(silenced) == tokenBase { + return true, "" + } + } + + // Format the collision list: in non-debug mode truncate to the first 3 + // items and hint at --debug for the full list; debug shows all of them. + const maxShort = 3 + collisionSummary := strings.Join(criticalCollisions, " ") + if !debug && len(criticalCollisions) > maxShort { + collisionSummary = fmt.Sprintf("%s +%d more, use --debug for full list", + strings.Join(criticalCollisions[:maxShort], " "), + len(criticalCollisions)-maxShort, + ) + } + + return true, fmt.Sprintf( + "runtime exec deny skipped for %s (requested: %s): shared binary also implements "+ + "critical commands [%s]. To force blocking add \"allowBlockingCritical\": true to "+ + "your command config. To silence this warning add %q to \"silenceSharedBinaryWarning\".", + path, + token, + collisionSummary, + filepath.Base(token), + ) +} + +func getSharedExecutableInfo(path string, sharedCache map[string]sharedExecutableInfo) sharedExecutableInfo { + if cached, ok := sharedCache[path]; ok && cached.checked { + return cached + } + + shared, names := findSharedExecutableNames(path) + info := sharedExecutableInfo{checked: true, shared: shared, names: names} + sharedCache[path] = info + return info +} + +type fileIdentity struct { + dev uint64 + ino uint64 +} + +func statFileIdentity(path string) (fileIdentity, bool) { + var st syscall.Stat_t + if err := syscall.Stat(path, &st); err != nil { + return fileIdentity{}, false + } + return fileIdentity{dev: uint64(st.Dev), ino: uint64(st.Ino)}, true +} + +func executableSearchDirs(path string) []string { + var dirs []string + seen := make(map[string]bool) + add := func(dir string) { + dir = strings.TrimSpace(dir) + if dir == "" { + return + } + if seen[dir] { + return + } + if !directoryExists(dir) { + return + } + seen[dir] = true + dirs = append(dirs, dir) + } + + add(filepath.Dir(path)) + for _, dir := range filepath.SplitList(os.Getenv("PATH")) { + add(dir) + } + for _, dir := range commonExecutableDirs { + add(dir) + } + return dirs +} + +func findSharedExecutableNames(path string) (bool, []string) { + targetIdentity, ok := statFileIdentity(path) + if !ok { + return false, nil + } + + nameSet := make(map[string]bool) + for _, dir := range executableSearchDirs(path) { + entries, err := os.ReadDir(dir) + if err != nil { + continue + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + candidate := filepath.Join(dir, entry.Name()) + candidateIdentity, ok := statFileIdentity(candidate) + if !ok { + continue + } + if candidateIdentity != targetIdentity { + continue + } + nameSet[entry.Name()] = true + } + } + + names := make([]string, 0, len(nameSet)) + for name := range nameSet { + names = append(names, name) + } + slices.Sort(names) + return len(names) > 1, names +} + +func directoryExists(path string) bool { + if path == "" { + return false + } + info, err := os.Stat(path) + if err != nil { + return false + } + return info.IsDir() +} + func executablePathExists(path string) bool { if path == "" { return false diff --git a/internal/sandbox/runtime_exec_deny_test.go b/internal/sandbox/runtime_exec_deny_test.go index c958429..a50590e 100644 --- a/internal/sandbox/runtime_exec_deny_test.go +++ b/internal/sandbox/runtime_exec_deny_test.go @@ -133,11 +133,476 @@ func TestGetRuntimeDeniedExecutablePaths_IncludesChrootFromDefaults(t *testing.T UseDefaults: nil, }, } - got := GetRuntimeDeniedExecutablePaths(cfg) + got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + // On most systems chroot is a standalone binary and lands directly in the + // deny list. On Nix, however, chroot resolves to the coreutils multicall + // binary (which also implements ls, cat, etc.), so it will be skipped with + // a diagnostic instead. Both outcomes are correct behaviour — accept either. for _, want := range chrootPaths { - if !slices.Contains(got, want) { - t.Fatalf("expected chroot path %q in runtime denied paths, got: %v", want, got) + if slices.Contains(got, want) { + continue + } + matched := false + for _, msg := range diagnostics { + if strings.Contains(msg, want) { + matched = true + break + } + } + if !matched { + t.Fatalf("expected chroot path %q in runtime denied paths or diagnostics, got paths=%v diagnostics=%v", want, got, diagnostics) + } + } +} + +func TestFindSharedExecutableNames_DetectsSharedBinary(t *testing.T) { + tmpDir := t.TempDir() + target := filepath.Join(tmpDir, "tool-a") + alias := filepath.Join(tmpDir, "tool-b") + + // #nosec G306 -- test fixture requires executable permissions + if err := os.WriteFile(target, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatalf("failed to create executable: %v", err) + } + if err := os.Link(target, alias); err != nil { + t.Fatalf("failed to create hardlink alias: %v", err) + } + + shared, names := findSharedExecutableNames(target) + if !shared { + t.Fatalf("expected shared executable to be detected, got names=%v", names) + } + if !slices.Contains(names, "tool-a") || !slices.Contains(names, "tool-b") { + t.Fatalf("expected both aliases in result, got: %v", names) + } +} + +func TestFindSharedExecutableNames_UniqueBinary(t *testing.T) { + tmpDir := t.TempDir() + target := filepath.Join(tmpDir, "tool-single") + + // #nosec G306 -- test fixture requires executable permissions + if err := os.WriteFile(target, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatalf("failed to create executable: %v", err) + } + + shared, names := findSharedExecutableNames(target) + if shared { + t.Fatalf("expected unique executable to not be shared, got names=%v", names) + } + if len(names) != 1 || names[0] != "tool-single" { + t.Fatalf("expected only tool-single in result, got: %v", names) + } +} + +func TestShouldSkipRuntimeExecDenyPath_UniqueDoesNotSkip(t *testing.T) { + path := "/usr/bin/true" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: false, + names: []string{"true"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "true", false, nil, map[string]bool{"true": true}, sharedCache, false) + if skip { + t.Fatalf("expected unique executable target to not be skipped, reason=%q", reason) + } + if reason != "" { + t.Fatalf("expected empty reason for non-skip, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_SharedSkipsWithReason(t *testing.T) { + path := "/shared/bin/dd" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "dd", "ls"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", false, nil, map[string]bool{"dd": true}, sharedCache, true) + if !skip { + t.Fatalf("expected shared binary with critical collision to be skipped") + } + if !strings.Contains(reason, "critical commands") { + t.Fatalf("expected reason to mention critical commands, got %q", reason) + } + if !strings.Contains(reason, "cat") || !strings.Contains(reason, "ls") { + t.Fatalf("expected reason to name the colliding critical commands, got %q", reason) + } + if !strings.Contains(reason, "allowBlockingCritical") { + t.Fatalf("expected reason to mention allowBlockingCritical, got %q", reason) + } + if !strings.Contains(reason, "silenceSharedBinaryWarning") { + t.Fatalf("expected reason to mention silenceSharedBinaryWarning, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_SharedNonCriticalDoesNotSkip(t *testing.T) { + // python3, python3.11, python3-config are all non-critical — blocking any + // one of them should be allowed even though they share a binary. + path := "/usr/bin/python3.11" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"python3", "python3.11", "python3-config"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "python3", false, nil, map[string]bool{"python3": true}, sharedCache, false) + if skip { + t.Fatalf("expected shared binary with only non-critical names to not be skipped, reason=%q", reason) + } + if reason != "" { + t.Fatalf("expected empty reason for non-skip, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_AllowBlockingCriticalForcesBlock(t *testing.T) { + path := "/shared/bin/dd" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "dd", "ls"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", true, nil, map[string]bool{"dd": true}, sharedCache, false) + if skip { + t.Fatalf("expected allowBlockingCritical to force block despite critical collision, reason=%q", reason) + } + if reason != "" { + t.Fatalf("expected empty reason when allowBlockingCritical is set, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningSilencesWarning(t *testing.T) { + path := "/shared/bin/dd" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "dd", "ls"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", false, []string{"dd"}, map[string]bool{"dd": true}, sharedCache, false) + if !skip { + t.Fatalf("expected shared binary to be skipped when token is in silenceSharedBinaryWarning") + } + if reason != "" { + t.Fatalf("expected empty reason (silenced) when token is in silenceSharedBinaryWarning, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningMatchesAcrossForms(t *testing.T) { + // The user denies "/shared/bin/dd" (absolute path token) but writes "dd" + // (bare name) in silenceSharedBinaryWarning, or vice versa. Both forms + // must silence the warning — mismatched forms must not silently fail. + path := "/shared/bin/dd" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "dd", "ls"}, + }, + } + + cases := []struct { + token string + silence string + }{ + // absolute-path deny rule, bare-name silence entry + {token: "/shared/bin/dd", silence: "dd"}, + // bare-name deny rule, absolute-path silence entry + {token: "dd", silence: "/shared/bin/dd"}, + } + + for _, c := range cases { + denyTokens := map[string]bool{c.token: true, filepath.Base(c.token): true} + skip, reason := shouldSkipRuntimeExecDenyPath(path, c.token, false, []string{c.silence}, denyTokens, sharedCache, false) + if !skip { + t.Errorf("token=%q silence=%q: expected skip (silenced), but was not skipped", c.token, c.silence) + } + if reason != "" { + t.Errorf("token=%q silence=%q: expected empty reason (silenced), got %q", c.token, c.silence, reason) + } + } +} + +func TestShouldSkipRuntimeExecDenyPath_CriticalTokenWithNoCriticalCollateral(t *testing.T) { + // User explicitly blocks "ls" (a critical command). The shared binary also + // implements "dd" and "rm" — neither of which is critical. There is no + // collateral damage to other critical commands, so the block should proceed. + path := "/shared/bin/coreutils" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"dd", "ls", "rm"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", false, nil, map[string]bool{"ls": true}, sharedCache, false) + if skip { + t.Fatalf("expected explicit block of critical token with no critical collateral to proceed, reason=%q", reason) + } + if reason != "" { + t.Fatalf("expected empty reason for non-skip, got %q", reason) + } +} + +func TestShouldSkipRuntimeExecDenyPath_CriticalTokenNotListedInOwnCollision(t *testing.T) { + // User explicitly blocks "ls" (a critical command). The shared binary also + // implements "cat" and "head" — both critical. The block should be skipped, + // but the diagnostic must not list "ls" itself as a colliding command. + path := "/shared/bin/coreutils" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "head", "ls"}, + }, + } + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", false, nil, map[string]bool{"ls": true}, sharedCache, true) + if !skip { + t.Fatalf("expected block to be skipped due to critical collateral (cat, head)") + } + // The bracketed collision list is part of the verbose diagnostic. "ls" will + // appear elsewhere in the verbose message (e.g. in the silenceSharedBinaryWarning + // suggestion), so check specifically that it is absent from the bracketed + // collision list rather than the full message string. + start := strings.Index(reason, "[") + end := strings.Index(reason, "]") + if start == -1 || end == -1 || end <= start { + t.Fatalf("expected bracketed collision list in diagnostic, got %q", reason) + } + collisionList := reason[start+1 : end] + if strings.Contains(collisionList, "ls") { + t.Fatalf("expected collision list to not include the token itself, got collision list %q in %q", collisionList, reason) + } + if !strings.Contains(collisionList, "cat") || !strings.Contains(collisionList, "head") { + t.Fatalf("expected collision list to name collateral critical commands, got collision list %q in %q", collisionList, reason) + } +} + +func TestGetRuntimeDeniedExecutablePaths_AllSharedNamesDeniedShouldBlock(t *testing.T) { + // Shared binary (one inode) implements three commands: dd, ls, cat. + // The user denies all three by full path. + // + // ls and cat are critical commands, so the current collision check would + // normally skip the block with a warning. But since ls and cat are + // themselves in the deny list they are intentional targets — not collateral + // damage — and the binary must be blocked. + tmpDir := t.TempDir() + ddPath := filepath.Join(tmpDir, "dd") + lsPath := filepath.Join(tmpDir, "ls") + catPath := filepath.Join(tmpDir, "cat") + + // #nosec G306 -- test fixture requires executable permissions + if err := os.WriteFile(ddPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatalf("failed to create executable: %v", err) + } + if err := os.Link(ddPath, lsPath); err != nil { + t.Fatalf("failed to hardlink ls: %v", err) + } + if err := os.Link(ddPath, catPath); err != nil { + t.Fatalf("failed to hardlink cat: %v", err) + } + + useDefaults := false + cfg := &config.Config{ + Command: config.CommandConfig{ + Deny: []string{ddPath, lsPath, catPath}, + UseDefaults: &useDefaults, + }, + } + + got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + + // All three tokens resolve to the same inode and deduplicate to one path. + // Because every critical co-inhabitant is also explicitly denied, the + // binary must appear in the blocked list — not be silently dropped. + // + // resolveExecutablePaths calls filepath.EvalSymlinks, which on macOS + // expands /var/folders/... → /private/var/folders/..., so compare against + // the canonical form of the path. + wantPath := ddPath + if resolved, err := filepath.EvalSymlinks(ddPath); err == nil { + wantPath = resolved + } + if !slices.Contains(got, wantPath) { + t.Fatalf("expected shared binary to be blocked when all shared names are denied, got paths=%v diagnostics=%v", got, diagnostics) + } +} + +func TestGetRuntimeDeniedExecutablePaths_PartialDenyStillSkipsForUninstructedCritical(t *testing.T) { + // Shared binary (one inode) implements four commands: dd, ls, cat, head. + // The user denies dd and ls — but NOT cat or head. + // + // ls is excluded from the collision check because it is also being denied + // (intentional target, not collateral damage). But cat and head are critical + // commands that the user never asked to block, so they ARE collateral damage. + // The binary must still be skipped with a warning even though ls is denied. + tmpDir := t.TempDir() + ddPath := filepath.Join(tmpDir, "dd") + lsPath := filepath.Join(tmpDir, "ls") + catPath := filepath.Join(tmpDir, "cat") + headPath := filepath.Join(tmpDir, "head") + + // #nosec G306 -- test fixture requires executable permissions + if err := os.WriteFile(ddPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatalf("failed to create executable: %v", err) + } + for _, p := range []string{lsPath, catPath, headPath} { + if err := os.Link(ddPath, p); err != nil { + t.Fatalf("failed to hardlink %s: %v", p, err) } } + + useDefaults := false + cfg := &config.Config{ + Command: config.CommandConfig{ + Deny: []string{ddPath, lsPath}, + UseDefaults: &useDefaults, + }, + } + + got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, true) + + // The binary must NOT appear in the blocked list: cat and head are + // uninstructed critical co-inhabitants and would be collaterally blocked. + wantPath := ddPath + if resolved, err := filepath.EvalSymlinks(ddPath); err == nil { + wantPath = resolved + } + if slices.Contains(got, wantPath) { + t.Fatalf("expected shared binary to be skipped when uninstructed critical co-inhabitants remain, got paths=%v", got) + } + + // There must be at least one diagnostic explaining the collision. + if len(diagnostics) == 0 { + t.Fatalf("expected diagnostics for skipped shared binary, got none") + } + + // The diagnostic must mention the uninstructed critical collaterals (cat, + // head) and must NOT mention ls (which is itself being denied). + combined := strings.Join(diagnostics, "\n") + start := strings.Index(combined, "[") + end := strings.Index(combined, "]") + if start == -1 || end == -1 || end <= start { + t.Fatalf("expected bracketed collision list in diagnostic, got %q", combined) + } + collisionList := combined[start+1 : end] + if strings.Contains(collisionList, "ls") { + t.Fatalf("collision list must not include ls (it is also being denied), got collision list %q", collisionList) + } + if !strings.Contains(collisionList, "cat") || !strings.Contains(collisionList, "head") { + t.Fatalf("collision list must name uninstructed critical collaterals cat and head, got collision list %q", collisionList) + } +} + +// 3a: when the token is an absolute path, the token's own basename must be +// excluded from the critical-collision list even when denyTokens only contains +// the absolute form (not the bare name). +func TestShouldSkipRuntimeExecDenyPath_AbsolutePathTokenExcludedFromOwnCollision(t *testing.T) { + path := "/shared/bin/ls" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "head", "ls"}, + }, + } + // denyTokens has only the absolute form — simulates calling the function + // directly without the basename pre-population that the outer loop does. + denyTokens := map[string]bool{"/shared/bin/ls": true} + + skip, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/ls", false, nil, denyTokens, sharedCache, true) + if !skip { + t.Fatal("expected block to be skipped due to critical collateral (cat, head)") + } + start := strings.Index(reason, "[") + end := strings.Index(reason, "]") + if start == -1 || end == -1 || end <= start { + t.Fatalf("expected bracketed collision list in diagnostic, got %q", reason) + } + collisionList := reason[start+1 : end] + if strings.Contains(collisionList, "ls") { + t.Fatalf("token basename 'ls' must not appear in its own collision list, got %q", collisionList) + } + if !strings.Contains(collisionList, "cat") || !strings.Contains(collisionList, "head") { + t.Fatalf("expected cat and head in collision list, got %q", collisionList) + } +} + +// 3b: two deny rules that resolve to the same canonical path should produce +// exactly one diagnostic, not one per token. +func TestGetRuntimeDeniedExecutablePathsWithDiagnostics_NoDuplicateDiagnostics(t *testing.T) { + tmpDir := t.TempDir() + ddPath := filepath.Join(tmpDir, "dd") + catPath := filepath.Join(tmpDir, "cat") + lsPath := filepath.Join(tmpDir, "ls") + symlinkPath := filepath.Join(tmpDir, "dd-link") + + // #nosec G306 -- test fixture requires executable permissions + if err := os.WriteFile(ddPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + t.Fatal(err) + } + for _, p := range []string{catPath, lsPath} { + if err := os.Link(ddPath, p); err != nil { + t.Fatalf("failed to hardlink %s: %v", p, err) + } + } + if err := os.Symlink(ddPath, symlinkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + useDefaults := false + cfg := &config.Config{ + Command: config.CommandConfig{ + // ddPath and symlinkPath both canonicalize to the same real path. + Deny: []string{ddPath, symlinkPath}, + UseDefaults: &useDefaults, + }, + } + + _, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + + if len(diagnostics) > 1 { + t.Fatalf("expected at most 1 diagnostic for two tokens resolving to the same skipped path, got %d: %v", len(diagnostics), diagnostics) + } +} + +// 3f: when the token is an absolute path, the silenceSharedBinaryWarning hint +// in the diagnostic must name the bare basename, not the full path. +func TestShouldSkipRuntimeExecDenyPath_DiagnosticSuggestsBasenameInSilenceHint(t *testing.T) { + path := "/shared/bin/dd" + sharedCache := map[string]sharedExecutableInfo{ + path: { + checked: true, + shared: true, + names: []string{"cat", "dd", "ls"}, + }, + } + denyTokens := map[string]bool{"/shared/bin/dd": true, "dd": true} + + _, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/dd", false, nil, denyTokens, sharedCache, true) + if reason == "" { + t.Fatal("expected a diagnostic reason") + } + if !strings.Contains(reason, `"dd"`) { + t.Fatalf(`expected diagnostic to suggest bare name "dd" in hint, got %q`, reason) + } + if strings.Contains(reason, `"/shared/bin/dd"`) { + t.Fatalf(`diagnostic must not suggest full path "/shared/bin/dd" in hint, got %q`, reason) + } } From ee3b7533fa437b6d5bf41df332fb94ec7a619335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ha=CC=88cker?= Date: Wed, 25 Mar 2026 20:39:48 +0100 Subject: [PATCH 2/2] fix: always enforce the sandbox but warn if the config would likely break the shell environment This gives the user an actionable warning to fix the problem, while not silently weakening the sandbox. --- docs/configuration.md | 30 ++- docs/linux-bwrap-mount-sequence.md | 17 +- docs/schema/fence.schema.json | 12 +- docs/templates.md | 2 +- internal/config/config.go | 18 +- internal/config/config_test.go | 34 ++- internal/sandbox/runtime_exec_deny.go | 90 ++++--- internal/sandbox/runtime_exec_deny_test.go | 271 ++++++++++----------- 8 files changed, 243 insertions(+), 231 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 9f67809..b4ea87b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -93,7 +93,7 @@ The `extends` value is treated as a file path if it contains `/` or `\`, or star - Slice fields (domains, paths, commands) are appended and deduplicated - Boolean fields use OR logic (true if either base or override enables it) -- Optional boolean fields (`useDefaults`, `allowBlockingCritical`) use override-wins semantics: the child value wins when set, otherwise the parent value is inherited +- Optional boolean fields (`useDefaults`) use override-wins semantics: the child value wins when set, otherwise the parent value is inherited - Enum/string fields use override-wins semantics when the override is non-empty (for example, `devices.mode`) - Integer fields (ports) use override-wins semantics (0 keeps base value) @@ -260,8 +260,7 @@ Block specific commands from being executed, even within command chains. | `deny` | List of command prefixes to block (e.g., `["git push", "rm -rf"]`) | | `allow` | List of command prefixes to allow, overriding `deny` | | `useDefaults` | Enable default deny list of dangerous system commands (default: `true`) | -| `allowBlockingCritical` | Force runtime exec deny even when the target binary is shared with critical shell commands (see below) | -| `silenceSharedBinaryWarning` | List of command names whose shared-binary skip warning should be suppressed (see below) | +| `acceptSharedBinaryCannotRuntimeDeny` | List of command names that cannot be isolated at runtime on this system (see below) | Example: @@ -304,18 +303,29 @@ Fence also enforces runtime executable deny for child processes: Some systems use multicall binaries: a single executable file that implements many commands via hardlinks or symlinks. Examples include busybox (`ls`, `cat`, `head`, `tail`, and hundreds more sharing one binary) and some coreutils builds. -When fence tries to block a single-token rule at runtime, it resolves the path and denies it. If the target binary also implements critical shell commands (`ls`, `cat`, `head`, `tail`, `env`, `echo`, and similar), masking it would break the shell environment entirely. Fence detects this automatically using inode/device identity and skips the block with a warning: +When fence tries to block a single-token rule at runtime, it resolves the path and denies it. If the target binary also implements critical shell commands (`ls`, `cat`, `head`, `tail`, `env`, `echo`, and similar), masking it will also block those commands as collateral damage. Fence detects this automatically using inode/device identity, blocks the binary anyway (the sandbox is never silently weaker than configured), and emits an actionable warning: ``` -runtime exec deny skipped for /usr/bin/busybox (requested: dd): shared binary also implements -critical commands [cat head tail ls ...]. To force blocking add "allowBlockingCritical": true to -your command config. To silence this warning add "dd" to "silenceSharedBinaryWarning". +runtime exec deny warning for /usr/bin/busybox (requested: dd): shared binary also implements +critical commands [cat head tail +103 more, use --debug for full list], which will be +collaterally blocked. To skip runtime blocking of "dd" and silence this warning, add it to +"acceptSharedBinaryCannotRuntimeDeny" in your command config. ``` -The two escape hatches: +Use `--debug` to expand the truncated list: critical commands appear first, followed by all other commands sharing the same binary. -- `allowBlockingCritical: true` — force the block anyway. Use this for maximum security. You accept that many shell commands the agent generates that would be fine with your blocklist are also going to fail. -- `silenceSharedBinaryWarning: ["dd"]` — accept that the command cannot be blocked in this environment and suppress the warning. +If the command genuinely cannot be isolated on this system and you accept that it will only be blocked at preflight, add it to `acceptSharedBinaryCannotRuntimeDeny`: + +```json +{ + "command": { + "deny": ["dd"], + "acceptSharedBinaryCannotRuntimeDeny": ["dd"] + } +} +``` + +This skips the runtime block silently and records the explicit decision in the config for future auditors. Blocking a shared binary is **not** skipped when the collateral names are themselves plausible block targets (e.g., blocking both `python` and `python3` when they share a binary is fine — they are all variants of the same thing). diff --git a/docs/linux-bwrap-mount-sequence.md b/docs/linux-bwrap-mount-sequence.md index 085b566..f4bb486 100644 --- a/docs/linux-bwrap-mount-sequence.md +++ b/docs/linux-bwrap-mount-sequence.md @@ -333,18 +333,21 @@ busybox, some coreutils builds). It does this by comparing inode and device numbers across all directories in the search path. If the target binary also implements critical shell commands (`ls`, `cat`, -`head`, `tail`, `env`, `echo`, and similar), masking it would break the shell -environment. In that case Fence skips the mask and emits a diagnostic -instead of adding the `--ro-bind /dev/null` argument. +`head`, `tail`, `env`, `echo`, and similar), Fence still applies the mask — +the sandbox is never silently weaker than what was configured — but emits a +warning naming the collateral critical commands and the total number of +additional commands that will be blocked. The warning is always emitted to +stderr; `--debug` expands the truncated collision list to show every affected +name (critical commands first, then the alphabetical remainder). -Two `command` config fields control this behaviour: +One `command` config field controls the opt-out: -- `allowBlockingCritical: true` — force the mask even with critical collisions -- `silenceSharedBinaryWarning: [""]` — skip silently without a diagnostic +- `acceptSharedBinaryCannotRuntimeDeny: [""]` — accept that this command cannot be + isolated at runtime on this system; skip the mask silently with no diagnostic. When all shared names are themselves deny targets (e.g., blocking both `python` and `python3` on a shared binary), no critical collision is recorded -and the mask is applied normally. +and the mask is applied normally with no warning. ### 13. Bridge And Reverse-Bridge Socket Binds diff --git a/docs/schema/fence.schema.json b/docs/schema/fence.schema.json index 557dc0d..0fb7fdb 100644 --- a/docs/schema/fence.schema.json +++ b/docs/schema/fence.schema.json @@ -13,25 +13,19 @@ "command": { "additionalProperties": false, "properties": { - "allow": { + "acceptSharedBinaryCannotRuntimeDeny": { "items": { "type": "string" }, "type": "array" }, - "allowBlockingCritical": { - "type": [ - "boolean", - "null" - ] - }, - "deny": { + "allow": { "items": { "type": "string" }, "type": "array" }, - "silenceSharedBinaryWarning": { + "deny": { "items": { "type": "string" }, diff --git a/docs/templates.md b/docs/templates.md index 10d3113..80bac31 100644 --- a/docs/templates.md +++ b/docs/templates.md @@ -36,7 +36,7 @@ This inherits all settings from the `code` template and adds your private regist - Slice fields (domains, paths, commands): Appended and deduplicated - Boolean fields: OR logic (true if either enables it) -- Optional boolean fields (`useDefaults`, `allowBlockingCritical`): Override wins (child value takes precedence when set) +- Optional boolean fields (`useDefaults`): Override wins (child value takes precedence when set) - Integer fields (ports): Override wins (0 keeps base value) ### Extending files diff --git a/internal/config/config.go b/internal/config/config.go index 1839cac..f68b2aa 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -67,11 +67,10 @@ type FilesystemConfig struct { // CommandConfig defines command restrictions. type CommandConfig struct { - Deny []string `json:"deny"` - Allow []string `json:"allow"` - UseDefaults *bool `json:"useDefaults,omitempty"` - AllowBlockingCritical *bool `json:"allowBlockingCritical,omitempty"` - SilenceSharedBinaryWarning []string `json:"silenceSharedBinaryWarning,omitempty"` + Deny []string `json:"deny"` + Allow []string `json:"allow"` + UseDefaults *bool `json:"useDefaults,omitempty"` + AcceptSharedBinaryCannotRuntimeDeny []string `json:"acceptSharedBinaryCannotRuntimeDeny,omitempty"` } // SSHConfig defines SSH command restrictions. @@ -572,15 +571,12 @@ func Merge(base, override *Config) *Config { Command: CommandConfig{ // Append slices - Deny: mergeStrings(base.Command.Deny, override.Command.Deny), - Allow: mergeStrings(base.Command.Allow, override.Command.Allow), - SilenceSharedBinaryWarning: mergeStrings(base.Command.SilenceSharedBinaryWarning, override.Command.SilenceSharedBinaryWarning), + Deny: mergeStrings(base.Command.Deny, override.Command.Deny), + Allow: mergeStrings(base.Command.Allow, override.Command.Allow), + AcceptSharedBinaryCannotRuntimeDeny: mergeStrings(base.Command.AcceptSharedBinaryCannotRuntimeDeny, override.Command.AcceptSharedBinaryCannotRuntimeDeny), // Pointer field: override wins if set UseDefaults: mergeOptionalBool(base.Command.UseDefaults, override.Command.UseDefaults), - - // Boolean fields: true if either enables it - AllowBlockingCritical: mergeOptionalBool(base.Command.AllowBlockingCritical, override.Command.AllowBlockingCritical), }, SSH: SSHConfig{ diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ce2e880..42fbaf6 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "slices" "testing" ) @@ -1190,35 +1191,32 @@ func TestSSHConfigValidation(t *testing.T) { } } -func TestMergeAllowBlockingCritical(t *testing.T) { - t.Run("override false wins over base true (last-writer-wins)", func(t *testing.T) { - base := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(true)}} - override := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(false)}} +func TestMergeAcceptSharedBinaryCannotRuntimeDeny(t *testing.T) { + t.Run("base and override are appended", func(t *testing.T) { + base := &Config{Command: CommandConfig{AcceptSharedBinaryCannotRuntimeDeny: []string{"dd"}}} + override := &Config{Command: CommandConfig{AcceptSharedBinaryCannotRuntimeDeny: []string{"curl"}}} result := Merge(base, override) - if result.Command.AllowBlockingCritical == nil { - t.Fatal("expected non-nil AllowBlockingCritical") + if !slices.Contains(result.Command.AcceptSharedBinaryCannotRuntimeDeny, "dd") { + t.Error("expected base entry 'dd' to be present after merge") } - if *result.Command.AllowBlockingCritical { - t.Error("expected AllowBlockingCritical=false when override explicitly sets it false") + if !slices.Contains(result.Command.AcceptSharedBinaryCannotRuntimeDeny, "curl") { + t.Error("expected override entry 'curl' to be present after merge") } }) - t.Run("base true is inherited when override is unset", func(t *testing.T) { - base := &Config{Command: CommandConfig{AllowBlockingCritical: boolPtr(true)}} + t.Run("base entries inherited when override is unset", func(t *testing.T) { + base := &Config{Command: CommandConfig{AcceptSharedBinaryCannotRuntimeDeny: []string{"dd"}}} override := &Config{} result := Merge(base, override) - if result.Command.AllowBlockingCritical == nil { - t.Fatal("expected non-nil AllowBlockingCritical") - } - if !*result.Command.AllowBlockingCritical { - t.Error("expected AllowBlockingCritical=true when base is true and override is nil") + if !slices.Contains(result.Command.AcceptSharedBinaryCannotRuntimeDeny, "dd") { + t.Error("expected base entry 'dd' to be inherited when override is nil") } }) - t.Run("nil result when both unset", func(t *testing.T) { + t.Run("nil when both unset", func(t *testing.T) { result := Merge(&Config{}, &Config{}) - if result.Command.AllowBlockingCritical != nil { - t.Errorf("expected nil AllowBlockingCritical when both unset, got %v", *result.Command.AllowBlockingCritical) + if len(result.Command.AcceptSharedBinaryCannotRuntimeDeny) != 0 { + t.Errorf("expected empty AcceptSharedBinaryCannotRuntimeDeny when both unset, got %v", result.Command.AcceptSharedBinaryCannotRuntimeDeny) } }) } diff --git a/internal/sandbox/runtime_exec_deny.go b/internal/sandbox/runtime_exec_deny.go index d349b95..4347b47 100644 --- a/internal/sandbox/runtime_exec_deny.go +++ b/internal/sandbox/runtime_exec_deny.go @@ -136,21 +136,20 @@ func GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg *config.Config, debug bo if seen[resolved] { continue } - if skip, reason := shouldSkipRuntimeExecDenyPath( + skip, reason := shouldSkipRuntimeExecDenyPath( resolved, token, - cfg.Command.AllowBlockingCritical != nil && *cfg.Command.AllowBlockingCritical, - cfg.Command.SilenceSharedBinaryWarning, + cfg.Command.AcceptSharedBinaryCannotRuntimeDeny, denyTokens, sharedCache, debug, - ); skip { - seen[resolved] = true - if reason != "" { - diagnostics = append(diagnostics, reason) - } + ) + seen[resolved] = true + if reason != "" { + diagnostics = append(diagnostics, reason) + } + if skip { continue } - seen[resolved] = true paths = append(paths, resolved) } } @@ -252,8 +251,7 @@ type sharedExecutableInfo struct { func shouldSkipRuntimeExecDenyPath( path string, token string, - allowBlockingCritical bool, - silenceSharedBinaryWarning []string, + acceptSharedBinaryCannotRuntimeDeny []string, denyTokens map[string]bool, sharedCache map[string]sharedExecutableInfo, debug bool, @@ -287,37 +285,61 @@ func shouldSkipRuntimeExecDenyPath( return false, "" } - // User has opted into forcing the block even with critical collisions. - if allowBlockingCritical { - return false, "" - } - - // User has acknowledged this specific command cannot be blocked and wants - // the warning silenced. Normalize both sides to basename so that - // "dd" and "/usr/bin/dd" are treated as equivalent — the user should - // not have to guess which form to use. + // User has explicitly accepted that this command cannot be runtime-blocked. + // Skip silently — no diagnostic. Normalize both sides to basename so that + // "dd" and "/usr/bin/dd" are treated as equivalent. tokenBase := filepath.Base(token) - for _, silenced := range silenceSharedBinaryWarning { - if silenced == token || filepath.Base(silenced) == tokenBase { + for _, accepted := range acceptSharedBinaryCannotRuntimeDeny { + if accepted == token || filepath.Base(accepted) == tokenBase { return true, "" } } - // Format the collision list: in non-debug mode truncate to the first 3 - // items and hint at --debug for the full list; debug shows all of them. + // Format the collision list. + // Non-debug: show the first maxShort critical names (highest priority first); + // the "+N more" count covers all remaining inode-sharers — not just critical + // ones — so the user sees the true blast radius. + // Debug: critical names first (priority order), then all other shared names + // appended alphabetically, with no repetitions. const maxShort = 3 - collisionSummary := strings.Join(criticalCollisions, " ") - if !debug && len(criticalCollisions) > maxShort { - collisionSummary = fmt.Sprintf("%s +%d more, use --debug for full list", - strings.Join(criticalCollisions[:maxShort], " "), - len(criticalCollisions)-maxShort, - ) + var collisionSummary string + if debug { + criticalSet := make(map[string]bool, len(criticalCollisions)) + for _, name := range criticalCollisions { + criticalSet[name] = true + } + var nonCritical []string + for _, name := range info.names { + if name == tokenBase || denyTokens[name] || criticalSet[name] { + continue + } + nonCritical = append(nonCritical, name) + } + slices.Sort(nonCritical) + all := make([]string, 0, len(criticalCollisions)+len(nonCritical)) + all = append(all, criticalCollisions...) + all = append(all, nonCritical...) + collisionSummary = strings.Join(all, " ") + } else { + shown := criticalCollisions + if len(shown) > maxShort { + shown = shown[:maxShort] + } + // remaining covers all other names sharing the inode minus the token + // itself and the names already shown in the excerpt. + remaining := len(info.names) - 1 - len(shown) + collisionSummary = strings.Join(shown, " ") + if remaining > 0 { + collisionSummary = fmt.Sprintf("%s +%d more, use --debug for full list", + collisionSummary, remaining) + } } - return true, fmt.Sprintf( - "runtime exec deny skipped for %s (requested: %s): shared binary also implements "+ - "critical commands [%s]. To force blocking add \"allowBlockingCritical\": true to "+ - "your command config. To silence this warning add %q to \"silenceSharedBinaryWarning\".", + return false, fmt.Sprintf( + "runtime exec deny warning for %s (requested: %s): shared binary also implements "+ + "critical commands [%s], which will be collaterally blocked. To skip runtime "+ + "blocking of %q and silence this warning, add it to \"acceptSharedBinaryCannotRuntimeDeny\" "+ + "in your command config.", path, token, collisionSummary, diff --git a/internal/sandbox/runtime_exec_deny_test.go b/internal/sandbox/runtime_exec_deny_test.go index a50590e..38a0277 100644 --- a/internal/sandbox/runtime_exec_deny_test.go +++ b/internal/sandbox/runtime_exec_deny_test.go @@ -133,69 +133,59 @@ func TestGetRuntimeDeniedExecutablePaths_IncludesChrootFromDefaults(t *testing.T UseDefaults: nil, }, } - got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) - - // On most systems chroot is a standalone binary and lands directly in the - // deny list. On Nix, however, chroot resolves to the coreutils multicall - // binary (which also implements ls, cat, etc.), so it will be skipped with - // a diagnostic instead. Both outcomes are correct behaviour — accept either. + got, _ := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + + // With the new security model fence always blocks even when the binary + // shares an inode with critical commands — blocking is the default and the + // user must explicitly opt out via acceptSharedBinaryCannotRuntimeDeny. chroot must + // therefore always appear in the blocked paths list regardless of whether + // it is a standalone binary (most distros) or part of a coreutils multicall + // binary (Nix/nix-darwin). for _, want := range chrootPaths { - if slices.Contains(got, want) { - continue - } - matched := false - for _, msg := range diagnostics { - if strings.Contains(msg, want) { - matched = true - break - } - } - if !matched { - t.Fatalf("expected chroot path %q in runtime denied paths or diagnostics, got paths=%v diagnostics=%v", want, got, diagnostics) + if !slices.Contains(got, want) { + t.Fatalf("expected chroot path %q in runtime denied paths, got: %v", want, got) } } } func TestFindSharedExecutableNames_DetectsSharedBinary(t *testing.T) { tmpDir := t.TempDir() - target := filepath.Join(tmpDir, "tool-a") - alias := filepath.Join(tmpDir, "tool-b") + aPath := filepath.Join(tmpDir, "aaa") + bPath := filepath.Join(tmpDir, "bbb") // #nosec G306 -- test fixture requires executable permissions - if err := os.WriteFile(target, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + if err := os.WriteFile(aPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { t.Fatalf("failed to create executable: %v", err) } - if err := os.Link(target, alias); err != nil { - t.Fatalf("failed to create hardlink alias: %v", err) + if err := os.Link(aPath, bPath); err != nil { + t.Fatalf("failed to create hard link: %v", err) } - shared, names := findSharedExecutableNames(target) + shared, names := findSharedExecutableNames(aPath) if !shared { - t.Fatalf("expected shared executable to be detected, got names=%v", names) + t.Fatalf("expected file sharing an inode to be detected as shared, got names=%v", names) } - if !slices.Contains(names, "tool-a") || !slices.Contains(names, "tool-b") { - t.Fatalf("expected both aliases in result, got: %v", names) + if !slices.Contains(names, "aaa") || !slices.Contains(names, "bbb") { + t.Fatalf("expected both names in shared list, got %v", names) } } func TestFindSharedExecutableNames_UniqueBinary(t *testing.T) { tmpDir := t.TempDir() - target := filepath.Join(tmpDir, "tool-single") + aPath := filepath.Join(tmpDir, "unique-binary") // #nosec G306 -- test fixture requires executable permissions - if err := os.WriteFile(target, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { + if err := os.WriteFile(aPath, []byte("#!/bin/sh\nexit 0\n"), 0o700); err != nil { t.Fatalf("failed to create executable: %v", err) } - shared, names := findSharedExecutableNames(target) + shared, names := findSharedExecutableNames(aPath) if shared { - t.Fatalf("expected unique executable to not be shared, got names=%v", names) - } - if len(names) != 1 || names[0] != "tool-single" { - t.Fatalf("expected only tool-single in result, got: %v", names) + t.Fatalf("expected unique file to not be detected as shared, got names=%v", names) } } +// Unique binary: no shared-inode detection fires → block with no diagnostic. func TestShouldSkipRuntimeExecDenyPath_UniqueDoesNotSkip(t *testing.T) { path := "/usr/bin/true" sharedCache := map[string]sharedExecutableInfo{ @@ -206,7 +196,7 @@ func TestShouldSkipRuntimeExecDenyPath_UniqueDoesNotSkip(t *testing.T) { }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "true", false, nil, map[string]bool{"true": true}, sharedCache, false) + skip, reason := shouldSkipRuntimeExecDenyPath(path, "true", nil, map[string]bool{"true": true}, sharedCache, false) if skip { t.Fatalf("expected unique executable target to not be skipped, reason=%q", reason) } @@ -215,7 +205,10 @@ func TestShouldSkipRuntimeExecDenyPath_UniqueDoesNotSkip(t *testing.T) { } } -func TestShouldSkipRuntimeExecDenyPath_SharedSkipsWithReason(t *testing.T) { +// Shared binary with critical collision: the new default is to BLOCK, but emit +// a diagnostic warning naming the collateral critical commands and telling the +// user how to opt out via acceptSharedBinaryCannotRuntimeDeny. +func TestShouldSkipRuntimeExecDenyPath_SharedBlocksWithWarning(t *testing.T) { path := "/shared/bin/dd" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -225,9 +218,12 @@ func TestShouldSkipRuntimeExecDenyPath_SharedSkipsWithReason(t *testing.T) { }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", false, nil, map[string]bool{"dd": true}, sharedCache, true) - if !skip { - t.Fatalf("expected shared binary with critical collision to be skipped") + skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", nil, map[string]bool{"dd": true}, sharedCache, true) + if skip { + t.Fatalf("expected shared binary with critical collision to be blocked (not skipped) by default") + } + if reason == "" { + t.Fatalf("expected a diagnostic warning reason when critical collision is detected") } if !strings.Contains(reason, "critical commands") { t.Fatalf("expected reason to mention critical commands, got %q", reason) @@ -235,17 +231,18 @@ func TestShouldSkipRuntimeExecDenyPath_SharedSkipsWithReason(t *testing.T) { if !strings.Contains(reason, "cat") || !strings.Contains(reason, "ls") { t.Fatalf("expected reason to name the colliding critical commands, got %q", reason) } - if !strings.Contains(reason, "allowBlockingCritical") { - t.Fatalf("expected reason to mention allowBlockingCritical, got %q", reason) + if !strings.Contains(reason, "acceptSharedBinaryCannotRuntimeDeny") { + t.Fatalf("expected reason to mention acceptSharedBinaryCannotRuntimeDeny, got %q", reason) } - if !strings.Contains(reason, "silenceSharedBinaryWarning") { - t.Fatalf("expected reason to mention silenceSharedBinaryWarning, got %q", reason) + // The removed option must never appear in any diagnostic. + if strings.Contains(reason, "allowBlockingCritical") { + t.Fatalf("removed option allowBlockingCritical must not appear in diagnostic, got %q", reason) } } +// Shared binary where all shared names are non-critical (python variants): +// blocking proceeds with no diagnostic — no collateral damage to critical commands. func TestShouldSkipRuntimeExecDenyPath_SharedNonCriticalDoesNotSkip(t *testing.T) { - // python3, python3.11, python3-config are all non-critical — blocking any - // one of them should be allowed even though they share a binary. path := "/usr/bin/python3.11" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -255,35 +252,18 @@ func TestShouldSkipRuntimeExecDenyPath_SharedNonCriticalDoesNotSkip(t *testing.T }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "python3", false, nil, map[string]bool{"python3": true}, sharedCache, false) + skip, reason := shouldSkipRuntimeExecDenyPath(path, "python3", nil, map[string]bool{"python3": true}, sharedCache, false) if skip { t.Fatalf("expected shared binary with only non-critical names to not be skipped, reason=%q", reason) } if reason != "" { - t.Fatalf("expected empty reason for non-skip, got %q", reason) - } -} - -func TestShouldSkipRuntimeExecDenyPath_AllowBlockingCriticalForcesBlock(t *testing.T) { - path := "/shared/bin/dd" - sharedCache := map[string]sharedExecutableInfo{ - path: { - checked: true, - shared: true, - names: []string{"cat", "dd", "ls"}, - }, - } - - skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", true, nil, map[string]bool{"dd": true}, sharedCache, false) - if skip { - t.Fatalf("expected allowBlockingCritical to force block despite critical collision, reason=%q", reason) - } - if reason != "" { - t.Fatalf("expected empty reason when allowBlockingCritical is set, got %q", reason) + t.Fatalf("expected empty reason for non-critical shared binary, got %q", reason) } } -func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningSilencesWarning(t *testing.T) { +// acceptSharedBinaryCannotRuntimeDeny: when the token is in the list the path is skipped +// silently — no diagnostic is emitted. +func TestShouldSkipRuntimeExecDenyPath_AcceptSharedBinaryCannotRuntimeDenySkipsSilently(t *testing.T) { path := "/shared/bin/dd" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -293,19 +273,18 @@ func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningSilencesWarning }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", false, []string{"dd"}, map[string]bool{"dd": true}, sharedCache, false) + skip, reason := shouldSkipRuntimeExecDenyPath(path, "dd", []string{"dd"}, map[string]bool{"dd": true}, sharedCache, false) if !skip { - t.Fatalf("expected shared binary to be skipped when token is in silenceSharedBinaryWarning") + t.Fatalf("expected shared binary to be skipped when token is in acceptSharedBinaryCannotRuntimeDeny") } if reason != "" { - t.Fatalf("expected empty reason (silenced) when token is in silenceSharedBinaryWarning, got %q", reason) + t.Fatalf("expected empty reason (silenced) when token is in acceptSharedBinaryCannotRuntimeDeny, got %q", reason) } } -func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningMatchesAcrossForms(t *testing.T) { - // The user denies "/shared/bin/dd" (absolute path token) but writes "dd" - // (bare name) in silenceSharedBinaryWarning, or vice versa. Both forms - // must silence the warning — mismatched forms must not silently fail. +// acceptSharedBinaryCannotRuntimeDeny matches regardless of whether the entry uses a bare +// name or an absolute path, so the user does not have to guess which form to write. +func TestShouldSkipRuntimeExecDenyPath_AcceptSharedBinaryCannotRuntimeDenyMatchesAcrossForms(t *testing.T) { path := "/shared/bin/dd" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -316,31 +295,31 @@ func TestShouldSkipRuntimeExecDenyPath_silenceSharedBinaryWarningMatchesAcrossFo } cases := []struct { - token string - silence string + token string + accept string }{ - // absolute-path deny rule, bare-name silence entry - {token: "/shared/bin/dd", silence: "dd"}, - // bare-name deny rule, absolute-path silence entry - {token: "dd", silence: "/shared/bin/dd"}, + // absolute-path deny rule, bare-name accept entry + {token: "/shared/bin/dd", accept: "dd"}, + // bare-name deny rule, absolute-path accept entry + {token: "dd", accept: "/shared/bin/dd"}, } for _, c := range cases { denyTokens := map[string]bool{c.token: true, filepath.Base(c.token): true} - skip, reason := shouldSkipRuntimeExecDenyPath(path, c.token, false, []string{c.silence}, denyTokens, sharedCache, false) + skip, reason := shouldSkipRuntimeExecDenyPath(path, c.token, []string{c.accept}, denyTokens, sharedCache, false) if !skip { - t.Errorf("token=%q silence=%q: expected skip (silenced), but was not skipped", c.token, c.silence) + t.Errorf("token=%q accept=%q: expected skip (accepted), but was not skipped", c.token, c.accept) } if reason != "" { - t.Errorf("token=%q silence=%q: expected empty reason (silenced), got %q", c.token, c.silence, reason) + t.Errorf("token=%q accept=%q: expected empty reason (silenced), got %q", c.token, c.accept, reason) } } } +// User explicitly blocks a critical command (ls). The shared binary only +// co-inhabits with non-critical commands (dd, rm) — no collateral damage to +// other critical commands → block proceeds with no diagnostic. func TestShouldSkipRuntimeExecDenyPath_CriticalTokenWithNoCriticalCollateral(t *testing.T) { - // User explicitly blocks "ls" (a critical command). The shared binary also - // implements "dd" and "rm" — neither of which is critical. There is no - // collateral damage to other critical commands, so the block should proceed. path := "/shared/bin/coreutils" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -350,7 +329,7 @@ func TestShouldSkipRuntimeExecDenyPath_CriticalTokenWithNoCriticalCollateral(t * }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", false, nil, map[string]bool{"ls": true}, sharedCache, false) + skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", nil, map[string]bool{"ls": true}, sharedCache, false) if skip { t.Fatalf("expected explicit block of critical token with no critical collateral to proceed, reason=%q", reason) } @@ -359,10 +338,11 @@ func TestShouldSkipRuntimeExecDenyPath_CriticalTokenWithNoCriticalCollateral(t * } } +// User explicitly blocks "ls". The shared binary also implements cat and head +// (both critical). Block proceeds with a diagnostic warning, but "ls" itself +// must NOT appear in the collision list — it was the intentional target, not +// collateral damage. func TestShouldSkipRuntimeExecDenyPath_CriticalTokenNotListedInOwnCollision(t *testing.T) { - // User explicitly blocks "ls" (a critical command). The shared binary also - // implements "cat" and "head" — both critical. The block should be skipped, - // but the diagnostic must not list "ls" itself as a colliding command. path := "/shared/bin/coreutils" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -372,14 +352,14 @@ func TestShouldSkipRuntimeExecDenyPath_CriticalTokenNotListedInOwnCollision(t *t }, } - skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", false, nil, map[string]bool{"ls": true}, sharedCache, true) - if !skip { - t.Fatalf("expected block to be skipped due to critical collateral (cat, head)") + skip, reason := shouldSkipRuntimeExecDenyPath(path, "ls", nil, map[string]bool{"ls": true}, sharedCache, true) + if skip { + t.Fatalf("expected block to proceed (not skip) despite critical collateral (cat, head)") } - // The bracketed collision list is part of the verbose diagnostic. "ls" will - // appear elsewhere in the verbose message (e.g. in the silenceSharedBinaryWarning - // suggestion), so check specifically that it is absent from the bracketed - // collision list rather than the full message string. + if reason == "" { + t.Fatalf("expected a diagnostic warning when critical collateral would be blocked") + } + // The bracketed collision list must include cat and head but not ls. start := strings.Index(reason, "[") end := strings.Index(reason, "]") if start == -1 || end == -1 || end <= start { @@ -394,14 +374,9 @@ func TestShouldSkipRuntimeExecDenyPath_CriticalTokenNotListedInOwnCollision(t *t } } +// All shared names are in the deny list — every co-inhabitant is an +// intentional target. No collateral damage → binary blocked with no diagnostic. func TestGetRuntimeDeniedExecutablePaths_AllSharedNamesDeniedShouldBlock(t *testing.T) { - // Shared binary (one inode) implements three commands: dd, ls, cat. - // The user denies all three by full path. - // - // ls and cat are critical commands, so the current collision check would - // normally skip the block with a warning. But since ls and cat are - // themselves in the deny list they are intentional targets — not collateral - // damage — and the binary must be blocked. tmpDir := t.TempDir() ddPath := filepath.Join(tmpDir, "dd") lsPath := filepath.Join(tmpDir, "ls") @@ -412,10 +387,10 @@ func TestGetRuntimeDeniedExecutablePaths_AllSharedNamesDeniedShouldBlock(t *test t.Fatalf("failed to create executable: %v", err) } if err := os.Link(ddPath, lsPath); err != nil { - t.Fatalf("failed to hardlink ls: %v", err) + t.Fatalf("failed to create hard link ls: %v", err) } if err := os.Link(ddPath, catPath); err != nil { - t.Fatalf("failed to hardlink cat: %v", err) + t.Fatalf("failed to create hard link cat: %v", err) } useDefaults := false @@ -430,11 +405,7 @@ func TestGetRuntimeDeniedExecutablePaths_AllSharedNamesDeniedShouldBlock(t *test // All three tokens resolve to the same inode and deduplicate to one path. // Because every critical co-inhabitant is also explicitly denied, the - // binary must appear in the blocked list — not be silently dropped. - // - // resolveExecutablePaths calls filepath.EvalSymlinks, which on macOS - // expands /var/folders/... → /private/var/folders/..., so compare against - // the canonical form of the path. + // binary must appear in the blocked list with no diagnostic warning. wantPath := ddPath if resolved, err := filepath.EvalSymlinks(ddPath); err == nil { wantPath = resolved @@ -442,16 +413,18 @@ func TestGetRuntimeDeniedExecutablePaths_AllSharedNamesDeniedShouldBlock(t *test if !slices.Contains(got, wantPath) { t.Fatalf("expected shared binary to be blocked when all shared names are denied, got paths=%v diagnostics=%v", got, diagnostics) } + if len(diagnostics) != 0 { + t.Fatalf("expected no diagnostics when all shared names are explicitly denied, got %v", diagnostics) + } } -func TestGetRuntimeDeniedExecutablePaths_PartialDenyStillSkipsForUninstructedCritical(t *testing.T) { - // Shared binary (one inode) implements four commands: dd, ls, cat, head. - // The user denies dd and ls — but NOT cat or head. - // - // ls is excluded from the collision check because it is also being denied - // (intentional target, not collateral damage). But cat and head are critical - // commands that the user never asked to block, so they ARE collateral damage. - // The binary must still be skipped with a warning even though ls is denied. +// Shared binary implements dd, ls, cat, head. User denies dd and ls but NOT +// cat or head. ls is excluded from the collision check (intentional target), +// but cat and head are uninstructed critical co-inhabitants. +// +// New default: the binary is still BLOCKED, but a diagnostic warning is emitted +// naming cat and head. ls must not appear in the collision list. +func TestGetRuntimeDeniedExecutablePaths_PartialDenyBlocksWithWarningForUninstructedCritical(t *testing.T) { tmpDir := t.TempDir() ddPath := filepath.Join(tmpDir, "dd") lsPath := filepath.Join(tmpDir, "ls") @@ -464,7 +437,7 @@ func TestGetRuntimeDeniedExecutablePaths_PartialDenyStillSkipsForUninstructedCri } for _, p := range []string{lsPath, catPath, headPath} { if err := os.Link(ddPath, p); err != nil { - t.Fatalf("failed to hardlink %s: %v", p, err) + t.Fatalf("failed to create hard link %s: %v", p, err) } } @@ -478,23 +451,22 @@ func TestGetRuntimeDeniedExecutablePaths_PartialDenyStillSkipsForUninstructedCri got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, true) - // The binary must NOT appear in the blocked list: cat and head are - // uninstructed critical co-inhabitants and would be collaterally blocked. + // The binary MUST appear in the blocked list: the new security model blocks + // by default even when uninstructed critical co-inhabitants are present. wantPath := ddPath if resolved, err := filepath.EvalSymlinks(ddPath); err == nil { wantPath = resolved } - if slices.Contains(got, wantPath) { - t.Fatalf("expected shared binary to be skipped when uninstructed critical co-inhabitants remain, got paths=%v", got) + if !slices.Contains(got, wantPath) { + t.Fatalf("expected shared binary to be blocked (new default) even with uninstructed critical co-inhabitants, got paths=%v", got) } - // There must be at least one diagnostic explaining the collision. + // There must be at least one diagnostic warning about the collision. if len(diagnostics) == 0 { - t.Fatalf("expected diagnostics for skipped shared binary, got none") + t.Fatalf("expected diagnostics warning about uninstructed critical co-inhabitants, got none") } - // The diagnostic must mention the uninstructed critical collaterals (cat, - // head) and must NOT mention ls (which is itself being denied). + // The diagnostic must mention cat and head but NOT ls (ls is also being denied). combined := strings.Join(diagnostics, "\n") start := strings.Index(combined, "[") end := strings.Index(combined, "]") @@ -510,7 +482,7 @@ func TestGetRuntimeDeniedExecutablePaths_PartialDenyStillSkipsForUninstructedCri } } -// 3a: when the token is an absolute path, the token's own basename must be +// When the token is an absolute path, the token's own basename must be // excluded from the critical-collision list even when denyTokens only contains // the absolute form (not the bare name). func TestShouldSkipRuntimeExecDenyPath_AbsolutePathTokenExcludedFromOwnCollision(t *testing.T) { @@ -526,9 +498,12 @@ func TestShouldSkipRuntimeExecDenyPath_AbsolutePathTokenExcludedFromOwnCollision // directly without the basename pre-population that the outer loop does. denyTokens := map[string]bool{"/shared/bin/ls": true} - skip, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/ls", false, nil, denyTokens, sharedCache, true) - if !skip { - t.Fatal("expected block to be skipped due to critical collateral (cat, head)") + skip, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/ls", nil, denyTokens, sharedCache, true) + if skip { + t.Fatal("expected block to proceed (not skip) despite critical collateral (cat, head)") + } + if reason == "" { + t.Fatal("expected a diagnostic warning when critical collateral would be blocked") } start := strings.Index(reason, "[") end := strings.Index(reason, "]") @@ -544,8 +519,8 @@ func TestShouldSkipRuntimeExecDenyPath_AbsolutePathTokenExcludedFromOwnCollision } } -// 3b: two deny rules that resolve to the same canonical path should produce -// exactly one diagnostic, not one per token. +// Two deny rules resolving to the same canonical path must produce exactly one +// diagnostic warning, not one per token. func TestGetRuntimeDeniedExecutablePathsWithDiagnostics_NoDuplicateDiagnostics(t *testing.T) { tmpDir := t.TempDir() ddPath := filepath.Join(tmpDir, "dd") @@ -559,7 +534,7 @@ func TestGetRuntimeDeniedExecutablePathsWithDiagnostics_NoDuplicateDiagnostics(t } for _, p := range []string{catPath, lsPath} { if err := os.Link(ddPath, p); err != nil { - t.Fatalf("failed to hardlink %s: %v", p, err) + t.Fatalf("failed to create hard link %s: %v", p, err) } } if err := os.Symlink(ddPath, symlinkPath); err != nil { @@ -575,16 +550,27 @@ func TestGetRuntimeDeniedExecutablePathsWithDiagnostics_NoDuplicateDiagnostics(t }, } - _, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + got, diagnostics := GetRuntimeDeniedExecutablePathsWithDiagnostics(cfg, false) + // The binary is blocked (new default). The two tokens resolve to the same + // canonical path, so there must be exactly one entry in the blocked list + // and at most one diagnostic warning. + wantPath := ddPath + if resolved, err := filepath.EvalSymlinks(ddPath); err == nil { + wantPath = resolved + } + if !slices.Contains(got, wantPath) { + t.Fatalf("expected shared binary to appear in blocked paths, got %v", got) + } if len(diagnostics) > 1 { - t.Fatalf("expected at most 1 diagnostic for two tokens resolving to the same skipped path, got %d: %v", len(diagnostics), diagnostics) + t.Fatalf("expected at most 1 diagnostic for two tokens resolving to the same path, got %d: %v", len(diagnostics), diagnostics) } } -// 3f: when the token is an absolute path, the silenceSharedBinaryWarning hint -// in the diagnostic must name the bare basename, not the full path. -func TestShouldSkipRuntimeExecDenyPath_DiagnosticSuggestsBasenameInSilenceHint(t *testing.T) { +// When the token is an absolute path, the acceptSharedBinaryCannotRuntimeDeny hint in the +// diagnostic must name the bare basename, not the full path — so the user +// writes a short, obvious entry in their config. +func TestShouldSkipRuntimeExecDenyPath_DiagnosticSuggestsBasenameInAcceptHint(t *testing.T) { path := "/shared/bin/dd" sharedCache := map[string]sharedExecutableInfo{ path: { @@ -595,7 +581,10 @@ func TestShouldSkipRuntimeExecDenyPath_DiagnosticSuggestsBasenameInSilenceHint(t } denyTokens := map[string]bool{"/shared/bin/dd": true, "dd": true} - _, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/dd", false, nil, denyTokens, sharedCache, true) + skip, reason := shouldSkipRuntimeExecDenyPath(path, "/shared/bin/dd", nil, denyTokens, sharedCache, true) + if skip { + t.Fatal("expected block to proceed (not skip) despite critical collision") + } if reason == "" { t.Fatal("expected a diagnostic reason") }