diff --git a/SECURITY.md b/SECURITY.md index 8aa752c..a2a5c94 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -153,7 +153,8 @@ externally connected wallet. Wallet-linked state and authentication state are never persisted in plaintext AsyncStorage. The `wallet-storage`, `session-storage`, `sync-storage`, and -`guildpass:reconciliation:v1` Zustand slices use +`guildpass:reconciliation:v1` Zustand slices, push tokens, and cached +attestation revocation registries use `expo-secure-store`, backed by the iOS Keychain and Android Keystore, with the iOS accessibility level set to `WHEN_UNLOCKED_THIS_DEVICE_ONLY`. This includes: @@ -161,7 +162,33 @@ iOS accessibility level set to `WHEN_UNLOCKED_THIS_DEVICE_ONLY`. This includes: - the connector kind (manual entry, WalletConnect, or another provider); and - session tokens, expiry timestamps, and their associated wallet address; - wallet-scoped sync metadata and reconciliation sequence numbers; and -- cached wallet role attestations, issuer verification keys, and their indexes. +- cached wallet role attestations, issuer verification keys, and their indexes; +- push-notification delivery tokens and cached issuer revocation registries. + +Secure persistence errors are surfaced to callers after any stale AsyncStorage +copy has been removed. Feature code must not treat a failed secure write as a +successful save. + +### Approved storage practices + +Classify a value before persisting it: + +- **Sensitive:** authentication/session state, provider tokens, wallet-linked + identifiers, signed credentials or attestations, access decisions, sync + metadata, and security settings. Use `migratingSecureStorage` from + `src/lib/storage` for small state. Large offline datasets must use the + AES-GCM encrypted persister with its device-bound key. +- **Non-sensitive:** presentation-only preferences that cannot identify a user + or affect authentication or authorization. These may use `asyncStorage` from + `src/lib/storage`. +- **Ephemeral:** private keys, seed phrases, raw signing material, one-time + challenges, and transient access scans. Keep these in provider/native custody + or memory; do not persist them in application storage. + +Feature modules must not import `AsyncStorage` or `expo-secure-store` directly. +Native storage access belongs in the centralized storage/encrypted-persister +modules. New legacy key families must also be registered in the startup +migration and covered by a plaintext-removal test. ### Upgrade migration diff --git a/docs/threat-model.md b/docs/threat-model.md index 7eb9eef..7471969 100644 --- a/docs/threat-model.md +++ b/docs/threat-model.md @@ -44,9 +44,9 @@ The hardening layer consists of two controls: | Asset | Sensitivity | Storage | | -------------------------------------- | ------------ | --------------------------------------------- | -| Wallet address / identifier | Medium | AsyncStorage (public data) | +| Wallet address / identifier | Medium | `expo-secure-store` | | Access-control decisions (QR payloads) | High | In-memory only | -| Signed attestations / proofs | High | Transient; not persisted | +| Signed attestations / proofs | High | `expo-secure-store` when cached for offline use | | Future: embedded private keys | **Critical** | Planned: `expo-secure-store` / Secure Enclave | | Session / auth tokens | High | `expo-secure-store` | diff --git a/src/lib/mutationQueue.ts b/src/lib/mutationQueue.ts index 35f6941..186bf2a 100644 --- a/src/lib/mutationQueue.ts +++ b/src/lib/mutationQueue.ts @@ -94,6 +94,9 @@ class MutationQueueManager { const envelope = JSON.parse(stored) as Partial; if (envelope.v !== ENVELOPE_MAGIC || !envelope.n || !envelope.t || !envelope.c) { + // Older releases could leave queue payloads as plaintext JSON. Never + // hydrate or retain an unrecognised value containing mutation payloads. + await AsyncStorage.removeItem(MUTATION_QUEUE_STORAGE_KEY); this.queueCache = []; this.notifyListeners(); return this.queueCache; @@ -123,6 +126,9 @@ class MutationQueueManager { this.queueCache = Array.isArray(decrypted) ? decrypted : []; } catch (e) { console.error("[MutationQueue] Error loading queue", e); + // A malformed legacy/corrupt entry may contain plaintext. Encrypted + // envelopes are authenticated, so removing either form is fail-closed. + await AsyncStorage.removeItem(MUTATION_QUEUE_STORAGE_KEY).catch(() => {}); this.queueCache = []; } diff --git a/src/lib/storage/index.ts b/src/lib/storage/index.ts index 8879f9b..d4eb33d 100644 --- a/src/lib/storage/index.ts +++ b/src/lib/storage/index.ts @@ -26,14 +26,18 @@ const SENSITIVE_LEGACY_KEYS = new Set([ "wallet-storage", "session-storage", "sync-storage", + "biometric-storage", "guildpass:reconciliation:v1", + "guildpass:push-notifications:v1", "guildpass:issuer-keys-index", + "guildpass:attestation-key-registry-index", ]); const SENSITIVE_LEGACY_KEY_PREFIXES = [ "guildpass:attestations:", "guildpass:attestation-index", "guildpass:issuer-keys:", + "guildpass:attestation-key-registry:", ]; function getLegacyEncodedSecureStorageKey(name: string): string { @@ -330,13 +334,16 @@ export const migratingSecureStorage: StateStorage = { }, setItem: async (name: string, value: string): Promise => { + let writeError: unknown; try { await rawSecureStorage.setItem(name, value); } catch (error) { console.error(`Error writing sensitive data to SecureStore: ${name}`, error); + writeError = error; } // Never retain a fallback or stale plaintext copy. await clearLegacyOrFailClosed(name); + if (writeError) throw writeError; }, removeItem: async (name: string): Promise => { diff --git a/tests/mutationQueue.test.ts b/tests/mutationQueue.test.ts index 8aadc4b..d0b1703 100644 --- a/tests/mutationQueue.test.ts +++ b/tests/mutationQueue.test.ts @@ -32,6 +32,26 @@ describe("MutationQueue", () => { expect(queue).toHaveLength(0); }); + it("fails closed and removes a legacy plaintext mutation queue", async () => { + vi.mocked(AsyncStorage.getItem).mockResolvedValueOnce( + JSON.stringify([ + { + id: "legacy", + type: MutationType.UPDATE_PROFILE, + payload: { email: "sensitive@example.com" }, + createdAt: 1, + status: "PENDING", + retryCount: 0, + }, + ]), + ); + + const queue = await mutationQueue.getQueue(); + + expect(queue).toEqual([]); + expect(AsyncStorage.removeItem).toHaveBeenCalledWith("GUILDPASS_MUTATION_QUEUE"); + }); + it("dequeues successfully", async () => { const item = await mutationQueue.enqueue(MutationType.UPDATE_PROFILE, { name: "Test" }); let queue = await mutationQueue.getQueue(); diff --git a/tests/storage.test.ts b/tests/storage.test.ts index ac6ec93..61e5953 100644 --- a/tests/storage.test.ts +++ b/tests/storage.test.ts @@ -346,6 +346,15 @@ describe("Persistence and Rehydration", () => { ], ["guildpass:issuer-keys:guild-1", JSON.stringify({ issuerAddress: walletAddress })], ["guildpass:issuer-keys-index", JSON.stringify(["guild-1"])], + [ + "guildpass:push-notifications:v1", + JSON.stringify({ state: { pushToken: "ExponentPushToken[legacy-secret]" } }), + ], + [ + "guildpass:attestation-key-registry:guild-1", + JSON.stringify({ guildId: "guild-1", revokedAddresses: [walletAddress] }), + ], + ["guildpass:attestation-key-registry-index", JSON.stringify(["guild-1"])], ]); const asyncGet = vi.mocked(AsyncStorage.getItem); const asyncRemove = vi.mocked(AsyncStorage.removeItem); @@ -380,7 +389,7 @@ describe("Persistence and Rehydration", () => { const report = await migrateLegacySensitiveStorage(); expect(report.failedKeys).toEqual([]); - expect(report.migratedKeys).toHaveLength(8); + expect(report.migratedKeys).toHaveLength(11); expect(legacyEntries.size).toBe(0); for (const [key] of vi.mocked(SecureStore.setItemAsync).mock.calls) { expect(key).toMatch(/^[\w.-]+$/); @@ -403,6 +412,22 @@ describe("Persistence and Rehydration", () => { } }); + it("reports secure write failures after removing any plaintext fallback", async () => { + const consoleError = vi.spyOn(console, "error").mockImplementation(() => undefined); + vi.mocked(SecureStore.getItemAsync).mockResolvedValueOnce(null); + vi.mocked(SecureStore.setItemAsync).mockRejectedValueOnce(new Error("Keystore unavailable")); + vi.mocked(AsyncStorage.getItem).mockResolvedValueOnce(null); + + try { + await expect( + migratingSecureStorage.setItem("session-storage", "sensitive-session"), + ).rejects.toThrow("Keystore unavailable"); + expect(AsyncStorage.removeItem).toHaveBeenCalledWith("session-storage"); + } finally { + consoleError.mockRestore(); + } + }); + it("does not hydrate legacy data when AsyncStorage cleanup cannot be verified", async () => { const consoleError = vi.spyOn(console, "error").mockImplementation(() => undefined); const asyncGet = vi.mocked(AsyncStorage.getItem);