From 124d2b4f1128291158d3148bbc67c0717dd29f83 Mon Sep 17 00:00:00 2001 From: ErenAri Date: Sun, 28 Jun 2026 01:05:58 +0300 Subject: [PATCH] security: harden data-derived paths + fix unchecked writable Close() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeQL code-scanning findings on the runtime/registry packages. Real hardening (path-injection on data-derived components): - Add internal/safepath.LocalJoin, a filepath.IsLocal-based choke point that rejects components escaping their base dir (CodeQL-recognized barrier). - Route the three sinks whose filename comes from data (not a human operator) through it: * runtime/fetch.go — artifact target name from registry record fields * artifact/artifact.go — staged basename from the source path * runtime/audit.go — decision-trace filename from caller-supplied DecisionID (reachable from the network-facing API) Real hygiene (unhandled-writable-file-close): - Surface Close() errors on append-mode writers instead of deferring and dropping them, so a failed buffer flush isn't silently lost: agent/load_ledger.go, cloudregistry/store.go, registry/history.go, registry/local.go, runtime/audit.go - Drop a dead assignment (cmd = newCmd) in vm/qemu.go; only qemuProc is read afterward. Operator-controlled CLI/config paths and the already-validated api/report_paths.go sink are intentionally not changed; those alerts are false positives against existing sanitizers and will be dismissed with reason. Co-Authored-By: Claude Opus 4.8 --- internal/agent/load_ledger.go | 6 +++- internal/artifact/artifact.go | 9 ++++- internal/cloudregistry/store.go | 5 ++- internal/registry/history.go | 6 +++- internal/registry/local.go | 6 +++- internal/runtime/audit.go | 15 ++++++-- internal/runtime/fetch.go | 8 ++++- internal/safepath/safepath.go | 47 ++++++++++++++++++++++++ internal/safepath/safepath_test.go | 57 ++++++++++++++++++++++++++++++ internal/vm/qemu.go | 3 +- 10 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 internal/safepath/safepath.go create mode 100644 internal/safepath/safepath_test.go diff --git a/internal/agent/load_ledger.go b/internal/agent/load_ledger.go index 2c9d9dc..7df64a9 100644 --- a/internal/agent/load_ledger.go +++ b/internal/agent/load_ledger.go @@ -96,14 +96,18 @@ func AppendLoadLedgerEntry(workDir string, entry LoadLedgerEntry) (string, error if err != nil { return "", fmt.Errorf("open load ledger: %w", err) } - defer f.Close() raw, err := json.Marshal(entry) if err != nil { + _ = f.Close() return "", fmt.Errorf("encode load ledger entry: %w", err) } if _, err := f.Write(append(raw, '\n')); err != nil { + _ = f.Close() return "", fmt.Errorf("write load ledger entry: %w", err) } + if err := f.Close(); err != nil { + return "", fmt.Errorf("close load ledger: %w", err) + } return path, nil } diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index 1a959f8..83c7166 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -5,6 +5,8 @@ import ( "io" "os" "path/filepath" + + "github.com/kernel-guard/bpfcompat/internal/safepath" ) func Stage(srcPath, dstDir string) (string, error) { @@ -27,7 +29,12 @@ func Stage(srcPath, dstDir string) (string, error) { } defer src.Close() - dstPath := filepath.Join(dstDirAbs, filepath.Base(srcAbs)) + // The destination filename is derived from the source basename; contain + // it to a single component under the destination directory. + dstPath, err := safepath.LocalJoin(dstDirAbs, filepath.Base(srcAbs)) + if err != nil { + return "", fmt.Errorf("resolve staged artifact path: %w", err) + } dst, err := os.Create(dstPath) if err != nil { return "", fmt.Errorf("create staged artifact: %w", err) diff --git a/internal/cloudregistry/store.go b/internal/cloudregistry/store.go index 1ea7e09..42d6bec 100644 --- a/internal/cloudregistry/store.go +++ b/internal/cloudregistry/store.go @@ -824,10 +824,13 @@ func (s Store) AppendAudit(event AuditEvent) (AuditEvent, error) { if err != nil { return AuditEvent{}, fmt.Errorf("open cloud registry audit stream: %w", err) } - defer f.Close() if _, err := f.Write(append(line, '\n')); err != nil { + _ = f.Close() return AuditEvent{}, fmt.Errorf("append cloud registry audit event: %w", err) } + if err := f.Close(); err != nil { + return AuditEvent{}, fmt.Errorf("close cloud registry audit stream: %w", err) + } return event, nil } diff --git a/internal/registry/history.go b/internal/registry/history.go index e982f0f..c666841 100644 --- a/internal/registry/history.go +++ b/internal/registry/history.go @@ -101,16 +101,20 @@ func PersistArtifactVersion(workDir string, record ArtifactVersionRecord) error if err != nil { return fmt.Errorf("open artifact history index: %w", err) } - defer f.Close() line, err := json.Marshal(record) if err != nil { + _ = f.Close() return fmt.Errorf("marshal artifact history record: %w", err) } line = append(line, '\n') if _, err := f.Write(line); err != nil { + _ = f.Close() return fmt.Errorf("append artifact history record: %w", err) } + if err := f.Close(); err != nil { + return fmt.Errorf("close artifact history index: %w", err) + } return nil } diff --git a/internal/registry/local.go b/internal/registry/local.go index adcc7fd..bb192d2 100644 --- a/internal/registry/local.go +++ b/internal/registry/local.go @@ -74,16 +74,20 @@ func appendRegistryJSONL(workDir string, record RunRecord) error { if err != nil { return fmt.Errorf("open registry index: %w", err) } - defer f.Close() line, err := json.Marshal(record) if err != nil { + _ = f.Close() return fmt.Errorf("marshal registry record: %w", err) } line = append(line, '\n') if _, err := f.Write(line); err != nil { + _ = f.Close() return fmt.Errorf("append registry record: %w", err) } + if err := f.Close(); err != nil { + return fmt.Errorf("close registry index: %w", err) + } return nil } diff --git a/internal/runtime/audit.go b/internal/runtime/audit.go index 5e1d7a2..1646cbb 100644 --- a/internal/runtime/audit.go +++ b/internal/runtime/audit.go @@ -11,6 +11,8 @@ import ( "strconv" "strings" "time" + + "github.com/kernel-guard/bpfcompat/internal/safepath" ) const ( @@ -192,7 +194,13 @@ func PersistDecisionTrace(workDir string, trace DecisionTrace) (DecisionPersistR if err := os.MkdirAll(traceDir, 0o755); err != nil { return DecisionPersistResult{}, fmt.Errorf("create runtime decision directory: %w", err) } - tracePath := filepath.Join(traceDir, trace.DecisionID+".json") + // DecisionID can arrive from the caller (including the network-facing + // API), so contain the derived filename to a single component under + // traceDir before writing. + tracePath, err := safepath.LocalJoin(traceDir, trace.DecisionID+".json") + if err != nil { + return DecisionPersistResult{}, fmt.Errorf("resolve decision trace path: %w", err) + } payload, err := json.MarshalIndent(trace, "", " ") if err != nil { return DecisionPersistResult{}, fmt.Errorf("marshal decision trace: %w", err) @@ -219,10 +227,13 @@ func PersistDecisionTrace(workDir string, trace DecisionTrace) (DecisionPersistR if err != nil { return DecisionPersistResult{}, fmt.Errorf("open runtime decision event stream: %w", err) } - defer f.Close() if _, err := f.Write(append(line, '\n')); err != nil { + _ = f.Close() return DecisionPersistResult{}, fmt.Errorf("append runtime decision event: %w", err) } + if err := f.Close(); err != nil { + return DecisionPersistResult{}, fmt.Errorf("close runtime decision event stream: %w", err) + } return DecisionPersistResult{ DecisionID: trace.DecisionID, diff --git a/internal/runtime/fetch.go b/internal/runtime/fetch.go index fa096ba..797d78d 100644 --- a/internal/runtime/fetch.go +++ b/internal/runtime/fetch.go @@ -17,6 +17,7 @@ import ( "github.com/kernel-guard/bpfcompat/internal/artifact" "github.com/kernel-guard/bpfcompat/internal/registry" + "github.com/kernel-guard/bpfcompat/internal/safepath" ) const fetchSchemaVersion = "runtime_fetch.v0.1" @@ -56,7 +57,12 @@ func FetchArtifact(record registry.ArtifactVersionRecord, outDir string) (FetchR if ext == "" { ext = ".bpf.o" } - targetPath := filepath.Join(outDir, sanitizeFragment(targetName)+ext) + // targetName/ext derive from registry record fields, so contain the + // resulting filename to a single component under outDir. + targetPath, err := safepath.LocalJoin(outDir, sanitizeFragment(targetName)+ext) + if err != nil { + return FetchResult{}, fmt.Errorf("resolve artifact target path: %w", err) + } stagedPath := targetPath if remote { diff --git a/internal/safepath/safepath.go b/internal/safepath/safepath.go new file mode 100644 index 0000000..8c72a7a --- /dev/null +++ b/internal/safepath/safepath.go @@ -0,0 +1,47 @@ +// Package safepath is the single choke point for turning data-derived names +// (registry record fields, decision IDs, URL basenames) into on-disk paths. +// +// The functions here reject any component that would escape its base directory +// — absolute paths, "..", volume names, or a leading separator — using +// filepath.IsLocal. They are deliberately strict: callers pass a trusted base +// directory plus one or more UNTRUSTED components, and never the other way +// around. Paths that come straight from a human operator (a CLI --out flag, a +// file the operator points the tool at) are intentionally NOT routed through +// here; those are operator-controlled by design. +package safepath + +import ( + "fmt" + "path/filepath" +) + +// ErrEscapesBase is returned when an untrusted component would resolve outside +// the base directory it is being joined to. +type ErrEscapesBase struct { + Base string + Component string +} + +func (e *ErrEscapesBase) Error() string { + return fmt.Sprintf("path component %q escapes base directory %q", e.Component, e.Base) +} + +// LocalJoin joins one or more untrusted components onto a trusted base +// directory and returns the cleaned absolute-or-relative path. It returns an +// *ErrEscapesBase if the joined components are not local to the base (i.e. they +// contain "..", an absolute path, or a volume name). +// +// The components are first joined and cleaned together, so a sequence that nets +// out local (e.g. "a/../b" -> "b") is allowed, while one that escapes +// ("a/../../b" -> "../b") is rejected. +func LocalJoin(base string, components ...string) (string, error) { + rel := filepath.Join(components...) + // filepath.IsLocal reports whether rel, when evaluated against any + // directory, stays within that directory. CodeQL recognises it as a + // path-traversal barrier, and it is the canonical stdlib guard since + // Go 1.20. + if !filepath.IsLocal(rel) { + return "", &ErrEscapesBase{Base: base, Component: rel} + } + return filepath.Join(base, rel), nil +} diff --git a/internal/safepath/safepath_test.go b/internal/safepath/safepath_test.go new file mode 100644 index 0000000..5e28ae0 --- /dev/null +++ b/internal/safepath/safepath_test.go @@ -0,0 +1,57 @@ +package safepath + +import ( + "errors" + "path/filepath" + "testing" +) + +func TestLocalJoinAllowsLocalNames(t *testing.T) { + base := filepath.Join("var", "lib", "bpfcompat") + cases := []struct { + name string + components []string + want string + }{ + {"plain file", []string{"falco.bpf.o"}, filepath.Join(base, "falco.bpf.o")}, + {"nested local", []string{"input", "probe.bpf.o"}, filepath.Join(base, "input", "probe.bpf.o")}, + {"dotdot that nets out local", []string{"a", "..", "b"}, filepath.Join(base, "b")}, + {"dots in name are fine", []string{"v1.2.3.bpf.o"}, filepath.Join(base, "v1.2.3.bpf.o")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := LocalJoin(base, tc.components...) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.want { + t.Fatalf("LocalJoin = %q, want %q", got, tc.want) + } + }) + } +} + +func TestLocalJoinRejectsTraversal(t *testing.T) { + base := filepath.Join("var", "lib", "bpfcompat") + cases := []struct { + name string + components []string + }{ + {"parent escape", []string{"..", "etc", "passwd"}}, + {"deep escape", []string{"a", "..", "..", "b"}}, + {"absolute path", []string{"/etc/passwd"}}, + {"parent then absolute-looking", []string{"..", "input", "x"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := LocalJoin(base, tc.components...) + if err == nil { + t.Fatalf("expected error for components %v", tc.components) + } + var escapeErr *ErrEscapesBase + if !errors.As(err, &escapeErr) { + t.Fatalf("expected *ErrEscapesBase, got %T: %v", err, err) + } + }) + } +} diff --git a/internal/vm/qemu.go b/internal/vm/qemu.go index 0e489a2..9827cc0 100644 --- a/internal/vm/qemu.go +++ b/internal/vm/qemu.go @@ -309,7 +309,8 @@ func ExecuteProfile(ctx context.Context, req ExecutionRequest) (result Execution result.InfraError = err.Error() return } - cmd = newCmd + // newCmd is the post-reboot QEMU process; only qemuProc is read + // afterward (for cleanup), so we don't reassign cmd here. qemuProc = newCmd.Process rebootCtx, cancelReboot := context.WithTimeout(ctx, req.Timeout)