Module shims lexicals leak internal live bindings object

[mhofman]

Description of the problem

Dynamic lexicals are used to emulate live bindings of transformed modules. From a high level, a transformed module has

• its imports rewritten into a let binding with an updater registered the get notified of value changes
• exports trigger the notifier when first initialized
• live binding exports (e.g. let) rewrite the initial identifier and setup a scope lexical object property with get and set functions to return or update the current value of the export.

The get is used when a module reads the value of the exported binding, which may be any value, primitive or not, generated by the local module or not. The case of interest here for this issue is exported live bindings which values are functions generated by another module.

A consequence of #31 is that when an exported function is called, it would receive the caller module's scope object as its this, which has all kind of interesting powers. Amongst others, the called function would gain direct and non-lexical access to set and get the calling module's exported bindings.

Even with the proposed split with (#905), this issue wouldn't go away, as its an intrinsic consequence of using with blocks. It actually would become worse if we used a plain object for the dynamicGlobalLexicals as it would allow the function to list all the properties representing the exported live bindings of the caller.

The call from a with environment is actually problematic because of the WithBaseObject logic in EvaluateCall. The get property operation performed on the scope object is not itself problematic, but because of with, whatever value is returned will be called with the scope object as this. A spec fix in the form of an @@hiddenWith symbol would be great, but the problem can be solved differently in this case.

Example

// a.js
function doEvil() {
  // Do something terrible
}

export function foo() {
  if (this) {
    this.foo = doEvil;
  }
}

// b.js
import {foo as aFoo} from './a.js';

export let foo;

export function setup(...args) {
  foo = aFoo;
  foo(...args); // leaks our scope object to `a.js#foo`
}

Impact

The danger is pretty esoteric. Only code following a fairly non common pattern would be impacted:

• Assign an untrusted function declared in another module to an exported live binding
• Call that function using the exported lexical binding

In another scenario, a module could use this mechanism to escape any syntactic audit that its exports are only modified lexically (aka following specified means).

Mitigations

Linting

Since the cause of the leak can be identified syntactically (a call to an exported lexical live binding), we could add a lint rule to warn that the this behavior leads to a leak of the internal module's scope, and that the function should be trusted.

Invalid non-identity preserving workaround

A crude and non compliant way to solve this would be to modify the get operation, and wrap any function returned from a live binding to its own module to a function which only forwards non scope objects as this context. However that would break identity checks within a module:

const foo = () => {};
export let bar = foo;
assert(bar === foo);

Complete fix

Since there is no way to trap an assignment to a lexical identifier without a with scope, and calling a function retrieved from a with scope is the cause of the leak, the only solution is to dissociate assignment and retrieval:

• Declaration of an exported live binding would be kept to the original declaration, preserving the original identifier, instead of rewriting as it happens right now. The live binding notifier would still be initialized at this point (taking it out of TDZ).
• Retrieval would keep using the original identifier, and it would no longer ever hit a scope object.
• Assignment to an exported live binding would keep triggering a set on a scope object, but with a rewritten identifier. The value assigned would be propagated to the "read" identifier using the existing notifier mechanism.

Rewriting the lexical identifier used for the assignments does move away from only rewriting import and export lines, however the impact could be mitigated somewhat by replacing the identifier with their equivalent "mathematical sans-serif normal" version. Since the code editor is likely using a sans-serif font anyway, it wouldn't be visible, and it would still use the same number of "characters" (code points). In the case no base latin characters are found in the identifier (people do use emojis as exports!), we could fall back to adding zero-width joiners. This would change the number of codepoints used by the identifier but not the number of glyphs, so it's unclear what impact this would have on debuggers and source code position mapping.

Similarly to the linting approach, only the lexical called locally could be re-written this way. However that requires the scope object and transforms to support 2 rewrite behaviors, and prevents us to consider the module lexical scope object as write only.

[mhofman]

After a quick chat with @michaelfig, I now understand that one concern is in case the babel transform misses an identifier usage rewrite (e.g. bug in babel), the failure should not be silent, or at least not get into an unexpected state.

The proposed solution does have this drawback. If an assignment doesn't get rewritten, it would end up being done on the originally defined identifier, which is obviously mutable, but without hitting the notification logic, so the state of the local module would be fine, but modules importing the exported binding would not receive the updated value.

Missing assignments in the syntax tree would seem like a major babel bug, but if we want to be resilient, the alternative is to rewrite all read usages of the exported live binding, and keep the write usage intact. A missed read usage for the original identifier would trigger a [[Get]] on the scope object, which should be setup to throw for live bindings (as that is the source of the leak).

[erights]

Please let's not rewrite either reads or writes, as was the original plan. The only things the module transform should rewrite are

• import declarations
• export declarations
• dynamic import(...) expressions
• import.meta
• The declarations of exported variables

Do we need to rewrite anything else?

[mhofman]

Do you have a suggestion to avoid the hazard introduced by the leak of the module scope object?

[erights]

Do you have a suggestion to avoid the hazard introduced by the leak of the module scope object?

Not yet. Reading the thread now. But if this leak does not pose a real danger, I'd rather rewrite less and live with the danger.

[erights]

I tried a few experiments and, AFAICT, our lint rules already prevent live bindings. If the module rewrite also rejects live bindings as a shim limitation, would anything we care about break?

If we stop supporting live bindings, as a shim limitation, does the above problem disappear?

[mhofman]

Interesting. I believe that without live bindings, there is indeed no issue. I am however surprised we disallow live bindings. Also not all users of live binding can get away with disabling them (e.g. lavamoat), and I believe we'll need a live binding like solution for support of commonJS.

I am wondering if we could make all this configurable somehow. At the very least the compartment shim with multiple with can skip one of them if there are no module lexicals.

[erights]

Does lavamoat expect to use the ses-shim's module rewriter, or only the cjs rewriter?

[mhofman]

As mentioned in #1293, to mitigate impact of this leak, we could split the lexicals used by the module shims and the lexicals provided by the compartment creator.

[kriskowal]

We have since this issue was raised removed the global lexicals feature from Compartment and greatly limited the module lexicals object. I think the issue remains in some cases (@mhofman please confirm) and that we’re willing to live with the problem (please confirm @erights) and I move that we reframe this issue as one of documenting a caveat.

[erights]

we’re willing to live with the problem (please confirm @erights)

Yes.

I move that we reframe this issue as one of documenting a caveat.

Yes.