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.
2026-08-13 04:55:42 +08:00 · 2026-07-28 23:44:14 -04:00
parent 694c3bb7ee
commit 413c7862d3
5 changed files with 276 additions and 26 deletions
--- 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

--- 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{
--- 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()
--- 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()
--- 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 {