fix(ses): Limit scope proxy exposure to discernably owned properties … (#2743)

…of host globalThis

Closes #1305

## Description

This change circles back to an old observation about the scope proxy,
where we deliberately allow guest code to sense which properties of the
host globalThis exist in order to provide a higher fidelity behavior for
`typeof` style feature detection. This change reflects a shift in our
preference to avoid this information leak at the expense of ecosystem
compatibility. We have come to believe closing the leak is worth the
expense, especially given that the work-around of checking `globalThis`
properties explicitly has become a norm.

### Security Considerations

This change increases confinement in exchange for a minor loss of
ecosystem compatibility.

### Scaling Considerations

None.

### Documentation Considerations

Forthcoming.

### Testing Considerations

This change will be validated with an integration PR with Agoric SDK to
expose any extant compatibility risks.

### Compatibility Considerations

This change may uncover existing code that relied on `typeof` for
feature detection, which is not strictly compatible. We are treating
these considerations as bug fixes.

### Upgrade Considerations

These changes will become observable to vats upon restart after this
software arrives in an Agoric chain software update.

Author: kriskowal

packages/ses/src/strict-scope-terminator.js

@@ -6,7 +6,6 @@ import {
   create,
   freeze,
   getOwnPropertyDescriptors,
-  globalThis,
 } from './commons.js';
 import { assert } from './error/assert.js';
 
@@ -55,7 +54,7 @@ const scopeProxyHandlerProperties = {
 
   has(_shadow, prop) {
     // we must at least return true for all properties on the realm globalThis
-    return prop in globalThis;
+    return true;
   },
 
   // note: this is likely a bug of safari

packages/ses/test/compartment.test.js

@@ -13,7 +13,7 @@ test('SES compartment does not see primal realm names', t => {
   // eslint-disable-next-line no-unused-vars
   const hidden = 1;
   const c = new Compartment();
-  t.throws(() => c.evaluate('hidden+1'), { instanceOf: ReferenceError });
+  t.is(c.evaluate('hidden'), undefined);
 });
 
 test('SES compartment also has compartments', t => {

packages/ses/test/evalTaming-default.test.js

@@ -7,7 +7,7 @@ test('safe eval when evalTaming is undefined.', t => {
   // eslint-disable-next-line no-unused-vars
   const a = 0;
   // eslint-disable-next-line no-eval
-  t.throws(() => eval('a'));
+  t.is(eval('a'), undefined);
   // eslint-disable-next-line no-eval
   t.is(eval('1 + 1'), 2);
 

packages/ses/test/evalTaming-safe-eval.test.js

@@ -7,7 +7,7 @@ test('safe eval when evalTaming is safe-eval.', t => {
   // eslint-disable-next-line no-unused-vars
   const a = 0;
   // eslint-disable-next-line no-eval
-  t.throws(() => eval('a'));
+  t.is(eval('a'), undefined);
   // eslint-disable-next-line no-eval
   t.is(eval('1 + 1'), 2);
   // eslint-disable-next-line no-eval

packages/ses/test/evalTaming-safeEval.test.js

@@ -8,7 +8,7 @@ test('safe eval when evalTaming is safeEval.', t => {
   // eslint-disable-next-line no-unused-vars
   const a = 0;
   // eslint-disable-next-line no-eval
-  t.throws(() => eval('a'));
+  t.is(eval('a'), undefined);
   // eslint-disable-next-line no-eval
   t.is(eval('1 + 1'), 2);
   // eslint-disable-next-line no-eval

packages/ses/test/global-lexicals-evaluate.test.js

@@ -36,9 +36,7 @@ test('endowments prototypically inherited properties are not mentionable', t =>
     __options__: true,
   });
 
-  t.throws(() => compartment.evaluate('hello'), {
-    message: /hello is not defined/,
-  });
+  t.is(compartment.evaluate('hello'), undefined);
 });
 
 test('endowments prototypically inherited properties are not enumerable', t => {

packages/ses/test/make-eval-function.test.js

@@ -6,14 +6,14 @@ import test from 'ava';
 import { makeEvalFunction } from '../src/make-eval-function.js';
 import { makeSafeEvaluator } from '../src/make-safe-evaluator.js';
 
-test('makeEvalFunction - leak', t => {
+test('makeEvalFunction - no leak', t => {
   t.plan(8);
 
   const globalObject = {};
   const { safeEvaluate } = makeSafeEvaluator({ globalObject });
   const safeEval = makeEvalFunction(safeEvaluate);
 
-  t.throws(() => safeEval('none'), { instanceOf: ReferenceError });
+  t.is(safeEval('none'), undefined);
   t.is(safeEval('this.none'), undefined);
 
   globalThis.none = 5;

packages/ses/test/make-safe-evaluator.test.js

@@ -4,17 +4,29 @@ import test from 'ava';
 import { makeSafeEvaluator } from '../src/make-safe-evaluator.js';
 
 test('safeEvaluate - default (non-sloppy, no moduleLexicals)', t => {
-  t.plan(6);
+  t.plan(7);
 
   const globalObject = { abc: 123 };
   const { safeEvaluate: evaluate } = makeSafeEvaluator({ globalObject });
 
   t.is(evaluate('typeof def'), 'undefined', 'typeof non declared global');
 
-  t.throws(
-    () => evaluate('def'),
-    { instanceOf: ReferenceError },
-    'non declared global cause a reference error',
+  // Expected to be undefined in sloppy mode.
+  t.is(
+    evaluate(
+      'def',
+      undefined,
+      'non-declared global is undefined in sloppy mode',
+    ),
+  );
+  // Known deviation from fidelity to Hardened JavaScript:
+  // In strict mode, an undeclared lexical name should produce a ReferenceError.
+  t.is(
+    evaluate(
+      '(() => { "use strict"; return def; })()',
+      undefined,
+      'non-declared global is undefined in strict mode',
+    ),
   );
 
   t.is(evaluate('abc'), 123, 'globals can be referenced');
@@ -61,9 +73,9 @@ test('safeEvaluate - module lexicals', t => {
 
   t.is(endowedEvaluate('abc'), 123, 'module lexicals can be referenced');
   t.is(endowedEvaluate('abc += 333'), 456, 'module lexicals can be mutated');
-  t.throws(
-    () => evaluate('abc'),
-    { instanceOf: ReferenceError },
+  t.is(
+    evaluate('abc'),
+    undefined,
     'module lexicals do not affect other evaluate scopes with same globalObject (do not persist)',
   );
 });

packages/ses/test/permits.test.js

@@ -10,18 +10,8 @@ test('indirect eval is possible', t => {
 
 test('SharedArrayBuffer should be removed because it is not permitted', t => {
   const c = new Compartment();
-  // we seem to manage both of these for properties that never existed
-  // in the first place
-  t.throws(() => c.evaluate('XYZ'), { instanceOf: ReferenceError });
-  t.is(c.evaluate('typeof XYZ'), 'undefined');
-  const have = typeof SharedArrayBuffer !== 'undefined';
-  if (have) {
-    // we ideally want both of these, but the realms magic can only
-    // manage one at a time (for properties that previously existed but
-    // which were removed by the permits check)
-    // t.throws(() => c.evaluate('SharedArrayBuffer'), ReferenceError);
-    t.is(c.evaluate('typeof SharedArrayBuffer'), 'undefined');
-  }
+  t.is(c.evaluate('typeof SharedArrayBuffer'), 'undefined');
+  t.is(c.evaluate('SharedArrayBuffer'), undefined);
 });
 
 test('remove RegExp.prototype.compile', t => {

packages/ses/test/scope.test.js

@@ -22,7 +22,7 @@ test('scope behavior - lookup behavior', t => {
   t.is(evaluate('globalThis'), undefined);
   t.is(evaluate('eval'), undefined);
   t.is(evaluate('realmGlobalProp'), undefined);
-  t.throws(() => evaluate('missingProp'), { instanceOf: ReferenceError });
+  t.is(evaluate('missingProp'), undefined);
 
   t.is(evaluate('globalProp'), globalObject.globalProp);
   t.is(evaluate('lexicalProp'), moduleLexicals.lexicalProp);
@@ -279,39 +279,35 @@ test('scope behavior - strict vs sloppy locally non-existing global set', t => {
   t.notThrows(() => evaluateSloppy('missingRealmGlobalProp = 456'));
 });
 
-test('scope behavior - realm globalThis property info leak', t => {
+test('scope behavior - no realm globalThis property info leak', t => {
   t.plan(8);
 
   const globalObject = {};
   const { safeEvaluate: evaluate } = makeSafeEvaluator({
     globalObject,
   });
 
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('eventuallyAssignedRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
+  const unvaryingAssertions = () => {
+    t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
+    t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
+    // Known loss of fidelity to native Hardened JavaScript: we expect a
+    // ReferenceError for accessing a missing lexical name.
+    t.is(evaluate('missingRealmGlobalProp'), undefined);
+    t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+  };
+
+  unvaryingAssertions();
 
   globalThis.eventuallyAssignedRealmGlobalProp = {};
   t.teardown(() => {
     delete globalThis.eventuallyAssignedRealmGlobalProp;
   });
 
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+  unvaryingAssertions();
 });
 
 test('scope behavior - Symbol.unscopables fidelity test', t => {
-  t.plan(33);
+  t.plan(41);
 
   const globalObject = {
     Symbol,
@@ -325,81 +321,49 @@ test('scope behavior - Symbol.unscopables fidelity test', t => {
     globalObject,
   });
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof localProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined');
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
+  const unvaryingAssertions = () => {
+    t.is(evaluate('typeof localProp'), 'undefined');
+    t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined');
+    t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
+    t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('localProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('eventuallyAssignedLocalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('eventuallyAssignedRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
+    // Known compromise in fidelity of the emulated script environment:
+    // In a native Hardened JavaScript, we would expect these to throw
+    // ReferenceError.
+    t.is(evaluate('localProp'), undefined);
+    t.is(evaluate('eventuallyAssignedLocalProp'), undefined);
+    t.is(evaluate('missingRealmGlobalProp'), undefined);
+    t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+  };
 
+  unvaryingAssertions();
+
+  // Compartment should not be sensitive to existence of a host globalThis
+  // property.
   globalThis.eventuallyAssignedRealmGlobalProp = {};
   t.teardown(() => {
     delete globalThis.eventuallyAssignedRealmGlobalProp;
   });
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof localProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined');
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
-
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('localProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('eventuallyAssignedLocalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+  unvaryingAssertions();
 
   evaluate(
     'this[Symbol.unscopables] = { eventuallyAssignedRealmGlobalProp: true, localProp: true, eventuallyAssignedLocalProp: true }',
   );
   // after property is created on globalObject, assignment is evaluated to
   // test if it is affected by the Symbol.unscopables configuration
   globalObject.eventuallyAssignedLocalProp = null;
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('eventuallyAssignedLocalProp = {}'), {
-    instanceOf: ReferenceError,
-  });
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof localProp'), 'undefined');
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined');
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
+  unvaryingAssertions();
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('localProp'), {
-    instanceOf: ReferenceError,
-  });
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('eventuallyAssignedLocalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
+  // Known compromise in fidelity to native Hardened JavaScript:
+  // We expect implicit assignment on globalThis to fail in strict mode but not
+  // in sloppy mode.
+  t.throws(() => evaluate('eventuallyAssignedLocalProp = {}'), {
     instanceOf: ReferenceError,
   });
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+
+  unvaryingAssertions();
 
   // move "Symbol.unscopables" to prototype
   delete globalObject[Symbol.unscopables];
@@ -410,24 +374,5 @@ test('scope behavior - Symbol.unscopables fidelity test', t => {
     eventuallyAssignedLocalProp: true,
   };
 
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof localProp'), 'undefined');
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('typeof eventuallyAssignedLocalProp'), 'undefined');
-  t.is(evaluate('typeof missingRealmGlobalProp'), 'undefined');
-  t.is(evaluate('typeof eventuallyAssignedRealmGlobalProp'), 'undefined');
-
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('localProp'), {
-    instanceOf: ReferenceError,
-  });
-  // Known compromise in fidelity of the emulated script environment:
-  t.throws(() => evaluate('eventuallyAssignedLocalProp'), {
-    instanceOf: ReferenceError,
-  });
-  t.throws(() => evaluate('missingRealmGlobalProp'), {
-    instanceOf: ReferenceError,
-  });
-  // Known compromise in fidelity of the emulated script environment:
-  t.is(evaluate('eventuallyAssignedRealmGlobalProp'), undefined);
+  unvaryingAssertions();
 });

packages/ses/test/ses.test.js

@@ -71,7 +71,7 @@ test('create', t => {
 test('SES compartment does not see primal realm names', t => {
   const hidden = 1; // eslint-disable-line no-unused-vars
   const c = new Compartment();
-  t.throws(() => c.evaluate('hidden+1'), { instanceOf: ReferenceError });
+  t.is(c.evaluate('hidden'), undefined);
 });
 
 test('SES compartment also has compartments', t => {

packages/ses/test/strict-scope-terminator.test.js

@@ -1,13 +1,13 @@
 import test from 'ava';
 import { strictScopeTerminator } from '../src/strict-scope-terminator.js';
 
-test('strictScopeTerminator/get - always has start compartment properties but they are undefined', t => {
+test('strictScopeTerminator/get - has all properties and they are undefined', t => {
   t.plan(4);
 
   t.is(Reflect.has(strictScopeTerminator, 'eval'), true);
   t.is(Reflect.get(strictScopeTerminator, 'eval'), undefined);
 
-  t.is(Reflect.has(strictScopeTerminator, 'xyz'), false);
+  t.is(Reflect.has(strictScopeTerminator, 'xyz'), true);
   t.is(Reflect.get(strictScopeTerminator, 'xyz'), undefined);
 });
 
@@ -30,7 +30,7 @@ test('strictScopeTerminator/getPrototypeOf - has null prototype', t => {
   t.is(Reflect.getPrototypeOf(strictScopeTerminator), null);
 });
 
-test('strictScopeTerminator/getOwnPropertyDescriptor - always has start compartment properties but provides no prop desc', t => {
+test('strictScopeTerminator/getOwnPropertyDescriptor - traps all properties and provides no prop desc', t => {
   t.plan(9);
 
   const originalWarn = console.warn;
@@ -46,7 +46,7 @@ test('strictScopeTerminator/getOwnPropertyDescriptor - always has start compartm
     undefined,
   );
   t.is(didWarn, 1);
-  t.is(Reflect.has(strictScopeTerminator, 'xyz'), false);
+  t.is(Reflect.has(strictScopeTerminator, 'xyz'), true);
   t.is(didWarn, 1);
   t.is(
     Reflect.getOwnPropertyDescriptor(strictScopeTerminator, 'xyz'),

packages/ses/test/typeof.test.js

@@ -7,9 +7,7 @@ test('typeof', t => {
 
   const c = new Compartment();
 
-  t.throws(() => c.evaluate('DEFINITELY_NOT_DEFINED'), {
-    instanceOf: ReferenceError,
-  });
+  t.is(c.evaluate('DEFINITELY_NOT_DEFINED'), undefined);
   t.is(c.evaluate('typeof DEFINITELY_NOT_DEFINED'), 'undefined');
 
   t.is(c.evaluate('typeof 4'), 'number');