Harden app-store install path and cap-state reads (#315)

* Harden app-store install path and cap-state reads

Three install-path security fixes in pilotctl appstore:

- Path traversal during staging: manifest binary.path was joined onto
  the bundle and staging dirs with no containment check, so a path like
  ../../etc/x escaped the staging tree. Add a resolveUnder guard
  (mirrors the supervisor's) at both join sites; reject any path that
  resolves outside the target dir.

- Unbounded catalogue bundle download: the bundle tarball was copied
  with no size limit, so a hostile or compromised CDN could exhaust disk
  before the post-copy sha256 check ran. Cap the compressed download
  with io.LimitReader (maxBundleBytes) and fail closed when exceeded.

- Cap-state reads no longer fail open: loadCapStateRecords silently
  skipped malformed lines and tampered HMAC records, under-reporting
  usage and hiding an erased cap. It now returns an error and the caps
  command fails closed on a malformed line, an HMAC mismatch, or a
  signed chain spliced with an unauthenticated record.

Tests: ../../etc/x and absolute/empty paths rejected; oversized bundle
download refused; malformed line and tampered/mixed cap-state records
fail closed; legacy and nil-key reads still accepted.

* Confine appstore caps app id and annotate guarded gosec paths

---------

Co-authored-by: Teodor Calin <teodor@vulturelabs.io>

Author: TeoSlayer

cmd/pilotctl/appstore.go

@@ -993,6 +993,33 @@ type installReport struct {
 // The daemon's supervisor only scans on Start, so an install while the
 // daemon is running is invisible until the next daemon restart — the
 // success message and JSON note callers about this.
+// resolveUnder joins rel onto base, cleans it, and verifies the result
+// is contained inside base. Returns an error otherwise.
+//
+// filepath.Join alone does NOT block traversal: a manifest with
+// binary.path = "../../etc/x" resolves outside the bundle/staging dir,
+// which would let a hostile manifest read an arbitrary host file as the
+// "binary" or plant a copy outside the staging tree. This mirrors the
+// supervisor's resolveUnder guard so the install path enforces the same
+// containment the supervisor relies on at spawn.
+func resolveUnder(base, rel string) (string, error) {
+	if rel == "" {
+		return "", fmt.Errorf("empty path")
+	}
+	if filepath.IsAbs(rel) {
+		return "", fmt.Errorf("absolute path not permitted")
+	}
+	absBase, err := filepath.Abs(base)
+	if err != nil {
+		return "", fmt.Errorf("abs base: %w", err)
+	}
+	joined := filepath.Clean(filepath.Join(absBase, rel))
+	if joined != absBase && !strings.HasPrefix(joined, absBase+string(filepath.Separator)) {
+		return "", fmt.Errorf("path %q escapes %s", rel, absBase)
+	}
+	return joined, nil
+}
+
 func cmdAppStoreInstall(args []string) {
 	if len(args) < 1 {
 		fatalHint("invalid_argument",
@@ -1076,7 +1103,12 @@ func cmdAppStoreInstall(args []string) {
 				"%v", err)
 		}
 	}
-	srcBin := filepath.Join(bundleDir, m.Binary.Path)
+	srcBin, err := resolveUnder(bundleDir, m.Binary.Path)
+	if err != nil {
+		fatalHint("invalid_argument",
+			"manifest binary.path must be a relative path inside the bundle; refusing a path that escapes the bundle dir",
+			"binary path %q escapes bundle: %v", m.Binary.Path, err)
+	}
 	if _, err := os.Stat(srcBin); err != nil {
 		fatalHint("invalid_argument",
 			fmt.Sprintf("manifest pins binary at %q", m.Binary.Path),
@@ -1122,7 +1154,17 @@ func cmdAppStoreInstall(args []string) {
 		fatalHint("io_error", "check install root permissions", "write manifest: %v", err)
 	}
 	// Place the binary at the manifest-declared path inside staging.
-	dstBin := filepath.Join(stagingDir, m.Binary.Path)
+	// Re-apply the containment guard against the staging root: defence
+	// in depth so the write target can't escape even if bundleDir and
+	// stagingDir ever diverge.
+	dstBin, err := resolveUnder(stagingDir, m.Binary.Path)
+	if err != nil {
+		// #nosec G703 -- stagingDir is appStoreRoot()/<m.ID>.staging; m.ID is reverse-DNS validated by m.Validate() above, so it cannot escape the install root
+		_ = os.RemoveAll(stagingDir)
+		fatalHint("invalid_argument",
+			"manifest binary.path must stay inside the staging dir",
+			"binary path %q escapes staging: %v", m.Binary.Path, err)
+	}
 	if err := os.MkdirAll(filepath.Dir(dstBin), 0o700); err != nil {
 		_ = os.RemoveAll(stagingDir)
 		fatalHint("io_error", "check install root permissions", "mkdir binary parent: %v", err)
@@ -1505,7 +1547,15 @@ func cmdAppStoreCaps(args []string) {
 			"missing app id")
 	}
 	appID := args[0]
-	appDir := filepath.Join(appStoreRoot(), appID)
+	// appID is user input; confine it to a single entry under the app
+	// store root so a crafted id (e.g. "../../etc") can't read or open
+	// files outside the install tree.
+	appDir, err := resolveUnder(appStoreRoot(), appID)
+	if err != nil {
+		fatalHint("invalid_argument",
+			"app ids are single directory names; try `pilotctl appstore list`",
+			"invalid app id %q: %v", appID, err)
+	}
 
 	mfRaw, err := os.ReadFile(filepath.Join(appDir, "manifest.json"))
 	if err != nil {
@@ -1528,7 +1578,15 @@ func cmdAppStoreCaps(args []string) {
 	// Derive HMAC key from daemon identity for cap-state integrity.
 	// Falls back to nil (no verification) if identity is unavailable.
 	hmacKey := loadCapStateHMACKey()
-	records := loadCapStateRecords(filepath.Join(appDir, "cap-state.jsonl"), hmacKey)
+	records, err := loadCapStateRecords(filepath.Join(appDir, "cap-state.jsonl"), hmacKey)
+	if err != nil {
+		// Fail closed: a tampered or corrupt spend log must surface as
+		// an error, not be papered over with an under-reported usage
+		// figure that makes an erased cap look healthy.
+		fatalHint("integrity_error",
+			"the wallet's cap-state spend log failed its integrity check; do not trust reported usage. Inspect cap-state.jsonl and, if the wallet's identity is intact, let the wallet rewrite it.",
+			"cap-state: %v", err)
+	}
 
 	now := time.Now()
 	reports := make([]capUsageReport, 0, len(caps))
@@ -1762,21 +1820,39 @@ type capStateRecord struct {
 	hmacOK bool
 }
 
-// loadCapStateRecords reads the wallet's persistent spend log.
-// When hmacKey is non-nil, records with a "hmac" field are verified
-// against the chained HMAC-SHA256; tampered records are skipped.
-// Records without "hmac" pass through (backward-compat).
-func loadCapStateRecords(path string, hmacKey []byte) []capStateRecord {
+// loadCapStateRecords reads the wallet's persistent spend log, failing
+// closed on any sign of corruption or tampering rather than silently
+// dropping records (which would under-report usage and make a tampered
+// or partially-erased cap look healthy).
+//
+// Behaviour:
+//   - a missing file is normal first-run state: returns (nil, nil);
+//   - a malformed JSON line is always an error — never silently skipped;
+//   - when hmacKey is non-nil and a line carries an "hmac" field, the
+//     chained HMAC-SHA256 must verify, else it's a tamper signal (error);
+//   - a file that mixes authenticated and unauthenticated records (only
+//     reachable by tampering with a signed chain) is rejected;
+//   - a wholly-unauthenticated (legacy) file is accepted for read-only
+//     reporting even when a key is available — the wallet migrates it to
+//     an authenticated chain on its next write.
+func loadCapStateRecords(path string, hmacKey []byte) ([]capStateRecord, error) {
+	// #nosec G304 -- path is appDir/cap-state.jsonl where appDir is confined to the app store root by resolveUnder in the sole production caller
 	f, err := os.Open(path)
 	if err != nil {
-		return nil
+		if errors.Is(err, os.ErrNotExist) {
+			return nil, nil
+		}
+		return nil, err
 	}
 	defer f.Close()
 	var out []capStateRecord
 	var prevHMAC []byte
+	sawHMAC, sawPlain := false, false
 	scanner := bufio.NewScanner(f)
 	scanner.Buffer(make([]byte, 0, 4*1024), 1024*1024)
+	lineNo := 0
 	for scanner.Scan() {
+		lineNo++
 		raw := scanner.Bytes()
 		if len(raw) == 0 {
 			continue
@@ -1785,10 +1861,18 @@ func loadCapStateRecords(path string, hmacKey []byte) []capStateRecord {
 		// Parse the full JSON line including optional HMAC.
 		var line capStateJSON
 		if err := json.Unmarshal(raw, &line); err != nil {
-			continue
+			// Fail closed: a garbage line is corruption or tampering,
+			// not something to drop on the floor.
+			return nil, fmt.Errorf("malformed cap-state line %d in %s: %w", lineNo, path, err)
 		}
 
-		rec := capStateRecord{at: line.At, asset: line.Asset, amount: line.Amount, hmacOK: true}
+		if line.HMAC != "" {
+			sawHMAC = true
+		} else {
+			sawPlain = true
+		}
+
+		rec := capStateRecord{at: line.At, asset: line.Asset, amount: line.Amount, hmacOK: line.HMAC == ""}
 
 		// HMAC verification — only when key is available AND this line
 		// carries an HMAC field (post-migration or wallet-written).
@@ -1803,24 +1887,23 @@ func loadCapStateRecords(path string, hmacKey []byte) []capStateRecord {
 			expected := base64.StdEncoding.EncodeToString(mac.Sum(nil))
 
 			if !hmac.Equal([]byte(expected), []byte(line.HMAC)) {
-				slog.Warn("cap-state record HMAC mismatch — skipping (possible tampering)",
-					"path", path,
-					"at", line.At.Format(time.RFC3339),
-				)
-				continue
+				return nil, fmt.Errorf("cap-state line %d in %s: HMAC mismatch — spend log tampered with or truncated", lineNo, path)
 			}
 			rec.hmacOK = true
 			prevHMAC, _ = base64.StdEncoding.DecodeString(line.HMAC)
-		} else if hmacKey != nil && line.HMAC == "" {
-			// Record without HMAC — backward-compat. Accept it,
-			// but reset the chain so the next HMAC-bearing record
-			// starts a fresh chain.
-			prevHMAC = nil
 		}
 
 		out = append(out, rec)
 	}
-	return out
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+	// A signed chain with an unauthenticated record spliced in can only
+	// arise from tampering. Reject (only meaningful when we have a key).
+	if hmacKey != nil && sawHMAC && sawPlain {
+		return nil, fmt.Errorf("cap-state %s mixes authenticated and unauthenticated records — refusing (possible tampering)", path)
+	}
+	return out, nil
 }
 
 // ── actions ────────────────────────────────────────────────────────────

cmd/pilotctl/appstore_catalogue.go

@@ -386,10 +386,23 @@ func fetchAndUnpackBundle(e catalogueEntry) (string, error) {
 	}
 	defer os.Remove(tmpTar.Name())
 	h := sha256.New()
-	if _, err := io.Copy(io.MultiWriter(tmpTar, h), body); err != nil {
+	// Cap the compressed bundle download. Without this a hostile or
+	// compromised CDN could stream an unbounded body and exhaust disk
+	// before the sha256 check (which only runs after the full copy) ever
+	// fires. maxBundleBytes bounds the COMPRESSED tarball; untarUnder
+	// separately bounds each extracted file against decompression bombs.
+	// We read one byte past the cap to distinguish "exactly at limit"
+	// from "over limit" and fail closed on the latter.
+	limited := io.LimitReader(body, maxBundleBytes+1)
+	written, err := io.Copy(io.MultiWriter(tmpTar, h), limited)
+	if err != nil {
 		_ = tmpTar.Close()
 		return "", fmt.Errorf("download body: %w", err)
 	}
+	if written > maxBundleBytes {
+		_ = tmpTar.Close()
+		return "", fmt.Errorf("bundle exceeds max size (%d bytes): %s — refusing", maxBundleBytes, bundleURL)
+	}
 	if err := tmpTar.Close(); err != nil {
 		return "", err
 	}
@@ -457,10 +470,19 @@ func httpGet(raw string) (io.ReadCloser, error) {
 	return resp.Body, nil
 }
 
+// maxBundleBytes caps the COMPRESSED bundle tarball download in
+// fetchAndUnpackBundle. The largest known pilot app is ~4 MiB
+// uncompressed; 128 MiB leaves generous headroom for a multi-file
+// bundle while bounding an unbounded-body attack from a hostile or
+// compromised CDN that would otherwise fill disk before the post-copy
+// sha256 check runs. A var (not const) so tests can lower it without
+// writing 128 MiB to disk.
+var maxBundleBytes int64 = 128 << 20 // 128 MiB
+
 // maxUntarFileSize is the per-file limit for entries extracted by
 // untarUnder. 64 MiB covers legitimate bundles (the largest known
 // pilot app is ~4 MiB) while bounding decompression bombs that pass
-// the 1 MiB tarball download cap via a high compression ratio.
+// the bundle download cap via a high compression ratio.
 const maxUntarFileSize int64 = 64 << 20 // 64 MiB
 
 // untarUnder writes every entry in r under dst, refusing any path

cmd/pilotctl/appstore_catalogue_test.go

@@ -4,12 +4,74 @@ import (
 	"archive/tar"
 	"bytes"
 	"compress/gzip"
+	"crypto/sha256"
+	"encoding/hex"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 )
 
+// buildBundleTar returns a gzipped tar containing a single file, plus
+// its sha256 — a minimal valid bundle for fetchAndUnpackBundle tests.
+func buildBundleTar(t *testing.T, name string, payload []byte) (gzBytes []byte, sha string) {
+	t.Helper()
+	var buf bytes.Buffer
+	gz := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gz)
+	if err := tw.WriteHeader(&tar.Header{Name: name, Size: int64(len(payload)), Typeflag: tar.TypeReg, Mode: 0o644}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := tw.Write(payload); err != nil {
+		t.Fatal(err)
+	}
+	if err := tw.Close(); err != nil {
+		t.Fatal(err)
+	}
+	if err := gz.Close(); err != nil {
+		t.Fatal(err)
+	}
+	out := buf.Bytes()
+	h := sha256.Sum256(out)
+	return out, hex.EncodeToString(h[:])
+}
+
+// TestFetchAndUnpackBundle_RejectsOversizedDownload is the item-3 cap:
+// the COMPRESSED bundle download must be bounded so a hostile CDN can't
+// stream an unbounded body and exhaust disk before the post-copy sha256
+// check ever runs. maxBundleBytes is lowered for the test so we don't
+// have to write 128 MiB.
+func TestFetchAndUnpackBundle_RejectsOversizedDownload(t *testing.T) {
+	orig := maxBundleBytes
+	defer func() { maxBundleBytes = orig }()
+
+	dir := t.TempDir()
+	gzBytes, sha := buildBundleTar(t, "bin/app", bytes.Repeat([]byte("A"), 4096))
+	tarPath := filepath.Join(dir, "bundle.tar.gz")
+	if err := os.WriteFile(tarPath, gzBytes, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	entry := catalogueEntry{ID: "io.test.big", BundleURL: "file://" + tarPath, BundleSHA: sha}
+
+	// Cap below the bundle size → must refuse.
+	maxBundleBytes = int64(len(gzBytes) - 1)
+	if _, err := fetchAndUnpackBundle(entry); err == nil {
+		t.Fatal("oversized bundle accepted — download cap missing")
+	} else if !strings.Contains(err.Error(), "exceeds max size") {
+		t.Fatalf("err %q should mention exceeds max size", err.Error())
+	}
+
+	// Cap above the bundle size → unpacks normally.
+	maxBundleBytes = int64(len(gzBytes) + 1)
+	unpacked, err := fetchAndUnpackBundle(entry)
+	if err != nil {
+		t.Fatalf("within-cap bundle rejected: %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(unpacked, "bin/app")); err != nil {
+		t.Fatalf("expected extracted bin/app: %v", err)
+	}
+}
+
 // TestUntarUnder_RejectsDecompressionBomb verifies that a tar entry
 // exceeding maxUntarFileSize is rejected (decompression bomb guard).
 func TestUntarUnder_RejectsDecompressionBomb(t *testing.T) {