Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/profile-sync-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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({
Expand All @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, string> = {};

#snapSignMessageCache: Record<string, string> = {};

readonly #keyringController = {
setupLockedStateSubscriptions: () => {
Expand Down Expand Up @@ -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;
},
},
},
Expand Down Expand Up @@ -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.
Expand All @@ -530,23 +598,23 @@ export class UserStorageController extends BaseController<
message: `metamask:${string}`,
entropySourceId?: string,
): Promise<string> {
// 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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { KeyringTypes } from '@metamask/keyring-controller';
import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger';
import type {
MockAnyNamespace,
Expand Down Expand Up @@ -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();
Expand Down
17 changes: 14 additions & 3 deletions packages/profile-sync-controller/src/sdk/user-storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,15 @@ export type UserStorageConfig = {
};

export type StorageOptions = {
getStorageKey: (message: `metamask:${string}`) => Promise<string | null>;
setStorageKey: (message: `metamask:${string}`, val: string) => Promise<void>;
getStorageKey: (
message: `metamask:${string}`,
entropySourceId?: string,
) => Promise<string | null>;
setStorageKey: (
message: `metamask:${string}`,
val: string,
entropySourceId?: string,
) => Promise<void>;
};

export type UserStorageOptions = {
Expand Down Expand Up @@ -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;
}
Expand All @@ -135,6 +145,7 @@ export class UserStorage {
await this.options.storage?.setStorageKey(
message,
hashedStorageKeySignature,
entropySourceId,
);
return hashedStorageKeySignature;
}
Expand Down
Loading