Add harden events utilities (#1221)

* Add harden events utilities Change lockdown execution order WIP: Experimental testing (for debugging purposes only) Update with version of code that will work after updating post-message-stream Add review refactoring (1) Add review refactoring (2) Add review refactoring (3) Update post-message-stream version and add test to improve coverage Fix coverage after rebase Try to fix coverage Refactor lockdown-events.ts Fix yarn conflicts after rebase Refactor test for events lockdown function Add lockdown events in other environments Revert "Add lockdown events in other environments" This reverts commit 4e89e4e. Add new proposal for implementing lockdown of events Fix coverage issues * Fix after rebase
MetaMask · Mar 15, 2023 · 6885e48 · 6885e48
1 parent 8dcb465
commit 6885e48
Show file tree

Hide file tree

Showing 11 changed files with 157 additions and 12 deletions.
diff --git a/packages/snaps-controllers/package.json b/packages/snaps-controllers/package.json
@@ -38,7 +38,7 @@
     "@metamask/base-controller": "^2.0.0",
     "@metamask/object-multiplex": "^1.1.0",
     "@metamask/permission-controller": "^3.0.0",
-    "@metamask/post-message-stream": "^6.1.0",
+    "@metamask/post-message-stream": "^6.1.1",
     "@metamask/rpc-methods": "^0.31.0",
     "@metamask/snaps-execution-environments": "^0.31.0",
     "@metamask/snaps-registry": "^1.1.1",

diff --git a/packages/snaps-controllers/src/services/iframe/IframeExecutionService.test.browser.ts b/packages/snaps-controllers/src/services/iframe/IframeExecutionService.test.browser.ts
@@ -167,4 +167,58 @@ describe('IframeExecutionService', () => {
       'Failed to access document of the snap iframe: SecurityError',
     );
   });
+
+  it('confirms that events are secured', async () => {
+    // Check if the security critical properties of the Event object
+    // are unavailable. This will confirm that executeLockdownEvents works
+    // inside snaps-execution-environments
+    const { service } = createService(IframeExecutionService, {
+      iframeUrl: new URL(IFRAME_URL),
+    });
+
+    await service.executeSnap({
+      snapId: MOCK_SNAP_ID,
+      sourceCode: `
+        module.exports.onRpcRequest = async ({ request }) => {
+            let result;
+            const promise = new Promise((resolve) => {
+                const xhr = new XMLHttpRequest();
+                xhr.open('GET', 'https://metamask.io/');
+                xhr.send();
+                xhr.onreadystatechange = (ev) => {
+                  result = ev;
+                  resolve();
+                };
+            });
+            await promise;
+
+            return {
+              targetIsUndefined: result.target === undefined,
+              currentTargetIsUndefined: result.target === undefined,
+              srcElementIsUndefined: result.target === undefined,
+              composedPathIsUndefined: result.target === undefined
+            };
+        };
+      `,
+      endowments: ['console', 'XMLHttpRequest'],
+    });
+
+    const result = await service.handleRpcRequest(MOCK_SNAP_ID, {
+      origin: 'foo',
+      handler: HandlerType.OnRpcRequest,
+      request: {
+        jsonrpc: '2.0',
+        id: 1,
+        method: 'foobar',
+        params: [],
+      },
+    });
+
+    expect(result).toStrictEqual({
+      targetIsUndefined: true,
+      currentTargetIsUndefined: true,
+      srcElementIsUndefined: true,
+      composedPathIsUndefined: true,
+    });
+  });
 });
diff --git a/packages/snaps-execution-environments/coverage.json b/packages/snaps-execution-environments/coverage.json
@@ -1,6 +1,6 @@
 {
-  "branches": 77.41,
-  "functions": 91.26,
-  "lines": 85.29,
-  "statements": 85.37
+  "branches": 78.62,
+  "functions": 91.4,
+  "lines": 87.95,
+  "statements": 88.02
 }
