removeProperties doesn't clean props of anonIntrinsics?

[warner]

[edit 2020-09-15 @kriskowal]

@erights has volunteered to verify that the below issue is no longer a problem.

---

@dtribble and I were looking at removeProperties.js, and noticed that we build and add the anonIntrinsics to the whitetable, but then clean() is called on the global object. Since these intrinsics are (by definition) not reachable by recursive property lookup on the global, clean() will never visit them, so any whitelist settings that might delete their properties won't ever get applied. This could lead to a surprise if e.g. TC39 adds a new unsafe property to AsyncFunction.

(this is unrelated to freezing the primordials, because in hardenPrimordials.js we re-generate the anonIntrinsics and attach them to a root object, which we then deep-freeze along with everything reachable from the global)

One fix might be to change the end of removeProperties to also clean everything on the anonIntrinsics list:

clean(global);
Object.getOwnPropertyNames(intr).forEach(name => clean(intr[name]));

Another might be to rearrange the whole clean/freeze pathway to create a single object that makes all intrinsics (named and anonymous) available in a hierarchy that mirrors the whitelist table, and use it both for cleaning and freezing. This table might then be useful to construct the "fringe" set that a standalone harden() might need.

[erights]

I thought that we created a new root object containing both global and anonIntrinsics, so that all primordials would be reached by simple navigation starting there.

[warner]

hardenPrimordials is where we create that root object (https://github.com/Agoric/SES/blob/a33cb8952b243dfd863c1df9c32f20dfadf4715c/src/bundle/hardenPrimordials.js#L6), so we're freezing everything. But removeProperties (https://github.com/Agoric/SES/blob/a33cb8952b243dfd863c1df9c32f20dfadf4715c/src/bundle/removeProperties.js#L164) walks the global object, which doesn't include the anonIntrinsics.

[Tartuffo]

@erights Is this still an issue? If not, please close.

[erights]

I need to take a look. I can imagine making this mistake, and it could be a severe problem. Leave open and with the "security blocker" label until then.

[Tartuffo]

@erights This does not have an area label that is covered by our weekly tech / planning meetings. Can you assign the proper label? We cover: agd, agoric-cosmos, amm, core economy, cosmic-swingset, endo, getrun, governance, installation-bundling, metering, run-protocol, staking, swingset, swingset-runner, token economy, ui, wallet, zoe, zoe contract

[kriskowal]

I believe this program confirms the appropriate behavior:

import 'ses';

// getAnonymousIntrinsics uses a utility function to construct an arguments
// object, since it cannot have one of its own and also be a const export.
function makeArguments() {
  // eslint-disable-next-line prefer-rest-params
  return arguments;
}

const calleeDesc = Object.getOwnPropertyDescriptor(makeArguments(), 'callee');

lockdown();

console.log(Object.isFrozen(calleeDesc.get)); // true

Adding a test to close this out.