diff --git a/cmd/pilotctl/appstore.go b/cmd/pilotctl/appstore.go index 82a3265c..70bffe69 100644 --- a/cmd/pilotctl/appstore.go +++ b/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()/.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 ──────────────────────────────────────────────────────────── diff --git a/cmd/pilotctl/appstore_catalogue.go b/cmd/pilotctl/appstore_catalogue.go index 61659ca7..86f1ed08 100644 --- a/cmd/pilotctl/appstore_catalogue.go +++ b/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 diff --git a/cmd/pilotctl/appstore_catalogue_test.go b/cmd/pilotctl/appstore_catalogue_test.go index 40bc581b..d233e6aa 100644 --- a/cmd/pilotctl/appstore_catalogue_test.go +++ b/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) { diff --git a/cmd/pilotctl/zz_appstore_helpers_test.go b/cmd/pilotctl/zz_appstore_helpers_test.go index 48c96c3a..677f3806 100644 --- a/cmd/pilotctl/zz_appstore_helpers_test.go +++ b/cmd/pilotctl/zz_appstore_helpers_test.go @@ -231,35 +231,48 @@ func TestParseCapsFromGrantsMinuteWindow(t *testing.T) { func TestLoadCapStateRecordsMissingFile(t *testing.T) { t.Parallel() - got := loadCapStateRecords(filepath.Join(t.TempDir(), "does-not-exist.jsonl"), nil) + got, err := loadCapStateRecords(filepath.Join(t.TempDir(), "does-not-exist.jsonl"), nil) + if err != nil { + t.Errorf("missing file should be nil error, got %v", err) + } if got != nil { t.Errorf("missing file should return nil, got %v", got) } } -func TestLoadCapStateRecordsSkipsMalformedLines(t *testing.T) { +// TestLoadCapStateRecordsMalformedLineFailsClosed asserts the fail-closed +// property: a garbage line is an error, NOT silently dropped. Silently +// skipping let an attacker erase a real spend by overwriting it with +// junk and have the reader under-report usage. +func TestLoadCapStateRecordsMalformedLineFailsClosed(t *testing.T) { t.Parallel() dir := t.TempDir() path := filepath.Join(dir, "cap-state.jsonl") content := `{"at":"2026-05-27T10:00:00Z","asset":"USDC","amount":5} not-json garbage line -{"at":"2026-05-27T10:05:00Z","asset":"ETH","amount":2} - {"at":"2026-05-27T10:10:00Z","asset":"USDC","amount":7} ` if err := os.WriteFile(path, []byte(content), 0o600); err != nil { t.Fatalf("write: %v", err) } - recs := loadCapStateRecords(path, nil) - if len(recs) != 3 { - t.Fatalf("got %d records, want 3 (malformed should be skipped): %+v", len(recs), recs) - } - if recs[0].asset != "USDC" || recs[0].amount != 5 { - t.Errorf("first rec wrong: %+v", recs[0]) + // Both with and without a key, a malformed line must fail closed. + for _, key := range [][]byte{nil, bytes32(1)} { + recs, err := loadCapStateRecords(path, key) + if err == nil { + t.Fatalf("key!=nil:%v — malformed line silently accepted (got %d records, want error)", key != nil, len(recs)) + } + if !strings.Contains(err.Error(), "malformed") { + t.Errorf("key!=nil:%v — err %q should mention malformed", key != nil, err.Error()) + } } - if recs[2].asset != "USDC" || recs[2].amount != 7 { - t.Errorf("third rec wrong: %+v", recs[2]) +} + +func bytes32(seed byte) []byte { + k := make([]byte, 32) + for i := range k { + k[i] = byte(i) + seed } + return k } func TestLoadCapStateRecordsHMACChain(t *testing.T) { @@ -281,33 +294,46 @@ func TestLoadCapStateRecordsHMACChain(t *testing.T) { h2 := recordHMAC("2026-05-27T10:05:00Z", "ETH", 2, mustDecodeB64(h1)) h3 := recordHMAC("2026-05-27T10:10:00Z", "USDC", 7, mustDecodeB64(h2)) - // Valid chain: 3 records. + // Valid chain: 3 records, no error. os.WriteFile(path, []byte(fmt.Sprintf(`{"at":"2026-05-27T10:00:00Z","asset":"USDC","amount":5,"hmac":"%s"} {"at":"2026-05-27T10:05:00Z","asset":"ETH","amount":2,"hmac":"%s"} {"at":"2026-05-27T10:10:00Z","asset":"USDC","amount":7,"hmac":"%s"} `, h1, h2, h3)), 0o600) - if recs := loadCapStateRecords(path, key); len(recs) != 3 { - t.Fatalf("got %d records, want 3", len(recs)) + recs, err := loadCapStateRecords(path, key) + if err != nil || len(recs) != 3 { + t.Fatalf("valid chain: got %d records, err %v; want 3, nil", len(recs), err) } - // Tampered: amount changed but HMAC not recomputed → skipped. + // Tampered: amount changed but HMAC not recomputed → fail closed. os.WriteFile(path, []byte(fmt.Sprintf(`{"at":"2026-05-27T10:00:00Z","asset":"USDC","amount":5,"hmac":"%s"} {"at":"2026-05-27T10:05:00Z","asset":"ETH","amount":9999,"hmac":"%s"} `, h1, h2)), 0o600) - if recs := loadCapStateRecords(path, key); len(recs) != 1 { - t.Fatalf("got %d after tamper, want 1", len(recs)) + if _, err := loadCapStateRecords(path, key); err == nil { + t.Fatal("tampered record accepted — integrity check missing (must fail closed)") + } else if !strings.Contains(err.Error(), "HMAC mismatch") { + t.Errorf("err %q should mention HMAC mismatch", err.Error()) } - // Backward-compat: no HMAC → pass through. + // Signed chain spliced with an unauthenticated record → fail closed. + os.WriteFile(path, []byte(fmt.Sprintf(`{"at":"2026-05-27T10:00:00Z","asset":"USDC","amount":5,"hmac":"%s"} +{"at":"2026-05-27T10:05:00Z","asset":"ETH","amount":2} +`, h1)), 0o600) + if _, err := loadCapStateRecords(path, key); err == nil { + t.Fatal("signed chain + plain record accepted — mixed-format tamper not caught") + } + + // Wholly-legacy file (no HMAC) → accepted for read-only reporting. os.WriteFile(path, []byte(`{"at":"2026-05-27T10:00:00Z","asset":"USDC","amount":5}`+"\n"), 0o600) - if recs := loadCapStateRecords(path, key); len(recs) != 1 { - t.Fatalf("got %d for no-HMAC, want 1", len(recs)) + recs, err = loadCapStateRecords(path, key) + if err != nil || len(recs) != 1 { + t.Fatalf("legacy file: got %d records, err %v; want 1, nil", len(recs), err) } - // nil key → no verification (all pass through). + // nil key → no verification (HMAC field ignored, still accepted). os.WriteFile(path, []byte(`{"at":"2026-05-27T10:00:00Z","asset":"ETH","amount":2,"hmac":"bogus"}`+"\n"), 0o600) - if recs := loadCapStateRecords(path, nil); len(recs) != 1 { - t.Fatalf("got %d with nil key, want 1", len(recs)) + recs, err = loadCapStateRecords(path, nil) + if err != nil || len(recs) != 1 { + t.Fatalf("nil key: got %d records, err %v; want 1, nil", len(recs), err) } } @@ -327,6 +353,45 @@ func mustDecodeB64(s string) []byte { return b } +// TestResolveUnderRejectsTraversal exercises the containment guard the +// install staging path uses on manifest binary.path. A path that +// resolves outside the bundle/staging dir (../../etc/x, an absolute +// path, an empty path) must be refused; a legitimate nested relative +// path must resolve inside the base. +func TestResolveUnderRejectsTraversal(t *testing.T) { + t.Parallel() + base := t.TempDir() + + rejected := []string{ + "../../etc/x", + "../escape", + "a/../../escape", + "/etc/passwd", + "", + } + for _, rel := range rejected { + if got, err := resolveUnder(base, rel); err == nil { + t.Errorf("resolveUnder(%q) = %q, want error (must be contained)", rel, got) + } + } + + accepted := map[string]string{ + "bin/app": filepath.Join(base, "bin/app"), + "app": filepath.Join(base, "app"), + "a/b/c/bin": filepath.Join(base, "a/b/c/bin"), + } + for rel, want := range accepted { + got, err := resolveUnder(base, rel) + if err != nil { + t.Errorf("resolveUnder(%q) errored: %v", rel, err) + continue + } + if got != want { + t.Errorf("resolveUnder(%q) = %q, want %q", rel, got, want) + } + } +} + func TestSHA256FileHandlesMissing(t *testing.T) { t.Parallel() // Missing file returns empty string, not panic.