diff --git a/packages/address-book-controller/src/AddressBookController.test.ts b/packages/address-book-controller/src/AddressBookController.test.ts index baaf512347d..3d8c42915e0 100644 --- a/packages/address-book-controller/src/AddressBookController.test.ts +++ b/packages/address-book-controller/src/AddressBookController.test.ts @@ -294,6 +294,13 @@ describe('AddressBookController', () => { ).toBe(true); }); + it('should return false to indicate an address book entry has NOT been deleted due to unsafe input', () => { + const controller = new AddressBookController(); + // @ts-expect-error Suppressing error to test runtime behavior + expect(controller.delete('__proto__', '0x01')).toBe(false); + expect(controller.delete(toHex(1), 'constructor')).toBe(false); + }); + it('should return false to indicate an address book entry has NOT been deleted', () => { const controller = new AddressBookController(); controller.set('0x32Be343B94f860124dC4fEe278FDCBD38C102D88', '0x00'); diff --git a/packages/address-book-controller/src/AddressBookController.ts b/packages/address-book-controller/src/AddressBookController.ts index 830e642f4d9..1101dffc94a 100644 --- a/packages/address-book-controller/src/AddressBookController.ts +++ b/packages/address-book-controller/src/AddressBookController.ts @@ -3,6 +3,7 @@ import { BaseControllerV1 } from '@metamask/base-controller'; import { normalizeEnsName, isValidHexAddress, + isSafeDynamicKey, toChecksumHexAddress, toHex, } from '@metamask/controller-utils'; @@ -110,6 +111,7 @@ export class AddressBookController extends BaseControllerV1< delete(chainId: Hex, address: string) { address = toChecksumHexAddress(address); if ( + ![chainId, address].every((key) => isSafeDynamicKey(key)) || !isValidHexAddress(address) || !this.state.addressBook[chainId] || !this.state.addressBook[chainId][address] diff --git a/packages/controller-utils/jest.config.js b/packages/controller-utils/jest.config.js index e77cd278680..1d042ba6d40 100644 --- a/packages/controller-utils/jest.config.js +++ b/packages/controller-utils/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 68.05, - functions: 80.55, - lines: 69.82, - statements: 70.17, + branches: 78.12, + functions: 85.41, + lines: 87.3, + statements: 86.5, }, }, diff --git a/packages/controller-utils/src/index.ts b/packages/controller-utils/src/index.ts index 14a22e8e00e..265872e6204 100644 --- a/packages/controller-utils/src/index.ts +++ b/packages/controller-utils/src/index.ts @@ -1,4 +1,32 @@ export * from './constants'; -export * from './util'; +export type { NonEmptyArray } from './util'; +export { + BNToHex, + convertHexToDecimal, + fetchWithErrorHandling, + fractionBN, + fromHex, + getBuyURL, + gweiDecToWEIBN, + handleFetch, + hexToBN, + hexToText, + isNonEmptyArray, + isPlainObject, + isSafeChainId, + isSafeDynamicKey, + isSmartContractCode, + isValidJson, + isValidHexAddress, + normalizeEnsName, + query, + safelyExecute, + safelyExecuteWithTimeout, + successfulFetch, + timeoutFetch, + toChecksumHexAddress, + toHex, + weiHexToGweiDec, +} from './util'; export * from './types'; export * from './siwe'; diff --git a/packages/controller-utils/src/util.test.ts b/packages/controller-utils/src/util.test.ts index 26e90b9da51..4bd6b16e2ec 100644 --- a/packages/controller-utils/src/util.test.ts +++ b/packages/controller-utils/src/util.test.ts @@ -11,12 +11,21 @@ const SOME_API = 'https://someapi.com'; const SOME_FAILING_API = 'https://somefailingapi.com'; describe('util', () => { + it('isSafeDynamicKey', () => { + expect(util.isSafeDynamicKey(util.toHex(MAX_SAFE_CHAIN_ID))).toBe(true); + expect(util.isSafeDynamicKey('')).toBe(true); + for (const badKey of util.PROTOTYPE_POLLUTION_BLOCKLIST) { + expect(util.isSafeDynamicKey(badKey)).toBe(false); + } + // @ts-expect-error - ensure that non-string input return false. + expect(util.isSafeDynamicKey(null)).toBe(false); + }); it('isSafeChainId', () => { expect(util.isSafeChainId(util.toHex(MAX_SAFE_CHAIN_ID + 1))).toBe(false); expect(util.isSafeChainId(util.toHex(MAX_SAFE_CHAIN_ID))).toBe(true); expect(util.isSafeChainId(util.toHex(0))).toBe(false); expect(util.isSafeChainId('0xinvalid')).toBe(false); - // @ts-expect-error - ensure that string args return false. + // @ts-expect-error - ensure that non-string args return false. expect(util.isSafeChainId('test')).toBe(false); }); diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index c9bf11c9b2a..3c2e4433fcc 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -16,6 +16,26 @@ import { MAX_SAFE_CHAIN_ID } from './constants'; const TIMEOUT_ERROR = new Error('timeout'); +export const PROTOTYPE_POLLUTION_BLOCKLIST = [ + '__proto__', + 'constructor', + 'prototype', +] as const; + +/** + * Checks whether a dynamic property key could be used in + * a [prototype pollution attack](https://portswigger.net/web-security/prototype-pollution). + * + * @param key - The dynamic key to validate. + * @returns Whether the given dynamic key is safe to use. + */ +export function isSafeDynamicKey(key: string): boolean { + return ( + typeof key === 'string' && + !PROTOTYPE_POLLUTION_BLOCKLIST.some((blockedKey) => key === blockedKey) + ); +} + /** * Checks whether the given number primitive chain ID is safe. * Because some cryptographic libraries we use expect the chain ID to be a diff --git a/packages/ens-controller/src/EnsController.test.ts b/packages/ens-controller/src/EnsController.test.ts index 735945b26c0..9a49dfe2caf 100644 --- a/packages/ens-controller/src/EnsController.test.ts +++ b/packages/ens-controller/src/EnsController.test.ts @@ -359,6 +359,16 @@ describe('EnsController', () => { expect(controller.state.ensEntries['0x1']).toBeUndefined(); }); + it('should return false if an ENS entry was NOT deleted due to unsafe input', () => { + const messenger = getMessenger(); + const controller = new EnsController({ + messenger, + }); + // @ts-expect-error Suppressing error to test runtime behavior + expect(controller.delete('__proto__', 'bar')).toBe(false); + expect(controller.delete(toHex(2), 'constructor')).toBe(false); + }); + it('should return false if an ENS entry was NOT deleted', () => { const messenger = getMessenger(); const controller = new EnsController({ diff --git a/packages/ens-controller/src/EnsController.ts b/packages/ens-controller/src/EnsController.ts index 76013ad6a9f..898c1697c9a 100644 --- a/packages/ens-controller/src/EnsController.ts +++ b/packages/ens-controller/src/EnsController.ts @@ -9,6 +9,7 @@ import type { ChainId } from '@metamask/controller-utils'; import { normalizeEnsName, isValidHexAddress, + isSafeDynamicKey, toChecksumHexAddress, CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP, convertHexToDecimal, @@ -194,6 +195,7 @@ export class EnsController extends BaseController< delete(chainId: Hex, ensName: string): boolean { const normalizedEnsName = normalizeEnsName(ensName); if ( + !isSafeDynamicKey(chainId) || !normalizedEnsName || !this.state.ensEntries[chainId] || !this.state.ensEntries[chainId][normalizedEnsName] diff --git a/packages/name-controller/package.json b/packages/name-controller/package.json index d637d960e77..cc0889cf8b5 100644 --- a/packages/name-controller/package.json +++ b/packages/name-controller/package.json @@ -43,6 +43,7 @@ }, "dependencies": { "@metamask/base-controller": "^5.0.1", + "@metamask/controller-utils": "^9.0.2", "@metamask/utils": "^8.3.0", "async-mutex": "^0.2.6" }, diff --git a/packages/name-controller/src/NameController.test.ts b/packages/name-controller/src/NameController.test.ts index 9159e256ee5..d05d3fa80a4 100644 --- a/packages/name-controller/src/NameController.test.ts +++ b/packages/name-controller/src/NameController.test.ts @@ -458,6 +458,64 @@ describe('NameController', () => { }); }); + it('does not update if passed unsafe input', () => { + const provider1 = createMockProvider(1); + + const controller = new NameController({ + ...CONTROLLER_ARGS_MOCK, + providers: [provider1], + state: { + names: { + [NameType.ETHEREUM_ADDRESS]: { + [VALUE_MOCK]: { + [CHAIN_ID_MOCK]: { + name: null, + sourceId: null, + origin: null, + proposedNames: { + [SOURCE_ID_MOCK]: { + proposedNames: [PROPOSED_NAME_MOCK, PROPOSED_NAME_2_MOCK], + lastRequestTime: null, + updateDelay: null, + }, + }, + }, + }, + }, + }, + }, + }); + + controller.setName({ + value: '__proto__', + type: NameType.ETHEREUM_ADDRESS, + name: NAME_MOCK, + sourceId: `${SOURCE_ID_MOCK}1`, + variation: CHAIN_ID_MOCK, + }); + + expect(controller.state.names).toStrictEqual< + NameControllerState['names'] + >({ + [NameType.ETHEREUM_ADDRESS]: { + [VALUE_MOCK]: { + [CHAIN_ID_MOCK]: { + name: null, + sourceId: null, + origin: null, + proposedNames: { + [SOURCE_ID_MOCK]: { + proposedNames: [PROPOSED_NAME_MOCK, PROPOSED_NAME_2_MOCK], + lastRequestTime: null, + updateDelay: null, + }, + }, + }, + }, + }, + }); + }); + it('does not throw if variation is fallback and type is Ethereum address', () => { const controller = new NameController(CONTROLLER_ARGS_MOCK); diff --git a/packages/name-controller/src/NameController.ts b/packages/name-controller/src/NameController.ts index 197015dd894..c4d534d955b 100644 --- a/packages/name-controller/src/NameController.ts +++ b/packages/name-controller/src/NameController.ts @@ -4,6 +4,7 @@ import type { RestrictedControllerMessenger, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; +import { isSafeDynamicKey } from '@metamask/controller-utils'; import type { NameProvider, @@ -441,6 +442,14 @@ export class NameController extends BaseController< const normalizedValue = this.#normalizeValue(value, type); const normalizedVariation = this.#normalizeVariation(variationKey, type); + if ( + [normalizedValue, normalizedVariation].some( + (key) => !isSafeDynamicKey(key), + ) + ) { + return; + } + this.update((state) => { const typeEntries = state.names[type] || {}; state.names[type] = typeEntries; diff --git a/yarn.lock b/yarn.lock index 0b12c12ec3f..ee7e0873a41 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2540,6 +2540,7 @@ __metadata: dependencies: "@metamask/auto-changelog": ^3.4.4 "@metamask/base-controller": ^5.0.1 + "@metamask/controller-utils": ^9.0.2 "@metamask/utils": ^8.3.0 "@types/jest": ^27.4.1 async-mutex: ^0.2.6