diff --git a/packages/snaps-execution-environments/jest.config.js b/packages/snaps-execution-environments/jest.config.js
@@ -5,7 +5,12 @@ const baseConfig = require('../../jest.config.base');
 delete baseConfig.coverageThreshold;
 
 module.exports = deepmerge(baseConfig, {
-  coveragePathIgnorePatterns: ['./src/index.ts', './src/common/test-utils'],
+  coveragePathIgnorePatterns: [
+    './src/index.ts',
+    './src/iframe/index.ts',
+    './src/offscreen/index.ts',
+    './src/common/test-utils',
+  ],
   coverageDirectory: './coverage/jest',
   testTimeout: 10000,
   testEnvironment: '<rootDir>/jest.environment.js',

diff --git a/packages/snaps-execution-environments/package.json b/packages/snaps-execution-environments/package.json
@@ -36,7 +36,7 @@
   },
   "dependencies": {
     "@metamask/object-multiplex": "^1.2.0",
-    "@metamask/post-message-stream": "^6.1.0",
+    "@metamask/post-message-stream": "^6.1.1",
     "@metamask/providers": "^10.2.0",
     "@metamask/rpc-methods": "^0.31.0",
     "@metamask/snaps-utils": "^0.31.0",

diff --git a/packages/snaps-execution-environments/src/common/lockdown/lockdown-events.test.browser.ts b/packages/snaps-execution-environments/src/common/lockdown/lockdown-events.test.browser.ts
@@ -0,0 +1,25 @@
+// eslint-disable-next-line import/unambiguous
+import { expect } from '@wdio/globals';
+
+import { executeLockdownEvents } from './lockdown-events';
+
+describe('lockdown events security', () => {
+  it('should lockdown events and made event properties inaccessible', async () => {
+    executeLockdownEvents();
+
+    const eventTarget = new EventTarget();
+
+    const promise = new Promise((resolve) => {
+      eventTarget.addEventListener('just-test-event', (eventObject) => {
+        // eslint-disable-next-line @typescript-eslint/unbound-method
+        resolve(eventObject.composedPath);
+      });
+    });
+
+    const testEvent = new Event('just-test-event');
+    eventTarget.dispatchEvent(testEvent);
+
+    const result = await promise;
+    expect(result).toBeUndefined();
+  });
+});
diff --git a/packages/snaps-execution-environments/src/common/lockdown/lockdown-events.ts b/packages/snaps-execution-environments/src/common/lockdown/lockdown-events.ts
@@ -0,0 +1,57 @@
+// When creating a sandbox, limitation of the events from leaking
+// sensitive objects is required. This is done by overriding own properties
+// of prototypes of all existing events.
+import { hasProperty } from '@metamask/utils';
+
+/**
+ * Targeted Event objects and properties.
+ * Note: This is a map of the prototypes that inherit from Events with
+ * properties that are identified to leak sensitive objects.
+ * Not all browsers support all event types, so checking its existence is required.
+ */
+const targetEvents = new Map();
+if (hasProperty(globalThis, 'UIEvent')) {
+  targetEvents.set(UIEvent.prototype, ['view']);
+}
+if (hasProperty(globalThis, 'MutationEvent')) {
+  targetEvents.set(MutationEvent.prototype, ['relatedNode']);
+}
+if (hasProperty(globalThis, 'MessageEvent')) {
+  targetEvents.set(MessageEvent.prototype, ['source']);
+}
+if (hasProperty(globalThis, 'FocusEvent')) {
+  targetEvents.set(FocusEvent.prototype, ['relatedTarget']);
+}
+if (hasProperty(globalThis, 'MouseEvent')) {
+  targetEvents.set(MouseEvent.prototype, [
+    'relatedTarget',
+    'fromElement',
+    'toElement',
+  ]);
+}
+if (hasProperty(globalThis, 'TouchEvent')) {
+  targetEvents.set(TouchEvent.prototype, ['targetTouches', 'touches']);
+}
+if (hasProperty(globalThis, 'Event')) {
+  targetEvents.set(Event.prototype, [
+    'target',
+    'currentTarget',
+    'srcElement',
+    'composedPath',
+  ]);
+}
+
+/**
+ * Attenuate Event objects by replacing its own properties.
+ */
+export function executeLockdownEvents() {
+  targetEvents.forEach((properties, prototype) => {
+    for (const property of properties) {
+      Object.defineProperty(prototype, property, {
+        value: undefined,
+        configurable: false,
+        writable: false,
+      });
+    }
+  });
+}
diff --git a/packages/snaps-execution-environments/src/iframe/index.ts b/packages/snaps-execution-environments/src/iframe/index.ts
@@ -1,8 +1,10 @@
 import { executeLockdown } from '../common/lockdown/lockdown';
+import { executeLockdownEvents } from '../common/lockdown/lockdown-events';
 import { executeLockdownMore } from '../common/lockdown/lockdown-more';
 import { IFrameSnapExecutor } from './IFrameSnapExecutor';
 
 executeLockdown();
 executeLockdownMore();
+executeLockdownEvents();
 
 IFrameSnapExecutor.initialize();
diff --git a/packages/snaps-execution-environments/src/offscreen/index.ts b/packages/snaps-execution-environments/src/offscreen/index.ts
@@ -1,11 +1,13 @@
 import { BrowserRuntimePostMessageStream } from '@metamask/post-message-stream';
 
 import { executeLockdown } from '../common/lockdown/lockdown';
+import { executeLockdownEvents } from '../common/lockdown/lockdown-events';
 import { executeLockdownMore } from '../common/lockdown/lockdown-more';
 import { OffscreenSnapExecutor } from './OffscreenSnapExecutor';
 
 executeLockdown();
 executeLockdownMore();
+executeLockdownEvents();
 
 // The stream from the offscreen document to the execution service.
 const parentStream = new BrowserRuntimePostMessageStream({

diff --git a/packages/snaps-utils/package.json b/packages/snaps-utils/package.json
@@ -82,7 +82,7 @@
     "@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.0",
+    "@metamask/post-message-stream": "^6.1.1",
     "@types/jest": "^27.5.1",
     "@types/mocha": "^10.0.1",
     "@types/semver": "^7.3.10",

diff --git a/yarn.lock b/yarn.lock
@@ -2937,7 +2937,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@metamask/post-message-stream@npm:^6.1.0":
+"@metamask/post-message-stream@npm:^6.1.1":
   version: 6.1.1
   resolution: "@metamask/post-message-stream@npm:6.1.1"
   dependencies:
@@ -3145,7 +3145,7 @@ __metadata:
     "@metamask/eslint-config-typescript": ^11.0.0
     "@metamask/object-multiplex": ^1.1.0
     "@metamask/permission-controller": ^3.0.0
-    "@metamask/post-message-stream": ^6.1.0
+    "@metamask/post-message-stream": ^6.1.1
     "@metamask/rpc-methods": ^0.31.0
     "@metamask/snaps-execution-environments": ^0.31.0
     "@metamask/snaps-registry": ^1.1.1
@@ -3228,7 +3228,7 @@ __metadata:
     "@metamask/eslint-config-nodejs": ^11.0.1
     "@metamask/eslint-config-typescript": ^11.0.0
     "@metamask/object-multiplex": ^1.2.0
-    "@metamask/post-message-stream": ^6.1.0
+    "@metamask/post-message-stream": ^6.1.1
     "@metamask/providers": ^10.2.0
     "@metamask/rpc-methods": ^0.31.0
     "@metamask/snaps-utils": ^0.31.0
@@ -3420,7 +3420,7 @@ __metadata:
     "@metamask/eslint-config-typescript": ^11.0.0
     "@metamask/key-tree": ^7.0.0
     "@metamask/permission-controller": ^3.0.0
-    "@metamask/post-message-stream": ^6.1.0
+    "@metamask/post-message-stream": ^6.1.1
     "@metamask/providers": ^10.2.1
     "@metamask/snaps-registry": ^1.1.1
     "@metamask/snaps-ui": ^0.31.0