build(extension): Robustify JavaScript module externalization #45

grypez · 2024-09-03T17:22:00Z

Closes: #14

In particular, we require that our trusted computing base (which includes the ocap kernel) be endoified so that it may be robustly composed with untrusted code without compromising the consistency of our plan (which, in the case of MetaMask, includes maintaining users' control over and privacy of their identities). Our strategy for meeting this requirement is largely encapsulated in the @ocap/shims/endoify shim, but also includes maintaining the following strict import procedure for all javascript entrypoints in the trusted computing base:

Import trusted code

Import endoify

Import untrusted code

We call steps 1 and 2 in order the trusted prelude of a file.

However, it is still possible for the bundling process to interfere with the endoification of the entrypoints by permuting the order of import statements in their corresponding bundles. Such a permutation could effectively move untrusted code into the trusted computing base, which our plans cannot tolerate. This issue aims to address that possibility with a strategy which is both effective and easily audited.

The strategy is to break out trusted preludes into their own file, and mark them as external (ignored by the bundler and statically copied into dist). In files which import the trusted header, we check if their first line in dist is to import the trusted header.

For total compartmentalization of this security concern, we should implement build tests that checks for first line import '*-trusted-prelude.js'; in minimally reliant proper js.

grypez · 2024-09-03T17:28:48Z

packages/extension/vite.config.ts

+    buildEnd: {
+      order: 'post',
+      handler(error) {
+        if (error !== undefined) {
+          return;
+        }
+        const trustedHeaders = Array.from(this.getModuleIds()).filter((moduleId) => isTrustedHeader(moduleId));
+        const importers = trustedHeaders.map((trustedHeader) => this.getModuleInfo(trustedHeader)?.importers.at(0)!);
+        importers.forEach(async (moduleId: string) => {
+          const code = this.getModuleInfo(moduleId)?.code;
+          if ( code === null || code === undefined ) {
+            this.error(
+              `NoCode: Module ${moduleId} was identified as a trusted header importer but has no code at buildEnd.`
+            );
+          } else {
+            const prefix = makeExpectedPrefix(moduleId);
+            if (code.match(prefix) === null) {
+              this.error(
+                `MissingTrustedHeaderImport: Module ${moduleId} was identified as a trusted header importer, ` + 
+                `but does not begin with trusted header import.\n` +
+                `ExpectedPrefix: ${prefix}\n` +
+                `ObservedCode: ${code.split(';').at(0)}`);
+            }
+          }
+        });
+      }
+    },


To remove rollup from the TCB of our security proof, this check should be handled in a build test which gates pushes to prod. It would look similar to what is implemented here, but would need its own method for identifying which files are meant to be trusted, be it systematic or declared.

packages/extension/plugins/vite-plugin-endoify-html-files.ts

rekmarks

Might I suggest "trusted prelude" instead of "trusted header"? In my mind, "header" in the context of a source file connotes metadata and/or config options, while "prelude" is something that is actually executed. See also: the "synchronous prelude" of an async function.

rekmarks

Some further suggested name changes:

plugins/ -> build/
- Just "plugins" invites the question "plugins for what?". Since the file names contain the word "plugin" and the name of the... "plug-ee", there's already no question that they're plugins or what they plug in to.
rollup-plugin-endoify-trusted-header.ts -> rollup-plugin-js-trusted-prelude.ts
vite-plugin-endoify-html-files.ts -> vite-plugin-html-trusted-prelude.ts

Note: the exports of the files should match the file names.

Edit: Since both plugins are in fact Vite plugins, we could also do this, which I think I prefer:

vite-plugins/
- html-trusted-prelude.ts
- js-trusted-prelude.ts

rekmarks

Some minor requested changes, mostly related to documentation and the naming scheme for the plugins.

I'll note what I believe is one limitation of the JS plugin: if we ever observe that the import order has changed, the build will fail with no recourse. Instead, could we not parameterize the entry point files to the plugin and insert the import statement ourselves? I don't think this would negatively impact devex, since we could still import the types of the trusted header / prelude files. In any event, something to consider.

packages/extension/plugins/rollup-plugin-endoify-trusted-header.ts

rekmarks · 2024-09-04T15:30:05Z

packages/extension/plugins/rollup-plugin-endoify-trusted-header.ts

+    console.log(expectedPrefix);
+    return expectedPrefix;
+  };
+  return {


The internals of this would benefit from some documentation links and/or a description of what the different properties are for.

packages/extension/plugins/rollup-plugin-endoify-trusted-header.ts

rekmarks · 2024-09-04T15:41:58Z

packages/extension/plugins/rollup-plugin-endoify-trusted-header.ts

+
+/**
+ * Vite plugin to ensure that the following are true:
+ * - Every entrypoint contains at most one import from a *trusted-header file.


Does this plugin actually check if any of the files are entrypoints, or just if they imported a trusted header / prelude?

It doesn't check for entrypoints. For now I have corrected the doc string to bundles. The current functionality ought to be sufficient for our purposes---to ensure our entrypoints have trusted headers. The risk of omitting the entrypoint check is that a dev could get the impression they have a trusted header even though their code is vulnerable to being imported after a malicious initialization. Given our organization processes, I'm comfortable with that risk.

Sorry I should have specified: my comment was more about the comment on L6, which says that the plugin ensures that something is true about entrypoints, but actually it just enforces the property for any file it comes across.

This incorrect description has been corrected in the latest docs commit.

/** * A Vite plugin to ensure the following. * - Every declared trusted prelude is handled externally (automatically merged into {@link RollupOptions.external}). * - Every declared trusted prelude importer: * - Is a declared entry point (throws during {@link Plugin.buildStart} otherwise). * - Imports at most one declared trusted prelude (throws during {@link Plugin.generateBundle} otherwise). * - Begins by importing its declared trusted prelude (prepended during {@link Plugin.generateBundle} if missing). * * ... */

packages/extension/plugins/vite-plugin-endoify-html-files.ts

grypez · 2024-09-05T21:18:15Z

I'll note what I believe is one limitation of the JS plugin: if we ever observe that the import order has changed, the build will fail with no recourse. Instead, could we not parameterize the entry point files to the plugin and insert the import statement ourselves? I don't think this would negatively impact devex, since we could still import the types of the trusted header / prelude files. In any event, something to consider.

Build failures with inscrutable root causes are a bad time, agreed.

Since import statements are idempotent, we could also leave the import statement in the source code and still prepend the import statement instead of checking for it. The bundled entrypoint of foo.ts would then typically look like this:

import"foo-trusted-prelude.js";import"foo-trusted-prelude.js";import{X as a,Rf as g}from"./bar.js";...

And sometimes like this:

import"foo-trusted-prelude.js";import{X as a,Rf as g}from"./bar.js";import"foo-trusted-prelude.js";...

packages/extension/vite.config.ts

grypez · 2024-09-09T19:35:18Z

The latest implementation does not rely on the naming convention *-trusted-prelude.js, instead requiring explicit declarations in vite.config.ts, but the naming convention is still recommended for reasons of collocation and obviation.

grypez · 2024-09-09T20:31:31Z

Notably the refactored plugins are not test covered, but the reviewer is encouraged to check out the following branches and run yarn build to verify behavior.

Allow multiple entry points with separate trusted headers.

git checkout grypez/feat-externalization-logic-valid-multi-entry

Throw because `background.js` was not declared an entry point.

git checkout grypez/feat-externalization-logic-invalid-non-entry

Throw because `background.js` gets a transitive trusted prelude import through `shared.js`.

git checkout grypez/feat-externalization-logic-invalid-multi-prelude

Warn and correct when interfering plugin changes first import.

git checkout grypez/feat-externalization-logic-interfering-plugin

…tions.

The implementation throws errors only if its configuration assumptions are violated. Otherwise, it proactively externalizes trusted preludes and attempts to correct any correctness-breaking import permutations that may have occurred during bundling, merely warning that some corrective action was necessary but not breaking the build. The configuration assumptions are as follows. - If a file has a declared trusted prelude, the file must be declared an entry point. - If a file has a declared trusted prelude, it must not transitively import more than one declared trusted prelude. The result is a plugin which ensures that a declared trusted prelude file is never imported after any other code, bandaging bundles which unintentionally violate this constraint and rejecting source code which intentionally violates this constaint.

rekmarks

Looks good! Just one minor thing and we're there.

rekmarks · 2024-09-10T20:39:24Z

packages/extension/vite-plugins/js-trusted-prelude.ts

+      getTrustedPrelude = (context: PluginContext, importer: string) => {
+        const trustedPrelude = resolvedTrustingImporters.get(importer);
+        // Ensure importer was declared and recognized as a trusting importer.
+        if (!trustedPrelude) {
+          // Shouldn't be possible without heavy interference from other plugins.
+          context.error(
+            `Module "${importer}" was identified as but not declared as a trusted prelude importer.`,
+          );
+        }
+        return trustedPrelude;
+      };


I did a double take reading this, because I wasn't sure if trustedPrelude referred to a filename or the actual trusted prelude content. This confusion can be precluded by:

Suggested change

getTrustedPrelude = (context: PluginContext, importer: string) => {

const trustedPrelude = resolvedTrustingImporters.get(importer);

// Ensure importer was declared and recognized as a trusting importer.

if (!trustedPrelude) {

// Shouldn't be possible without heavy interference from other plugins.

context.error(

`Module "${importer}" was identified as but not declared as a trusted prelude importer.`,

);

}

return trustedPrelude;

};

getTrustedPreludeFileName = (context: PluginContext, importer: string) => {

const trustedPreludeFileName = resolvedTrustingImporters.get(importer);

// Ensure importer was declared and recognized as a trusting importer.

if (!trustedPreludeFileName) {

// Shouldn't be possible without heavy interference from other plugins.

context.error(

`Module "${importer}" was identified as but not declared as a trusted prelude importer.`,

);

}

return trustedPreludeFileName;

};

rekmarks

LGTM!

grypez force-pushed the grypez/feat-externalization-logic branch from 7e97006 to 117951b Compare September 3, 2024 17:23

grypez commented Sep 3, 2024

View reviewed changes

grypez force-pushed the grypez/feat-externalization-logic branch from 117951b to cd4878c Compare September 3, 2024 20:09

grypez commented Sep 3, 2024

View reviewed changes

packages/extension/plugins/vite-plugin-endoify-html-files.ts Outdated Show resolved Hide resolved

grypez marked this pull request as ready for review September 3, 2024 20:34

grypez requested a review from a team as a code owner September 3, 2024 20:34

rekmarks reviewed Sep 4, 2024

View reviewed changes

rekmarks requested changes Sep 4, 2024

View reviewed changes

rekmarks changed the title ~~build(extension): Introduce module externalization logic~~ build(extension): Robustify JavaScript module externalization Sep 5, 2024

rekmarks reviewed Sep 5, 2024

View reviewed changes

packages/extension/vite.config.ts Outdated Show resolved Hide resolved

grypez force-pushed the grypez/feat-externalization-logic branch from 248f8f2 to 72fc236 Compare September 5, 2024 23:51

grypez force-pushed the grypez/feat-externalization-logic branch from 7620df9 to 060cebe Compare September 9, 2024 19:36

grypez requested a review from rekmarks September 9, 2024 19:39

grypez added 13 commits September 10, 2024 13:08

build(extension): Add *-trusted-header externalization logic.

ff276f9

chore(extension): Granted trusted header lint exceptions.

6aeef36

refactor(extension): Modularize build plugins.

7761df5

fix(extension): Remove error prefixes.

2eb4e0f

chore(extension): Demote some labels from PascalCase.

5ffdad9

docs(extension): Normalize and explicate build plugin docs.

8cf0435

refactor(extension): Relocate trusted s/header/prelude/g vite plugins.

71c731d

fix(extension): Tidy the vite-plugins.

deca52e

refactor(build): Remove Plugin suffix from exported Plugin maker func…

5a1de6a

…tions.

docs(extension): Annotate jsTrustedPrelude.

25f6e58

docs(extension): Permute plugin doc to align temporally.

aae768a

docs(extension): s/header/prelude/g

a939b92

docs(extension): Resolve TODO comment.

92ce904

grypez force-pushed the grypez/feat-externalization-logic branch from 26a761c to 92ce904 Compare September 10, 2024 18:22

rekmarks requested changes Sep 10, 2024

View reviewed changes

docs(extension): Clarify some plugin internal names.

6808a58

grypez enabled auto-merge (squash) September 10, 2024 23:42

rekmarks approved these changes Sep 10, 2024

View reviewed changes

grypez merged commit 21cf8a8 into main Sep 10, 2024

grypez deleted the grypez/feat-externalization-logic branch September 10, 2024 23:48

build(extension): Robustify JavaScript module externalization #45

build(extension): Robustify JavaScript module externalization #45

Uh oh!

Conversation

grypez commented Sep 3, 2024 • edited by rekmarks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grypez Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rekmarks Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rekmarks Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

grypez Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

rekmarks Sep 9, 2024

Choose a reason for hiding this comment

Uh oh!

grypez Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grypez commented Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

grypez commented Sep 9, 2024

Uh oh!

grypez commented Sep 9, 2024

Allow multiple entry points with separate trusted headers.

Throw because background.js was not declared an entry point.

Throw because background.js gets a transitive trusted prelude import through shared.js.

Warn and correct when interfering plugin changes first import.

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

rekmarks Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grypez commented Sep 3, 2024 •

edited by rekmarks

Loading

rekmarks left a comment •

edited

Loading

grypez Sep 10, 2024 •

edited

Loading

grypez commented Sep 5, 2024 •

edited

Loading

Throw because `background.js` was not declared an entry point.

Throw because `background.js` gets a transitive trusted prelude import through `shared.js`.