From 413c7862d31e429b1e1a7a016c17dcfc45aaa900 Mon Sep 17 00:00:00 2001 From: Reese Norris Date: Tue, 28 Jul 2026 23:44:14 -0400 Subject: [PATCH] fix: address review feedback for xPilot adapter Tighten Verify (manifest+bak+size; no bak-only short-circuit), run Verify from HealthCheck, block non-ASCII length immediates, document upgrade safety and ASCII-only length-imm contract. --- docs/client-injector/research/xpilot.md | 26 ++- .../clientinject/adapters/xpilot/adapter.go | 86 ++++++++-- .../adapters/xpilot/adapter_test.go | 149 ++++++++++++++++++ .../clientinject/adapters/xpilot/health.go | 16 +- internal/clientinject/adapters/xpilot/plan.go | 25 ++- 5 files changed, 276 insertions(+), 26 deletions(-) diff --git a/docs/client-injector/research/xpilot.md b/docs/client-injector/research/xpilot.md index a029cd3..c8ef9b2 100644 --- a/docs/client-injector/research/xpilot.md +++ b/docs/client-injector/research/xpilot.md @@ -71,6 +71,27 @@ file_offset = section.raw_offset + (section_address_va - section.virtual_start) LEA RIP displacements in prior art point at the **relocated** `.idata` slots (not the stock string sites). They are **content-independent** for a fixed slot address. Length immediates **must** match the new URL length at Apply time. +### Length immediate + non-ASCII hosts + +Prior-art length patches are **one byte** set to the sample URL’s character count (ASCII: `len == rune count`). The adapter: + +- Uses `utf8.RuneCountInString` for the length imm (equals byte length for ASCII). +- **Blocks non-ASCII** `WebBaseURL` / status / JWT URLs at Plan time — multi-byte UTF-8 would make “character count vs byte length” ambiguous vs the PE imm, and was never proven against a live xPilot build. + +openfsd production hosts are expected to be ASCII DNS labels. + +### PE identity / upgrade safety (Verify) + +| Live PE | Bak | Manifest | Result | +|---------|-----|----------|--------| +| SHA-1 = stock | any | any | **Accept** (first-time / post-Revert stock) | +| SHA-1 ≠ stock | missing or not stock | — | **Refuse** unknown hash | +| SHA-1 ≠ stock | stock | missing | **Refuse** (leftover bak after upgrade) | +| SHA-1 ≠ stock | stock | status `reverted` / `failed` | **Refuse** (leftover bak after Revert) | +| SHA-1 ≠ stock | stock | `applied` or `in_progress` + ProfileID match + **live size == bak size** | **Accept** (re-apply / mid-Apply HealthCheck) | + +In-place `padded_string` / `raw_overwrite` never change PE length, so size mismatch with bak means the live binary was replaced. `size_bytes` / SHA-256 remain unknown until a maintainer re-hashes a user-owned install; bak-size equality covers the upgrade case without inventing numbers. + Example prior-art URLs (length reference only): - `https://yourfsdserver.com/api/v1/data/status.json` → length `49` @@ -97,12 +118,13 @@ Example prior-art URLs (length reference only): ## Honesty / known gaps -- [ ] Re-hash a maintainer-owned 3.0.1 install; add SHA-256 + `size_bytes` when confirmed. +- [ ] Re-hash a maintainer-owned 3.0.1 install; add SHA-256 + `size_bytes` when confirmed (adapter already enforces live size == bak size on re-apply). - [ ] Confirm live connect path after status.json + fsd-jwt retarget (server list fields xPilot expects). - [ ] Inventory AFV / voice base URL sites in the same PE (and companion DLLs if any). -- [ ] Confirm single-byte length immediates for non-ASCII hosts (openfsd URLs are ASCII). +- [x] Non-ASCII length imm: **blocked at Plan** (ASCII-only contract); prior art unproven for non-ASCII. - [ ] Antivirus / code-signing interaction when rewriting signed `xPilot.exe`. - [ ] Newer xPilot versions need **new** version-pinned profiles — do not reuse 3.0.1 offsets. +- [x] Verify bak short-circuit tightened: require applied manifest + bak stock + size parity (not bak alone). ## Legal / product constraints diff --git a/internal/clientinject/adapters/xpilot/adapter.go b/internal/clientinject/adapters/xpilot/adapter.go index 94a5898..068e0d5 100644 --- a/internal/clientinject/adapters/xpilot/adapter.go +++ b/internal/clientinject/adapters/xpilot/adapter.go @@ -131,7 +131,22 @@ func (a *Adapter) Discover(ctx context.Context) ([]clientinject.InstallCandidate return cands, nil } -// Verify checks primary PE SHA-1 against the profile (stock bak allowed post-Apply). +// Verify checks primary PE identity against the profile. +// +// Accept when: +// 1. Live PE SHA-1 equals profile stock (first-time / post-Revert stock), or +// 2. Re-apply path: live is not stock, but +// - sibling .openfsd-bak content hashes to stock, +// - live PE size equals bak size (in-place patches never resize the PE), +// - inject manifest exists under install root with matching ProfileID, +// - manifest status is "applied" or "in_progress" (in_progress while +// Engine.Apply runs HealthCheck; applied for re-plan/re-apply). Status +// "reverted"/"failed" is refused so leftover bak after Revert + upgrade +// cannot pass, +// - optional profile size_bytes matches live (and thus bak). +// +// Never accept a non-stock live PE solely because bak matches stock or +// install.HashSHA1 is pre-seeded to stock. func (a *Adapter) Verify(install clientinject.Install, profile *clientinject.Profile) error { if profile == nil { return fmt.Errorf("xpilot: nil profile") @@ -154,24 +169,61 @@ func (a *Adapter) Verify(install clientinject.Install, profile *clientinject.Pro if want == "" { return fmt.Errorf("xpilot: profile has no primary_binary.sha1") } - if !strings.EqualFold(got, want) { - bak := clientinject.BackupPath(pe) - if _, bakErr := w.Stat(bak); bakErr == nil { - if install.HashSHA1 != "" && strings.EqualFold(install.HashSHA1, want) { - return nil - } - if bakData, rerr := w.ReadFile(bak); rerr == nil { - bakSum := sha1.Sum(bakData) - if strings.EqualFold(hex.EncodeToString(bakSum[:]), want) { - return nil - } - } + wantSize := profile.PrimaryBinary.SizeBytes + + if strings.EqualFold(got, want) { + if wantSize > 0 && int64(len(data)) != wantSize { + return fmt.Errorf("xpilot: PE size %d does not match profile %d", len(data), wantSize) } + return nil + } + + // Live PE is not stock. Only allow re-apply of a PE we previously patched. + bak := clientinject.BackupPath(pe) + bakData, bakErr := w.ReadFile(bak) + if bakErr != nil { return fmt.Errorf("xpilot: PE sha1 %s does not match profile %s (stock %s); refuse unknown hash", got, profile.ProfileID, want) } - if wantSize := profile.PrimaryBinary.SizeBytes; wantSize > 0 && int64(len(data)) != wantSize { - return fmt.Errorf("xpilot: PE size %d does not match profile %d", len(data), wantSize) + bakSum := sha1.Sum(bakData) + if !strings.EqualFold(hex.EncodeToString(bakSum[:]), want) { + return fmt.Errorf("xpilot: PE sha1 %s does not match profile %s (stock %s); bak is not stock either; refuse unknown hash", + got, profile.ProfileID, want) + } + // In-place padded_string / raw_overwrite never change file length. + // A size mismatch means the live PE was replaced (upgrade) while bak lingered. + if len(data) != len(bakData) { + return fmt.Errorf("xpilot: live PE size %d != stock bak size %d (client upgraded/replaced while bak retained?); refuse unknown hash", + len(data), len(bakData)) + } + if wantSize > 0 && int64(len(data)) != wantSize { + return fmt.Errorf("xpilot: PE size %d does not match profile %d; refuse unknown hash", len(data), wantSize) + } + + root := install.RootDir + if root == "" { + root = filepath.Dir(pe) + } + m, mErr := clientinject.ReadManifest(w, root) + if mErr != nil { + return fmt.Errorf("xpilot: PE sha1 %s is not stock %s and no inject manifest under %s (leftover bak after upgrade? remove %s or reinstall matching PE); refuse unknown hash", + got, want, root, bak) + } + if m.ProfileID != "" && m.ProfileID != profile.ProfileID { + return fmt.Errorf("xpilot: manifest profile_id %q != %q; refuse unknown hash", m.ProfileID, profile.ProfileID) + } + if m.ProfileID == "" { + return fmt.Errorf("xpilot: inject manifest missing profile_id; refuse unknown hash") + } + switch m.Status { + case clientinject.ManifestStatusApplied, clientinject.ManifestStatusInProgress: + // ok — applied = prior successful inject; in_progress = mid-Apply healthcheck + default: + return fmt.Errorf("xpilot: live PE not stock (sha1 %s) and manifest status %q (want applied|in_progress); refuse unknown hash — if you upgraded xPilot, reinstall the pinned version or remove bak/manifest", + got, m.Status) + } + if m.PESHA1 != "" && !strings.EqualFold(m.PESHA1, want) { + return fmt.Errorf("xpilot: manifest pe_sha1 %s != profile stock %s; refuse unknown hash", m.PESHA1, want) } return nil } @@ -192,7 +244,7 @@ func (a *Adapter) EndpointConstraints(profile *clientinject.Profile) []clientinj Field: "StatusJSONURL", MaxRunes: maxRunes, Strategy: "padded_string", - Description: fmt.Sprintf("UTF-8 padded slot %d bytes; length imm is 1 byte (max %d)", s.PayloadBudgetBytes, maxLengthImm), + Description: fmt.Sprintf("UTF-8 padded slot %d bytes; length imm is 1 byte (max %d); ASCII hosts only", s.PayloadBudgetBytes, maxLengthImm), }) } if s, ok := profile.Strings["fsd_jwt"]; ok && s.PayloadBudgetBytes > 0 { @@ -205,7 +257,7 @@ func (a *Adapter) EndpointConstraints(profile *clientinject.Profile) []clientinj Field: "JWTURL", MaxRunes: maxRunes, Strategy: "padded_string", - Description: fmt.Sprintf("UTF-16LE padded slot %d bytes (~%d runes); length imm max %d", s.PayloadBudgetBytes, maxRunes, maxLengthImm), + Description: fmt.Sprintf("UTF-16LE padded slot %d bytes (~%d runes); length imm max %d; ASCII hosts only", s.PayloadBudgetBytes, maxRunes, maxLengthImm), }) } out = append(out, clientinject.Constraint{ diff --git a/internal/clientinject/adapters/xpilot/adapter_test.go b/internal/clientinject/adapters/xpilot/adapter_test.go index a555359..f288566 100644 --- a/internal/clientinject/adapters/xpilot/adapter_test.go +++ b/internal/clientinject/adapters/xpilot/adapter_test.go @@ -235,6 +235,106 @@ func TestVerify_RefuseUnknownHash(t *testing.T) { } } +// Regression OPEN-1: stock bak alone must not accept a wrong live PE. +func TestVerify_RefuseWrongLiveWithStockBakNoManifest(t *testing.T) { + _, stock, install, profile, a := setupInstall(t) + // Leave stock bak (as after Apply or leftover after Revert forensics). + if err := os.WriteFile(clientinject.BackupPath(install.PrimaryPE), stock, 0o644); err != nil { + t.Fatal(err) + } + // Replace live with different content same size (upgrade same length). + wrong := bytes.Repeat([]byte{0xAB}, len(stock)) + if err := os.WriteFile(install.PrimaryPE, wrong, 0o644); err != nil { + t.Fatal(err) + } + // install.HashSHA1 still stock — old short-circuit would have accepted. + install.HashSHA1 = profile.PrimaryBinary.SHA1 + err := a.Verify(install, profile) + if err == nil || !strings.Contains(err.Error(), "refuse unknown hash") { + t.Fatalf("expected refuse without applied manifest, err=%v", err) + } +} + +func TestVerify_RefuseWrongLiveWithBakRevertedManifest(t *testing.T) { + root, stock, install, profile, a := setupInstall(t) + if err := os.WriteFile(clientinject.BackupPath(install.PrimaryPE), stock, 0o644); err != nil { + t.Fatal(err) + } + wrong := bytes.Repeat([]byte{0xCD}, len(stock)) + if err := os.WriteFile(install.PrimaryPE, wrong, 0o644); err != nil { + t.Fatal(err) + } + w := clientinject.OSFileWriter{} + m := &clientinject.Manifest{ + ClientID: clientID, + ProfileID: profile.ProfileID, + PESHA1: profile.PrimaryBinary.SHA1, + Status: clientinject.ManifestStatusReverted, + InstallRoot: root, + } + if err := clientinject.WriteManifest(w, m); err != nil { + t.Fatal(err) + } + err := a.Verify(install, profile) + if err == nil || !strings.Contains(err.Error(), "refuse unknown hash") { + t.Fatalf("expected refuse on reverted manifest, err=%v", err) + } +} + +func TestVerify_RefuseSizeMismatchWithStockBak(t *testing.T) { + _, stock, install, profile, a := setupInstall(t) + if err := os.WriteFile(clientinject.BackupPath(install.PrimaryPE), stock, 0o644); err != nil { + t.Fatal(err) + } + // Different size live PE (upgrade). + if err := os.WriteFile(install.PrimaryPE, append(stock, 0x00, 0x01, 0x02), 0o644); err != nil { + t.Fatal(err) + } + // Even with applied manifest, size mismatch must fail. + w := clientinject.OSFileWriter{} + m := &clientinject.Manifest{ + ClientID: clientID, + ProfileID: profile.ProfileID, + PESHA1: profile.PrimaryBinary.SHA1, + Status: clientinject.ManifestStatusApplied, + InstallRoot: install.RootDir, + } + if err := clientinject.WriteManifest(w, m); err != nil { + t.Fatal(err) + } + err := a.Verify(install, profile) + if err == nil || !strings.Contains(err.Error(), "refuse unknown hash") { + t.Fatalf("expected size mismatch refuse, err=%v", err) + } +} + +func TestVerify_AcceptPatchedLiveWithAppliedManifest(t *testing.T) { + _, stock, install, profile, a := setupInstall(t) + // Simulate post-Apply: bak=stock, live=patched same size, status=applied. + if err := os.WriteFile(clientinject.BackupPath(install.PrimaryPE), stock, 0o644); err != nil { + t.Fatal(err) + } + patched := append([]byte(nil), stock...) + copy(patched[testStatusOff:], []byte("https://fsd.ex.co/api/v1/data/status.json")) + if err := os.WriteFile(install.PrimaryPE, patched, 0o644); err != nil { + t.Fatal(err) + } + w := clientinject.OSFileWriter{} + m := &clientinject.Manifest{ + ClientID: clientID, + ProfileID: profile.ProfileID, + PESHA1: profile.PrimaryBinary.SHA1, + Status: clientinject.ManifestStatusApplied, + InstallRoot: install.RootDir, + } + if err := clientinject.WriteManifest(w, m); err != nil { + t.Fatal(err) + } + if err := a.Verify(install, profile); err != nil { + t.Fatal(err) + } +} + func TestApplyHealth_RoundTrip(t *testing.T) { _, stock, install, profile, a := setupInstall(t) ep := clientinject.Endpoints{ @@ -248,10 +348,24 @@ func TestApplyHealth_RoundTrip(t *testing.T) { if len(plan.Blockers) != 0 { t.Fatalf("blockers: %v", plan.Blockers) } + // Engine-like bak + applied manifest so HealthCheck→Verify accepts patched PE. + if err := os.WriteFile(clientinject.BackupPath(install.PrimaryPE), stock, 0o644); err != nil { + t.Fatal(err) + } w := clientinject.OSFileWriter{} if err := a.Apply(context.Background(), plan, w); err != nil { t.Fatal(err) } + m := &clientinject.Manifest{ + ClientID: clientID, + ProfileID: profile.ProfileID, + PESHA1: profile.PrimaryBinary.SHA1, + Status: clientinject.ManifestStatusApplied, + InstallRoot: install.RootDir, + } + if err := clientinject.WriteManifest(w, m); err != nil { + t.Fatal(err) + } if err := a.HealthCheck(install, ep); err != nil { t.Fatal(err) } @@ -283,6 +397,41 @@ func TestApplyHealth_RoundTrip(t *testing.T) { } } +func TestHealthCheck_RefusesUnknownPE(t *testing.T) { + _, _, install, _, a := setupInstall(t) + if err := os.WriteFile(install.PrimaryPE, []byte("wrong-pe"), 0o644); err != nil { + t.Fatal(err) + } + err := a.HealthCheck(install, clientinject.Endpoints{ + WebBaseURL: "https://fsd.ex.co", + FSDHost: "fsd.ex.co", + }) + if err == nil || !strings.Contains(err.Error(), "verify") { + t.Fatalf("err=%v", err) + } +} + +func TestPlan_NonASCIIBlocker(t *testing.T) { + _, _, install, profile, a := setupInstall(t) + plan, err := a.Plan(install, profile, clientinject.Endpoints{ + WebBaseURL: "https://fsd.exämple.co", + FSDHost: "fsd.exämple.co", + }) + if err != nil { + t.Fatal(err) + } + found := false + for _, b := range plan.Blockers { + if strings.Contains(b, "ASCII") { + found = true + break + } + } + if !found { + t.Fatalf("expected ASCII blocker, blockers=%v", plan.Blockers) + } +} + func TestEngine_ApplyRevert_WithXPilot(t *testing.T) { root, stock, install, profile, a := setupInstall(t) store := clientinject.NewProfileStore() diff --git a/internal/clientinject/adapters/xpilot/health.go b/internal/clientinject/adapters/xpilot/health.go index 38976f4..d4ddc2b 100644 --- a/internal/clientinject/adapters/xpilot/health.go +++ b/internal/clientinject/adapters/xpilot/health.go @@ -10,7 +10,8 @@ import ( "github.com/renorris/openfsd/internal/clientinject" ) -// HealthCheck validates padded-string slots and raw length/break sites after Apply. +// HealthCheck validates PE identity (Verify) then padded-string slots and raw +// length/break sites after Apply. func (a *Adapter) HealthCheck(install clientinject.Install, ep clientinject.Endpoints) error { ep = ep.Normalize() w := a.writer() @@ -18,15 +19,20 @@ func (a *Adapter) HealthCheck(install clientinject.Install, ep clientinject.Endp if pe == "" { return fmt.Errorf("xpilot: healthcheck: empty PrimaryPE") } - data, err := w.ReadFile(pe) - if err != nil { - return fmt.Errorf("xpilot: healthcheck read PE: %w", err) - } profile, err := a.loadProfileForInstall(install) if err != nil { return fmt.Errorf("xpilot: healthcheck profile: %w", err) } + // Refuse unknown / wrong-version PE before reading slots at profile offsets. + if err := a.Verify(install, profile); err != nil { + return fmt.Errorf("xpilot: healthcheck verify: %w", err) + } + + data, err := w.ReadFile(pe) + if err != nil { + return fmt.Errorf("xpilot: healthcheck read PE: %w", err) + } statusURL := ep.StatusJSONURL() jwtURL := ep.JWTURL() diff --git a/internal/clientinject/adapters/xpilot/plan.go b/internal/clientinject/adapters/xpilot/plan.go index 1bfa136..35714be 100644 --- a/internal/clientinject/adapters/xpilot/plan.go +++ b/internal/clientinject/adapters/xpilot/plan.go @@ -29,6 +29,14 @@ func (a *Adapter) Plan(install clientinject.Install, profile *clientinject.Profi plan.Blockers = append(plan.Blockers, "FSDHost is required (used with openfsd status feed / operator checklist)") } + // Length immediates are single-byte character counts proven only for ASCII + // (prior-art examples). Non-ASCII hosts would make UTF-8 byte length diverge + // from rune count used for the imm — refuse rather than guess. + if ep.WebBaseURL != "" && !isASCII(ep.WebBaseURL) { + plan.Blockers = append(plan.Blockers, + "WebBaseURL must be ASCII; xPilot 3.0.1 PE length immediates are single-byte character counts (non-ASCII hosts unproven)") + } + statusURL := ep.StatusJSONURL() jwtURL := ep.JWTURL() // Prefer full /api/v1/fsd-jwt; PreferShortJWTPath still works via Endpoints.JWTURL. @@ -183,7 +191,10 @@ func (a *Adapter) Plan(install clientinject.Install, profile *clientinject.Profi } func checkURLFits(url string, profile *clientinject.Profile, stringKey, encoding string) error { - n := utf8.RuneCountInString(url) + if !isASCII(url) { + return fmt.Errorf("%s URL must be ASCII; xPilot 3.0.1 length immediates are single-byte character counts (non-ASCII unproven)", stringKey) + } + n := utf8.RuneCountInString(url) // equals len(url) for ASCII if n > maxLengthImm { return fmt.Errorf("%s URL length %d exceeds single-byte length immediate max %d", stringKey, n, maxLengthImm) } @@ -200,7 +211,7 @@ func checkURLFits(url string, profile *clientinject.Profile, stringKey, encoding case "utf16le", "utf-16le", "utf16": need = (n + 1) * 2 // runes + NUL default: - // utf8/ascii: bytes + NUL; openfsd URLs are ASCII so runes==bytes. + // utf8/ascii: bytes + NUL (ASCII: runes == bytes). need = len(url) + 1 } if need > budget { @@ -209,6 +220,16 @@ func checkURLFits(url string, profile *clientinject.Profile, stringKey, encoding return nil } +// isASCII reports whether s contains only bytes < 128 (openfsd production hosts). +func isASCII(s string) bool { + for i := 0; i < len(s); i++ { + if s[i] > 127 { + return false + } + } + return true +} + func updateConstraintStrategy(plan *clientinject.Plan, field, strategy string) { for i := range plan.Constraints { if plan.Constraints[i].Field == field {