Lint to disallow SES polymorphic calls #827

kumavis · 2021-07-09T19:18:24Z

[edit kriskowal 2021-07-15]

This adds an eslint plugin that @kumavis authored for us to help us validate that SES shim captures all of the behavior of intrinsics that it needs during initialization in order to provide a faithful emulation of a native SES implementation, to the extent that’s possible and given that SES runs before any intrinsic is altered away from specified behavior.

Then follows changes by me to get SES shim to obey the lint rule.

@erights and I concluded that console and assert should in general be bound late and their methods dispatched dynamically.

Some compartment code moved around to prevent alteration of the Compartment prototype from affecting the behavior of “native” compartment methods.

kumavis · 2021-07-09T19:20:46Z

here's one example. should we be concerned that this could be dangerous?
https://github.com/endojs/endo/blob/master/packages/ses/src/scope-constants.js#L155

✖ 2078 problems (0 errors, 2078 warnings)

kumavis · 2021-07-09T19:50:21Z

added a "preview" of the call to the lint message

/home/xyz/Development/endo/packages/ses/src/module-instance.js
   17:5   warning  Polymorphic call: "compartmentPrivateFields.get". May be vulnerable to corruption or trap        @endo/no-polymorphic-call
   26:8   warning  Polymorphic call: "Array.isArray". May be vulnerable to corruption or trap                       @endo/no-polymorphic-call
   27:7   warning  Polymorphic call: "staticModuleRecord.exports.some". May be vulnerable to corruption or trap     @endo/no-polymorphic-call
   33:5   warning  Polymorphic call: "staticModuleRecord.exports.forEach". May be vulnerable to corruption or trap  @endo/no-polymorphic-call
   54:9   warning  Polymorphic call: "updaters.push". May be vulnerable to corruption or trap                       @endo/no-polymorphic-call
   68:9   warning  Polymorphic call: "staticModuleRecord.execute". May be vulnerable to corruption or trap          @endo/no-polymorphic-call
   99:29  warning  Polymorphic call: "privateFields.get". May be vulnerable to corruption or trap                   @endo/no-polymorphic-call
  135:3   warning  Polymorphic call: "[[CallExpression]].forEach". May be vulnerable to corruption or trap          @endo/no-polymorphic-call
  180:11  warning  Polymorphic call: "optUpdaters.push". May be vulnerable to corruption or trap                    @endo/no-polymorphic-call
  205:3   warning  Polymorphic call: "[[CallExpression]].forEach". May be vulnerable to corruption or trap          @endo/no-polymorphic-call
  261:11  warning  Polymorphic call: "updaters.push". May be vulnerable to corruption or trap                       @endo/no-polymorphic-call
  318:47  warning  Polymorphic call: "updateRecord.entries". May be vulnerable to corruption or trap                @endo/no-polymorphic-call
  319:24  warning  Polymorphic call: "importedInstances.get". May be vulnerable to corruption or trap               @endo/no-polymorphic-call
  320:7   warning  Polymorphic call: "instance.execute". May be vulnerable to corruption or trap                    @endo/no-polymorphic-call
  322:44  warning  Polymorphic call: "importUpdaters.entries". May be vulnerable to corruption or trap              @endo/no-polymorphic-call
  333:11  warning  Polymorphic call: "exportAlls.includes". May be vulnerable to corruption or trap                 @endo/no-polymorphic-call
  374:5   warning  Polymorphic call: "[[CallExpression]].forEach". May be vulnerable to corruption or trap          @endo/no-polymorphic-call
  374:5   warning  Polymorphic call: "[[CallExpression]].sort". May be vulnerable to corruption or trap             @endo/no-polymorphic-call
  382:20  warning  Polymorphic call: "compartment.evaluate". May be vulnerable to corruption or trap                @endo/no-polymorphic-call

kriskowal · 2021-07-15T18:50:49Z

@kumavis I did a rebase and force-pushed with extra commits to cut the warnings down by half. I’m shooting to have most of them addressed with another commit or so today.

One of the interesting ones is using the compartment API itself as intrinsics, which will take some refactoring.

