From a23717890d305eccbfc66d66071f790e88868833 Mon Sep 17 00:00:00 2001 From: abhisek Date: Sat, 25 Jul 2026 18:34:09 +0530 Subject: [PATCH] fix: Safe rendering of sandbox violation messages --- cmd/sandbox/violations_list.go | 17 ++-- cmd/sandbox/violations_list_test.go | 12 +++ internal/ui/sandbox_violation.go | 37 ++++++-- internal/ui/sandbox_violation_test.go | 34 +++++++ sandbox/all_overrides_test.go | 7 +- .../platform/landlock_diagnostics_linux.go | 54 ++++++----- .../landlock_diagnostics_linux_test.go | 24 ++++- .../platform/landlock_helper_linux_test.go | 3 +- sandbox/platform/landlock_seccomp_linux.go | 93 ++++++++++++------- .../platform/landlock_seccomp_linux_test.go | 16 ++++ sandbox/platform/landlock_translator_linux.go | 12 +-- .../landlock_translator_linux_test.go | 23 ++++- sandbox/platform/probe_apparmor_linux.go | 37 +++++++- sandbox/platform/probe_apparmor_linux_test.go | 29 +++++- sandbox/violation.go | 43 ++++++++- sandbox/violation_test.go | 39 +++++++- 16 files changed, 381 insertions(+), 99 deletions(-) diff --git a/cmd/sandbox/violations_list.go b/cmd/sandbox/violations_list.go index 1a432fb5..3d5bea4f 100644 --- a/cmd/sandbox/violations_list.go +++ b/cmd/sandbox/violations_list.go @@ -113,22 +113,21 @@ func renderViolationsListTable(out io.Writer, entries []pmgsandbox.ViolationCach sandboxName := "" profile := "" + var primary *pmgsandbox.Violation if e.Record.Report != nil { sandboxName = string(e.Record.Report.SandboxName) profile = e.Record.Report.PolicyName + primary = ui.SandboxViolationForTerminal(pmgsandbox.BuildExplanation(e.Record.Report).Primary) } kind := dash target := dash - if e.Record.Report != nil { - primary := pmgsandbox.BuildExplanation(e.Record.Report).Primary - if primary != nil { - if primary.Kind != "" { - kind = string(primary.Kind) - } - if primary.Target != "" { - target = truncate(primary.Target, 60) - } + if primary != nil { + if primary.Kind != "" { + kind = string(primary.Kind) + } + if primary.Target != "" { + target = truncate(primary.Target, 60) } } diff --git a/cmd/sandbox/violations_list_test.go b/cmd/sandbox/violations_list_test.go index 836597ad..5914d458 100644 --- a/cmd/sandbox/violations_list_test.go +++ b/cmd/sandbox/violations_list_test.go @@ -75,6 +75,18 @@ func TestViolationsList_TwoEntriesNewestFirst(t *testing.T) { assert.Less(t, newerIdx, olderIdx, "newest entry should appear first") } +func TestViolationsListEscapesAuditControls(t *testing.T) { + factory, cache := newTestCacheFactory(t) + + _, err := cache.Write(sampleViolationsReport("/tmp/bad\npath\x1b[2J")) + require.NoError(t, err) + + stdout, _, err := runList(t, factory) + require.NoError(t, err) + assert.Contains(t, stdout, `bad\npath\x1b[2J`) + assert.NotContains(t, stdout, "\x1b") +} + func TestViolationsList_LimitOne(t *testing.T) { factory, cache := newTestCacheFactory(t) diff --git a/internal/ui/sandbox_violation.go b/internal/ui/sandbox_violation.go index 2e3c15d5..1f93557b 100644 --- a/internal/ui/sandbox_violation.go +++ b/internal/ui/sandbox_violation.go @@ -3,6 +3,7 @@ package ui import ( "fmt" "io" + "strconv" "strings" "time" @@ -97,6 +98,7 @@ func RenderSandboxViolation(out io.Writer, rec *pmgsandbox.ViolationCacheRecord) } exp := pmgsandbox.BuildExplanation(rec.Report) + primary := SandboxViolationForTerminal(exp.Primary) recordedAt := "" if !rec.RecordedAt.IsZero() { @@ -121,7 +123,7 @@ func RenderSandboxViolation(out io.Writer, rec *pmgsandbox.ViolationCacheRecord) return err } - hint := FormatSandboxHint(exp.Primary, exp.Override) + hint := FormatSandboxHint(primary, exp.Override) if hint != "" { if _, err := fmt.Fprintln(out, hint); err != nil { return err @@ -131,7 +133,7 @@ func RenderSandboxViolation(out io.Writer, rec *pmgsandbox.ViolationCacheRecord) } } - details := FormatSandboxDetails(rec.Report, exp.Primary) + details := FormatSandboxDetails(rec.Report, primary) if details != "" { if _, err := fmt.Fprintln(out, Colors.Bold("Details:")); err != nil { return err @@ -168,21 +170,21 @@ func RenderSandboxViolation(out io.Writer, rec *pmgsandbox.ViolationCacheRecord) } } - if exp.Primary != nil { + if primary != nil { if _, err := fmt.Fprintln(out, Colors.Bold("Primary violation:")); err != nil { return err } - if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Kind:"), string(exp.Primary.Kind)); err != nil { + if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Kind:"), string(primary.Kind)); err != nil { return err } - if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Target:"), exp.Primary.Target); err != nil { + if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Target:"), primary.Target); err != nil { return err } - if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Rule:"), exp.Primary.RuleLabel); err != nil { + if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Rule:"), primary.RuleLabel); err != nil { return err } - if exp.Primary.Process != "" { - if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Process:"), exp.Primary.Process); err != nil { + if primary.Process != "" { + if _, err := fmt.Fprintf(out, " %s %s\n", Colors.Dim("Process:"), primary.Process); err != nil { return err } } @@ -191,6 +193,25 @@ func RenderSandboxViolation(out io.Writer, rec *pmgsandbox.ViolationCacheRecord) return nil } +// SandboxViolationForTerminal returns a display-only copy with audit-controlled +// strings escaped. Structured JSON output should continue using the original. +func SandboxViolationForTerminal(violation *pmgsandbox.Violation) *pmgsandbox.Violation { + if violation == nil { + return nil + } + + safe := *violation + safe.Target = quoteTerminalText(safe.Target) + safe.Process = quoteTerminalText(safe.Process) + safe.RuleLabel = quoteTerminalText(safe.RuleLabel) + return &safe +} + +func quoteTerminalText(value string) string { + quoted := strconv.QuoteToGraphic(value) + return quoted[1 : len(quoted)-1] +} + // shellQuote wraps value in single quotes, escaping any embedded single // quotes. Used so suggested override flags can be copy-pasted into a POSIX // shell verbatim regardless of spaces or quotes in the target. diff --git a/internal/ui/sandbox_violation_test.go b/internal/ui/sandbox_violation_test.go index f944d0d5..dde6957d 100644 --- a/internal/ui/sandbox_violation_test.go +++ b/internal/ui/sandbox_violation_test.go @@ -48,6 +48,16 @@ func TestFormatSandboxHintEmpty(t *testing.T) { assert.Equal(t, "Reason: sandbox denied an operation", FormatSandboxHint(nil, nil)) } +func TestSandboxViolationForTerminalEscapesControlCharacters(t *testing.T) { + violation := &pmgsandbox.Violation{ + Target: "bad\npath\x1b[2J\tend\x7f", + } + + safe := SandboxViolationForTerminal(violation) + assert.Equal(t, `bad\npath\x1b[2J\tend\x7f`, safe.Target) + assert.Equal(t, "bad\npath\x1b[2J\tend\x7f", violation.Target) +} + func TestFormatSandboxHintIncludesOverride(t *testing.T) { primary := &pmgsandbox.Violation{ Kind: pmgsandbox.ViolationKindFSRead, @@ -60,6 +70,30 @@ func TestFormatSandboxHintIncludesOverride(t *testing.T) { assert.Contains(t, hint, "Override: --sandbox-allow read='./.env'") } +func TestRenderSandboxViolationEscapesAuditControls(t *testing.T) { + rec := &pmgsandbox.ViolationCacheRecord{ + SchemaVersion: pmgsandbox.ViolationCacheSchemaVersion, + Report: &pmgsandbox.ViolationReport{ + SandboxName: pmgsandbox.DriverLandlock, + PolicyName: "test", + Violations: []pmgsandbox.Violation{{ + Kind: pmgsandbox.ViolationKindFSRead, + Target: "/tmp/bad\npath\x1b[2J", + RuleTarget: "/tmp/rule", + RuleLabel: "read access denied: /tmp/bad\npath\x1b[2J", + Process: "bad\nprocess", + }}, + }, + } + + var buf bytes.Buffer + require.NoError(t, RenderSandboxViolation(&buf, rec)) + out := buf.String() + assert.Contains(t, out, `bad\npath\x1b[2J`) + assert.Contains(t, out, `bad\nprocess`) + assert.NotContains(t, out, "\x1b") +} + func TestFormatSandboxDetailsIncludesMatchedRule(t *testing.T) { report := &pmgsandbox.ViolationReport{ SandboxName: "seatbelt", diff --git a/sandbox/all_overrides_test.go b/sandbox/all_overrides_test.go index 161763a6..6c58d471 100644 --- a/sandbox/all_overrides_test.go +++ b/sandbox/all_overrides_test.go @@ -11,9 +11,10 @@ func TestBuildAllOverridesMapsAllSafeFSAndExec(t *testing.T) { Violations: []Violation{ {Kind: ViolationKindFSWrite, Target: "/repo/.astro"}, {Kind: ViolationKindExec, Target: "/usr/bin/sh"}, - {Kind: ViolationKindFSWrite, Target: "/repo/.astro"}, // duplicate collapsed - {Kind: ViolationKindGenericDeny, Target: "/something/else"}, // unsupported kind dropped - {Kind: ViolationKindFSRead, Target: "**/*.env"}, // unsafe target dropped + {Kind: ViolationKindFSWrite, Target: "/repo/.astro"}, // duplicate collapsed + {Kind: ViolationKindGenericDeny, Target: "/something/else"}, // unsupported kind dropped + {Kind: ViolationKindFSRead, Target: "**/*.env"}, // unsafe target dropped + {Kind: ViolationKindFSRead, Target: "/repo/.env", RuleTarget: "**/.env"}, }, } got := BuildAllOverrides(report) diff --git a/sandbox/platform/landlock_diagnostics_linux.go b/sandbox/platform/landlock_diagnostics_linux.go index b2acd8e8..394ceda4 100644 --- a/sandbox/platform/landlock_diagnostics_linux.go +++ b/sandbox/platform/landlock_diagnostics_linux.go @@ -132,23 +132,23 @@ func extractLandlockViolations(events []capturedAuditEvent) []sandbox.Violation continue } - key := landlockDenyKey(e.auditEvent) - if seen[key] { - continue + for _, kind := range landlockViolationKinds(e.auditEvent) { + key := string(kind) + "\x00" + e.Path + if seen[key] { + continue + } + seen[key] = true + + violations = append(violations, sandbox.Violation{ + Kind: kind, + RawKind: e.Syscall, + Target: e.Path, + RuleTarget: e.RulePath, + Process: e.Comm, + RawLog: e.raw, + RuleLabel: summarizeLandlockViolation(kind, e.Path), + }) } - seen[key] = true - - kind := landlockViolationKind(e.auditEvent) - - violations = append(violations, sandbox.Violation{ - Kind: kind, - RawKind: e.Syscall, - Target: e.Path, - RuleTarget: e.RulePath, - Process: e.Comm, - RawLog: e.raw, - RuleLabel: summarizeLandlockViolation(kind, e.Path), - }) } return violations @@ -159,20 +159,30 @@ func extractLandlockViolations(events []capturedAuditEvent) []sandbox.Violation // (captureAuditEvents) and extract-time dedupe (extractLandlockViolations) // must agree on this identity, so both use this function. func landlockDenyKey(e auditEvent) string { - return string(landlockViolationKind(e)) + "\x00" + e.Path + key := "" + for _, kind := range landlockViolationKinds(e) { + key += string(kind) + "\x00" + } + return key + e.Path } -func landlockViolationKind(e auditEvent) sandbox.ViolationKind { +func landlockViolationKinds(e auditEvent) []sandbox.ViolationKind { switch e.Syscall { case "execve", "execveat": - return sandbox.ViolationKindExec + return []sandbox.ViolationKind{sandbox.ViolationKindExec} case "openat", "openat2": if e.Access == "read" { - return sandbox.ViolationKindFSRead + return []sandbox.ViolationKind{sandbox.ViolationKindFSRead} + } + if e.Access == "read_write" { + return []sandbox.ViolationKind{ + sandbox.ViolationKindFSRead, + sandbox.ViolationKindFSWrite, + } } - return sandbox.ViolationKindFSWrite + return []sandbox.ViolationKind{sandbox.ViolationKindFSWrite} default: - return sandbox.ViolationKindGenericDeny + return []sandbox.ViolationKind{sandbox.ViolationKindGenericDeny} } } diff --git a/sandbox/platform/landlock_diagnostics_linux_test.go b/sandbox/platform/landlock_diagnostics_linux_test.go index 93e1abd6..de79cd7f 100644 --- a/sandbox/platform/landlock_diagnostics_linux_test.go +++ b/sandbox/platform/landlock_diagnostics_linux_test.go @@ -169,6 +169,28 @@ func TestExtractLandlockViolations(t *testing.T) { }, }, }, + { + name: "read write denial maps to both permissions", + events: []capturedAuditEvent{denyEvent("openat", "/home/dev/.npmrc", "read_write", "node")}, + want: []sandbox.Violation{ + { + Kind: sandbox.ViolationKindFSRead, + RawKind: "openat", + Target: "/home/dev/.npmrc", + Process: "node", + RawLog: `{"type":"seccomp_deny"}`, + RuleLabel: "read access denied: /home/dev/.npmrc", + }, + { + Kind: sandbox.ViolationKindFSWrite, + RawKind: "openat", + Target: "/home/dev/.npmrc", + Process: "node", + RawLog: `{"type":"seccomp_deny"}`, + RuleLabel: "write access denied: /home/dev/.npmrc", + }, + }, + }, } for _, tc := range tests { @@ -256,7 +278,7 @@ func TestDenyAccessLabelReportsFiredRule(t *testing.T) { assert.Equal(t, "write", denyAccessLabel(denyWrite, unix.O_RDWR)) assert.Equal(t, "read", denyAccessLabel(denyBoth, unix.O_RDONLY)) - assert.Equal(t, "write", denyAccessLabel(denyBoth, unix.O_RDWR)) + assert.Equal(t, "read_write", denyAccessLabel(denyBoth, unix.O_RDWR)) } func TestBestEffortViolationNilOnSuccess(t *testing.T) { diff --git a/sandbox/platform/landlock_helper_linux_test.go b/sandbox/platform/landlock_helper_linux_test.go index e98182a3..ff8bf710 100644 --- a/sandbox/platform/landlock_helper_linux_test.go +++ b/sandbox/platform/landlock_helper_linux_test.go @@ -22,7 +22,7 @@ func TestReadLandlockPolicyFromFile(t *testing.T) { {Path: "/tmp", Access: 0xFF}, }, DenyPaths: []denyPathEntry{ - {Path: "/home/user/.ssh/", Mode: denyBoth}, + {Path: "/home/user/.ssh/", RulePath: "$HOME/.ssh/**", Mode: denyBoth}, }, DenyExecPaths: []string{"/usr/bin/curl"}, AllowPTY: true, @@ -54,6 +54,7 @@ func TestReadLandlockPolicyFromFile(t *testing.T) { assert.Equal(t, uint64(0x0C), got.FilesystemRules[0].Access) assert.Len(t, got.DenyPaths, 1) assert.Equal(t, "/home/user/.ssh/", got.DenyPaths[0].Path) + assert.Equal(t, "$HOME/.ssh/**", got.DenyPaths[0].RulePath) assert.Equal(t, denyBoth, got.DenyPaths[0].Mode) assert.Equal(t, []string{"/usr/bin/curl"}, got.DenyExecPaths) assert.True(t, got.AllowPTY) diff --git a/sandbox/platform/landlock_seccomp_linux.go b/sandbox/platform/landlock_seccomp_linux.go index 4df86271..6e32aefb 100644 --- a/sandbox/platform/landlock_seccomp_linux.go +++ b/sandbox/platform/landlock_seccomp_linux.go @@ -76,8 +76,9 @@ const ( // denyPathEntry pairs a filesystem path with the access mode to deny. type denyPathEntry struct { - Path string - Mode denyMode + Path string + RulePath string `json:"rule_path,omitempty"` + Mode denyMode } // auditEventType categorizes security audit events. @@ -156,36 +157,52 @@ func landlockBuildBPFFilter() (*unix.SockFprog, error) { // deny entry without slash is treated as "this path OR anything beneath it") // - Must NOT match partial names: /home/.envrc does NOT match deny /home/.env func matchDeniedPath(path string, flags int, denyPaths []denyPathEntry) (denyPathEntry, bool) { + matches := matchingDeniedPaths(path, flags, denyPaths) + if len(matches) == 0 { + return denyPathEntry{}, false + } + return matches[0], true +} + +func matchingDeniedPaths(path string, flags int, denyPaths []denyPathEntry) []denyPathEntry { accessMode := flags & unix.O_ACCMODE + var readEntry, writeEntry *denyPathEntry for _, entry := range denyPaths { - matched := false - if strings.HasSuffix(entry.Path, "/") { - // Directory prefix match: path must start with the deny prefix. - matched = strings.HasPrefix(path, entry.Path) - } else { - // Exact match OR any path under this entry as a directory. - matched = path == entry.Path || strings.HasPrefix(path, entry.Path+"/") - } - - if !matched { + if !matchesDeniedPath(path, entry.Path) { continue } switch entry.Mode { case denyRead: - if accessMode == unix.O_RDONLY || accessMode == unix.O_RDWR { - return entry, true + if (accessMode == unix.O_RDONLY || accessMode == unix.O_RDWR) && readEntry == nil { + e := entry + readEntry = &e } case denyWrite: - if accessMode == unix.O_WRONLY || accessMode == unix.O_RDWR { - return entry, true + if (accessMode == unix.O_WRONLY || accessMode == unix.O_RDWR) && writeEntry == nil { + e := entry + writeEntry = &e } case denyBoth: - return entry, true + return []denyPathEntry{entry} } } - return denyPathEntry{}, false + matches := make([]denyPathEntry, 0, 2) + if readEntry != nil { + matches = append(matches, *readEntry) + } + if writeEntry != nil { + matches = append(matches, *writeEntry) + } + return matches +} + +func matchesDeniedPath(path, rulePath string) bool { + if strings.HasSuffix(rulePath, "/") { + return strings.HasPrefix(path, rulePath) + } + return path == rulePath || strings.HasPrefix(path, rulePath+"/") } // isPathDenied reports whether matchDeniedPath finds a deny entry for path. @@ -580,18 +597,25 @@ func (s *seccompSupervisor) handleOpen(notif *seccompNotification, phase *seccom flags := classifyOpenFlags(notif.Data.Nr, notif.Data.Args, memFd) - if entry, denied := matchDeniedPath(resolved, flags, phase.denyPaths); denied { + if entries := matchingDeniedPaths(resolved, flags, phase.denyPaths); len(entries) > 0 { if phase.auditWriter != nil { - _ = landlockWriteAuditEvent(phase.auditWriter, auditEvent{ - Type: auditSeccompDeny, - Syscall: syscallName(notif.Data.Nr), - Path: resolved, - Access: denyAccessLabel(entry.Mode, flags), - RulePath: entry.Path, - Comm: procComm(notif.PID), - PID: int(notif.PID), - Ts: time.Now().UnixNano(), - }) + comm := procComm(notif.PID) + for _, entry := range entries { + rulePath := entry.RulePath + if rulePath == "" { + rulePath = entry.Path + } + _ = landlockWriteAuditEvent(phase.auditWriter, auditEvent{ + Type: auditSeccompDeny, + Syscall: syscallName(notif.Data.Nr), + Path: resolved, + Access: denyAccessLabel(entry.Mode, flags), + RulePath: rulePath, + Comm: comm, + PID: int(notif.PID), + Ts: time.Now().UnixNano(), + }) + } } _ = respondDeny(s.notifyFd, notif.ID) return @@ -617,7 +641,6 @@ func syscallName(nr int32) string { } // accessModeString maps an O_ACCMODE value to the audit event access label. -// O_RDWR counts as write: the denial applies to the stronger access. func accessModeString(flags int) string { if flags&unix.O_ACCMODE == unix.O_RDONLY { return "read" @@ -626,15 +649,19 @@ func accessModeString(flags int) string { } // denyAccessLabel reports the direction of the deny rule that fired, not the -// requested access. An O_RDWR open denied by a read-only rule must surface as -// a read denial: the write override prunes only deny_write entries, so only a -// read allowance unblocks it. denyBoth falls back to the requested access. +// requested access. An O_RDWR open denied in both directions reports both +// permissions so diagnostics do not advertise a single ineffective override. func denyAccessLabel(mode denyMode, flags int) string { switch mode { case denyRead: return "read" case denyWrite: return "write" + case denyBoth: + if flags&unix.O_ACCMODE == unix.O_RDWR { + return "read_write" + } + return accessModeString(flags) default: return accessModeString(flags) } diff --git a/sandbox/platform/landlock_seccomp_linux_test.go b/sandbox/platform/landlock_seccomp_linux_test.go index 1ffcc3d4..f65109c8 100644 --- a/sandbox/platform/landlock_seccomp_linux_test.go +++ b/sandbox/platform/landlock_seccomp_linux_test.go @@ -10,6 +10,8 @@ import ( "testing" "unsafe" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "golang.org/x/sys/unix" ) @@ -255,6 +257,20 @@ func TestIsPathDenied_DenyBoth(t *testing.T) { } } +func TestMatchingDeniedPathsReportsBothDirectionsForReadWrite(t *testing.T) { + deny := []denyPathEntry{ + {Path: "/home/user/.env", RulePath: "*/.env", Mode: denyRead}, + {Path: "/home/user/.env", RulePath: "/home/user/.env", Mode: denyWrite}, + } + + matches := matchingDeniedPaths("/home/user/.env", unix.O_RDWR, deny) + require.Len(t, matches, 2) + assert.Equal(t, denyRead, matches[0].Mode) + assert.Equal(t, "*/.env", matches[0].RulePath) + assert.Equal(t, denyWrite, matches[1].Mode) + assert.Equal(t, "/home/user/.env", matches[1].RulePath) +} + func TestIsPathDenied_ExactMatch(t *testing.T) { deny := []denyPathEntry{ {Path: "/home/user/.env", Mode: denyBoth}, diff --git a/sandbox/platform/landlock_translator_linux.go b/sandbox/platform/landlock_translator_linux.go index 8369bb5d..105af0fa 100644 --- a/sandbox/platform/landlock_translator_linux.go +++ b/sandbox/platform/landlock_translator_linux.go @@ -254,10 +254,10 @@ func landlockTranslatePolicy(policy *sandbox.SandboxPolicy, abi *landlockABI) (* continue } for _, m := range matches { - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, Mode: denyRead}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, RulePath: pattern, Mode: denyRead}) } } else { - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: expanded, Mode: denyRead}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: expanded, RulePath: pattern, Mode: denyRead}) } } @@ -285,10 +285,10 @@ func landlockTranslatePolicy(policy *sandbox.SandboxPolicy, abi *landlockABI) (* continue } for _, m := range matches { - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, Mode: denyWrite}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, RulePath: pattern, Mode: denyWrite}) } } else { - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: expanded, Mode: denyWrite}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: expanded, RulePath: pattern, Mode: denyWrite}) } } @@ -354,11 +354,11 @@ func landlockTranslatePolicy(policy *sandbox.SandboxPolicy, abi *landlockABI) (* return } for _, m := range matches { - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, Mode: mode}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: m, RulePath: pattern, Mode: mode}) } return } - ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: pattern, Mode: mode}) + ep.DenyPaths = append(ep.DenyPaths, denyPathEntry{Path: pattern, RulePath: pattern, Mode: mode}) } for _, p := range mandatoryResult.DenyRead { diff --git a/sandbox/platform/landlock_translator_linux_test.go b/sandbox/platform/landlock_translator_linux_test.go index ebb73e29..b5d73744 100644 --- a/sandbox/platform/landlock_translator_linux_test.go +++ b/sandbox/platform/landlock_translator_linux_test.go @@ -198,6 +198,23 @@ func TestLandlockTranslatePolicy_DenyRead(t *testing.T) { } } +func TestLandlockTranslatePolicy_PreservesDenyGlobRulePath(t *testing.T) { + dir := t.TempDir() + secret := filepath.Join(dir, "secret") + require.NoError(t, os.WriteFile(secret, []byte("secret"), 0o600)) + + pattern := filepath.Join(dir, "**") + policy := newTestPolicy() + policy.Filesystem.DenyRead = []string{pattern} + + ep, err := landlockTranslatePolicy(policy, newLandlockABI(3)) + require.NoError(t, err) + + entry := findDenyPath(ep.DenyPaths, secret) + require.NotNil(t, entry) + assert.Equal(t, pattern, entry.RulePath) +} + func TestLandlockTranslatePolicy_DenyWrite(t *testing.T) { policy := newTestPolicy() // DenyWrite is only effective within writable areas. Add /etc as writable @@ -536,10 +553,10 @@ func TestLandlockTranslatePolicy_AllowGitConfig_True(t *testing.T) { func TestLandlockPolicyExplicitlyAllowsProc(t *testing.T) { tests := []struct { - name string - readPaths []string + name string + readPaths []string writePaths []string - want bool + want bool }{ { name: "no proc paths", diff --git a/sandbox/platform/probe_apparmor_linux.go b/sandbox/platform/probe_apparmor_linux.go index 03fa7fa4..30c49e82 100644 --- a/sandbox/platform/probe_apparmor_linux.go +++ b/sandbox/platform/probe_apparmor_linux.go @@ -10,17 +10,25 @@ import ( "github.com/safedep/pmg/sandbox" ) -const apparmorUsernsSysctlPath = "/proc/sys/kernel/apparmor_restrict_unprivileged_userns" +const ( + apparmorUsernsSysctlPath = "/proc/sys/kernel/apparmor_restrict_unprivileged_userns" + apparmorCurrentPath = "/proc/self/attr/apparmor/current" +) type apparmorProbe struct { - env probeEnv - path string + env probeEnv + path string + currentPath string } // NewAppArmorUsernsProbe returns a probe that warns when AppArmor restricts // unprivileged user namespaces (which breaks bwrap-based sandboxing). func NewAppArmorUsernsProbe() sandbox.Probe { - return &apparmorProbe{env: defaultProbeEnv{}, path: apparmorUsernsSysctlPath} + return &apparmorProbe{ + env: defaultProbeEnv{}, + path: apparmorUsernsSysctlPath, + currentPath: apparmorCurrentPath, + } } func (p *apparmorProbe) Name() string { return sandbox.ProbeAppArmorUserns } @@ -45,6 +53,19 @@ func (p *apparmorProbe) Run(_ context.Context) sandbox.ProbeResult { } } + currentPath := p.currentPath + if currentPath == "" { + currentPath = apparmorCurrentPath + } + if current, currentErr := p.env.readFile(currentPath); currentErr == nil && isPMGAppArmorProfile(string(current)) { + return sandbox.ProbeResult{ + Name: sandbox.ProbeAppArmorUserns, + Status: sandbox.ProbeStatusOK, + Summary: "AppArmor profile permits pmg to create user namespaces", + Detail: "The system-wide AppArmor restriction remains enabled, with a per-binary exception for pmg.", + } + } + return sandbox.ProbeResult{ Name: sandbox.ProbeAppArmorUserns, Status: sandbox.ProbeStatusWarn, @@ -65,3 +86,11 @@ func (p *apparmorProbe) Run(_ context.Context) sandbox.ProbeResult { }, } } + +func isPMGAppArmorProfile(current string) bool { + label := strings.TrimSpace(current) + if mode := strings.LastIndex(label, " ("); mode >= 0 { + label = label[:mode] + } + return label == "pmg" +} diff --git a/sandbox/platform/probe_apparmor_linux_test.go b/sandbox/platform/probe_apparmor_linux_test.go index 145c2925..b767003c 100644 --- a/sandbox/platform/probe_apparmor_linux_test.go +++ b/sandbox/platform/probe_apparmor_linux_test.go @@ -21,14 +21,33 @@ func TestAppArmorProbe(t *testing.T) { }{ { name: "ok unrestricted", - env: &fakeProbeEnv{readFileFn: func(string) ([]byte, error) { return []byte("0\n"), nil }}, + env: &fakeProbeEnv{readFileFn: func(path string) ([]byte, error) { + assert.Equal(t, apparmorUsernsSysctlPath, path) + return []byte("0\n"), nil + }}, want: sandbox.ProbeStatusOK, }, { name: "warn restricted", - env: &fakeProbeEnv{readFileFn: func(string) ([]byte, error) { return []byte("1\n"), nil }}, + env: &fakeProbeEnv{readFileFn: func(path string) ([]byte, error) { + if path == apparmorUsernsSysctlPath { + return []byte("1\n"), nil + } + return []byte("unconfined\n"), nil + }}, want: sandbox.ProbeStatusWarn, }, + { + name: "ok restricted with pmg profile", + env: &fakeProbeEnv{readFileFn: func(path string) ([]byte, error) { + if path == apparmorUsernsSysctlPath { + return []byte("1\n"), nil + } + assert.Equal(t, apparmorCurrentPath, path) + return []byte("pmg (enforce)\n"), nil + }}, + want: sandbox.ProbeStatusOK, + }, { name: "skip missing", env: &fakeProbeEnv{readFileFn: func(string) ([]byte, error) { return nil, errors.New("not found") }}, @@ -38,7 +57,11 @@ func TestAppArmorProbe(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - p := &apparmorProbe{env: tc.env, path: apparmorUsernsSysctlPath} + p := &apparmorProbe{ + env: tc.env, + path: apparmorUsernsSysctlPath, + currentPath: apparmorCurrentPath, + } res := p.Run(context.Background()) assert.Equal(t, sandbox.ProbeAppArmorUserns, res.Name) assert.Equal(t, tc.want, res.Status) diff --git a/sandbox/violation.go b/sandbox/violation.go index c0de1879..1f0ab9b0 100644 --- a/sandbox/violation.go +++ b/sandbox/violation.go @@ -4,6 +4,9 @@ import ( "os" "path/filepath" "strings" + "unicode" + + "github.com/safedep/pmg/sandbox/util" ) // Explanation is structured data extracted from a ViolationReport. It carries @@ -40,7 +43,9 @@ func BuildExplanation(report *ViolationReport) Explanation { primary := primaryViolation(report) exp := Explanation{Primary: primary} if primary != nil { - exp.Override = overrideSuggestion(*primary) + if !hasCompanionFilesystemDenial(report, *primary) { + exp.Override = overrideSuggestion(*primary) + } if report != nil && len(report.Violations) > 1 { exp.AdditionalDenials = len(report.Violations) - 1 } @@ -49,7 +54,11 @@ func BuildExplanation(report *ViolationReport) Explanation { } func overrideSuggestion(v Violation) *OverrideSuggestion { - if !isSafeOverrideTarget(v.Target) { + target := v.Target + if v.RuleTarget != "" { + target = v.RuleTarget + } + if !isSafeOverrideTarget(target) || containsRuntimePathVariable(target) { return nil } @@ -58,12 +67,38 @@ func overrideSuggestion(v Violation) *OverrideSuggestion { ViolationKindFSWrite, ViolationKindFSDeleteOrRename, ViolationKindExec: - return &OverrideSuggestion{Kind: v.Kind, Target: v.Target} + return &OverrideSuggestion{Kind: v.Kind, Target: target} default: return nil } } +func containsRuntimePathVariable(value string) bool { + for _, variable := range util.SupportedVariables { + if strings.Contains(value, variable) { + return true + } + } + return false +} + +func hasCompanionFilesystemDenial(report *ViolationReport, primary Violation) bool { + if report == nil || (primary.Kind != ViolationKindFSRead && primary.Kind != ViolationKindFSWrite) { + return false + } + + companion := ViolationKindFSRead + if primary.Kind == ViolationKindFSRead { + companion = ViolationKindFSWrite + } + for _, v := range report.Violations { + if v.Kind == companion && v.Target == primary.Target { + return true + } + } + return false +} + func primaryViolation(report *ViolationReport) *Violation { if report == nil || len(report.Violations) == 0 { return nil @@ -139,7 +174,7 @@ func isSafeOverrideTarget(value string) bool { } for _, r := range value { - if r == 0 || r < 0x20 || r == 0x7f { + if unicode.IsControl(r) { return false } } diff --git a/sandbox/violation_test.go b/sandbox/violation_test.go index b2c64149..58641cd8 100644 --- a/sandbox/violation_test.go +++ b/sandbox/violation_test.go @@ -11,8 +11,9 @@ import ( func TestOverrideSuggestionSkipsGlobRuleTarget(t *testing.T) { assert.Nil(t, overrideSuggestion(Violation{ - Kind: ViolationKindFSRead, - Target: "**/.env", + Kind: ViolationKindFSRead, + Target: "/repo/.env", + RuleTarget: "**/.env", })) } @@ -26,6 +27,16 @@ func TestOverrideSuggestionUsesConcretePath(t *testing.T) { assert.Equal(t, "./.env", o.Target) } +func TestOverrideSuggestionUsesExactRuleTarget(t *testing.T) { + o := overrideSuggestion(Violation{ + Kind: ViolationKindFSRead, + Target: "/home/dev/.ssh/id_rsa", + RuleTarget: "/home/dev/.ssh", + }) + require.NotNil(t, o) + assert.Equal(t, "/home/dev/.ssh", o.Target) +} + func TestOverrideSuggestionPreservesRawTargetCharacters(t *testing.T) { // Shell escaping is the presentation layer's job — the domain layer // returns the raw target verbatim, special characters included. @@ -42,6 +53,18 @@ func TestOverrideSuggestionSkipsControlCharacters(t *testing.T) { Kind: ViolationKindFSRead, Target: "/tmp/bad\npath", })) + assert.Nil(t, overrideSuggestion(Violation{ + Kind: ViolationKindFSRead, + Target: "/tmp/bad\u009bpath", + })) +} + +func TestOverrideSuggestionSkipsVariableRuleTarget(t *testing.T) { + assert.Nil(t, overrideSuggestion(Violation{ + Kind: ViolationKindFSRead, + Target: "/home/dev/.ssh/id_rsa", + RuleTarget: "${HOME}/.ssh", + })) } func TestOverrideSuggestionMapsAllSupportedKinds(t *testing.T) { @@ -213,6 +236,18 @@ func TestBuildExplanationEmptyReport(t *testing.T) { assert.Equal(t, 0, exp.AdditionalDenials) } +func TestBuildExplanationSkipsSingleOverrideForReadWriteDenial(t *testing.T) { + exp := BuildExplanation(&ViolationReport{ + Violations: []Violation{ + {Kind: ViolationKindFSRead, Target: "/repo/.env", RuleTarget: "/repo/.env"}, + {Kind: ViolationKindFSWrite, Target: "/repo/.env", RuleTarget: "/repo/.env"}, + }, + }) + + require.NotNil(t, exp.Primary) + assert.Nil(t, exp.Override) +} + func TestIsSensitiveProjectTargetExported(t *testing.T) { assert.True(t, IsSensitiveProjectTarget("./.env")) assert.True(t, IsSensitiveProjectTarget(filepath.Join("/repo", ".npmrc")))