diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 3eac03e6a1..ea74007481 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Scope the storage-key and snap-signature caches in `UserStorageController` per `entropySourceId` ([#8948](https://github.com/MetaMask/core/pull/8948)) + - In the edge case where two SRPs resolve to the same `profileId` (e.g. a shared canonical profile after pairing), the previously message-keyed caches could hand one SRP the other's cached key — letting it read/decrypt the other SRP's user storage (which surfaced as a second SRP inheriting the first SRP's account names) or write under its key. Scoping by `entropySourceId` keeps each SRP's key isolated even then; the signed `metamask:${profileId}` message is unchanged, so existing storage keys are preserved. + ## [28.1.1] ### Changed diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 89adf56922..1081690d8f 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -1,7 +1,14 @@ import { deriveStateFromMetadata } from '@metamask/base-controller'; -import type nock from 'nock'; +import { KeyringTypes } from '@metamask/keyring-controller'; +import nock from 'nock'; -import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; +import { Env, getEnvUrls } from '../../sdk'; +import { createSHA256Hash } from '../../shared/encryption'; +import type { UserStorageGenericPathWithFeatureAndKey } from '../../shared/storage-schema'; +import { + createEntryPath, + USER_STORAGE_FEATURE_NAMES, +} from '../../shared/storage-schema'; import { mockUserStorageMessenger } from './__fixtures__/mockMessenger'; import { mockEndpointBatchUpsertUserStorage, @@ -714,6 +721,126 @@ describe('UserStorageController', () => { expect(await controller.getStorageKey()).toBe(MOCK_STORAGE_KEY); }); + it('does not serve the cached primary key to a new primary after a vault restore regenerates the entropy source id', async () => { + const messengerMocks = mockUserStorageMessenger(); + // The signed message (`metamask:${profileId}`) is identical across both + // calls, so the only thing that can isolate the two vaults is the entropy + // scope. The HD keyring metadata id is randomly regenerated on restore. + messengerMocks.mockSnapSignMessage + .mockResolvedValueOnce('signature-before-restore') + .mockResolvedValueOnce('signature-after-restore'); + + const controller = new UserStorageController({ + messenger: messengerMocks.messenger, + }); + + const keyBeforeRestore = await controller.getStorageKey(); + + // Simulate a vault restore: same primary slot, brand-new entropy id. + messengerMocks.mockKeyringGetState.mockReturnValue({ + isUnlocked: true, + keyrings: [ + { + type: KeyringTypes.hd, + accounts: [], + metadata: { id: 'restored-entropy-source-id', name: '' }, + }, + ], + }); + + const keyAfterRestore = await controller.getStorageKey(); + + // The regenerated id changes the cache scope, so the new primary must + // re-derive its own key instead of inheriting the previous vault's cached + // key — proving no `'primary'`-style stable key carries across restores. + expect(messengerMocks.mockSnapSignMessage).toHaveBeenCalledTimes(2); + expect(keyAfterRestore).not.toBe(keyBeforeRestore); + expect(keyAfterRestore).toBe(createSHA256Hash('signature-after-restore')); + }); + + it('serves the snap signature from the entropy-scoped cache for the same entropy source, even after the storage-key cache is flushed', async () => { + const messengerMocks = mockUserStorageMessenger(); + const controller = new UserStorageController({ + messenger: messengerMocks.messenger, + }); + + const mockAPI1 = await mockEndpointGetUserStorage(); + const mockAPI2 = await mockEndpointGetUserStorage(); + + await controller.performGetStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + 'entropy-source-1', + ); + // Drop the derived storage key so the next call must re-derive it. The + // signature must still come from the entropy-scoped snap-signature cache, + // so the snap is never asked to sign a second time for this SRP. (Without + // this flush the storage-key cache would short-circuit before the + // signature cache is ever exercised.) + controller.flushStorageKeyCache(); + await controller.performGetStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + 'entropy-source-1', + ); + + mockAPI1.done(); + mockAPI2.done(); + expect(messengerMocks.mockSnapSignMessage).toHaveBeenCalledTimes(1); + }); + + it('derives a distinct storage key per entropy source even when both resolve to the same profileId', async () => { + const messengerMocks = mockUserStorageMessenger(); + + // Precondition (made explicit, not relying on the fixture default): both + // SRPs resolve to the SAME `profileId`, so the signed + // `metamask:${profileId}` message is identical. Isolation must therefore + // come from `entropySourceId`, never the message — otherwise one SRP + // reuses another SRP's storage key and reads/writes its user storage. + // `profileId === canonicalProfileId` mirrors the real bug: a secondary + // SRP whose own profileId was overwritten with the shared canonical. + messengerMocks.mockAuthGetSessionProfile.mockResolvedValue({ + identifierId: 'shared-identifier-id', + profileId: 'shared-profile-id', + canonicalProfileId: 'shared-profile-id', + metaMetricsId: 'shared-metametrics-id', + }); + // Each entropy source signs with its own key, so the identical message + // yields a different signature — and thus a different derived storage key. + messengerMocks.mockSnapSignMessage + .mockResolvedValueOnce('signature-for-entropy-source-1') + .mockResolvedValueOnce('signature-for-entropy-source-2'); + + const controller = new UserStorageController({ + messenger: messengerMocks.messenger, + }); + + const featureKeyPath = + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings` as UserStorageGenericPathWithFeatureAndKey; + const baseUrl = `${getEnvUrls(Env.PRD).userStorageApiUrl}/api/v1/userstorage`; + const pathForSource1 = `${baseUrl}/${createEntryPath( + featureKeyPath, + createSHA256Hash('signature-for-entropy-source-1'), + )}`; + const pathForSource2 = `${baseUrl}/${createEntryPath( + featureKeyPath, + createSHA256Hash('signature-for-entropy-source-2'), + )}`; + + // One endpoint per derived storage key. If the caches were shared, the + // second source would reuse the first's key, hit `pathForSource1` again, + // and leave `mockSource2` unsatisfied (and the second request unmatched). + const mockSource1 = nock(pathForSource1).get('').reply(404); + const mockSource2 = nock(pathForSource2).get('').reply(404); + + await controller.performGetStorage(featureKeyPath, 'entropy-source-1'); + await controller.performGetStorage(featureKeyPath, 'entropy-source-2'); + + // Both distinct paths were requested → the two SRPs derived different + // storage keys despite the shared profileId. + mockSource1.done(); + mockSource2.done(); + expect(messengerMocks.mockSnapSignMessage).toHaveBeenCalledTimes(2); + }); + it('throws if the wallet is locked', async () => { const messengerMocks = mockUserStorageMessenger(); messengerMocks.mockKeyringGetState.mockReturnValue({ @@ -737,7 +864,13 @@ describe('UserStorageController', () => { messengerMocks.mockKeyringGetState.mockReturnValue({ isUnlocked: true, - keyrings: [], + keyrings: [ + { + type: KeyringTypes.hd, + accounts: [], + metadata: { id: 'primary-entropy-source-id', name: '' }, + }, + ], }); const controller = new UserStorageController({ diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 719b53af53..2e4d74ea86 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -242,7 +242,13 @@ export class UserStorageController extends BaseController< #isUnlocked = false; - #storageKeyCache: Record<`metamask:${string}`, string> = {}; + // Both caches are keyed by `${entropySourceId}:${message}` (the primary SRP + // resolves to its HD keyring metadata ID) so two SRPs that transiently + // resolve to the same `profileId` can never share a cached storage key / + // signature and leak data across each other's user storage. + #storageKeyCache: Record = {}; + + #snapSignMessageCache: Record = {}; readonly #keyringController = { setupLockedStateSubscriptions: () => { @@ -323,10 +329,23 @@ export class UserStorageController extends BaseController< }, { storage: { - getStorageKey: async (message) => - this.#storageKeyCache[message] ?? null, - setStorageKey: async (message, key) => { - this.#storageKeyCache[message] = key; + getStorageKey: async (message, entropySourceId) => { + // No derived key can exist while locked (the KeyringController + // clears its keyrings on lock, so the scope is unresolvable). + if (!this.#isUnlocked) { + return null; + } + return ( + this.#storageKeyCache[ + this.#scopedCacheKey(message, entropySourceId) + ] ?? null + ); + }, + // Only ever reached after a successful signature, i.e. while unlocked. + setStorageKey: async (message, key, entropySourceId) => { + this.#storageKeyCache[ + this.#scopedCacheKey(message, entropySourceId) + ] = key; }, }, }, @@ -510,13 +529,62 @@ export class UserStorageController extends BaseController< ); } + return this.#getHdKeyringEntropySourceIds(); + } + + /** + * Reads the HD keyring entropy source IDs (metadata IDs) from the + * KeyringController, primary first. Returns an empty array when none are + * available (e.g. the wallet is locked, where `keyrings` is cleared). + * + * @returns The HD keyring metadata IDs, primary first. + */ + #getHdKeyringEntropySourceIds(): string[] { const { keyrings } = this.messenger.call('KeyringController:getState'); - return keyrings + return (keyrings ?? []) .filter((keyring) => keyring.type === KeyringTypes.hd.toString()) .map((keyring) => keyring.metadata.id); } - #_snapSignMessageCache: Record<`metamask:${string}`, string> = {}; + /** + * Resolves the primary SRP's entropy source ID (the first HD keyring's + * metadata ID), used to scope the primary's cache entries. The ID is randomly + * regenerated whenever the vault is recreated (e.g. on restore), so a new + * primary can never inherit a previous vault's cached key. + * + * @returns The primary HD keyring metadata ID. + * @throws If no HD keyring is available; callers must only resolve the scope + * while the wallet is unlocked. + */ + #getPrimaryEntropySourceId(): string { + const [primaryEntropySourceId] = this.#getHdKeyringEntropySourceIds(); + if (!primaryEntropySourceId) { + throw new Error('#getPrimaryEntropySourceId - no HD keyring available'); + } + return primaryEntropySourceId; + } + + /** + * Builds a cache key scoped to a specific entropy source, so each SRP's + * signature/storage key derivation stays isolated even when two SRPs + * transiently resolve to the same `profileId` (see `#storageKeyCache`). + * + * When `entropySourceId` is omitted (primary SRP), it is resolved to the + * primary HD keyring's metadata ID rather than a stable literal. Because that + * ID is randomly regenerated whenever the vault is recreated (e.g. on + * restore), the cached entry is naturally invalidated across vaults — a + * different SRP can never inherit the previous primary's cached key. + * + * @param message - The tagged message used for signing. + * @param entropySourceId - The entropy source ID. Omit for the primary SRP. + * @returns The scoped cache key. + */ + #scopedCacheKey( + message: `metamask:${string}`, + entropySourceId?: string, + ): string { + return `${entropySourceId ?? this.#getPrimaryEntropySourceId()}:${message}`; + } /** * Signs a specific message using an underlying auth snap. @@ -530,23 +598,23 @@ export class UserStorageController extends BaseController< message: `metamask:${string}`, entropySourceId?: string, ): Promise { - // the message is SRP specific already, so there's no need to use the entropySourceId in the cache - if (this.#_snapSignMessageCache[message]) { - return this.#_snapSignMessageCache[message]; - } - if (!this.#isUnlocked) { throw new Error( '#snapSignMessage - unable to call snap, wallet is locked', ); } + const cacheKey = this.#scopedCacheKey(message, entropySourceId); + if (this.#snapSignMessageCache[cacheKey]) { + return this.#snapSignMessageCache[cacheKey]; + } + const result = (await this.messenger.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message, entropySourceId), )) as string; - this.#_snapSignMessageCache[message] = result; + this.#snapSignMessageCache[cacheKey] = result; return result; } diff --git a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts index 2ec6930e25..74be400a3c 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockMessenger.ts @@ -1,3 +1,4 @@ +import { KeyringTypes } from '@metamask/keyring-controller'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { MockAnyNamespace, @@ -152,7 +153,13 @@ export function mockUserStorageMessenger( 'KeyringController:getState', ).mockReturnValue({ isUnlocked: true, - keyrings: [], + keyrings: [ + { + type: KeyringTypes.hd, + accounts: [], + metadata: { id: 'primary-entropy-source-id', name: '' }, + }, + ], }); const mockAccountsListAccounts = jest.fn(); diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 5f1f743152..df3b59754f 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -22,8 +22,15 @@ export type UserStorageConfig = { }; export type StorageOptions = { - getStorageKey: (message: `metamask:${string}`) => Promise; - setStorageKey: (message: `metamask:${string}`, val: string) => Promise; + getStorageKey: ( + message: `metamask:${string}`, + entropySourceId?: string, + ) => Promise; + setStorageKey: ( + message: `metamask:${string}`, + val: string, + entropySourceId?: string, + ) => Promise; }; export type UserStorageOptions = { @@ -122,7 +129,10 @@ export class UserStorage { const userProfile = await this.config.auth.getUserProfile(entropySourceId); const message = `metamask:${userProfile.profileId}` as const; - const storageKey = await this.options.storage?.getStorageKey(message); + const storageKey = await this.options.storage?.getStorageKey( + message, + entropySourceId, + ); if (storageKey) { return storageKey; } @@ -135,6 +145,7 @@ export class UserStorage { await this.options.storage?.setStorageKey( message, hashedStorageKeySignature, + entropySourceId, ); return hashedStorageKeySignature; }