Validate that master private key and seed are within bounds (#118)

* Validate that master private key is within bounds

* Check for private key validity in BIP-39 deriver

* Update tests to always pass curve

src/BIP44Node.test.ts

@@ -1,6 +1,6 @@
 import { bytesToHex } from '@metamask/utils';
 
-import { BIP44Node, BIP44PurposeNodeToken } from '.';
+import { BIP44Node, BIP44PurposeNodeToken, secp256k1 } from '.';
 import fixtures from '../test/fixtures';
 import { compressPublicKey } from './curves/secp256k1';
 import { createBip39KeyFromSeed, deriveChildKey } from './derivers/bip39';
@@ -19,6 +19,7 @@ describe('BIP44Node', () => {
     it('initializes a new node from a private key', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       // Ethereum coin type node
@@ -45,6 +46,7 @@ describe('BIP44Node', () => {
     it('initializes a new node from JSON', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       // Ethereum coin type node
@@ -84,6 +86,7 @@ describe('BIP44Node', () => {
     it('throws if the depth is invalid', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       await expect(
@@ -405,6 +408,7 @@ describe('BIP44Node', () => {
       async ({ index, publicKey }) => {
         const { privateKey, chainCode } = await createBip39KeyFromSeed(
           hexStringToBytes(hexSeed),
+          secp256k1,
         );
 
         const node = await BIP44Node.fromExtendedKey({
@@ -434,6 +438,7 @@ describe('BIP44Node', () => {
       async ({ index, address }) => {
         const { privateKey, chainCode } = await createBip39KeyFromSeed(
           hexStringToBytes(hexSeed),
+          secp256k1,
         );
 
         const node = await BIP44Node.fromExtendedKey({

src/SLIP10Node.test.ts

@@ -16,6 +16,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from a private key', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       const node = await SLIP10Node.fromExtendedKey({
@@ -35,6 +36,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from a hexadecimal private key and chain code', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       const node = await SLIP10Node.fromExtendedKey({
@@ -54,6 +56,7 @@ describe('SLIP10Node', () => {
     it('initializes a new ed25519 node from a private key', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: ed25519,
       });
 
       const node = await SLIP10Node.fromExtendedKey({
@@ -88,6 +91,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from a public key', async () => {
       const { publicKeyBytes, chainCodeBytes } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       const node = await SLIP10Node.fromExtendedKey({
@@ -127,6 +131,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from a hexadecimal public key and chain code', async () => {
       const { publicKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       const node = await SLIP10Node.fromExtendedKey({
@@ -146,6 +151,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from JSON', async () => {
       const node = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       expect(await SLIP10Node.fromJSON(node.toJSON())).toStrictEqual(node);
@@ -154,6 +160,7 @@ describe('SLIP10Node', () => {
     it('initializes a new node from JSON with a public key', async () => {
       const { privateKey, chainCode } = await deriveChildKey({
         path: fixtures.local.mnemonic,
+        curve: secp256k1,
       });
 
       const node = await SLIP10Node.fromExtendedKey({

src/derivation.test.ts

@@ -2,6 +2,7 @@ import { bytesToHex } from '@metamask/utils';
 
 import fixtures from '../test/fixtures';
 import { HDPathTuple } from './constants';
+import { secp256k1 } from './curves';
 import { deriveKeyFromPath, validatePathSegment } from './derivation';
 import { derivers } from './derivers';
 import { privateKeyToEthAddress } from './derivers/bip32';
@@ -223,31 +224,35 @@ describe('derivation', () => {
       let node: SLIP10Node;
 
       /* eslint-disable require-atomic-updates */
-      node = await bip39Derive({ path: mnemonic });
+      node = await bip39Derive({ path: mnemonic, curve: secp256k1 });
       node = await bip32Derive({
         path: `44'`,
         node,
+        curve: secp256k1,
       });
 
       node = await bip32Derive({
         path: `60'`,
         node,
+        curve: secp256k1,
       });
 
       node = await bip32Derive({
         path: `0'`,
         node,
+        curve: secp256k1,
       });
 
       node = await bip32Derive({
         path: `0`,
         node,
+        curve: secp256k1,
       });
       /* eslint-enable require-atomic-updates */
 
       const keys = await Promise.all(
         expectedAddresses.map(async (_, index) => {
-          return bip32Derive({ path: `${index}`, node });
+          return bip32Derive({ path: `${index}`, node, curve: secp256k1 });
         }),
       );
 
@@ -258,7 +263,7 @@ describe('derivation', () => {
     });
 
     it('throws for invalid inputs', async () => {
-      const node = await bip39Derive({ path: mnemonic });
+      const node = await bip39Derive({ path: mnemonic, curve: secp256k1 });
       const inputs = [
         String(-1),
         String(1.1),
@@ -274,6 +279,7 @@ describe('derivation', () => {
           bip32Derive({
             path: input,
             node,
+            curve: secp256k1,
           }),
         ).rejects.toThrow(
           'Invalid BIP-32 index: The index must be a non-negative decimal integer less than 2147483648.',
@@ -285,20 +291,22 @@ describe('derivation', () => {
       await expect(
         bip32Derive({
           path: '0',
+          curve: secp256k1,
         }),
       ).rejects.toThrow(
         'Invalid parameters: Must specify a node to derive from.',
       );
     });
 
     it('throws when trying to derive from a public key node', async () => {
-      const node = await bip39Derive({ path: mnemonic });
+      const node = await bip39Derive({ path: mnemonic, curve: secp256k1 });
       const publicNode = node.neuter();
 
       await expect(
         bip32Derive({
           path: `0'`,
           node: publicNode,
+          curve: secp256k1,
         }),
       ).rejects.toThrow(
         'Invalid parameters: Cannot derive hardened child keys without a private key.',

src/derivers/bip39.test.ts

@@ -0,0 +1,67 @@
+import {
+  assert,
+  bigIntToBytes,
+  concatBytes,
+  hexToBytes,
+} from '@metamask/utils';
+import * as hmacModule from '@noble/hashes/hmac';
+
+import { secp256k1 } from '../curves';
+import { createBip39KeyFromSeed } from './bip39';
+
+describe('createBip39KeyFromSeed', () => {
+  const RANDOM_SEED = hexToBytes(
+    '0xea82e6ee9d319c083007d0b011a37b0e480ae02417a988ac90355abd53cd04fc',
+  );
+
+  it('throws if the seed is less than 16 bytes', async () => {
+    await expect(
+      createBip39KeyFromSeed(new Uint8Array(15), secp256k1),
+    ).rejects.toThrow(
+      'Invalid seed: The seed must be between 16 and 64 bytes long.',
+    );
+  });
+
+  it('throws if the seed is greater than 64 bytes', async () => {
+    await expect(
+      createBip39KeyFromSeed(new Uint8Array(65), secp256k1),
+    ).rejects.toThrow(
+      'Invalid seed: The seed must be between 16 and 64 bytes long.',
+    );
+  });
+
+  it('throws if the private key is zero', async () => {
+    // Mock the hmac function to return a zero private key.
+    jest.spyOn(hmacModule, 'hmac').mockImplementation(() => new Uint8Array(64));
+
+    await expect(
+      createBip39KeyFromSeed(RANDOM_SEED, secp256k1),
+    ).rejects.toThrow(
+      'Invalid private key: The private key must greater than 0 and less than the curve order.',
+    );
+  });
+
+  it.each([
+    bigIntToBytes(secp256k1.curve.n),
+    concatBytes([secp256k1.curve.n + BigInt(1)]),
+  ])(
+    'throws if the private key is greater than or equal to the curve order',
+    async (privateKey) => {
+      // For this test to be effective, the private key must be 32 bytes.
+      assert(privateKey.length === 32);
+
+      // Mock the hmac function to return a private key larger than the curve order.
+      jest
+        .spyOn(hmacModule, 'hmac')
+        .mockImplementation(() =>
+          concatBytes([privateKey, new Uint8Array(32)]),
+        );
+
+      await expect(
+        createBip39KeyFromSeed(RANDOM_SEED, secp256k1),
+      ).rejects.toThrow(
+        'Invalid private key: The private key must greater than 0 and less than the curve order.',
+      );
+    },
+  );
+});

src/derivers/bip39.ts

@@ -1,11 +1,12 @@
 import { mnemonicToSeed } from '@metamask/scure-bip39';
 import { wordlist as englishWordlist } from '@metamask/scure-bip39/dist/wordlists/english';
+import { assert } from '@metamask/utils';
 import { hmac } from '@noble/hashes/hmac';
 import { sha512 } from '@noble/hashes/sha512';
 
 import { DeriveChildKeyArgs } from '.';
-import { BIP39StringNode } from '../constants';
-import { Curve, secp256k1 } from '../curves';
+import { BIP39StringNode, BYTES_KEY_LENGTH } from '../constants';
+import { Curve } from '../curves';
 import { SLIP10Node } from '../SLIP10Node';
 import { getFingerprint } from '../utils';
 
@@ -47,11 +48,21 @@ export async function deriveChildKey({
  */
 export async function createBip39KeyFromSeed(
   seed: Uint8Array,
-  curve: Curve = secp256k1,
+  curve: Curve,
 ): Promise<SLIP10Node> {
+  assert(
+    seed.length >= 16 && seed.length <= 64,
+    'Invalid seed: The seed must be between 16 and 64 bytes long.',
+  );
+
   const key = hmac(sha512, curve.secret, seed);
-  const privateKey = key.slice(0, 32);
-  const chainCode = key.slice(32);
+  const privateKey = key.slice(0, BYTES_KEY_LENGTH);
+  const chainCode = key.slice(BYTES_KEY_LENGTH);
+
+  assert(
+    curve.isValidPrivateKey(privateKey),
+    'Invalid private key: The private key must greater than 0 and less than the curve order.',
+  );
 
   const masterFingerprint = getFingerprint(
     await curve.getPublicKey(privateKey, true),

src/derivers/index.ts

@@ -14,7 +14,7 @@ export type DerivedKeys = {
 
 export type DeriveChildKeyArgs = {
   path: Uint8Array | string;
-  curve?: Curve;
+  curve: Curve;
   node?: SLIP10Node;
 };
 

test/reference-implementations.test.ts

@@ -4,7 +4,7 @@ import {
   BIP44PurposeNodeToken,
   HDPathTuple,
 } from '../src';
-import { ed25519 } from '../src/curves';
+import { ed25519, secp256k1 } from '../src/curves';
 import { deriveKeyFromPath } from '../src/derivation';
 import { createBip39KeyFromSeed } from '../src/derivers/bip39';
 import {
@@ -145,7 +145,7 @@ describe('reference implementation tests', () => {
 
     describe('BIP44Node', () => {
       it('derives the same keys as the reference implementation', async () => {
-        const parentNode = await createBip39KeyFromSeed(seed);
+        const parentNode = await createBip39KeyFromSeed(seed, secp256k1);
         const node = await parentNode.derive(path.ours.tuple);
 
         expect(node.privateKey).toStrictEqual(privateKey);
@@ -161,7 +161,7 @@ describe('reference implementation tests', () => {
       });
 
       it('derives the same keys as the reference implementation using public key derivation', async () => {
-        const parentNode = await createBip39KeyFromSeed(seed);
+        const parentNode = await createBip39KeyFromSeed(seed, secp256k1);
         const node = await parentNode.derive(path.ours.tuple);
 
         expect(node.privateKey).toStrictEqual(privateKey);
@@ -180,7 +180,7 @@ describe('reference implementation tests', () => {
 
     describe('deriveKeyFromPath', () => {
       it('derives the same keys as the reference implementation', async () => {
-        const node = await createBip39KeyFromSeed(seed);
+        const node = await createBip39KeyFromSeed(seed, secp256k1);
         const childNode = await deriveKeyFromPath({
           path: path.ours.tuple,
           node,
@@ -210,7 +210,7 @@ describe('reference implementation tests', () => {
       it('derives the test vector keys', async () => {
         for (const vector of vectors) {
           const seed = hexStringToBytes(vector.hexSeed);
-          const node = await createBip39KeyFromSeed(seed);
+          const node = await createBip39KeyFromSeed(seed, secp256k1);
 
           for (const keyObj of vector.keys) {
             const { path, privateKey } = keyObj;