From 43e28013b3c7a7163b7764c6611a1a5288c6bc2b Mon Sep 17 00:00:00 2001 From: Hassan Malik <41640681+hmalik88@users.noreply.github.com> Date: Tue, 20 Jun 2023 15:07:30 -0400 Subject: [PATCH] Fix BIP-32 path validation (#1506) * added audit bip32 validation fix * Use key-tree util for BIP32 validation * Update shasums --------- Co-authored-by: Frederik Bolding --- .../signer/packages/bitcoin/package.json | 2 +- .../signer/packages/core/package.json | 2 +- .../signer/packages/core/snap.manifest.json | 2 +- .../signer/packages/ethereum/package.json | 2 +- packages/rpc-methods/package.json | 2 +- packages/snaps-utils/package.json | 2 +- .../src/manifest/validation.test.ts | 19 ++++++---- .../snaps-utils/src/manifest/validation.ts | 6 +-- yarn.lock | 38 +++++++++---------- 9 files changed, 39 insertions(+), 36 deletions(-) diff --git a/packages/examples/examples/signer/packages/bitcoin/package.json b/packages/examples/examples/signer/packages/bitcoin/package.json index 4e3ca061e7..f72525fcdb 100644 --- a/packages/examples/examples/signer/packages/bitcoin/package.json +++ b/packages/examples/examples/signer/packages/bitcoin/package.json @@ -11,7 +11,7 @@ "start": "mm-snap serve" }, "dependencies": { - "@metamask/key-tree": "^7.0.0", + "@metamask/key-tree": "^7.1.1", "@metamask/snaps-types": "workspace:^", "@metamask/utils": "^6.0.1", "@noble/secp256k1": "^1.7.1", diff --git a/packages/examples/examples/signer/packages/core/package.json b/packages/examples/examples/signer/packages/core/package.json index 46e7584a3d..48647d6550 100644 --- a/packages/examples/examples/signer/packages/core/package.json +++ b/packages/examples/examples/signer/packages/core/package.json @@ -15,7 +15,7 @@ "async-mutex": "^0.4.0" }, "devDependencies": { - "@metamask/key-tree": "^7.0.0", + "@metamask/key-tree": "^7.1.1", "@metamask/snaps-cli": "workspace:^", "@metamask/snaps-types": "workspace:^" }, diff --git a/packages/examples/examples/signer/packages/core/snap.manifest.json b/packages/examples/examples/signer/packages/core/snap.manifest.json index e1a789d26b..0474006e94 100644 --- a/packages/examples/examples/signer/packages/core/snap.manifest.json +++ b/packages/examples/examples/signer/packages/core/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snaps-monorepo.git" }, "source": { - "shasum": "gnA/2nadebS8BdjfHgPA1cxfQVdP9y89XSEY6i4qAHc=", + "shasum": "XEeIJvHwYV6utwGRkFLofazQRlGAXjmrH8lIKcgp3ok=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/examples/examples/signer/packages/ethereum/package.json b/packages/examples/examples/signer/packages/ethereum/package.json index 28fd21d13b..652008cab6 100644 --- a/packages/examples/examples/signer/packages/ethereum/package.json +++ b/packages/examples/examples/signer/packages/ethereum/package.json @@ -12,7 +12,7 @@ }, "dependencies": { "@ethereumjs/tx": "^3.5.2", - "@metamask/key-tree": "^7.0.0", + "@metamask/key-tree": "^7.1.1", "@metamask/snaps-types": "workspace:^", "@metamask/utils": "^6.0.1", "buffer": "^6.0.3" diff --git a/packages/rpc-methods/package.json b/packages/rpc-methods/package.json index 5c79a788aa..79c5dddf41 100644 --- a/packages/rpc-methods/package.json +++ b/packages/rpc-methods/package.json @@ -29,7 +29,7 @@ }, "dependencies": { "@metamask/browser-passworder": "^4.0.2", - "@metamask/key-tree": "^7.0.0", + "@metamask/key-tree": "^7.1.1", "@metamask/permission-controller": "^4.0.0", "@metamask/snaps-ui": "workspace:^", "@metamask/snaps-utils": "workspace:^", diff --git a/packages/snaps-utils/package.json b/packages/snaps-utils/package.json index b116724699..d34402500f 100644 --- a/packages/snaps-utils/package.json +++ b/packages/snaps-utils/package.json @@ -56,6 +56,7 @@ "@babel/core": "^7.18.6", "@babel/types": "^7.18.7", "@metamask/base-controller": "^3.0.0", + "@metamask/key-tree": "^7.1.1", "@metamask/permission-controller": "^4.0.0", "@metamask/providers": "^10.2.1", "@metamask/snaps-registry": "^1.2.1", @@ -82,7 +83,6 @@ "@metamask/eslint-config-jest": "^11.0.0", "@metamask/eslint-config-nodejs": "^11.0.1", "@metamask/eslint-config-typescript": "^11.0.0", - "@metamask/key-tree": "^7.0.0", "@metamask/post-message-stream": "^6.1.1", "@types/jest": "^27.5.1", "@types/mocha": "^10.0.1", diff --git a/packages/snaps-utils/src/manifest/validation.test.ts b/packages/snaps-utils/src/manifest/validation.test.ts index 34fd441ad1..e594bfccec 100644 --- a/packages/snaps-utils/src/manifest/validation.test.ts +++ b/packages/snaps-utils/src/manifest/validation.test.ts @@ -52,14 +52,17 @@ describe('Bip32PathStruct', () => { ); }); - it.each(["m/0'/123/asd", 'm/0"/123', 'm/1/2/3/_', "m/1'/2'/3'/-1"])( - 'requires numbers or hardened numbers', - (path) => { - expect(() => assert(path.split('/'), Bip32PathStruct)).toThrow( - 'Path must be a valid BIP-32 derivation path array.', - ); - }, - ); + it.each([ + "m/0'/123/asd", + 'm/0"/123', + 'm/1/2/3/_', + "m/1'/2'/3'/-1", + "m/1'/2147483648'", + ])('requires numbers or hardened numbers per BIP32', (path) => { + expect(() => assert(path.split('/'), Bip32PathStruct)).toThrow( + 'Path must be a valid BIP-32 derivation path array.', + ); + }); it('throws for forbidden purposes', () => { expect(() => assert(['m', "1399742832'", '0'], Bip32PathStruct)).toThrow( diff --git a/packages/snaps-utils/src/manifest/validation.ts b/packages/snaps-utils/src/manifest/validation.ts index af193de378..517e030d62 100644 --- a/packages/snaps-utils/src/manifest/validation.ts +++ b/packages/snaps-utils/src/manifest/validation.ts @@ -1,3 +1,4 @@ +import { isValidBIP32PathSegment } from '@metamask/key-tree'; import { assertStruct, ChecksumStruct, VersionStruct } from '@metamask/utils'; import { array, @@ -40,11 +41,10 @@ const FORBIDDEN_PATHS: string[][] = FORBIDDEN_COIN_TYPES.map((coinType) => [ `${coinType}'`, ]); -const BIP32_INDEX_REGEX = /^\d+'?$/u; export const Bip32PathStruct = refine( array(string()), 'BIP-32 path', - (path) => { + (path: string[]) => { if (path.length === 0) { return 'Path must be a non-empty BIP-32 derivation path array'; } @@ -57,7 +57,7 @@ export const Bip32PathStruct = refine( return 'Paths must have a length of at least three.'; } - if (path.slice(1).some((part) => !BIP32_INDEX_REGEX.test(part))) { + if (path.slice(1).some((part) => !isValidBIP32PathSegment(part))) { return 'Path must be a valid BIP-32 derivation path array.'; } diff --git a/yarn.lock b/yarn.lock index 3b3308ca8d..265d9f1b88 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3050,17 +3050,17 @@ __metadata: languageName: node linkType: hard -"@metamask/key-tree@npm:^7.0.0": - version: 7.0.0 - resolution: "@metamask/key-tree@npm:7.0.0" +"@metamask/key-tree@npm:^7.1.1": + version: 7.1.1 + resolution: "@metamask/key-tree@npm:7.1.1" dependencies: "@metamask/scure-bip39": ^2.1.0 - "@metamask/utils": ^3.3.0 + "@metamask/utils": ^6.0.1 "@noble/ed25519": ^1.6.0 "@noble/hashes": ^1.0.0 "@noble/secp256k1": ^1.5.5 "@scure/base": ^1.0.0 - checksum: e3b8371ba41766e1936ff0b0b34c46b7a1297e51fe177246ae6446c4f15a6601a6ed4fc5d01d0594e8b1e9b1682096900f7ab4e7e15df94041ec025116a635b9 + checksum: ddab7917e1214c51508f821a680f8e8819a8c866fd91244e0debf28464331dfcbf9a357e01716fc593cf00a8c71f95fd45ed4ff686ee6abc4c4061aed629537c languageName: node linkType: hard @@ -3172,7 +3172,7 @@ __metadata: "@metamask/eslint-config-jest": ^11.0.0 "@metamask/eslint-config-nodejs": ^11.0.1 "@metamask/eslint-config-typescript": ^11.0.0 - "@metamask/key-tree": ^7.0.0 + "@metamask/key-tree": ^7.1.1 "@metamask/permission-controller": ^4.0.0 "@metamask/snaps-ui": "workspace:^" "@metamask/snaps-utils": "workspace:^" @@ -3614,7 +3614,7 @@ __metadata: "@metamask/eslint-config-jest": ^11.0.0 "@metamask/eslint-config-nodejs": ^11.0.1 "@metamask/eslint-config-typescript": ^11.0.0 - "@metamask/key-tree": ^7.0.0 + "@metamask/key-tree": ^7.1.1 "@metamask/permission-controller": ^4.0.0 "@metamask/post-message-stream": ^6.1.1 "@metamask/providers": ^10.2.1 @@ -3724,7 +3724,7 @@ __metadata: languageName: node linkType: hard -"@metamask/utils@npm:^3.3.0, @metamask/utils@npm:^3.4.1": +"@metamask/utils@npm:^3.4.1": version: 3.6.0 resolution: "@metamask/utils@npm:3.6.0" dependencies: @@ -4581,20 +4581,20 @@ __metadata: languageName: node linkType: hard -"@types/node@npm:*, @types/node@npm:^18.0.0, @types/node@npm:^18.14.0": - version: 18.14.2 - resolution: "@types/node@npm:18.14.2" - checksum: 53c07e721f6ae33de71306f6a0b75dae6066a4f55bd5484c93bd59ff25f0c5f004ceafeef509a4d0cb9e24a247efc34d50489bcc1b05a53ecc68e2fc088e65cb - languageName: node - linkType: hard - -"@types/node@npm:^20.3.1": +"@types/node@npm:*, @types/node@npm:^20.3.1": version: 20.3.1 resolution: "@types/node@npm:20.3.1" checksum: 63a393ab6d947be17320817b35d7277ef03728e231558166ed07ee30b09fd7c08861be4d746f10fdc63ca7912e8cd023939d4eab887ff6580ff704ff24ed810c languageName: node linkType: hard +"@types/node@npm:^18.0.0, @types/node@npm:^18.14.0": + version: 18.14.2 + resolution: "@types/node@npm:18.14.2" + checksum: 53c07e721f6ae33de71306f6a0b75dae6066a4f55bd5484c93bd59ff25f0c5f004ceafeef509a4d0cb9e24a247efc34d50489bcc1b05a53ecc68e2fc088e65cb + languageName: node + linkType: hard + "@types/normalize-package-data@npm:^2.4.1": version: 2.4.1 resolution: "@types/normalize-package-data@npm:2.4.1" @@ -15944,7 +15944,7 @@ __metadata: version: 0.0.0-use.local resolution: "signer-bitcoin@workspace:packages/examples/examples/signer/packages/bitcoin" dependencies: - "@metamask/key-tree": ^7.0.0 + "@metamask/key-tree": ^7.1.1 "@metamask/snaps-cli": "workspace:^" "@metamask/snaps-types": "workspace:^" "@metamask/utils": ^6.0.1 @@ -15958,7 +15958,7 @@ __metadata: version: 0.0.0-use.local resolution: "signer-core@workspace:packages/examples/examples/signer/packages/core" dependencies: - "@metamask/key-tree": ^7.0.0 + "@metamask/key-tree": ^7.1.1 "@metamask/snaps-cli": "workspace:^" "@metamask/snaps-types": "workspace:^" "@metamask/utils": ^6.0.1 @@ -15971,7 +15971,7 @@ __metadata: resolution: "signer-ethereum@workspace:packages/examples/examples/signer/packages/ethereum" dependencies: "@ethereumjs/tx": ^3.5.2 - "@metamask/key-tree": ^7.0.0 + "@metamask/key-tree": ^7.1.1 "@metamask/snaps-cli": "workspace:^" "@metamask/snaps-types": "workspace:^" "@metamask/utils": ^6.0.1