security: harden data-derived paths + fix unchecked writable Close()

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
 }
 

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)

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
 }
 

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
 }
 

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
 }

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,

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 {

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
+}

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)
+			}
+		})
+	}
+}

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)