diff --git a/packages/keyring-eth-hd/CHANGELOG.md b/packages/keyring-eth-hd/CHANGELOG.md index 92f6ebf13..cc0af24ba 100644 --- a/packages/keyring-eth-hd/CHANGELOG.md +++ b/packages/keyring-eth-hd/CHANGELOG.md @@ -13,6 +13,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Wraps legacy `HdKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type. - Extends `EthKeyringWrapper` for common Ethereum logic. +### Fixed + +- Enforce mnemonics validation ([#450](https://github.com/MetaMask/accounts/pull/450)) + - Validates mnemonics against BIP39 specification (word count, wordlist, checksum) before use. + - Throws for invalid mnemonics. + ## [13.0.0] ### Added diff --git a/packages/keyring-eth-hd/src/hd-keyring.test.ts b/packages/keyring-eth-hd/src/hd-keyring.test.ts index 6d410fa09..8edd327e2 100644 --- a/packages/keyring-eth-hd/src/hd-keyring.test.ts +++ b/packages/keyring-eth-hd/src/hd-keyring.test.ts @@ -250,7 +250,7 @@ describe('hd-keyring', () => { expect(cryptographicFunctions.pbkdf2Sha512).toHaveBeenCalledTimes(1); }); - it('throws on invalid mnemonic', async () => { + it('throws on invalid mnemonic with wrong number of words', async () => { const keyring = new HdKeyring(); await expect( @@ -259,10 +259,110 @@ describe('hd-keyring', () => { numberOfAccounts: 2, }), ).rejects.toThrow( - 'Invalid mnemonic phrase: The mnemonic phrase must consist of 12, 15, 18, 21, or 24 words.', + 'Eth-Hd-Keyring: Invalid secret recovery phrase provided', ); }); + it('throws on mnemonic with invalid words', async () => { + const keyring = new HdKeyring(); + + // 12 words but invalid (not in BIP39 wordlist) + await expect( + keyring.deserialize({ + mnemonic: + 'invalid words that are not in the bip39 wordlist at all here', + numberOfAccounts: 1, + }), + ).rejects.toThrow( + 'Eth-Hd-Keyring: Invalid secret recovery phrase provided', + ); + }); + + it('throws on mnemonic with invalid checksum', async () => { + const keyring = new HdKeyring(); + + // Valid BIP39 words but invalid checksum (last word changed from 'mango' to 'abandon') + await expect( + keyring.deserialize({ + mnemonic: + 'finish oppose decorate face calm tragic certain desk hour urge dinosaur abandon', + numberOfAccounts: 1, + }), + ).rejects.toThrow( + 'Eth-Hd-Keyring: Invalid secret recovery phrase provided', + ); + }); + + it('throws on invalid mnemonic passed as Buffer', async () => { + const keyring = new HdKeyring(); + + // Invalid mnemonic as raw Buffer + await expect( + keyring.deserialize({ + // @ts-expect-error testing Buffer mnemonic directly + mnemonic: Buffer.from('invalid mnemonic phrase here', 'utf8'), + numberOfAccounts: 1, + }), + ).rejects.toThrow( + 'Eth-Hd-Keyring: Invalid secret recovery phrase provided', + ); + }); + + it('deserializes valid mnemonic passed as Buffer', async () => { + const keyring = new HdKeyring(); + + await keyring.deserialize({ + // @ts-expect-error testing Buffer mnemonic directly + mnemonic: Buffer.from(sampleMnemonic, 'utf8'), + numberOfAccounts: 1, + }); + + const accounts = await keyring.getAccounts(); + expect(accounts[0]).toStrictEqual(firstAcct); + }); + + it('validates mnemonic passed as Uint8Array with valid mnemonic', async () => { + const keyring = new HdKeyring(); + + // Serialize a valid keyring to get the Uint8Array format + const tempKeyring = new HdKeyring(); + await tempKeyring.deserialize({ mnemonic: sampleMnemonic }); + const { mnemonic } = tempKeyring; + assert(mnemonic, 'Mnemonic should be defined'); + + await keyring.deserialize({ + // @ts-expect-error testing Uint8Array mnemonic directly + mnemonic, + numberOfAccounts: 1, + }); + + const accounts = await keyring.getAccounts(); + expect(accounts[0]).toStrictEqual(firstAcct); + }); + + it('validates mnemonic passed as plain object (simulating encryption/decryption cycle)', async () => { + const keyring = new HdKeyring(); + + const tempKeyring = new HdKeyring(); + await tempKeyring.deserialize({ mnemonic: sampleMnemonic }); + const { mnemonic } = tempKeyring; + assert(mnemonic, 'Mnemonic should be defined'); + + const mnemonicAsPlainObject: Record = {}; + mnemonic.forEach((value, index) => { + mnemonicAsPlainObject[index] = value; + }); + + await keyring.deserialize({ + // @ts-expect-error testing plain object mnemonic directly + mnemonic: mnemonicAsPlainObject, + numberOfAccounts: 1, + }); + + const accounts = await keyring.getAccounts(); + expect(accounts[0]).toStrictEqual(firstAcct); + }); + it('throws when numberOfAccounts is passed with no mnemonic', async () => { const keyring = new HdKeyring(); diff --git a/packages/keyring-eth-hd/src/hd-keyring.ts b/packages/keyring-eth-hd/src/hd-keyring.ts index a4b09b827..3a0e8f985 100644 --- a/packages/keyring-eth-hd/src/hd-keyring.ts +++ b/packages/keyring-eth-hd/src/hd-keyring.ts @@ -20,7 +20,7 @@ import { mnemonicToSeed, } from '@metamask/key-tree'; import type { Keyring } from '@metamask/keyring-utils'; -import { generateMnemonic } from '@metamask/scure-bip39'; +import { generateMnemonic, validateMnemonic } from '@metamask/scure-bip39'; import { wordlist } from '@metamask/scure-bip39/dist/wordlists/english'; import { add0x, @@ -38,6 +38,8 @@ import { keccak256 } from 'ethereum-cryptography/keccak'; const hdPathString = `m/44'/60'/0'/0`; const type = 'HD Key Tree'; +type Mnemonic = string | number[] | SerializedBuffer | Buffer | Uint8Array; + export type HDKeyringOptions = { cryptographicFunctions?: CryptographicFunctions; }; @@ -481,9 +483,7 @@ export class HdKeyring implements Keyring { * @param mnemonic - The mnemonic seed phrase. * @returns The Uint8Array mnemonic. */ - #mnemonicToUint8Array( - mnemonic: Buffer | SerializedBuffer | string | Uint8Array | number[], - ): Uint8Array { + #mnemonicToUint8Array(mnemonic: Mnemonic): Uint8Array { let mnemonicData: unknown = mnemonic; // When using `Buffer.toJSON()`, the Buffer is serialized into an object // with the structure `{ type: 'Buffer', data: [...] }` @@ -601,16 +601,18 @@ export class HdKeyring implements Keyring { * as a string, an array of UTF-8 bytes, or a Buffer. Mnemonic input * passed as type buffer or array of UTF-8 bytes must be NFKD normalized. */ - async #initFromMnemonic( - mnemonic: string | number[] | SerializedBuffer | Buffer | Uint8Array, - ): Promise { + async #initFromMnemonic(mnemonic: Mnemonic): Promise { if (this.root) { throw new Error( 'Eth-Hd-Keyring: Secret recovery phrase already provided', ); } - this.mnemonic = this.#mnemonicToUint8Array(mnemonic); + // Convert and validate before assigning to instance property + // to avoid inconsistent state if validation fails + const mnemonicAsUint8Array = this.#mnemonicToUint8Array(mnemonic); + this.#assertValidMnemonic(mnemonicAsUint8Array); + this.mnemonic = mnemonicAsUint8Array; this.seed = await mnemonicToSeed( this.mnemonic, @@ -644,4 +646,20 @@ export class HdKeyring implements Keyring { assert(normalized, 'Expected address to be set'); return add0x(normalized); } + + /** + * Assert that the mnemonic seed phrase is valid. + * Throws an error if the mnemonic is not a valid BIP39 phrase. + * + * @param mnemonic - The mnemonic seed phrase to validate (as Uint8Array). + * @throws If the mnemonic is not a valid BIP39 secret recovery phrase. + */ + #assertValidMnemonic(mnemonic: Uint8Array): void { + const mnemonicString = this.#uint8ArrayToString(mnemonic); + if (!validateMnemonic(mnemonicString, wordlist)) { + throw new Error( + 'Eth-Hd-Keyring: Invalid secret recovery phrase provided', + ); + } + } }