Fix prototype-polluting assignments

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');

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]

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,
     },
   },
 

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';

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);
   });
 

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

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({

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]

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"
   },

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);
 

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;

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