From ff000347bba50ca396e68be5b8060536723749fd Mon Sep 17 00:00:00 2001 From: matthew-pilot Date: Thu, 28 May 2026 07:39:20 +0000 Subject: [PATCH] fix(appstore): refuse downgrade install (lower app_version) (PILOT-105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The install flow had no monotonic version check — re-installing an older app_version on top of a newer one succeeded silently. This is a classic rollback-attack vector: a CVE patched in v1.2 can be re-introduced by reinstalling v1.1. This adds: - compareVersions() — basic semver comparison (MAJOR.MINOR.PATCH[-PRE]) - registerInstalled() now refuses to replace an in-memory entry with a lower app_version for the same app ID (startup path). - rescanForNew() now detects on-disk manifest version changes for already-known apps and refuses downgrades; upgrades are accepted and trigger a clean restart of the supervise goroutine. - Audit events (downgrade-refused) in the supervisor log for forensics. The check is best-effort defense-in-depth — full protection requires signed manifests (PILOT-98, the #5 dependency) to prevent an attacker from forging the app_version field alongside the binary. Closes PILOT-105 --- plugin/appstore/supervisor.go | 115 ++++++++++- plugin/appstore/zz4_downgrade_test.go | 272 ++++++++++++++++++++++++++ 2 files changed, 385 insertions(+), 2 deletions(-) create mode 100644 plugin/appstore/zz4_downgrade_test.go diff --git a/plugin/appstore/supervisor.go b/plugin/appstore/supervisor.go index c50975a..d3bbc62 100644 --- a/plugin/appstore/supervisor.go +++ b/plugin/appstore/supervisor.go @@ -13,6 +13,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "sync" "syscall" @@ -277,10 +278,24 @@ func (s *supervisor) scanInstalled() ([]*installedApp, error) { // in-memory table. Called by Service.Start *before* the supervise // goroutine kicks off, so callers of Apps() / Call() see the right // set the moment Start returns (no race against run()'s startup). +// +// Refuses to register an app if the manifest's app_version is a +// downgrade from an already-registered entry for the same app ID. +// This guards against re-installs that would silently roll back +// a security patch (see downgrade protection, PILOT-105). func (s *supervisor) registerInstalled(apps []*installedApp) { s.mu.Lock() defer s.mu.Unlock() for _, a := range apps { + if existing, ok := s.installed[a.Manifest.ID]; ok { + if compareVersions(a.Manifest.AppVersion, existing.Manifest.AppVersion) < 0 { + s.logger.Printf("downgrade refused: app=%s new=%s old=%s — keeping existing version", + a.Manifest.ID, a.Manifest.AppVersion, existing.Manifest.AppVersion) + s.writeAuditLine(a, auditEvent{Event: "downgrade-refused", + Reason: fmt.Sprintf("refusing %s (existing %s)", a.Manifest.AppVersion, existing.Manifest.AppVersion)}) + continue + } + } s.installed[a.Manifest.ID] = a } } @@ -355,6 +370,11 @@ func (s *supervisor) run(ctx context.Context, apps []*installedApp) { // concurrent Apps()/Call() reads see them as soon as a supervise // goroutine starts. Errors are logged and treated as "no new apps"; // a transient FS issue shouldn't kill the supervisor. +// +// For apps already in the in-memory map, detects on-disk manifest +// changes (e.g. a re-install by pilotctl while the daemon runs) and +// refuses to accept a version downgrade. Same-app different-version +// upgrades are accepted; the new supervise goroutine replaces the old. func (s *supervisor) rescanForNew() []*installedApp { apps, err := s.scanInstalled() if err != nil { @@ -365,8 +385,28 @@ func (s *supervisor) rescanForNew() []*installedApp { defer s.mu.Unlock() var fresh []*installedApp for _, a := range apps { - if _, exists := s.installed[a.Manifest.ID]; exists { - continue + if existing, ok := s.installed[a.Manifest.ID]; ok { + if existing.Manifest.AppVersion == a.Manifest.AppVersion { + continue // same version, nothing to do + } + if compareVersions(a.Manifest.AppVersion, existing.Manifest.AppVersion) < 0 { + s.logger.Printf("rescan: downgrade refused: app=%s new=%s old=%s — keeping existing version", + a.Manifest.ID, a.Manifest.AppVersion, existing.Manifest.AppVersion) + s.writeAuditLine(a, auditEvent{Event: "downgrade-refused", + Reason: fmt.Sprintf("rescan: refusing %s (existing %s)", a.Manifest.AppVersion, existing.Manifest.AppVersion)}) + continue + } + // Version upgrade detected on disk: cancel the old supervise + // goroutine and register the new manifest. The old app + // will be torn down by its ctx cancel; the rescan loop + // (run()) will spawn a fresh goroutine for this entry. + if cancel, cancelOk := s.appCancel[a.Manifest.ID]; cancelOk { + cancel() + delete(s.appCancel, a.Manifest.ID) + } + delete(s.ready, a.Manifest.ID) + s.logger.Printf("rescan: version upgrade detected: app=%s %s → %s — restarting", + a.Manifest.ID, existing.Manifest.AppVersion, a.Manifest.AppVersion) } s.installed[a.Manifest.ID] = a fresh = append(fresh, a) @@ -469,6 +509,77 @@ func (s *supervisor) rescanForResume() []*installedApp { return resumed } +// compareVersions compares two semver strings (MAJOR.MINOR.PATCH[-PRERELEASE]). +// Returns -1 if a < b, 0 if equal, 1 if a > b. +// Both must be valid per semverPattern; behaviour on invalid input is undefined. +func compareVersions(a, b string) int { + if a == b { + return 0 + } + // Strip prerelease suffix for numeric comparison. + aNum, aPre := splitSemver(a) + bNum, bPre := splitSemver(b) + if c := compareNumericParts(aNum, bNum); c != 0 { + return c + } + // Same numeric parts: no prerelease > prerelease. + if aPre == "" && bPre != "" { + return 1 + } + if aPre != "" && bPre == "" { + return -1 + } + // Both have prerelease: lexical tiebreak. + if aPre < bPre { + return -1 + } + if aPre > bPre { + return 1 + } + return 0 +} + +// splitSemver splits "1.2.3-alpha" into ("1.2.3", "alpha"). +func splitSemver(v string) (numeric, prerelease string) { + idx := strings.IndexByte(v, '-') + if idx < 0 { + return v, "" + } + return v[:idx], v[idx+1:] +} + +// compareNumericParts compares "MAJOR.MINOR.PATCH" dotted triples numerically. +func compareNumericParts(a, b string) int { + aParts := strings.Split(a, ".") + bParts := strings.Split(b, ".") + for i := 0; i < 3; i++ { + av := atoiOrZero(indexOrEmpty(aParts, i)) + bv := atoiOrZero(indexOrEmpty(bParts, i)) + if av < bv { + return -1 + } + if av > bv { + return 1 + } + } + return 0 +} + +func indexOrEmpty(parts []string, i int) string { + if i < len(parts) { + return parts[i] + } + return "0" +} + +func atoiOrZero(s string) int { + n, err := strconv.Atoi(s) + if err != nil { + return 0 + } + return n +} + // superviseOne runs one app forever, respawning on exit until ctx is canceled. func (s *supervisor) superviseOne(ctx context.Context, a *installedApp) { // Mark the start + end of supervision. The pair tells forensics diff --git a/plugin/appstore/zz4_downgrade_test.go b/plugin/appstore/zz4_downgrade_test.go new file mode 100644 index 0000000..610dbd5 --- /dev/null +++ b/plugin/appstore/zz4_downgrade_test.go @@ -0,0 +1,272 @@ +package appstore + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestCompareVersions exercises the semver comparison helper. +func TestCompareVersions(t *testing.T) { + tests := []struct { + a, b string + want int + }{ + {"1.0.0", "1.0.0", 0}, + {"1.0.0", "2.0.0", -1}, + {"2.0.0", "1.0.0", 1}, + {"1.2.0", "1.1.0", 1}, + {"1.1.0", "1.2.0", -1}, + {"1.0.1", "1.0.0", 1}, + {"1.0.0", "1.0.1", -1}, + {"0.0.1", "0.0.0", 1}, + {"9.99.9", "10.0.0", -1}, + {"1.0.0-alpha", "1.0.0", -1}, // prerelease < release + {"1.0.0", "1.0.0-alpha", 1}, // release > prerelease + {"1.0.0-alpha", "1.0.0-beta", -1}, + {"1.0.0-beta", "1.0.0-alpha", 1}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%s_vs_%s", tt.a, tt.b), func(t *testing.T) { + got := compareVersions(tt.a, tt.b) + if got != tt.want { + t.Errorf("compareVersions(%q, %q) = %d, want %d", tt.a, tt.b, got, tt.want) + } + }) + } +} + +// TestRegisterRefusesDowngrade confirms that registerInstalled skips +// a manifest whose app_version is lower than an already-registered +// entry for the same app ID. +func TestRegisterRefusesDowngrade(t *testing.T) { + root := t.TempDir() + appDir := writeValidAppDir(t, root, "io.test.app") + + sup := newSupervisor(Config{InstallRoot: root}, Deps{}, newQuietLogger(t)) + + // Register the current (newer) version first. + current := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + current.Manifest.AppVersion = "2.0.0" + sup.registerInstalled([]*installedApp{current}) + if _, ok := sup.installed["io.test.app"]; !ok { + t.Fatal("io.test.app not registered") + } + if sup.installed["io.test.app"].Manifest.AppVersion != "2.0.0" { + t.Fatalf("expected 2.0.0, got %s", sup.installed["io.test.app"].Manifest.AppVersion) + } + + // Now try to register a downgrade: v1.0.0. + downgrade := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + downgrade.Manifest.AppVersion = "1.0.0" + sup.registerInstalled([]*installedApp{downgrade}) + + // The in-memory entry must still be 2.0.0. + if got := sup.installed["io.test.app"].Manifest.AppVersion; got != "2.0.0" { + t.Errorf("downgrade was accepted — version now %s, want 2.0.0", got) + } +} + +// TestRegisterAllowsUpgrade confirms registerInstalled accepts a +// same-app-id entry with a higher app_version. +func TestRegisterAllowsUpgrade(t *testing.T) { + root := t.TempDir() + appDir := writeValidAppDir(t, root, "io.test.app") + + sup := newSupervisor(Config{InstallRoot: root}, Deps{}, newQuietLogger(t)) + + old := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + old.Manifest.AppVersion = "1.0.0" + sup.registerInstalled([]*installedApp{old}) + + upgrade := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + upgrade.Manifest.AppVersion = "3.0.0" + sup.registerInstalled([]*installedApp{upgrade}) + + if got := sup.installed["io.test.app"].Manifest.AppVersion; got != "3.0.0" { + t.Errorf("upgrade not accepted — version now %s, want 3.0.0", got) + } +} + +// TestRegisterSameVersionIsIdempotent confirms registering the same +// version twice is a no-op. +func TestRegisterSameVersionIsIdempotent(t *testing.T) { + root := t.TempDir() + appDir := writeValidAppDir(t, root, "io.test.app") + + sup := newSupervisor(Config{InstallRoot: root}, Deps{}, newQuietLogger(t)) + + a1 := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + a1.Manifest.AppVersion = "1.5.0" + a2 := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + a2.Manifest.AppVersion = "1.5.0" + + sup.registerInstalled([]*installedApp{a1}) + sup.registerInstalled([]*installedApp{a2}) + if sup.installed["io.test.app"].Manifest.AppVersion != "1.5.0" { + t.Errorf("expected 1.5.0, got %s", sup.installed["io.test.app"].Manifest.AppVersion) + } +} + +// writeAppDirWithVersion creates a valid app dir with a manifest +// carrying the given version, and a stub binary. +func writeAppDirWithVersion(t *testing.T, root, id, version string) string { + t.Helper() + dir := filepath.Join(root, id) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + binDir := filepath.Join(dir, "bin") + if err := os.MkdirAll(binDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(binDir, "x"), []byte("#!/bin/sh\necho ok"), 0o755); err != nil { + t.Fatal(err) + } + raw := fmt.Sprintf( + `{"id":%q,"app_version":%q,"manifest_version":1,"binary":{"runtime":"go","path":"bin/x","sha256":"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"},"grants":[{"cap":"net.dial","target":"*"}],"store":{"publisher":"ed25519:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=","signature":"sig"}}`, + id, version, + ) + if err := os.WriteFile(filepath.Join(dir, "manifest.json"), []byte(raw), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +// TestRescanRefusesDowngradeMidRun verifies that when an on-disk +// manifest is replaced with a lower app_version while the daemon +// is running, the rescan loop refuses to accept it. +func TestRescanRefusesDowngradeMidRun(t *testing.T) { + root := t.TempDir() + appDir := writeAppDirWithVersion(t, root, "io.test.app", "2.0.0") + + sup := newSupervisor(Config{ + InstallRoot: root, + RescanInterval: 20 * 1e6, // not used directly in this test + }, Deps{}, newQuietLogger(t)) + + // Register the app in-memory as if already discovered at startup. + entry := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + entry.Manifest.AppVersion = "2.0.0" + sup.mu.Lock() + sup.installed["io.test.app"] = entry + sup.mu.Unlock() + + // Replace on-disk manifest with a downgrade (v1.0.0). + writeAppDirWithVersion(t, root, "io.test.app", "1.0.0") + fresh := sup.rescanForNew() + if len(fresh) != 0 { + t.Errorf("rescanForNew returned %d apps, want 0 (downgrade should be refused)", len(fresh)) + } + sup.mu.RLock() + v := sup.installed["io.test.app"].Manifest.AppVersion + sup.mu.RUnlock() + if v != "2.0.0" { + t.Errorf("version was downgraded: got %s, want 2.0.0", v) + } +} + +// TestRescanAllowsUpgradeMidRun verifies that when an on-disk +// manifest is replaced with a higher app_version while the daemon +// is running, the rescan loop detects and accepts it. +func TestRescanAllowsUpgradeMidRun(t *testing.T) { + root := t.TempDir() + appDir := writeAppDirWithVersion(t, root, "io.test.app", "1.0.0") + + sup := newSupervisor(Config{ + InstallRoot: root, + RescanInterval: 20 * 1e6, + }, Deps{}, newQuietLogger(t)) + + entry := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + entry.Manifest.AppVersion = "1.0.0" + sup.mu.Lock() + sup.installed["io.test.app"] = entry + sup.mu.Unlock() + + // Replace on-disk manifest with upgrade (v2.0.0). + writeAppDirWithVersion(t, root, "io.test.app", "2.0.0") + fresh := sup.rescanForNew() + if len(fresh) != 1 { + t.Fatalf("rescanForNew returned %d, want 1 (upgrade should be accepted)", len(fresh)) + } + sup.mu.RLock() + v := sup.installed["io.test.app"].Manifest.AppVersion + sup.mu.RUnlock() + if v != "2.0.0" { + t.Errorf("upgrade not applied: got %s, want 2.0.0", v) + } +} + +// TestRescanAuditLogsDowngradeRefusal confirms the supervisor writes an +// audit event when refusing a downgrade during rescan. +func TestRescanAuditLogsDowngradeRefusal(t *testing.T) { + root := t.TempDir() + appDir := writeAppDirWithVersion(t, root, "io.test.app", "2.0.0") + + sup := newSupervisor(Config{ + InstallRoot: root, + RescanInterval: 20 * 1e6, + }, Deps{}, newQuietLogger(t)) + + entry := &installedApp{ + Dir: appDir, + Manifest: parseDummyManifest(t, "io.test.app"), + BinaryPath: filepath.Join(appDir, "bin/x"), + } + entry.Manifest.AppVersion = "2.0.0" + sup.mu.Lock() + sup.installed["io.test.app"] = entry + sup.mu.Unlock() + + // Replace on-disk with downgrade. + writeAppDirWithVersion(t, root, "io.test.app", "1.0.0") + fresh := sup.rescanForNew() + if len(fresh) != 0 { + t.Errorf("downgrade should have been refused") + } + + // Check the audit log for downgrade-refused event. + auditPath := filepath.Join(appDir, supervisorLogName) + data, err := os.ReadFile(auditPath) + if err != nil { + t.Fatalf("read audit log: %v", err) + } + if !strings.Contains(string(data), "downgrade-refused") { + t.Errorf("audit log missing downgrade-refused event:\n%s", string(data)) + } +}