diff --git a/packages/wallet-cli/CHANGELOG.md b/packages/wallet-cli/CHANGELOG.md index 1f4ed792e4..89731d6eaa 100644 --- a/packages/wallet-cli/CHANGELOG.md +++ b/packages/wallet-cli/CHANGELOG.md @@ -15,4 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `mm daemon stop`, `mm daemon status`, `mm daemon purge` manage daemon lifecycle and state. - Persist daemon state to a SQLite database at `/wallet.db`; subsequent `daemon start` runs reuse the persisted KeyringController vault instead of re-importing the SRP ([#8446](https://github.com/MetaMask/core/pull/8446)). +### Changed + +- Daemon password and secret recovery phrase are now wrapped in opaque `Password` and `Srp` classes that redact themselves under `util.inspect`, `JSON.stringify`, `toString`, and template-literal interpolation; the underlying string is reachable only via `unwrap()` at trust boundaries ([#8778](https://github.com/MetaMask/core/issues/8778)). + - `Srp.from` validates word count (12/15/18/21/24) and that every word is in the BIP-39 English wordlist, surfacing typos at the CLI boundary instead of producing a malformed mnemonic downstream. + [Unreleased]: https://github.com/MetaMask/core/ diff --git a/packages/wallet-cli/package.json b/packages/wallet-cli/package.json index 000c5b4a7e..0ec27ed587 100644 --- a/packages/wallet-cli/package.json +++ b/packages/wallet-cli/package.json @@ -49,6 +49,7 @@ "@inquirer/confirm": "^6.0.11", "@metamask/remote-feature-flag-controller": "^4.2.0", "@metamask/rpc-errors": "^7.0.2", + "@metamask/scure-bip39": "^2.1.1", "@metamask/utils": "^11.9.0", "@metamask/wallet": "^0.0.0", "@oclif/core": "^4.10.5", diff --git a/packages/wallet-cli/src/commands/daemon/start.test.ts b/packages/wallet-cli/src/commands/daemon/start.test.ts index 68e044010e..db4d6ae3a7 100644 --- a/packages/wallet-cli/src/commands/daemon/start.test.ts +++ b/packages/wallet-cli/src/commands/daemon/start.test.ts @@ -6,13 +6,15 @@ jest.mock('../../daemon/daemon-spawn'); const mockEnsureDaemon = jest.mocked(ensureDaemon); +const SRP = 'test test test test test test test test test test test ball'; + const FLAGS = [ '--infura-project-id', 'key', '--password', 'pw', '--srp', - 'phrase', + SRP, ]; describe('daemon start', () => { diff --git a/packages/wallet-cli/src/commands/daemon/start.ts b/packages/wallet-cli/src/commands/daemon/start.ts index fb14e29f4d..193ef0ef79 100644 --- a/packages/wallet-cli/src/commands/daemon/start.ts +++ b/packages/wallet-cli/src/commands/daemon/start.ts @@ -1,6 +1,7 @@ import { Command, Flags } from '@oclif/core'; import { ensureDaemon } from '../../daemon/daemon-spawn'; +import { Password, Srp } from '../../daemon/secrets'; export default class DaemonStart extends Command { static override description = 'Start the wallet daemon'; @@ -33,7 +34,8 @@ export default class DaemonStart extends Command { public async run(): Promise { const { flags } = await this.parse(DaemonStart); const infuraProjectId = flags['infura-project-id']; - const { password, srp } = flags; + const password = Password.from(flags.password); + const srp = Srp.from(flags.srp); const { state, socketPath } = await ensureDaemon({ dataDir: this.config.dataDir, diff --git a/packages/wallet-cli/src/daemon/daemon-entry.test.ts b/packages/wallet-cli/src/daemon/daemon-entry.test.ts index 6b81d63da2..252c179bb1 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.test.ts @@ -185,13 +185,21 @@ describe('daemon-entry', () => { recursive: true, mode: 0o700, }); - expect(mockCreateWallet).toHaveBeenCalledWith({ - databasePath: '/tmp/wallet.db', - infuraProjectId: 'key', - password: 'pass', - srp: 'test test test test test test test test test test test ball', - log: expect.any(Function), - }); + expect(mockCreateWallet).toHaveBeenCalledWith( + expect.objectContaining({ + databasePath: '/tmp/wallet.db', + infuraProjectId: 'key', + log: expect.any(Function), + }), + ); + // The Password/Srp instances are constructed in an isolated module scope + // (jest.isolateModulesAsync), so their class identity differs from any + // import in this test file. Verify structurally via `.unwrap()`. + const passedConfig = mockCreateWallet.mock.calls[0][0]; + expect(passedConfig.password.unwrap()).toBe('pass'); + expect(passedConfig.srp.unwrap()).toBe( + 'test test test test test test test test test test test ball', + ); expect(mockWriteFile).toHaveBeenCalledWith( '/tmp/daemon.pid', expect.stringMatching(new RegExp(`^${process.pid}\\n\\d+\\n$`, 'u')), diff --git a/packages/wallet-cli/src/daemon/daemon-entry.ts b/packages/wallet-cli/src/daemon/daemon-entry.ts index 8a27f7df40..cd51a183ac 100644 --- a/packages/wallet-cli/src/daemon/daemon-entry.ts +++ b/packages/wallet-cli/src/daemon/daemon-entry.ts @@ -8,6 +8,7 @@ import { pingDaemon } from './daemon-client'; import { getDaemonPaths } from './paths'; import { startRpcSocketServer } from './rpc-socket-server'; import type { RpcSocketServerHandle } from './rpc-socket-server'; +import { Password, Srp } from './secrets'; import type { DaemonStatusInfo, RpcHandlerMap } from './types'; import { isErrorWithCode, isProcessAlive, readPidFile } from './utils'; import { createWallet } from './wallet-factory'; @@ -33,15 +34,17 @@ async function main(): Promise { throw new Error('INFURA_PROJECT_ID environment variable is required'); } - const password = process.env.MM_WALLET_PASSWORD; - if (!password) { + const passwordRaw = process.env.MM_WALLET_PASSWORD; + if (!passwordRaw) { throw new Error('MM_WALLET_PASSWORD environment variable is required'); } + const password = Password.from(passwordRaw); - const srp = process.env.MM_WALLET_SRP; - if (!srp) { + const srpRaw = process.env.MM_WALLET_SRP; + if (!srpRaw) { throw new Error('MM_WALLET_SRP environment variable is required'); } + const srp = Srp.from(srpRaw); // 0o700: owner-only. The daemon exposes the full wallet messenger over // the socket inside this directory, so anyone who can traverse the dir diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts index 4828f4d33b..8fe5c1675f 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.test.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.test.ts @@ -4,6 +4,7 @@ import { existsSync } from 'node:fs'; import { pingDaemon } from './daemon-client'; import { ensureDaemon } from './daemon-spawn'; import { getDaemonPaths } from './paths'; +import { Password, Srp } from './secrets'; import type { DaemonSpawnConfig } from './types'; jest.mock('node:child_process'); @@ -16,11 +17,13 @@ const mockExistsSync = jest.mocked(existsSync); const mockPingDaemon = jest.mocked(pingDaemon); const mockGetDaemonPaths = jest.mocked(getDaemonPaths); +const SRP = 'test test test test test test test test test test test ball'; + const CONFIG: DaemonSpawnConfig = { dataDir: '/tmp/data', infuraProjectId: 'test-key', - password: 'test-pass', - srp: 'test test test test test test test test test test test ball', + password: Password.from('test-pass'), + srp: Srp.from(SRP), packageRoot: '/pkg', }; @@ -134,8 +137,7 @@ describe('ensureDaemon', () => { MM_DAEMON_SOCKET_PATH: '/tmp/test.sock', INFURA_PROJECT_ID: 'test-key', MM_WALLET_PASSWORD: 'test-pass', - MM_WALLET_SRP: - 'test test test test test test test test test test test ball', + MM_WALLET_SRP: SRP, }), }), ); diff --git a/packages/wallet-cli/src/daemon/daemon-spawn.ts b/packages/wallet-cli/src/daemon/daemon-spawn.ts index 834ed8b267..81f95ac8e3 100644 --- a/packages/wallet-cli/src/daemon/daemon-spawn.ts +++ b/packages/wallet-cli/src/daemon/daemon-spawn.ts @@ -71,8 +71,8 @@ export async function ensureDaemon( MM_DAEMON_DATA_DIR: config.dataDir, MM_DAEMON_SOCKET_PATH: socketPath, INFURA_PROJECT_ID: config.infuraProjectId, - MM_WALLET_PASSWORD: config.password, - MM_WALLET_SRP: config.srp, + MM_WALLET_PASSWORD: config.password.unwrap(), + MM_WALLET_SRP: config.srp.unwrap(), }, }); diff --git a/packages/wallet-cli/src/daemon/secrets.test.ts b/packages/wallet-cli/src/daemon/secrets.test.ts new file mode 100644 index 0000000000..7c363d6571 --- /dev/null +++ b/packages/wallet-cli/src/daemon/secrets.test.ts @@ -0,0 +1,140 @@ +import { inspect } from 'node:util'; + +import { Password, Srp } from './secrets'; + +const VALID_SRP_12 = + 'test test test test test test test test test test test ball'; +const VALID_SRP_24 = + 'test test test test test test test test test test test test ' + + 'test test test test test test test test test test test ball'; + +describe('Password', () => { + describe('from', () => { + it('wraps a non-empty string', () => { + const password = Password.from('hunter2'); + expect(password).toBeInstanceOf(Password); + }); + + it('throws on an empty string', () => { + expect(() => Password.from('')).toThrow( + 'Password must be a non-empty string', + ); + }); + }); + + describe('unwrap', () => { + it('returns the original value', () => { + expect(Password.from('hunter2').unwrap()).toBe('hunter2'); + }); + }); + + describe('redaction', () => { + const SECRET = 'do-not-log-me'; + let password: Password; + + beforeEach(() => { + password = Password.from(SECRET); + }); + + it('redacts under util.inspect', () => { + const inspected = inspect(password); + expect(inspected).toBe('[redacted]'); + expect(inspected).not.toContain(SECRET); + }); + + it('redacts inside an inspected object', () => { + const inspected = inspect({ password }); + expect(inspected).toContain('[redacted]'); + expect(inspected).not.toContain(SECRET); + }); + + it('redacts under JSON.stringify', () => { + const serialized = JSON.stringify({ password }); + expect(serialized).toBe('{"password":"[redacted]"}'); + expect(serialized).not.toContain(SECRET); + }); + + it('redacts under String() conversion', () => { + expect(String(password)).toBe('[redacted]'); + }); + + it('redacts under template-literal interpolation', () => { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- We are intentionally exercising the redacting toString(). + const message = `password is ${password}`; + expect(message).toBe('password is [redacted]'); + expect(message).not.toContain(SECRET); + }); + }); +}); + +describe('Srp', () => { + describe('from', () => { + it.each([12, 15, 18, 21, 24])( + 'accepts a %i-word mnemonic of valid words', + (count) => { + const phrase = Array.from({ length: count - 1 }, () => 'test') + .concat('ball') + .join(' '); + expect(Srp.from(phrase)).toBeInstanceOf(Srp); + }, + ); + + it('throws when the word count is invalid', () => { + expect(() => Srp.from('test test test')).toThrow( + /must be 12, 15, 18, 21, or 24 words \(got 3\)/u, + ); + }); + + it('throws when a word is not in the BIP-39 wordlist', () => { + const phrase = + 'notabip39word test test test test test test test test test test ball'; + expect(() => Srp.from(phrase)).toThrow( + 'Secret recovery phrase contains a word not in the BIP-39 English wordlist', + ); + }); + }); + + describe('unwrap', () => { + it('returns the original phrase', () => { + expect(Srp.from(VALID_SRP_12).unwrap()).toBe(VALID_SRP_12); + expect(Srp.from(VALID_SRP_24).unwrap()).toBe(VALID_SRP_24); + }); + }); + + describe('redaction', () => { + let srp: Srp; + + beforeEach(() => { + srp = Srp.from(VALID_SRP_12); + }); + + it('redacts under util.inspect', () => { + const inspected = inspect(srp); + expect(inspected).toBe('[redacted]'); + expect(inspected).not.toContain('ball'); + }); + + it('redacts inside an inspected object', () => { + const inspected = inspect({ srp }); + expect(inspected).toContain('[redacted]'); + expect(inspected).not.toContain('ball'); + }); + + it('redacts under JSON.stringify', () => { + const serialized = JSON.stringify({ srp }); + expect(serialized).toBe('{"srp":"[redacted]"}'); + expect(serialized).not.toContain('ball'); + }); + + it('redacts under String() conversion', () => { + expect(String(srp)).toBe('[redacted]'); + }); + + it('redacts under template-literal interpolation', () => { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- We are intentionally exercising the redacting toString(). + const message = `srp is ${srp}`; + expect(message).toBe('srp is [redacted]'); + expect(message).not.toContain('ball'); + }); + }); +}); diff --git a/packages/wallet-cli/src/daemon/secrets.ts b/packages/wallet-cli/src/daemon/secrets.ts new file mode 100644 index 0000000000..44e3370c91 --- /dev/null +++ b/packages/wallet-cli/src/daemon/secrets.ts @@ -0,0 +1,135 @@ +import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; + +const REDACTED = '[redacted]'; + +const VALID_SRP_WORD_COUNTS: readonly number[] = [12, 15, 18, 21, 24]; + +const INSPECT_CUSTOM = Symbol.for('nodejs.util.inspect.custom'); + +const WORDLIST_SET: ReadonlySet = new Set(wordlist); + +/** + * Opaque wrapper around a wallet password. + * + * Constructed via {@link Password.from}, which validates the input. The + * underlying string is only reachable through {@link Password.unwrap}; every + * other path (`toString`, `JSON.stringify`, `util.inspect`, template-literal + * interpolation) yields `[redacted]`. This makes accidental logging produce a + * harmless placeholder instead of leaking the secret. + */ +export class Password { + readonly #value: string; + + // See .from() for why this is private. + // eslint-disable-next-line no-restricted-syntax + private constructor(value: string) { + this.#value = value; + } + + /** + * Wrap a non-empty string as a {@link Password}. + * + * Matches the `@metamask/keyring-controller` convention: any non-empty + * string is acceptable; minimum-length policy is left to the keyring. + * + * @param value - The raw password string. + * @returns A redacting {@link Password} wrapper. + * @throws If `value` is empty. + */ + static from(value: string): Password { + if (value.length === 0) { + throw new Error('Password must be a non-empty string'); + } + return new Password(value); + } + + /** + * Reveal the underlying password string. Call this only at trust boundaries + * (e.g. handing the value to the keyring or to a child-process env var). + * + * @returns The original password string. + */ + unwrap(): string { + return this.#value; + } + + toString(): string { + return REDACTED; + } + + toJSON(): string { + return REDACTED; + } + + [INSPECT_CUSTOM](): string { + return REDACTED; + } +} + +/** + * Opaque wrapper around a BIP-39 secret recovery phrase. + * + * Constructed via {@link Srp.from}, which validates the word count + * (12/15/18/21/24) and that every word is present in the BIP-39 English + * wordlist. The underlying string is only reachable through {@link Srp.unwrap}; + * every other path yields `[redacted]`. + */ +export class Srp { + readonly #value: string; + + // See .from() for why this is private. + // eslint-disable-next-line no-restricted-syntax + private constructor(value: string) { + this.#value = value; + } + + /** + * Validate and wrap a BIP-39 mnemonic phrase. + * + * The phrase is expected to be a single space-separated string. Catching + * malformed input here (rather than letting it reach + * `KeyringController:createNewVaultAndRestore`) produces a clearer error. + * + * @param value - The raw mnemonic string. + * @returns A redacting {@link Srp} wrapper. + * @throws If the word count is not one of 12/15/18/21/24, or if any word is + * not present in the BIP-39 English wordlist. + */ + static from(value: string): Srp { + const words = value.split(' '); + if (!VALID_SRP_WORD_COUNTS.includes(words.length)) { + throw new Error( + `Secret recovery phrase must be 12, 15, 18, 21, or 24 words (got ${words.length})`, + ); + } + for (const word of words) { + if (!WORDLIST_SET.has(word)) { + throw new Error( + 'Secret recovery phrase contains a word not in the BIP-39 English wordlist', + ); + } + } + return new Srp(value); + } + + /** + * Reveal the underlying mnemonic string. Call this only at trust boundaries. + * + * @returns The original mnemonic string. + */ + unwrap(): string { + return this.#value; + } + + toString(): string { + return REDACTED; + } + + toJSON(): string { + return REDACTED; + } + + [INSPECT_CUSTOM](): string { + return REDACTED; + } +} diff --git a/packages/wallet-cli/src/daemon/types.ts b/packages/wallet-cli/src/daemon/types.ts index 8e9e8be50e..f11a4096ab 100644 --- a/packages/wallet-cli/src/daemon/types.ts +++ b/packages/wallet-cli/src/daemon/types.ts @@ -1,5 +1,7 @@ import type { Json } from '@metamask/utils'; +import type { Password, Srp } from './secrets'; + /** * A function that handles a JSON-RPC method call. * @@ -36,7 +38,7 @@ export type DaemonStatusInfo = { export type DaemonSpawnConfig = { dataDir: string; infuraProjectId: string; - password: string; - srp: string; + password: Password; + srp: Srp; packageRoot: string; }; diff --git a/packages/wallet-cli/src/daemon/wallet-factory.test.ts b/packages/wallet-cli/src/daemon/wallet-factory.test.ts index f6f0269ade..ddb48f1e56 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.test.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.test.ts @@ -6,6 +6,7 @@ import { join } from 'node:path'; import { KeyValueStore } from '../persistence/KeyValueStore'; import * as persistenceModule from '../persistence/persistence'; +import { Password, Srp } from './secrets'; import { createWallet } from './wallet-factory'; jest.mock('@metamask/wallet'); @@ -37,11 +38,13 @@ function tempDbPath(label: string): string { const MockWallet = jest.mocked(Wallet); const mockImportSrp = jest.mocked(importSecretRecoveryPhrase); +const SRP = 'test test test test test test test test test test test ball'; + const CONFIG = { databasePath: ':memory:', infuraProjectId: 'test-key', - password: 'test-pass', - srp: 'test test test test test test test test test test test ball', + password: Password.from('test-pass'), + srp: Srp.from(SRP), }; describe('createWallet', () => { @@ -104,7 +107,7 @@ describe('createWallet', () => { expect(mockImportSrp).toHaveBeenCalledWith( expect.objectContaining({ messenger: mockMessenger }), 'test-pass', - 'test test test test test test test test test test test ball', + SRP, ); }); diff --git a/packages/wallet-cli/src/daemon/wallet-factory.ts b/packages/wallet-cli/src/daemon/wallet-factory.ts index 08a95f9833..5e4e0317f2 100644 --- a/packages/wallet-cli/src/daemon/wallet-factory.ts +++ b/packages/wallet-cli/src/daemon/wallet-factory.ts @@ -9,6 +9,7 @@ import { rm } from 'node:fs/promises'; import { KeyValueStore } from '../persistence/KeyValueStore'; import { loadState, subscribeToChanges } from '../persistence/persistence'; +import type { Password, Srp } from './secrets'; const IN_MEMORY_DATABASE_PATH = ':memory:'; @@ -56,8 +57,8 @@ export async function createWallet({ }: { databasePath: string; infuraProjectId: string; - password: string; - srp: string; + password: Password; + srp: Srp; /** * Optional logger for persistence-write failures. Without it, failures * fall back to `console.error` (which a detached daemon's @@ -93,7 +94,7 @@ export async function createWallet({ subscribeToChanges(wallet.messenger, wallet.controllerMetadata, store, log); if (wasFirstRun) { - await importSecretRecoveryPhrase(wallet, password, srp); + await importSecretRecoveryPhrase(wallet, password.unwrap(), srp.unwrap()); } return { wallet, store }; diff --git a/yarn.lock b/yarn.lock index be0f14d051..482da23932 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5820,6 +5820,7 @@ __metadata: "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/remote-feature-flag-controller": "npm:^4.2.0" "@metamask/rpc-errors": "npm:^7.0.2" + "@metamask/scure-bip39": "npm:^2.1.1" "@metamask/utils": "npm:^11.9.0" "@metamask/wallet": "npm:^0.0.0" "@oclif/core": "npm:^4.10.5"