From db0de2dd085560fa3f46a70fb8775a2bf462b9b2 Mon Sep 17 00:00:00 2001 From: Ben Gittins <5601410+ihavespoons@users.noreply.github.com> Date: Tue, 3 Nov 2026 08:51:06 +1000 Subject: [PATCH 1/2] Check package installs that a macOS session process starts Only the command that `agentmon exec` started was package-checked. An install that an agent ran under `agentmon wrap`, or that a command started, was not. On build 1678, `pip install django==2.2` (CVE-2022-34265, CVSS 9.8) installed under wrap, although agent-default blocks a critical vulnerability. PolicyAdapter.CheckExec now runs the package check on an exec that the command rules allow. A block or a failed check denies the exec. An approve verdict denies it and asks for a dialog, as an approve exec rule does. ES reports a script's interpreter as the executable, so `pip3 install x` arrives as python3.15. The kernel puts the script path in argv[1], and the extension now sends the script, so the daemon classifies argv[1]. pkgcheck also reads `python -m pip` as pip, and a versioned name such as pip3.15 as pip3. A check resolves the install and asks the providers, which takes seconds. An exec whose arguments hold an install word now waits until 1 s before the ES deadline, at most 30 s, on a socket connection of its own. Every other exec keeps the 2 s limit on the shared connection. The exec API and the extension share one Checker, which reuses a verdict for the same install for 30 s, so one `agentmon exec pip install` does not resolve twice. Co-Authored-By: Claude Opus 5.5 --- internal/pkgcheck/checker.go | 72 +++++++++- internal/pkgcheck/checker_recent_test.go | 79 +++++++++++ internal/pkgcheck/interceptor.go | 48 +++++++ .../platform/darwin/policysock/handler.go | 11 +- .../darwin/policysock/handler_test.go | 95 +++++++++++++ .../darwin/policysock/package_check.go | 125 ++++++++++++++++++ .../platform/darwin/policysock/protocol.go | 14 ++ internal/platform/darwin/policysock/server.go | 6 +- internal/server/policysock_darwin.go | 8 +- internal/server/policysock_other.go | 4 +- internal/server/server.go | 8 +- macos/AgentMon/ESFClient.swift | 74 +++++++++-- macos/AgentMon/PolicySocketClient.swift | 37 +++++- scripts/test-macos-package-checks.sh | 81 ++++++++++++ 14 files changed, 642 insertions(+), 20 deletions(-) create mode 100644 internal/pkgcheck/checker_recent_test.go create mode 100644 internal/platform/darwin/policysock/package_check.go create mode 100755 scripts/test-macos-package-checks.sh diff --git a/internal/pkgcheck/checker.go b/internal/pkgcheck/checker.go index e014ea59..9af4a56c 100644 --- a/internal/pkgcheck/checker.go +++ b/internal/pkgcheck/checker.go @@ -3,7 +3,10 @@ package pkgcheck import ( "context" "fmt" + "path/filepath" "strings" + "sync" + "time" "github.com/diffsec/agentmon/internal/policy" ) @@ -28,8 +31,24 @@ type Checker struct { cfg CheckerConfig orch *Orchestrator eval *Evaluator + + // recent holds the verdicts of the last recentTTL, by install. On macOS + // one `agentmon exec pip install x` is checked twice: by the exec API, + // and again when the system extension sees the exec. Without this, the + // second check resolves the same install again. + mu sync.Mutex + recent map[string]recentVerdict + now func() time.Time +} + +type recentVerdict struct { + verdict Verdict + at time.Time } +// recentTTL is how long Check reuses a verdict for the same install. +const recentTTL = 30 * time.Second + // NewChecker creates a new Checker wired to the given config. func NewChecker(cfg CheckerConfig) *Checker { pf := NewPrivacyFilter(cfg.Privacy) @@ -39,19 +58,68 @@ func NewChecker(cfg CheckerConfig) *Checker { Providers: cfg.Providers, PrivacyFilter: pf, }), - eval: NewEvaluator(cfg.Rules), + eval: NewEvaluator(cfg.Rules), + recent: make(map[string]recentVerdict), + now: time.Now, } } // Check evaluates a command. Returns a nil verdict if the command is not a // recognised package-install operation. +// +// A verdict of allow, warn or block for the same install in the same +// directory is reused for recentTTL. An approve verdict is not reused, +// because each one asks a person. func (c *Checker) Check(ctx context.Context, command string, args []string, workDir string) (*Verdict, error) { - // 1. Classify the command. + // `python -m pip install x` is a pip install. + command, args = UnwrapInstallCommand(command, args) intent := ClassifyInstallCommand(command, args, c.cfg.Scope) if intent == nil { return nil, nil } + key := recentKey(intent, workDir) + if v, ok := c.recentVerdict(key); ok { + return v, nil + } + v, err := c.check(ctx, command, args, workDir, intent) + if err == nil && v != nil && v.Action != VerdictApprove { + c.mu.Lock() + c.recent[key] = recentVerdict{verdict: *v, at: c.now()} + c.mu.Unlock() + } + return v, err +} + +// recentKey names an install: the tool, the directory and the arguments. +// The directory is resolved, because the exec API and macOS ES can name the +// same one differently, for example /var and /private/var. +func recentKey(intent *InstallIntent, workDir string) string { + if dir, err := filepath.EvalSymlinks(workDir); err == nil { + workDir = dir + } + return intent.Tool + "\x00" + workDir + "\x00" + strings.Join(intent.OrigArgs, "\x00") +} + +// recentVerdict returns a copy of a verdict from the last recentTTL, and +// drops the ones that are older. +func (c *Checker) recentVerdict(key string) (*Verdict, bool) { + c.mu.Lock() + defer c.mu.Unlock() + now := c.now() + for k, r := range c.recent { + if now.Sub(r.at) > recentTTL { + delete(c.recent, k) + } + } + r, ok := c.recent[key] + if !ok { + return nil, false + } + v := r.verdict + return &v, true +} +func (c *Checker) check(ctx context.Context, command string, args []string, workDir string, intent *InstallIntent) (*Verdict, error) { // 2. Find a resolver that can handle this tool. // Strip leading global flags so args[0] is the subcommand, // which is what resolvers' CanResolve expects. diff --git a/internal/pkgcheck/checker_recent_test.go b/internal/pkgcheck/checker_recent_test.go new file mode 100644 index 00000000..a80da6a4 --- /dev/null +++ b/internal/pkgcheck/checker_recent_test.go @@ -0,0 +1,79 @@ +package pkgcheck + +import ( + "context" + "testing" + "time" +) + +// countingResolver resolves every pip install to one package and counts the +// calls, so that a test can see whether Check resolved again. +type countingResolver struct{ calls int } + +func (r *countingResolver) Name() string { return "counting" } + +func (r *countingResolver) CanResolve(tool string, _ []string) bool { return tool == "pip" } + +func (r *countingResolver) Resolve(context.Context, string, []string) (*InstallPlan, error) { + r.calls++ + return &InstallPlan{ + Tool: "pip", + Ecosystem: EcosystemPyPI, + Direct: []PackageRef{{Name: "requests", Version: "2.0.0", Direct: true}}, + }, nil +} + +func TestCheckReusesRecentVerdict(t *testing.T) { + res := &countingResolver{} + c := NewChecker(CheckerConfig{Resolvers: []Resolver{res}}) + clock := time.Unix(1000, 0) + c.now = func() time.Time { return clock } + ctx := context.Background() + dir := t.TempDir() + + if _, err := c.Check(ctx, "pip3", []string{"install", "requests"}, dir); err != nil { + t.Fatal(err) + } + // The same install as macOS ES reports it: a versioned name. + if _, err := c.Check(ctx, "/opt/homebrew/bin/pip3.15", []string{"install", "requests"}, dir); err != nil { + t.Fatal(err) + } + // The same install as `python3 -m pip`. + if _, err := c.Check(ctx, "/opt/homebrew/bin/python3", []string{"-m", "pip", "install", "requests"}, dir); err != nil { + t.Fatal(err) + } + if res.calls != 1 { + t.Fatalf("resolved %d times within recentTTL, want 1", res.calls) + } + + clock = clock.Add(recentTTL + time.Second) + if _, err := c.Check(ctx, "pip3", []string{"install", "requests"}, dir); err != nil { + t.Fatal(err) + } + if res.calls != 2 { + t.Errorf("resolved %d times after recentTTL, want 2", res.calls) + } +} + +func TestUnwrapInstallCommand(t *testing.T) { + for _, c := range []struct { + cmd string + args []string + wantCmd string + wantArgs int + }{ + {"/opt/homebrew/bin/python3", []string{"-m", "pip", "install", "x"}, "pip", 2}, + {"python3.15", []string{"-I", "-m", "pip3", "install", "x"}, "pip", 2}, + {"python3", []string{"script.py", "-m", "pip"}, "python3", 3}, + {"python3", []string{"-m", "venv", "x"}, "python3", 3}, + {"node", []string{"-m", "pip"}, "node", 2}, + } { + cmd, args := UnwrapInstallCommand(c.cmd, c.args) + if cmd != c.wantCmd || len(args) != c.wantArgs { + t.Errorf("UnwrapInstallCommand(%s %v) = %s %v", c.cmd, c.args, cmd, args) + } + } + if intent := ClassifyInstallCommand("/opt/homebrew/bin/pip3.15", []string{"install", "x"}, ""); intent == nil || intent.Tool != "pip" { + t.Errorf("pip3.15 install x: intent = %+v, want a pip install", intent) + } +} diff --git a/internal/pkgcheck/interceptor.go b/internal/pkgcheck/interceptor.go index d9f71897..caf2e895 100644 --- a/internal/pkgcheck/interceptor.go +++ b/internal/pkgcheck/interceptor.go @@ -3,6 +3,7 @@ package pkgcheck import ( "path/filepath" "strings" + "unicode" ) // InstallIntent captures the parsed intent of an install command. @@ -68,6 +69,9 @@ func ClassifyInstallCommand(command string, args []string, scope string) *Instal base = strings.TrimSuffix(base, ".exe") base = strings.TrimSuffix(base, ".cmd") base = strings.TrimSuffix(base, ".bat") + // A versioned name such as "pip3.15" is the same tool. Homebrew installs + // pip3.15 next to pip3, and on macOS ES reports resolved paths. + base = trimVersionSuffix(base) switch base { case "npm": @@ -87,6 +91,50 @@ func ClassifyInstallCommand(command string, args []string, scope string) *Instal } } +// trimVersionSuffix removes trailing "." parts: "pip3.15" gives +// "pip3". The part that stays must hold a letter. +func trimVersionSuffix(name string) string { + for { + dot := strings.LastIndexByte(name, '.') + if dot <= 0 || dot == len(name)-1 || !strings.ContainsFunc(name[:dot], unicode.IsLetter) { + return name + } + if strings.Trim(name[dot+1:], "0123456789") != "" { + return name + } + name = name[:dot] + } +} + +// UnwrapInstallCommand returns the package manager command inside a +// `python -m pip ...` command, as ("pip", the arguments after the module). +// Any other command comes back unchanged. +// +// The interpreter's own flags before -m are skipped. A flag that takes a +// value, such as -W or -X, is not expected before -m, so the first argument +// that does not start with "-" ends the search. +func UnwrapInstallCommand(command string, args []string) (string, []string) { + base := strings.ToLower(filepath.Base(command)) + if !strings.HasPrefix(trimVersionSuffix(base), "python") { + return command, args + } + for i, a := range args { + if a == "-m" { + if i+1 < len(args) { + switch trimVersionSuffix(strings.ToLower(args[i+1])) { + case "pip", "pip3": + return "pip", args[i+2:] + } + } + return command, args + } + if !strings.HasPrefix(a, "-") { + return command, args + } + } + return command, args +} + // classifyNPM handles npm install/i/add/ci commands. func classifyNPM(command string, args []string, scope string) *InstallIntent { if len(args) == 0 { diff --git a/internal/platform/darwin/policysock/handler.go b/internal/platform/darwin/policysock/handler.go index 849a6817..62cf7da5 100644 --- a/internal/platform/darwin/policysock/handler.go +++ b/internal/platform/darwin/policysock/handler.go @@ -28,6 +28,7 @@ type PolicyAdapter struct { sessions SessionResolver sessionEngine func(sessionID string) *policy.Engine approvals *approvals.Manager + packages PackageChecker } // SetSessionEngineLookup gives the adapter each session's own policy engine. @@ -117,7 +118,7 @@ func (a *PolicyAdapter) ResolveSession(pid int32) string { // CheckExec evaluates a command through the exec pipeline, returning // the full decision and action for the ESF client to act on. -func (a *PolicyAdapter) CheckExec(executable string, args []string, pid int32, parentPID int32, sessionID string, _ ExecContext) ExecCheckResult { +func (a *PolicyAdapter) CheckExec(executable string, args []string, pid int32, parentPID int32, sessionID string, ec ExecContext) ExecCheckResult { engine := a.engineFor(sessionID) if engine == nil { slog.Error("policy socket: no policy engine installed, denying exec (fail closed)", @@ -174,12 +175,18 @@ func (a *PolicyAdapter) CheckExec(executable string, args []string, pid int32, p action = "deny" } - return ExecCheckResult{ + result := ExecCheckResult{ Decision: decision, Action: action, Rule: dec.Rule, Message: dec.Message, } + if action == "continue" { + if pkg, ok := a.checkPackageInstall(sessionID, executable, args, ec); ok { + return pkg + } + } + return result } // denyAllSnapshot returns a snapshot that denies everything for the session. diff --git a/internal/platform/darwin/policysock/handler_test.go b/internal/platform/darwin/policysock/handler_test.go index b2a06f0a..8c8f0209 100644 --- a/internal/platform/darwin/policysock/handler_test.go +++ b/internal/platform/darwin/policysock/handler_test.go @@ -3,8 +3,12 @@ package policysock import ( + "context" + "errors" "testing" + "time" + "github.com/diffsec/agentmon/internal/pkgcheck" "github.com/diffsec/agentmon/internal/policy" ) @@ -541,3 +545,94 @@ func TestBuildPolicySnapshot_FailsClosed(t *testing.T) { }) } } + +// fakePackageChecker returns one verdict and records what it was asked. +type fakePackageChecker struct { + verdict *pkgcheck.Verdict + err error + command string + args []string + workDir string + deadline time.Duration +} + +func (f *fakePackageChecker) Check(ctx context.Context, command string, args []string, workDir string) (*pkgcheck.Verdict, error) { + f.command, f.args, f.workDir = command, args, workDir + if d, ok := ctx.Deadline(); ok { + f.deadline = time.Until(d) + } + return f.verdict, f.err +} + +// TestCheckExec_PackageInstall: an exec that the command rules allow is then +// package-checked. ES reports a script's interpreter as the executable, so +// `pip3 install x` must be classified from argv[1]. +func TestCheckExec_PackageInstall(t *testing.T) { + p := &policy.Policy{Version: 1, CommandRules: []policy.CommandRule{ + {Name: "allow-all", Commands: []string{"*"}, Decision: "allow"}, + }} + engine, err := policy.NewEngine(p, false, true) + if err != nil { + t.Fatal(err) + } + const py = "/opt/homebrew/Cellar/python@3.15/3.15.0/bin/python3.15" + argv := []string{py, "/opt/homebrew/bin/pip3", "install", "evil"} + ec := ExecContext{CWDPath: "/tmp/ws", Script: "/opt/homebrew/Cellar/python@3.15/3.15.0/bin/pip3", Deadline: 10 * time.Second} + + for _, c := range []struct { + name string + checker *fakePackageChecker + wantAction string + wantRule string + }{ + {"block", &fakePackageChecker{verdict: &pkgcheck.Verdict{Action: pkgcheck.VerdictBlock, Summary: "evil is malware"}}, "deny", "package-check"}, + {"allow", &fakePackageChecker{verdict: &pkgcheck.Verdict{Action: pkgcheck.VerdictAllow}}, "continue", "allow-all"}, + {"warn", &fakePackageChecker{verdict: &pkgcheck.Verdict{Action: pkgcheck.VerdictWarn}}, "continue", "allow-all"}, + {"not an install", &fakePackageChecker{}, "continue", "allow-all"}, + {"error", &fakePackageChecker{err: errors.New("pip dry-run failed")}, "deny", "package-check"}, + {"approve without approvals", &fakePackageChecker{verdict: &pkgcheck.Verdict{Action: pkgcheck.VerdictApprove}}, "deny", "package-check"}, + } { + t.Run(c.name, func(t *testing.T) { + a := NewPolicyAdapter(engine, nil) + a.SetPackageChecker(c.checker) + got := a.CheckExec(py, argv, 100, 1, "s1", ec) + if got.Action != c.wantAction || got.Rule != c.wantRule { + t.Errorf("CheckExec = %s by %q (%s), want %s by %q", got.Action, got.Rule, got.Message, c.wantAction, c.wantRule) + } + if c.checker.command != "/opt/homebrew/bin/pip3" || len(c.checker.args) != 2 || c.checker.workDir != "/tmp/ws" { + t.Errorf("checked %s %v in %s, want /opt/homebrew/bin/pip3 [install evil] in /tmp/ws", + c.checker.command, c.checker.args, c.checker.workDir) + } + if c.checker.deadline <= 0 || c.checker.deadline > 10*time.Second-packageCheckMargin { + t.Errorf("check deadline = %s, want under %s", c.checker.deadline, 10*time.Second-packageCheckMargin) + } + }) + } +} + +// TestCheckExec_PackageCheckSkippedOnDeny: an exec that the command rules +// deny is not package-checked. +func TestCheckExec_PackageCheckSkippedOnDeny(t *testing.T) { + engine, err := policy.NewEngine(&policy.Policy{Version: 1}, false, true) + if err != nil { + t.Fatal(err) + } + f := &fakePackageChecker{verdict: &pkgcheck.Verdict{Action: pkgcheck.VerdictAllow}} + a := NewPolicyAdapter(engine, nil) + a.SetPackageChecker(f) + got := a.CheckExec("/usr/bin/pip3", []string{"/usr/bin/pip3", "install", "x"}, 100, 1, "s1", ExecContext{}) + if got.Action != "deny" || f.command != "" { + t.Errorf("CheckExec = %s, checker asked for %q; want deny with no package check", got.Action, f.command) + } +} + +func TestInstallCommand(t *testing.T) { + cmd, args := installCommand("/usr/local/bin/uv", []string{"uv", "pip", "install", "x"}, "") + if cmd != "/usr/local/bin/uv" || len(args) != 3 { + t.Errorf("binary: %s %v", cmd, args) + } + cmd, args = installCommand("/usr/bin/python3", []string{"python3", "/usr/local/bin/pip3", "install", "x"}, "/usr/local/bin/pip3") + if cmd != "/usr/local/bin/pip3" || len(args) != 2 { + t.Errorf("script: %s %v", cmd, args) + } +} diff --git a/internal/platform/darwin/policysock/package_check.go b/internal/platform/darwin/policysock/package_check.go new file mode 100644 index 00000000..855f3685 --- /dev/null +++ b/internal/platform/darwin/policysock/package_check.go @@ -0,0 +1,125 @@ +//go:build darwin + +package policysock + +import ( + "context" + "log/slog" + "path/filepath" + "strings" + "time" + + "github.com/diffsec/agentmon/internal/approvals" + "github.com/diffsec/agentmon/internal/pkgcheck" +) + +// PackageChecker checks a package install. *pkgcheck.Checker implements it. +// It returns a nil verdict for a command that is not an install. +type PackageChecker interface { + Check(ctx context.Context, command string, args []string, workDir string) (*pkgcheck.Verdict, error) +} + +// SetPackageChecker makes CheckExec check package installs, as the exec API +// does for the command that it starts. With it, an install that an agent +// starts under `agentmon wrap`, or from inside a command, is checked too. +func (a *PolicyAdapter) SetPackageChecker(c PackageChecker) { + a.packages = c +} + +const ( + // packageCheckFallback is the time for a package check when the + // extension does not say how long it waits. Its own default wait is 2s. + packageCheckFallback = 1500 * time.Millisecond + + // packageCheckMargin is kept free before the extension stops waiting, + // so that the daemon answers before the extension denies on its own. + packageCheckMargin = 500 * time.Millisecond + + // packageRule names the rule in a deny that the package check caused. + packageRule = "package-check" +) + +// installCommand returns the command and arguments to classify as an +// install, from an exec as ES reports it. args holds argv, with argv[0]. +// +// For a script, ES reports the interpreter as the executable. The kernel +// puts the script path, as the caller named it, in argv[1]. So `pip3 +// install x` arrives as python3.15 with argv [python3.15, /opt/homebrew/bin/ +// pip3, install, x], and the install command is pip3 with [install, x]. +func installCommand(executable string, args []string, script string) (string, []string) { + if script != "" && len(args) >= 2 { + return args[1], args[2:] + } + if len(args) >= 1 { + return executable, args[1:] + } + return executable, nil +} + +// checkPackageInstall runs the package check on an exec that the command +// rules allow. ok is false when the exec is not an install, or when the +// check allows it. Then the command rules' verdict stands. +func (a *PolicyAdapter) checkPackageInstall(sessionID, executable string, args []string, ec ExecContext) (ExecCheckResult, bool) { + if a.packages == nil { + return ExecCheckResult{}, false + } + command, cmdArgs := installCommand(executable, args, ec.Script) + + budget := packageCheckFallback + if ec.Deadline > 0 { + budget = ec.Deadline - packageCheckMargin + } + if budget <= 0 { + return packageDeny("package check: no time left before the system extension stops waiting"), true + } + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + verdict, err := a.packages.Check(ctx, command, cmdArgs, ec.CWDPath) + if err != nil { + // The exec API denies on the same error (api/core.go). + slog.Warn("policy socket: package check failed, denying the exec", + "command", command, "session_id", sessionID, "error", err) + return packageDeny("package check failed: " + err.Error()), true + } + if verdict == nil { + return ExecCheckResult{}, false + } + + switch verdict.Action { + case pkgcheck.VerdictBlock: + return packageDeny(verdict.Summary), true + case pkgcheck.VerdictApprove: + return a.packageApproval(sessionID, command, cmdArgs, verdict), true + case pkgcheck.VerdictWarn: + slog.Warn("policy socket: package install warning", "session_id", sessionID, "summary", verdict.Summary) + } + return ExecCheckResult{}, false +} + +func packageDeny(message string) ExecCheckResult { + return ExecCheckResult{Decision: "deny", Action: "deny", Rule: packageRule, Message: message} +} + +// packageApproval handles an approve verdict as execApproval handles an +// approve rule. The extension cannot hold an exec while a person reads a +// dialog, so the install is denied and a dialog opens. If the person +// approves, the same install succeeds when the agent tries it again. +func (a *PolicyAdapter) packageApproval(sessionID, command string, args []string, verdict *pkgcheck.Verdict) ExecCheckResult { + target := strings.Join(append([]string{filepath.Base(command)}, args...), " ") + if a.approvals == nil || sessionID == "" { + return packageDeny("package install needs approval, and no approval dialog is available: " + verdict.Summary) + } + if a.approvals.Granted(sessionID, "package", target, "install") { + return ExecCheckResult{Decision: "approve", Action: "continue", Rule: packageRule, Message: verdict.Summary} + } + a.approvals.Ask(approvals.Request{ + SessionID: sessionID, + Kind: "package", + Target: target, + Rule: packageRule, + Message: verdict.Summary, + Fields: map[string]any{"operation": "install"}, + }) + return packageDeny("package install needs approval: " + verdict.Summary) +} diff --git a/internal/platform/darwin/policysock/protocol.go b/internal/platform/darwin/policysock/protocol.go index 6e3ca947..49ccd5c0 100644 --- a/internal/platform/darwin/policysock/protocol.go +++ b/internal/platform/darwin/policysock/protocol.go @@ -2,6 +2,8 @@ package policysock +import "time" + // RequestType identifies the type of policy check. type RequestType string @@ -112,6 +114,13 @@ type PolicyRequest struct { // Exec context fields (from ESF event) TTYPath string `json:"tty_path,omitempty"` // Controlling terminal path CWDPath string `json:"cwd_path,omitempty"` // Working directory of the exec'ing process + // Script is the script that an interpreter runs, as ES reports it. For + // `pip3 install x`, the executable is python3.15 and args[1] is the + // pip3 script. Empty for a binary. + Script string `json:"script,omitempty"` + // DeadlineMS is the time left before the extension stops waiting for + // the verdict. 0 means the request does not say. + DeadlineMS int64 `json:"deadline_ms,omitempty"` // Exec depth tracking (matches Linux seccomp depth) Depth int `json:"depth,omitempty"` @@ -173,6 +182,11 @@ type PolicyResponse struct { type ExecContext struct { TTYPath string // Controlling terminal path (e.g. /dev/ttys001) CWDPath string // Working directory of the exec'ing process + // Script is the script that an interpreter runs. Empty for a binary. + Script string + // Deadline is how long the extension waits for the verdict. 0 means + // unknown. + Deadline time.Duration } // ApprovalResponse represents a pending approval request returned to the client. diff --git a/internal/platform/darwin/policysock/server.go b/internal/platform/darwin/policysock/server.go index e2f2f282..9ace4b9c 100644 --- a/internal/platform/darwin/policysock/server.go +++ b/internal/platform/darwin/policysock/server.go @@ -511,8 +511,10 @@ func (s *Server) handleExecCheck(req *PolicyRequest) PolicyResponse { } result := h.CheckExec(req.Path, req.Args, req.PID, req.ParentPID, req.SessionID, ExecContext{ - TTYPath: req.TTYPath, - CWDPath: req.CWDPath, + TTYPath: req.TTYPath, + CWDPath: req.CWDPath, + Script: req.Script, + Deadline: time.Duration(req.DeadlineMS) * time.Millisecond, }) return PolicyResponse{ Allow: result.Action == "continue", diff --git a/internal/server/policysock_darwin.go b/internal/server/policysock_darwin.go index 3de4f527..ca81d433 100644 --- a/internal/server/policysock_darwin.go +++ b/internal/server/policysock_darwin.go @@ -12,6 +12,7 @@ import ( "github.com/diffsec/agentmon/internal/config" "github.com/diffsec/agentmon/internal/events" "github.com/diffsec/agentmon/internal/netmonitor" + "github.com/diffsec/agentmon/internal/pkgcheck" "github.com/diffsec/agentmon/internal/platform/darwin" "github.com/diffsec/agentmon/internal/platform/darwin/policysock" "github.com/diffsec/agentmon/internal/policy" @@ -26,7 +27,7 @@ const approvalGrantTTL = 60 * time.Second // startPolicySocket creates and starts the policy socket server for macOS // system extension IPC. It sets the policySockCancel and policySockDone // fields on the Server so the socket is shut down when the server stops. -func (s *Server) startPolicySocket(cfg *config.Config, engine *policy.Engine, approvalsMgr *approvals.Manager) { +func (s *Server) startPolicySocket(cfg *config.Config, engine *policy.Engine, approvalsMgr *approvals.Manager, pkgChecker *pkgcheck.Checker) { sockPath := cfg.PolicySocket.Path if sockPath == "" { return @@ -85,6 +86,11 @@ func (s *Server) startPolicySocket(cfg *config.Config, engine *policy.Engine, ap adapter.SetApprovals(approvalsMgr) } psrv.SetApprovalAsker(adapter) + // A nil *pkgcheck.Checker in the interface would not be nil, so the + // adapter gets it only when package checks are on. + if pkgChecker != nil { + adapter.SetPackageChecker(pkgChecker) + } psrv.SetFlowHandler(flowHandler{ sessions: s.sessions, engine: engine, diff --git a/internal/server/policysock_other.go b/internal/server/policysock_other.go index ca2cdf78..3348f40d 100644 --- a/internal/server/policysock_other.go +++ b/internal/server/policysock_other.go @@ -5,9 +5,11 @@ package server import ( "github.com/diffsec/agentmon/internal/approvals" "github.com/diffsec/agentmon/internal/config" + "github.com/diffsec/agentmon/internal/pkgcheck" "github.com/diffsec/agentmon/internal/policy" ) // startPolicySocket is a no-op on non-darwin platforms. // The policy socket server is only available on macOS for system extension IPC. -func (s *Server) startPolicySocket(_ *config.Config, _ *policy.Engine, _ *approvals.Manager) {} +func (s *Server) startPolicySocket(_ *config.Config, _ *policy.Engine, _ *approvals.Manager, _ *pkgcheck.Checker) { +} diff --git a/internal/server/server.go b/internal/server/server.go index 0b1b8cf6..205f5529 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -650,6 +650,10 @@ func New(cfg *config.Config) (*Server, error) { // the proxy after compiling the session-local DB unavoidability bundle. // Initialize package checker (optional). + // pkgChecker stays nil when package checks are off. The policy socket + // uses the same checker, so the exec API and the macOS extension share + // one verdict for one install. + var pkgChecker *pkgcheck.Checker if cfg.PackageChecks.Enabled { // Resolve and apply fail mode so Snyk/Socket OnFailure reflects // the operator's policy before provider entries are constructed. @@ -706,7 +710,7 @@ func New(cfg *config.Config) (*Server, error) { rules := append([]policy.PackageRule(nil), engine.PackageRules()...) rules = append(rules, config.CompileBlockOnRules(cfg.PackageChecks.BlockOn)...) - pkgChecker := pkgcheck.NewChecker(pkgcheck.CheckerConfig{ + pkgChecker = pkgcheck.NewChecker(pkgcheck.CheckerConfig{ Scope: cfg.PackageChecks.Scope, Resolvers: resolvers, Providers: providerEntries, @@ -867,7 +871,7 @@ func New(cfg *config.Config) (*Server, error) { } // Start the policy socket server (macOS only; no-op on other platforms). - srv.startPolicySocket(cfg, engine, approvalsMgr) + srv.startPolicySocket(cfg, engine, approvalsMgr, pkgChecker) // Wire cmdResolver and sessionTracker into the app (darwin only). if srv.cmdResolver != nil { app.SetCmdResolver(srv.cmdResolver) diff --git a/macos/AgentMon/ESFClient.swift b/macos/AgentMon/ESFClient.swift index a6953ab1..7bfb8b36 100644 --- a/macos/AgentMon/ESFClient.swift +++ b/macos/AgentMon/ESFClient.swift @@ -976,13 +976,50 @@ private func sendExecDeniedEvent(path: String, args: [String], pid: pid_t, paren ) } -/// How long to wait for the daemon's exec verdict before failing closed. +/// The shortest wait for the daemon's exec verdict before failing closed. /// -/// Comfortably inside the ES deadline for AUTH_EXEC, and enormous next to the -/// observed round trip, which is a single unix-socket request on the same -/// machine. +/// Enormous next to the observed round trip, which is a single unix-socket +/// request on the same machine. A package install needs longer, because the +/// daemon resolves it and asks the package providers. See execWaitTime. private let execDecisionTimeout: TimeInterval = 2.0 +/// The longest wait for the daemon's exec verdict. +private let execDecisionMaxWait: TimeInterval = 30.0 + +/// Time kept free before the ES deadline. The extension must answer before +/// it, or ES ends the extension's client. +private let execDeadlineMargin: TimeInterval = 1.0 + +/// Words that start a package install in the package managers that the +/// daemon checks: npm, pnpm, yarn, pip, uv and poetry. +private let packageInstallWords: Set = ["install", "i", "add", "ci", "sync"] + +/// Whether an exec can be a package install, from its arguments. A false +/// answer for a real install is safe: the daemon then has 1.5 s, and an +/// unfinished check denies the exec. +private func execMayBePackageInstall(_ args: [String]) -> Bool { + args.dropFirst().contains { packageInstallWords.contains($0) } +} + +/// How long to wait for the daemon's exec verdict: until execDeadlineMargin +/// before the message's ES deadline, but no longer than execDecisionMaxWait. +/// When the deadline leaves less than execDecisionTimeout, the wait ends +/// 0.2 s before the deadline. +private func execWaitTime(_ message: UnsafePointer) -> TimeInterval { + var timebase = mach_timebase_info_data_t() + mach_timebase_info(&timebase) + let now = mach_absolute_time() + let deadline = message.pointee.deadline + guard deadline > now, timebase.denom != 0 else { return execDecisionTimeout } + let ticks = Double(deadline - now) + let remaining = ticks * Double(timebase.numer) / Double(timebase.denom) / 1e9 + let wait = min(remaining - execDeadlineMargin, execDecisionMaxWait) + if wait >= execDecisionTimeout { + return wait + } + return max(min(execDecisionTimeout, remaining - 0.2), 0.1) +} + /// Queue for the fail-closed watchdog below. Concurrent: each exec's watchdog /// is independent and must not be delayed by another's. private let execWatchdogQueue = DispatchQueue( @@ -1074,6 +1111,19 @@ private func handleAuthExec(client: OpaquePointer, event: UnsafePointer= 2, let script = execPtr.pointee.script { + scriptPath = String(cString: script.pointee.path.data) + } + // An exec that can be a package install may need seconds: the daemon + // resolves the install and asks the package providers. Only such an + // exec waits longer, on a connection of its own. Every other exec keeps + // the 2 s limit. If this test misses an install, the daemon has 1.5 s + // for the check, and an unfinished check denies the exec. + let mayInstall = execMayBePackageInstall(args) + let waitTime = mayInstall ? execWaitTime(event) : execDecisionTimeout // Ask the daemon rather than deciding from the local cache. // @@ -1097,14 +1147,14 @@ private func handleAuthExec(client: OpaquePointer, event: UnsafePointer Void = { response in switch response?["action"] as? String { case "continue": responder.respond(ES_AUTH_RESULT_ALLOW) @@ -1148,6 +1201,11 @@ private func handleAuthExec(client: OpaquePointer, event: UnsafePointer Int32 { if requestFD >= 0 { return requestFD } + requestFD = try openConnection(timeout: timeout) + return requestFD + } + + /// Queue for requestOnce. Concurrent, so that one slow request does not + /// hold up another. + private let onceQueue = DispatchQueue(label: "dev.diffsec.agentmon.policysocket.once", + attributes: .concurrent) + + /// Send one request on a new connection of its own, and wait up to + /// `timeout` for the answer. + /// + /// request() sends every request on one kept connection, one at a time, + /// with a 5 s read timeout. A package install check can take longer than + /// that, and on the kept connection it would hold up every other exec + /// check. completion gets nil when the request fails. + func requestOnce(_ request: [String: Any], timeout: TimeInterval, + completion: @escaping ([String: Any]?) -> Void) { + onceQueue.async { + var fd: Int32 = -1 + defer { if fd >= 0 { close(fd) } } + do { + fd = try self.openConnection(timeout: timeout) + completion(try self.exchange(request, on: fd)) + } catch { + NSLog("PolicySocketClient: one-off request failed: \(error)") + completion(nil) + } + } + } + /// Open a connection to the daemon and check the daemon's signature. + private func openConnection(timeout: TimeInterval) throws -> Int32 { let fd = socket(AF_UNIX, SOCK_STREAM, 0) guard fd >= 0 else { throw SocketError.creation } @@ -510,7 +542,9 @@ class PolicySocketClient { } // Timeouts - var tv = timeval(tv_sec: Int(timeout), tv_usec: 0) + let seconds = Int(timeout) + var tv = timeval(tv_sec: seconds, + tv_usec: Int32((timeout - Double(seconds)) * 1_000_000)) setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, socklen_t(MemoryLayout.size)) setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &tv, socklen_t(MemoryLayout.size)) @@ -519,7 +553,6 @@ class PolicySocketClient { var nosigpipe: Int32 = 1 setsockopt(fd, SOL_SOCKET, SO_NOSIGPIPE, &nosigpipe, socklen_t(MemoryLayout.size)) - requestFD = fd return fd } diff --git a/scripts/test-macos-package-checks.sh b/scripts/test-macos-package-checks.sh new file mode 100755 index 00000000..99e24b33 --- /dev/null +++ b/scripts/test-macos-package-checks.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash +# Hardware test: a package install inside a macOS session is checked, also +# when the agent starts it and not `agentmon exec`. +# +# Before build 1679, only the command that `agentmon exec` started was +# package-checked. An install that an agent ran under `agentmon wrap` was +# not. agent-default blocks a package with a critical vulnerability, and +# django 2.2 has CVE-2022-34265 (CVSS 9.8). +# +# Run it on a Mac where AgentMon.app is installed, its extension is enabled, +# and the daemon runs with `package_checks.enabled: true`. It needs internet +# access to pypi.org and api.osv.dev. Run it before each release. +set -euo pipefail + +A=${AGENTMON:-/Applications/AgentMon.app/Contents/MacOS/agentmon} +PY=${PYTHON:-} +if [ -z "$PY" ]; then + for p in /opt/homebrew/bin/python3 /usr/local/bin/python3; do + if [ -x "$p" ]; then PY=$p; break; fi + done +fi +[ -n "$PY" ] || { echo "no python3 found; set PYTHON" >&2; exit 2; } + +WS=$(mktemp -d "${TMPDIR:-/tmp}/ampkg.XXXXXX") +WS=$(cd "$WS" && pwd -P) +status=0 +trap 'rm -rf "$WS"' EXIT + +ok() { echo "ok $*"; } +fail() { echo "FAIL $*"; status=1; } + +# A pip configuration on this Mac can name another index, which +# agent-default does not allow. --isolated ignores it, and every install +# names pypi.org. +INDEX=https://pypi.org/simple + +# The venv is made outside the session. Homebrew's python refuses a plain +# pip install (PEP 668), so every install goes into the venv. +"$PY" -m venv "$WS/.venv" + +# install.py runs one install and prints "installed" or "refused". A denied +# exec ends the child with SIGKILL. +cat > "$WS/install.py" <<'PY' +import subprocess, sys +r = subprocess.run(sys.argv[1:], capture_output=True) +print("installed" if r.returncode == 0 else "refused rc=%d" % r.returncode) +PY + +wrap() { + (cd "$WS" && "$A" wrap --policy agent-default --report=false -- "$@" 2>>"$WS/err") +} + +expect() { + local want=$1 got=$2 what=$3 + case "$got" in + "$want"*) ok "$what: $got" ;; + *) fail "$what: got '$got', want '$want'" ;; + esac +} + +# pip3 is a script. ES reports the python3.15 interpreter as the executable, +# and the daemon must find pip in argv[1]. +got=$(wrap "$PY" "$WS/install.py" "$WS/.venv/bin/pip" --isolated install --no-cache-dir --index-url "$INDEX" "django==2.2" | tail -n 1) || true +expect refused "$got" "pip install django==2.2 (critical CVE) under wrap" + +got=$(wrap "$PY" "$WS/install.py" "$WS/.venv/bin/python" -m pip --isolated install --no-cache-dir --index-url "$INDEX" "django==2.2" | tail -n 1) || true +expect refused "$got" "python -m pip install django==2.2 under wrap" + +got=$(wrap "$PY" "$WS/install.py" "$WS/.venv/bin/pip" --isolated install --no-cache-dir --index-url "$INDEX" "six==1.16.0" | tail -n 1) || true +expect installed "$got" "pip install six==1.16.0 (no findings) under wrap" + +# The events search runs on compact JSON, with no space after a colon. +n=$("$A" events query --since 10m --type execve --text-like '%django%' 2>/dev/null \ + | grep -c '"rule": "package-check"' || true) +if [ "$n" -ge 2 ]; then ok "$n execve events from the package check"; else fail "$n execve events from the package check, want 2 or more"; fi + +if [ $status -ne 0 ] && [ -s "$WS/err" ]; then + echo "wrap stderr:" + sed 's/^/ /' "$WS/err" +fi +exit $status From c600aaa963b74e0cf71eaa4b8d7224aec7c0f7e5 Mon Sep 17 00:00:00 2001 From: Ben Gittins <5601410+ihavespoons@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:57:11 +1000 Subject: [PATCH 2/2] Make package checks see real severities and the session's rules Testing the macOS package check on hardware found four defects that stopped any package check from working as agent-default intends. They affect the exec API path as well. - The OSV provider called /v1/querybatch, which returns only the ID of each vulnerability. Every finding defaulted to medium, so a rule for critical vulnerabilities never matched. The provider now fetches each record from /v1/vulns/{id}, 16 at a time, and caches it. Django 2.2 now gives 6 critical findings. - The pip resolver ran without --quiet. pip 26 writes "Collecting ..." lines to stdout before the JSON report, so the report never parsed. - The checker used the package rules of the daemon's default policy for every session. CheckForPolicy now takes the session's own rules, on the exec API path and on the extension path. - agent-default had no rule for findings other than critical vulnerabilities and malware. A finding that no rule matches blocks, and the local provider reports "license unknown" for every package, so every install was blocked. agent-default now ends with an allow for every other finding. ComposeRules applies a policy's final catch-all after the block_on rules, so a stricter block_on still blocks. On build 1682, scripts/test-macos-package-checks.sh passes 4 of 4: an agent's `pip install django==2.2` and `python -m pip install django==2.2` under wrap are refused for the critical vulnerability, and `pip install six==1.16.0` installs. Co-Authored-By: Claude Opus 5.5 --- configs/policies/agent-default.yaml | 9 ++ internal/api/core.go | 9 +- internal/pkgcheck/checker.go | 61 +++++++++- internal/pkgcheck/checker_recent_test.go | 41 +++++++ internal/pkgcheck/compose_rules_test.go | 49 ++++++++ internal/pkgcheck/provider/osv.go | 115 +++++++++++++++++- internal/pkgcheck/provider/osv_test.go | 40 ++++++ internal/pkgcheck/resolver/pip.go | 7 +- .../darwin/policysock/handler_test.go | 2 +- .../darwin/policysock/package_check.go | 14 ++- internal/policy/agent_policies_test.go | 6 +- internal/server/server.go | 16 +-- scripts/test-macos-package-checks.sh | 30 ++++- 13 files changed, 365 insertions(+), 34 deletions(-) create mode 100644 internal/pkgcheck/compose_rules_test.go diff --git a/configs/policies/agent-default.yaml b/configs/policies/agent-default.yaml index 6b5e5dcf..b4470fce 100644 --- a/configs/policies/agent-default.yaml +++ b/configs/policies/agent-default.yaml @@ -843,6 +843,15 @@ package_rules: action: block reason: "Known malware detected" + # Every other finding is allowed. A finding that no rule matches blocks the + # install, and the local provider reports "license unknown" for every + # package, so without this rule every install was blocked. The daemon + # applies this last rule after its package_checks.block_on rules, so a + # stricter block_on still blocks. + - match: {} + action: allow + reason: "No critical vulnerability or malware" + # ============================================================================= # RESOURCE LIMITS # ============================================================================= diff --git a/internal/api/core.go b/internal/api/core.go index 39466259..268558e1 100644 --- a/internal/api/core.go +++ b/internal/api/core.go @@ -931,7 +931,14 @@ func (a *App) execInSessionCore(ctx context.Context, id string, req types.ExecRe // Package install check (after command policy check and approval, before event emission). if a.pkgChecker != nil && pre.EffectiveDecision != types.DecisionDeny { - verdict, pkgErr := a.pkgChecker.Check(ctx, req.Command, req.Args, req.WorkingDir) + // The session's own policy rules, as on the macOS exec path. + var verdict *pkgcheck.Verdict + var pkgErr error + if eng := s.PolicyEngine(); eng != nil { + verdict, pkgErr = a.pkgChecker.CheckForPolicy(ctx, eng.PackageRules(), req.Command, req.Args, req.WorkingDir) + } else { + verdict, pkgErr = a.pkgChecker.Check(ctx, req.Command, req.Args, req.WorkingDir) + } if pkgErr != nil { slog.Warn("package check error", "err", pkgErr) pre.EffectiveDecision = types.DecisionDeny diff --git a/internal/pkgcheck/checker.go b/internal/pkgcheck/checker.go index 9af4a56c..f9582fe1 100644 --- a/internal/pkgcheck/checker.go +++ b/internal/pkgcheck/checker.go @@ -16,7 +16,12 @@ type CheckerConfig struct { Scope string // "new_packages_only" | "all_installs" Resolvers []Resolver Providers map[string]ProviderEntry - Rules []policy.PackageRule + // Rules are the package rules of the daemon's default policy. Check uses + // them. CheckForPolicy takes a session's own rules instead. + Rules []policy.PackageRule + // BlockOn are the rules from package_checks.block_on. They apply after a + // policy's rules, except its final catch-all. See ComposeRules. + BlockOn []policy.PackageRule Allowlist *Allowlist // Privacy configures which packages are sent to external providers. // An empty PrivacyConfig (the zero value) means no filtering — all @@ -58,7 +63,7 @@ func NewChecker(cfg CheckerConfig) *Checker { Providers: cfg.Providers, PrivacyFilter: pf, }), - eval: NewEvaluator(cfg.Rules), + eval: NewEvaluator(ComposeRules(cfg.Rules, cfg.BlockOn)), recent: make(map[string]recentVerdict), now: time.Now, } @@ -71,17 +76,32 @@ func NewChecker(cfg CheckerConfig) *Checker { // directory is reused for recentTTL. An approve verdict is not reused, // because each one asks a person. func (c *Checker) Check(ctx context.Context, command string, args []string, workDir string) (*Verdict, error) { + return c.check(ctx, c.eval, "default", command, args, workDir) +} + +// CheckForPolicy is Check with a session policy's package rules in place of +// the default policy's. The block_on rules still apply. Before, every check +// used the daemon's default policy, so an agent-default session never got +// agent-default's package rules. +func (c *Checker) CheckForPolicy(ctx context.Context, rules []policy.PackageRule, command string, args []string, workDir string) (*Verdict, error) { + eval := NewEvaluator(ComposeRules(rules, c.cfg.BlockOn)) + return c.check(ctx, eval, fmt.Sprintf("%+v", rules), command, args, workDir) +} + +// check classifies the command and returns a recent verdict for the same +// install under the same rules, or checks it. rulesKey names the rules. +func (c *Checker) check(ctx context.Context, eval *Evaluator, rulesKey string, command string, args []string, workDir string) (*Verdict, error) { // `python -m pip install x` is a pip install. command, args = UnwrapInstallCommand(command, args) intent := ClassifyInstallCommand(command, args, c.cfg.Scope) if intent == nil { return nil, nil } - key := recentKey(intent, workDir) + key := rulesKey + "\x00" + recentKey(intent, workDir) if v, ok := c.recentVerdict(key); ok { return v, nil } - v, err := c.check(ctx, command, args, workDir, intent) + v, err := c.resolveAndEvaluate(ctx, eval, command, args, workDir, intent) if err == nil && v != nil && v.Action != VerdictApprove { c.mu.Lock() c.recent[key] = recentVerdict{verdict: *v, at: c.now()} @@ -119,7 +139,7 @@ func (c *Checker) recentVerdict(key string) (*Verdict, bool) { return &v, true } -func (c *Checker) check(ctx context.Context, command string, args []string, workDir string, intent *InstallIntent) (*Verdict, error) { +func (c *Checker) resolveAndEvaluate(ctx context.Context, eval *Evaluator, command string, args []string, workDir string, intent *InstallIntent) (*Verdict, error) { // 2. Find a resolver that can handle this tool. // Strip leading global flags so args[0] is the subcommand, // which is what resolvers' CanResolve expects. @@ -191,7 +211,7 @@ func (c *Checker) check(ctx context.Context, command string, args []string, work } // 6. Evaluate findings against policy rules. - verdict := c.eval.EvaluateWithContext(EvalContext{ + verdict := eval.EvaluateWithContext(EvalContext{ Findings: findings, Ecosystem: plan.Ecosystem, ProviderErrors: providerErrs, @@ -261,3 +281,32 @@ func buildCheckerSummary(intent *InstallIntent, plan *InstallPlan, verdict *Verd } return sb.String() } + +// ComposeRules puts a policy's package rules first, then the block_on rules. +// A policy's last rule that matches every finding moves to the end. +// Otherwise it would shadow every block_on rule: agent-default ends with an +// allow for every finding, and block_on.vulnerability=high must still block +// a high vulnerability. A policy without a catch-all keeps the evaluator's +// default deny for a finding that no rule matches. +func ComposeRules(policyRules, blockOn []policy.PackageRule) []policy.PackageRule { + out := make([]policy.PackageRule, 0, len(policyRules)+len(blockOn)) + var catchAll *policy.PackageRule + if n := len(policyRules); n > 0 && IsCatchAllRule(policyRules[n-1]) { + catchAll = &policyRules[n-1] + policyRules = policyRules[:n-1] + } + out = append(out, policyRules...) + out = append(out, blockOn...) + if catchAll != nil { + out = append(out, *catchAll) + } + return out +} + +// IsCatchAllRule reports whether a package rule matches every finding. +func IsCatchAllRule(r policy.PackageRule) bool { + m := r.Match + return m.FindingType == "" && m.Severity == "" && len(m.Packages) == 0 && + len(m.NamePatterns) == 0 && len(m.Reasons) == 0 && m.Ecosystem == "" && + m.LicenseSPDX == nil && len(m.Options) == 0 +} diff --git a/internal/pkgcheck/checker_recent_test.go b/internal/pkgcheck/checker_recent_test.go index a80da6a4..bc0e6085 100644 --- a/internal/pkgcheck/checker_recent_test.go +++ b/internal/pkgcheck/checker_recent_test.go @@ -4,6 +4,8 @@ import ( "context" "testing" "time" + + "github.com/diffsec/agentmon/internal/policy" ) // countingResolver resolves every pip install to one package and counts the @@ -77,3 +79,42 @@ func TestUnwrapInstallCommand(t *testing.T) { t.Errorf("pip3.15 install x: intent = %+v, want a pip install", intent) } } + +// findingResolver resolves to one package, and findingProvider reports a +// "license unknown" finding for it, as the local provider does. +type findingProvider struct{} + +func (findingProvider) Name() string { return "fake" } +func (findingProvider) Capabilities() []FindingType { return []FindingType{FindingLicense} } +func (findingProvider) CheckBatch(_ context.Context, req CheckRequest) (*CheckResponse, error) { + var out []Finding + for _, p := range req.Packages { + out = append(out, Finding{Type: FindingLicense, Severity: SeverityInfo, Package: p, Provider: "fake"}) + } + return &CheckResponse{Provider: "fake", Findings: out}, nil +} + +// TestCheckForPolicyUsesSessionRules: the default policy has no package +// rules, so a license finding blocks. A session policy that allows every +// other finding, as agent-default does, must allow the same install. +func TestCheckForPolicyUsesSessionRules(t *testing.T) { + c := NewChecker(CheckerConfig{ + Resolvers: []Resolver{&countingResolver{}}, + Providers: map[string]ProviderEntry{"fake": {Provider: findingProvider{}}}, + }) + ctx := context.Background() + dir := t.TempDir() + + v, err := c.Check(ctx, "pip3", []string{"install", "requests"}, dir) + if err != nil || v == nil || v.Action != VerdictBlock { + t.Fatalf("Check with the default rules = %+v, %v; want block", v, err) + } + session := []policy.PackageRule{ + {Match: policy.PackageMatch{FindingType: "malware"}, Action: "block"}, + {Match: policy.PackageMatch{}, Action: "allow"}, + } + v, err = c.CheckForPolicy(ctx, session, "pip3", []string{"install", "requests"}, dir) + if err != nil || v == nil || v.Action != VerdictAllow { + t.Errorf("CheckForPolicy with a catch-all allow = %+v, %v; want allow", v, err) + } +} diff --git a/internal/pkgcheck/compose_rules_test.go b/internal/pkgcheck/compose_rules_test.go new file mode 100644 index 00000000..b9e011c5 --- /dev/null +++ b/internal/pkgcheck/compose_rules_test.go @@ -0,0 +1,49 @@ +package pkgcheck + +import ( + "testing" + + "github.com/diffsec/agentmon/internal/policy" +) + +// TestComposeRules_CatchAllLast: agent-default ends with an allow for +// every finding. A stricter block_on must still block, so the catch-all moves +// after the block_on rules. +func TestComposeRules_CatchAllLast(t *testing.T) { + policyRules := []policy.PackageRule{ + {Match: policy.PackageMatch{FindingType: "malware"}, Action: "block"}, + {Match: policy.PackageMatch{}, Action: "allow"}, + } + // What config.CompileBlockOnRules gives for block_on.vulnerability=high. + blockOn := []policy.PackageRule{ + {Match: policy.PackageMatch{FindingType: "vulnerability", Severity: "critical"}, Action: "deny"}, + {Match: policy.PackageMatch{FindingType: "vulnerability", Severity: "high"}, Action: "deny"}, + } + rules := ComposeRules(policyRules, blockOn) + if last := rules[len(rules)-1]; last.Action != "allow" || !IsCatchAllRule(last) { + t.Fatalf("last rule = %+v, want the policy's catch-all allow", last) + } + + e := NewEvaluator(rules) + high := Finding{Type: FindingVulnerability, Severity: SeverityHigh, + Package: PackageRef{Name: "x"}} + if v := e.Evaluate([]Finding{high}, EcosystemPyPI); v.Action != VerdictBlock { + t.Errorf("high vulnerability with block_on.vulnerability=high: %s, want block", v.Action) + } + license := Finding{Type: FindingLicense, Severity: SeverityInfo, + Package: PackageRef{Name: "x"}} + if v := e.Evaluate([]Finding{license}, EcosystemPyPI); v.Action != VerdictAllow { + t.Errorf("license unknown: %s, want allow", v.Action) + } +} + +// TestComposeRules_NoCatchAll keeps a policy without a catch-all in +// order, so the evaluator's default deny still applies to it. +func TestComposeRules_NoCatchAll(t *testing.T) { + policyRules := []policy.PackageRule{{Match: policy.PackageMatch{FindingType: "malware"}, Action: "block"}} + blockOn := []policy.PackageRule{{Match: policy.PackageMatch{FindingType: "license"}, Action: "deny"}} + rules := ComposeRules(policyRules, blockOn) + if len(rules) != 2 || rules[0].Match.FindingType != "malware" || rules[1].Match.FindingType != "license" { + t.Errorf("rules = %+v", rules) + } +} diff --git a/internal/pkgcheck/provider/osv.go b/internal/pkgcheck/provider/osv.go index 01f989b3..4ddc1f70 100644 --- a/internal/pkgcheck/provider/osv.go +++ b/internal/pkgcheck/provider/osv.go @@ -7,8 +7,10 @@ import ( "fmt" "io" "net/http" + "net/url" "strconv" "strings" + "sync" "time" "github.com/diffsec/agentmon/internal/pkgcheck" @@ -29,8 +31,16 @@ type OSVConfig struct { type osvProvider struct { baseURL string client *http.Client + + // details caches full vulnerability records by ID. A record changes + // rarely, and one install can name the same IDs again and again. + mu sync.Mutex + details map[string]osvVuln } +// osvDetailFetches is how many vulnerability records are fetched at once. +const osvDetailFetches = 16 + // NewOSVProvider returns a CheckProvider that queries OSV.dev for vulnerabilities. func NewOSVProvider(cfg OSVConfig) pkgcheck.CheckProvider { baseURL := cfg.BaseURL @@ -44,6 +54,7 @@ func NewOSVProvider(cfg OSVConfig) pkgcheck.CheckProvider { return &osvProvider{ baseURL: strings.TrimRight(baseURL, "/"), client: &http.Client{Timeout: timeout}, + details: make(map[string]osvVuln), } } @@ -80,11 +91,11 @@ type osvResult struct { } type osvVuln struct { - ID string `json:"id"` - Summary string `json:"summary"` - Details string `json:"details"` - Severity []osvSeverity `json:"severity,omitempty"` - References []osvReference `json:"references,omitempty"` + ID string `json:"id"` + Summary string `json:"summary"` + Details string `json:"details"` + Severity []osvSeverity `json:"severity,omitempty"` + References []osvReference `json:"references,omitempty"` } type osvSeverity struct { @@ -152,6 +163,14 @@ func (p *osvProvider) CheckBatch(ctx context.Context, req pkgcheck.CheckRequest) iterCount = len(req.Packages) } + // /v1/querybatch returns only the ID and the modified time of each + // vulnerability, with no severity. Without the full records, every + // finding defaulted to medium, so a rule for critical vulnerabilities, + // such as agent-default's, never matched. + if !p.fillDetails(ctx, batchResp.Results[:iterCount]) { + partial = true + } + var findings []pkgcheck.Finding for i := 0; i < iterCount; i++ { result := batchResp.Results[i] @@ -190,6 +209,92 @@ func (p *osvProvider) CheckBatch(ctx context.Context, req pkgcheck.CheckRequest) }, nil } +// fillDetails replaces each vulnerability that has only an ID with its full +// record from /v1/vulns/{id}. It returns false when a record could not be +// fetched. That vulnerability then keeps no severity, and counts as medium. +func (p *osvProvider) fillDetails(ctx context.Context, results []osvResult) bool { + var missing []string + seen := map[string]bool{} + p.mu.Lock() + for _, r := range results { + for _, v := range r.Vulns { + if !needsDetails(v) || seen[v.ID] { + continue + } + seen[v.ID] = true + if _, ok := p.details[v.ID]; !ok { + missing = append(missing, v.ID) + } + } + } + p.mu.Unlock() + + ok := true + if len(missing) > 0 { + var wg sync.WaitGroup + var failMu sync.Mutex + sem := make(chan struct{}, osvDetailFetches) + for _, id := range missing { + wg.Add(1) + sem <- struct{}{} + go func(id string) { + defer wg.Done() + defer func() { <-sem }() + v, err := p.fetchVuln(ctx, id) + if err != nil { + failMu.Lock() + ok = false + failMu.Unlock() + return + } + p.mu.Lock() + p.details[id] = v + p.mu.Unlock() + }(id) + } + wg.Wait() + } + + p.mu.Lock() + defer p.mu.Unlock() + for i := range results { + for j, v := range results[i].Vulns { + if d, found := p.details[v.ID]; found && needsDetails(v) { + results[i].Vulns[j] = d + } + } + } + return ok +} + +// needsDetails reports whether a vulnerability holds only its ID, as the +// batch API returns it. +func needsDetails(v osvVuln) bool { + return v.ID != "" && len(v.Severity) == 0 && v.Summary == "" && v.Details == "" +} + +// fetchVuln gets one vulnerability record from /v1/vulns/{id}. +func (p *osvProvider) fetchVuln(ctx context.Context, id string) (osvVuln, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.baseURL+"/v1/vulns/"+url.PathEscape(id), nil) + if err != nil { + return osvVuln{}, err + } + resp, err := p.client.Do(req) + if err != nil { + return osvVuln{}, err + } + // The body is only read. A close error cannot change the record. + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return osvVuln{}, fmt.Errorf("osv: vulnerability %s: status %d", id, resp.StatusCode) + } + var v osvVuln + if err := json.NewDecoder(resp.Body).Decode(&v); err != nil { + return osvVuln{}, fmt.Errorf("osv: decode vulnerability %s: %w", id, err) + } + return v, nil +} + // mapEcosystemOSV converts our Ecosystem type to the OSV ecosystem string. func mapEcosystemOSV(eco pkgcheck.Ecosystem) string { switch eco { diff --git a/internal/pkgcheck/provider/osv_test.go b/internal/pkgcheck/provider/osv_test.go index 38101f8c..f32920f6 100644 --- a/internal/pkgcheck/provider/osv_test.go +++ b/internal/pkgcheck/provider/osv_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "sync/atomic" "testing" "github.com/diffsec/agentmon/internal/pkgcheck" @@ -223,3 +224,42 @@ func TestParseCVSSBaseScore(t *testing.T) { }) } } + +// TestOSVProvider_FetchesSeverity: /v1/querybatch returns only the ID of each +// vulnerability. The provider must fetch the record to learn the severity. +// Before, every OSV finding was medium, so no critical rule ever matched. +func TestOSVProvider_FetchesSeverity(t *testing.T) { + var detailCalls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v1/querybatch": + _, _ = w.Write([]byte(`{"results":[{"vulns":[{"id":"GHSA-crit","modified":"2024-01-01T00:00:00Z"},{"id":"GHSA-gone","modified":"2024-01-01T00:00:00Z"}]}]}`)) + case "/v1/vulns/GHSA-crit": + detailCalls.Add(1) + _, _ = w.Write([]byte(`{"id":"GHSA-crit","summary":"SQL injection","severity":[{"type":"CVSS_V3","score":"CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H"}]}`)) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + p := NewOSVProvider(OSVConfig{BaseURL: server.URL}) + req := pkgcheck.CheckRequest{ + Ecosystem: pkgcheck.EcosystemPyPI, + Packages: []pkgcheck.PackageRef{{Name: "django", Version: "2.2"}}, + } + resp, err := p.CheckBatch(context.Background(), req) + require.NoError(t, err) + require.Len(t, resp.Findings, 2) + assert.Equal(t, pkgcheck.SeverityCritical, resp.Findings[0].Severity) + assert.Equal(t, "SQL injection", resp.Findings[0].Title) + // A record that could not be fetched keeps the medium default, and the + // response is marked partial. + assert.Equal(t, pkgcheck.SeverityMedium, resp.Findings[1].Severity) + assert.True(t, resp.Metadata.Partial) + + // The second check of the same package reuses the cached record. + _, err = p.CheckBatch(context.Background(), req) + require.NoError(t, err) + assert.Equal(t, int32(1), detailCalls.Load()) +} diff --git a/internal/pkgcheck/resolver/pip.go b/internal/pkgcheck/resolver/pip.go index 6b1832b3..f893b55a 100644 --- a/internal/pkgcheck/resolver/pip.go +++ b/internal/pkgcheck/resolver/pip.go @@ -69,11 +69,14 @@ func (r *pipResolver) Resolve(ctx context.Context, workDir string, command []str ctx, cancel := context.WithTimeout(ctx, r.cfg.Timeout) defer cancel() - // pip install --dry-run --report - [--index-url ] [--extra-index-url ] + // pip install --quiet --dry-run --report - [--index-url ] [--extra-index-url ] // Forward any --index-url / -i / --extra-index-url flags from the original // command so the dry-run resolves against the same index the actual install // will use. - cmdArgs := []string{"install", "--dry-run", "--report", "-"} + // + // --quiet is required. Without it, pip 26 writes "Collecting ..." lines + // to stdout before the JSON report, and the report never parses. + cmdArgs := []string{"install", "--quiet", "--dry-run", "--report", "-"} cmdArgs = append(cmdArgs, r.extractIndexURLFlags(command)...) cmdArgs = append(cmdArgs, packages...) allArgs := append(append([]string(nil), r.prefixArgs...), cmdArgs...) diff --git a/internal/platform/darwin/policysock/handler_test.go b/internal/platform/darwin/policysock/handler_test.go index 8c8f0209..9e04048a 100644 --- a/internal/platform/darwin/policysock/handler_test.go +++ b/internal/platform/darwin/policysock/handler_test.go @@ -556,7 +556,7 @@ type fakePackageChecker struct { deadline time.Duration } -func (f *fakePackageChecker) Check(ctx context.Context, command string, args []string, workDir string) (*pkgcheck.Verdict, error) { +func (f *fakePackageChecker) CheckForPolicy(ctx context.Context, _ []policy.PackageRule, command string, args []string, workDir string) (*pkgcheck.Verdict, error) { f.command, f.args, f.workDir = command, args, workDir if d, ok := ctx.Deadline(); ok { f.deadline = time.Until(d) diff --git a/internal/platform/darwin/policysock/package_check.go b/internal/platform/darwin/policysock/package_check.go index 855f3685..1e86b979 100644 --- a/internal/platform/darwin/policysock/package_check.go +++ b/internal/platform/darwin/policysock/package_check.go @@ -11,12 +11,14 @@ import ( "github.com/diffsec/agentmon/internal/approvals" "github.com/diffsec/agentmon/internal/pkgcheck" + "github.com/diffsec/agentmon/internal/policy" ) -// PackageChecker checks a package install. *pkgcheck.Checker implements it. -// It returns a nil verdict for a command that is not an install. +// PackageChecker checks a package install against a session policy's +// package rules. *pkgcheck.Checker implements it. It returns a nil verdict +// for a command that is not an install. type PackageChecker interface { - Check(ctx context.Context, command string, args []string, workDir string) (*pkgcheck.Verdict, error) + CheckForPolicy(ctx context.Context, rules []policy.PackageRule, command string, args []string, workDir string) (*pkgcheck.Verdict, error) } // SetPackageChecker makes CheckExec check package installs, as the exec API @@ -75,7 +77,11 @@ func (a *PolicyAdapter) checkPackageInstall(sessionID, executable string, args [ ctx, cancel := context.WithTimeout(context.Background(), budget) defer cancel() - verdict, err := a.packages.Check(ctx, command, cmdArgs, ec.CWDPath) + var rules []policy.PackageRule + if engine := a.engineFor(sessionID); engine != nil { + rules = engine.PackageRules() + } + verdict, err := a.packages.CheckForPolicy(ctx, rules, command, cmdArgs, ec.CWDPath) if err != nil { // The exec API denies on the same error (api/core.go). slog.Warn("policy socket: package check failed, denying the exec", diff --git a/internal/policy/agent_policies_test.go b/internal/policy/agent_policies_test.go index 9cd33ffa..b8f4654f 100644 --- a/internal/policy/agent_policies_test.go +++ b/internal/policy/agent_policies_test.go @@ -201,12 +201,16 @@ func TestAgentPolicies_DefaultRuleDetails(t *testing.T) { assert.Contains(t, p.EnvPolicy.Deny, "*_SECRET*") // --- Package rules --- - require.Len(t, p.PackageRules, 2) + require.Len(t, p.PackageRules, 3) assert.Equal(t, "block", p.PackageRules[0].Action) assert.Equal(t, "vulnerability", p.PackageRules[0].Match.FindingType) assert.Equal(t, "critical", p.PackageRules[0].Match.Severity) assert.Equal(t, "block", p.PackageRules[1].Action) assert.Equal(t, "malware", p.PackageRules[1].Match.FindingType) + // Every other finding is allowed. Without this rule, the local + // provider's "license unknown" finding blocked every install. + assert.Equal(t, "allow", p.PackageRules[2].Action) + assert.Equal(t, PackageMatch{}, p.PackageRules[2].Match) // --- Resource limits --- assert.Equal(t, 8192, p.ResourceLimits.MaxMemoryMB) diff --git a/internal/server/server.go b/internal/server/server.go index 205f5529..e0b5748e 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -703,18 +703,18 @@ func New(cfg *config.Config) (*Server, error) { return nil, fmt.Errorf("package_checks.enabled=true but no providers were initialized — at least one provider must be configured and have its API key (if required) set") } - // Compose rules: engine.PackageRules() takes precedence, then block_on - // shorthand fills in the remaining finding types. We use the no-catch-all - // variant here so that an operator's policy file can deliberately omit - // a catch-all rule and rely on the evaluator's default-deny. - rules := append([]policy.PackageRule(nil), engine.PackageRules()...) - rules = append(rules, config.CompileBlockOnRules(cfg.PackageChecks.BlockOn)...) - + // Rules: the default policy's package rules, then the block_on + // shorthand (pkgcheck.ComposeRules). block_on uses the no-catch-all + // variant, so that an operator's policy file can deliberately omit a + // catch-all rule and rely on the evaluator's default-deny. A check + // for a session uses that session's policy rules instead + // (Checker.CheckForPolicy). pkgChecker = pkgcheck.NewChecker(pkgcheck.CheckerConfig{ Scope: cfg.PackageChecks.Scope, Resolvers: resolvers, Providers: providerEntries, - Rules: rules, + Rules: engine.PackageRules(), + BlockOn: config.CompileBlockOnRules(cfg.PackageChecks.BlockOn), Allowlist: pkgcheck.NewAllowlist(30 * time.Second), Privacy: pkgcheck.PrivacyConfig{ ExternalScanRegistries: cfg.PackageChecks.Privacy.ExternalScanRegistries, diff --git a/scripts/test-macos-package-checks.sh b/scripts/test-macos-package-checks.sh index 99e24b33..5728249d 100755 --- a/scripts/test-macos-package-checks.sh +++ b/scripts/test-macos-package-checks.sh @@ -7,9 +7,18 @@ # not. agent-default blocks a package with a critical vulnerability, and # django 2.2 has CVE-2022-34265 (CVSS 9.8). # -# Run it on a Mac where AgentMon.app is installed, its extension is enabled, -# and the daemon runs with `package_checks.enabled: true`. It needs internet -# access to pypi.org and api.osv.dev. Run it before each release. +# Run it on a Mac where AgentMon.app is installed and its extension is +# enabled. It needs internet access to pypi.org and api.osv.dev. Run it +# before each release. The daemon configuration needs: +# +# package_checks: +# enabled: true +# resolvers: +# pip: +# dry_run_command: /path/to/a/venv/bin/pip +# +# launchd starts the daemon with no Homebrew directory in PATH, so the +# default resolver command "pip" is not found, and every check fails. set -euo pipefail A=${AGENTMON:-/Applications/AgentMon.app/Contents/MacOS/agentmon} @@ -21,6 +30,7 @@ if [ -z "$PY" ]; then fi [ -n "$PY" ] || { echo "no python3 found; set PYTHON" >&2; exit 2; } +START=$(date -u +%Y-%m-%dT%H:%M:%SZ) WS=$(mktemp -d "${TMPDIR:-/tmp}/ampkg.XXXXXX") WS=$(cd "$WS" && pwd -P) status=0 @@ -70,9 +80,17 @@ got=$(wrap "$PY" "$WS/install.py" "$WS/.venv/bin/pip" --isolated install --no-ca expect installed "$got" "pip install six==1.16.0 (no findings) under wrap" # The events search runs on compact JSON, with no space after a colon. -n=$("$A" events query --since 10m --type execve --text-like '%django%' 2>/dev/null \ - | grep -c '"rule": "package-check"' || true) -if [ "$n" -ge 2 ]; then ok "$n execve events from the package check"; else fail "$n execve events from the package check, want 2 or more"; fi +# A failed check also denies, so the reason must name the vulnerability. +# The events of this run are the ones newer than the start of the script. +events=$("$A" events query --since "$START" --type execve --text-like '%django%' 2>/dev/null || true) +n=$(printf '%s\n' "$events" | grep -c '"rule": "package-check"' || true) +failed=$(printf '%s\n' "$events" | grep -c 'package check failed' || true) +if [ "$n" -ge 2 ] && [ "$failed" -eq 0 ]; then + ok "$n execve events from the package check, none from a failed check" +else + fail "$n execve events from the package check, $failed from a failed check; want 2 or more, and 0" + printf '%s\n' "$events" | grep '"message"' | sed 's/^/ /' +fi if [ $status -ne 0 ] && [ -s "$WS/err" ]; then echo "wrap stderr:"