kriskowal · 2021-07-16T04:51:18Z

@kumavis Added commits to eliminate the warnings. This is ready for broader review and I’ll look into rebasing and cleanup tomorrow.

Jack-Works · 2021-07-19T04:00:17Z

packages/eslint-plugin/lib/rules/no-polymorphic-call.js

+  let propertyHint
+  if (object.type === 'Identifier') {
+    objectHint = object.name
+  } else if (object.type === 'MemberExpression') {


Does it cover a[b]?

This certainly did produce lint errors for a[b]() syntax.

yes same member expression but with computed flag set to true

kriskowal · 2021-07-19T04:15:32Z

It does catch that, indeed.

…

On Sun, Jul 18, 2021 at 9:00 PM Jack Works ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/eslint-plugin/lib/rules/no-polymorphic-call.js <#827 (comment)>: > + return + } + const reportHint = prepareMemberExpressionHint(node.callee) + context.report(node, `Polymorphic call: "${reportHint}". May be vulnerable to corruption or trap`) + } + } + }, +} + +function prepareMemberExpressionHint (node) { + const { object, property, computed } = node + let objectHint + let propertyHint + if (object.type === 'Identifier') { + objectHint = object.name + } else if (object.type === 'MemberExpression') { Does it cover a[b]? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#827 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOXBQ7Q2LPQSZKT2VWFXLTYOPNZANCNFSM5ADJ3JBQ> .

kriskowal · 2021-07-21T23:39:30Z

packages/ses/src/compartment-evaluate.js

@@ -0,0 +1,72 @@
+/// <reference types="ses">


The Compartment.evaluate implementation needed to be extracted into a stand-alone module so it could be used from both compartment-shim.js and module-instance.js without dynamic dispatch. The internals communicate purely through their captured private fields.

I (MarkM) did not review this refactoring.

kriskowal · 2021-07-21T23:41:33Z

packages/ses/src/error/console.js

        baseConsole[name](...args);
      }
    };
    return [name, freeze(method)];
  });
  const filteringConsole = fromEntries(methods);
-  return freeze(filteringConsole);
+  return /** @type {VirtualConsole} */ (freeze(filteringConsole));


This is an unrelated change that was necessary to appease the IDE type-checker, but not our TSC lint CI job.

kriskowal · 2021-07-21T23:43:07Z

packages/ses/src/error/fatal-assert.js

@@ -18,7 +18,9 @@ if (typeof abandon === 'function') {
  /** @param {Error} reason */
  raise = reason => {
    // Check `console` each time `raise` is called.
+    // eslint-disable-next-line no-restricted-globals


@erights and I expressly decided that lazy binding console and assert and dynamic dispatch on their methods were the desired, albeit exceptional, behavior. The reason being that SES changes these on lockdown. The machinery to have lockdown incidentally change some shared module state is far too complex to warrant.

kriskowal · 2021-07-21T23:45:11Z

packages/ses/src/intrinsics.js

-          continue;
-        }
-        intrinsics[namePrototype] = intrinsicPrototype;
+  const addIntrinsics = newIntrinsics => {


The change below here is merely changing the intrinsicsCollector to a bag of closure functions instead of using concise methods. They were already used as bare closure functions, but this needed to be more explicit to avoid dynamic dispatch.

This is a case where dynamic dispatch would have been just as safe, but it was as easy to refactor this way and avoid lint-rule comment noise.

kumavis

I've created a monster 👹
(looks good)

erights · 2021-08-24T18:10:37Z

I think we made a mistake prior to this PR that should be fixed before this PR: commons.js should not be capturing and reexporting things like Error which, if captured this early, would be the original Error constructor that is supposed to be completely inaccessible after repair, even in the start compartment. The original Error constructor should be completely encapsulated within the repaired Error constructors.

Thus, these repair modules should do their own initial capture of these objects and not reexport them. But commons.js, capturing the originals and reexporting them, keeps them accessible rather than safely encapsulated.

erights

At #827 (comment) I write:

I think we made a mistake prior to this PR that should be fixed before this PR: commons.js should not be capturing and reexporting things like Error which, if captured this early, would be the original Error constructor that is supposed to be completely inaccessible after repair, even in the start compartment. The original Error constructor should be completely encapsulated within the repaired Error constructors.

Thus, these repair modules should do their own initial capture of these objects and not reexport them. But commons.js, capturing the originals and reexporting them, keeps them accessible rather than safely encapsulated.

I'm posting that as a "request changes" so it is repaired before this PR proceeds.

erights · 2021-08-24T18:30:58Z

Just eyeballing commons.js quickly, the violations that jump out at me:

Error
RegExp
FERAL_EVAL
FERAL_FUNCTION

We should either remove all four from commons.js, or we should follow the FERAL_ALL_CAPS convention for all four. The FERAL_ convention is adequate for the safety issue at stake here. All the code that could import commons.js is in the TCB, so the potential problem is really the reviewability hazard.

Note that WeakMap and WeakSet are not violations, even though @FUDCo virtualizes them at a higher level. Chip does that in a new compartment. The start compartment should continue to have the initial ones, and so commons.js should as well.

The thread above already alerts us to the issue of the non-standard powerful SES-shim globals assert and console.

Upcoming dangerous standard globals for us to eventually incorporate: WeakRef and FinalizationRegistery. For both, the powerful ones should be only in the start compartment. Their shared prototypes should be in the whitelist. And the shared prototype's constructor should point at a safe one. Perhaps the safe one should be inert, following the evaluator pattern, or perhaps it should simply be powerless, i.e., strongly pointing, following the SharedDate pattern.

So, for now, unless I missed something, just exporting the original Error and RegExp as FERAL_ERROR and FERAL_REGEXP looks like an adequate fix.

erights

The FERAL_ naming only works if it occurs rarely enough to set off alarm bells when you see it. Most of the occurrences here are just throw FERAL_ERROR which probably should have been throw TypeError anyway as that is the more usual choice. With those fixed, the problem is much reduced, and the phrase will continue to alarm.

packages/ses/index.js

erights · 2021-08-28T03:50:10Z

packages/ses/index.js

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-import { globalThis, Error, assign } from './src/commons.js';
+import { globalThis, FERAL_ERROR, assign } from './src/commons.js';


See note below

Suggested change

import { globalThis, FERAL_ERROR, assign } from './src/commons.js';

import { globalThis, TypeError, assign } from './src/commons.js';

erights · 2021-08-28T03:54:42Z

packages/ses/src/compartment-evaluate.js

@@ -0,0 +1,72 @@
+/// <reference types="ses">


I (MarkM) did not review this refactoring.

packages/ses/src/compartment-shim.js

erights · 2021-08-28T03:57:20Z

packages/ses/src/enable-property-overrides.js

@@ -194,7 +196,7 @@ export default function enablePropertyOverrides(
      break;
    }
    default: {
-      throw new Error(`unrecognized overrideTaming ${overrideTaming}`);
+      throw new FERAL_ERROR(`unrecognized overrideTaming ${overrideTaming}`);


erights · 2021-08-28T04:27:40Z

packages/ses/src/module-link.js

@@ -163,7 +163,7 @@ export const instantiate = (
      resolvedImports,
    );
  } else {
-    throw new Error(
+    throw new FERAL_ERROR(


erights · 2021-08-28T04:30:04Z

packages/ses/src/scope-handler.js

      console.warn(
        `getOwnPropertyDescriptor trap on scopeHandler for ${quotedProp}`,
-        new Error().stack,
+        new FERAL_ERROR().stack,


Any error would do equally well here.

Suggested change

new FERAL_ERROR().stack,

new TypeError().stack,

erights · 2021-08-28T04:31:13Z

packages/ses/src/tame-locale-methods.js

@@ -39,7 +40,7 @@ const nonLocaleCompare = tamedMethods.localeCompare;

 export default function tameLocaleMethods(intrinsics, localeTaming = 'safe') {
  if (localeTaming !== 'safe' && localeTaming !== 'unsafe') {
-    throw new Error(`unrecognized dateTaming ${localeTaming}`);
+    throw new FERAL_ERROR(`unrecognized dateTaming ${localeTaming}`);


erights · 2021-08-28T04:32:10Z

packages/ses/src/tame-regexp-constructor.js

-  Error,
-  RegExp as OriginalRegExp,
+  FERAL_ERROR,
+  FERAL_REG_EXP,


This is the one occurrence I expected

erights · 2021-08-28T04:32:42Z

packages/ses/src/whitelist-intrinsics.js

@@ -114,7 +116,9 @@ export default function whitelistIntrinsics(
    }

    // We can't clean [[prototype]], therefore abort.
-    throw new Error(`Unexpected intrinsic ${path}.__proto__ at ${protoName}`);
+    throw new FERAL_ERROR(


kriskowal · 2021-08-28T21:49:18Z

All your feedback on minimizing invocations of FERAL_* was solid and I’ve incorporated it all. Up for your review again.

erights

LGTM. Thanks!

erights · 2021-08-28T21:58:03Z

packages/ses/src/commons.js

+ * isError tests whether an object descends from the intrinsic Error
+ * constructor.


I don't know what "descends" means here.

Suggested change

* isError tests whether an object descends from the intrinsic Error

* constructor.

* isError tests whether an object inherits from the intrinsic

* `Error.prototype`.

erights · 2021-08-28T22:00:26Z

packages/ses/src/commons.js

+ * signal for reviewers that we are handling an object with excess authority
+ * that we are carefully hiding from client code, like stack trace inspection.


Suggested change

* signal for reviewers that we are handling an object with excess authority

* that we are carefully hiding from client code, like stack trace inspection.

* signal for reviewers that we are handling an object with excess authority,

* like stack trace inspection, that we are carefully hiding from client code.

avoids a parsing ambiguity.

erights · 2021-08-28T22:02:23Z

packages/ses/src/error/assert.js

@@ -214,21 +226,21 @@ const tagError = (err, optErrorName = err.name) => {
 */
 const makeError = (
  optDetails = redactedDetails`Assert failed`,
-  ErrorConstructor = Error,
+  ErrorConstructor = globalThis.Error,


erights · 2021-08-28T22:09:13Z

packages/ses/src/tame-locale-methods.js

@@ -39,7 +39,7 @@ const nonLocaleCompare = tamedMethods.localeCompare;

 export default function tameLocaleMethods(intrinsics, localeTaming = 'safe') {
  if (localeTaming !== 'safe' && localeTaming !== 'unsafe') {
-    throw new Error(`unrecognized dateTaming ${localeTaming}`);
+    throw new TypeError(`unrecognized dateTaming ${localeTaming}`);


Suggested change

throw new TypeError(`unrecognized dateTaming ${localeTaming}`);

throw new TypeError(`unrecognized localeTaming ${localeTaming}`);

I'm sure this was my mistake, but is a good time to fix it.

erights · 2021-08-28T22:10:17Z

packages/ses/src/tame-locale-methods.js

@@ -39,7 +39,7 @@ const nonLocaleCompare = tamedMethods.localeCompare;

 export default function tameLocaleMethods(intrinsics, localeTaming = 'safe') {


We still need a special case for Number.toLocaleString, to avoid the same bug that XS had. Yes?

kriskowal added the audit-sestival label Jul 10, 2021

kriskowal self-assigned this Jul 10, 2021

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from ab9f4d3 to cfc4709 Compare July 15, 2021 06:33

kriskowal requested a review from erights July 16, 2021 04:51

kriskowal changed the title ~~eslint plugin - no polymorphic call mvp~~ Lint to disallow SES polymorphic calls Jul 16, 2021

kriskowal marked this pull request as ready for review July 16, 2021 04:56

Jack-Works reviewed Jul 19, 2021

View reviewed changes

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from c58c520 to 82c38bf Compare July 21, 2021 23:36

kriskowal reviewed Jul 21, 2021

View reviewed changes

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 82c38bf to fef1e82 Compare July 21, 2021 23:47

kumavis commented Jul 22, 2021

View reviewed changes

kriskowal linked an issue Aug 13, 2021 that may be closed by this pull request

Host code in start compartment can corrupt SES between initialization and lockdown #818

Closed

erights requested changes Aug 24, 2021

View reviewed changes

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from fef1e82 to 67d0117 Compare August 28, 2021 00:50

kriskowal changed the base branch from master to kris-freeze-handlers August 28, 2021 00:50

Base automatically changed from kris-freeze-handlers to master August 28, 2021 00:51

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 67d0117 to b1123b9 Compare August 28, 2021 03:38

kriskowal requested a review from erights August 28, 2021 03:39

erights requested changes Aug 28, 2021

View reviewed changes

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 7e63539 to 52edbbc Compare August 28, 2021 21:26

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 52edbbc to 4c33308 Compare August 28, 2021 21:40

kumavis and others added 2 commits August 28, 2021 14:40

feat(eslint-plugin): Add no-polymorphic-call rule

03e8c5f

refactor(ses): Address all polymorphic call sites

21a6390

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 4c33308 to 0c72ad6 Compare August 28, 2021 21:40

kriskowal requested a review from erights August 28, 2021 21:48

erights approved these changes Aug 28, 2021

View reviewed changes

refactor(ses): Capture Error and RegExp as FERAL_*

0273165

kriskowal force-pushed the eslint-plugin/no-polymorphic-call branch from 0c72ad6 to 0273165 Compare August 28, 2021 23:11

kriskowal merged commit 3a3afdd into master Aug 28, 2021

kriskowal deleted the eslint-plugin/no-polymorphic-call branch August 28, 2021 23:14

mhofman mentioned this pull request Mar 29, 2022

Rework xsnap/src/object-inspect.js Agoric/agoric-sdk#4767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint to disallow SES polymorphic calls #827

Lint to disallow SES polymorphic calls #827

kumavis commented Jul 9, 2021 •

edited by kriskowal

Loading

kumavis commented Jul 9, 2021 •

edited

Loading

kumavis commented Jul 9, 2021

kriskowal commented Jul 15, 2021

kriskowal commented Jul 16, 2021

Jack-Works Jul 19, 2021

kriskowal Jul 21, 2021

kumavis Jul 22, 2021

kriskowal commented Jul 19, 2021 via email

kriskowal Jul 21, 2021

erights Aug 28, 2021

kriskowal Jul 21, 2021

kriskowal Jul 21, 2021

kriskowal Jul 21, 2021

erights Aug 28, 2021

kumavis left a comment

erights commented Aug 24, 2021

erights left a comment

erights commented Aug 24, 2021

erights left a comment

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

kriskowal commented Aug 28, 2021

erights left a comment

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

erights Aug 28, 2021

	import { globalThis, FERAL_ERROR, assign } from './src/commons.js';
	import { globalThis, TypeError, assign } from './src/commons.js';

		* isError tests whether an object descends from the intrinsic Error
		* constructor.

		* signal for reviewers that we are handling an object with excess authority
		* that we are carefully hiding from client code, like stack trace inspection.

	throw new TypeError(`unrecognized dateTaming ${localeTaming}`);
	throw new TypeError(`unrecognized localeTaming ${localeTaming}`);

		@@ -39,7 +39,7 @@ const nonLocaleCompare = tamedMethods.localeCompare;

		export default function tameLocaleMethods(intrinsics, localeTaming = 'safe') {

Lint to disallow SES polymorphic calls #827

Lint to disallow SES polymorphic calls #827

Conversation

kumavis commented Jul 9, 2021 • edited by kriskowal Loading

kumavis commented Jul 9, 2021 • edited Loading

kumavis commented Jul 9, 2021

kriskowal commented Jul 15, 2021

kriskowal commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Jul 19, 2021 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis left a comment

Choose a reason for hiding this comment

erights commented Aug 24, 2021

erights left a comment

Choose a reason for hiding this comment

erights commented Aug 24, 2021

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Aug 28, 2021

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis commented Jul 9, 2021 •

edited by kriskowal

Loading

kumavis commented Jul 9, 2021 •

edited

Loading