fix(ses): Patch leak of globalLexicals thru moduleLexicals #1341
Conversation
kriskowal
force-pushed
the
912-global-lexicals-leak
branch
from
7bc0846
to
80f4772
Compare
kriskowal
force-pushed
the
912-global-lexicals-leak
branch
from
80f4772
to
a952fa0
Compare
There was a problem hiding this comment.
Preliminary observations, not a full review yet, and nothing actionable, just stream of thoughts.
So I'm wondering, should moduleLexicals be allowed to shadow globalLexicals, I suppose a global lexical should come with a transform denying normal lexical access to these bindings, but I don't quite remember if that global transform would apply before or after the module transforms which introduce the corresponding module lexical usages.
Then, if no shadowing is allowed between global and module lexical, and given the use case of moduleLexicals which do not get optimized (not immutable since they're getter / setters), I'm wondering if the removal of the evaluate factory wasn't premature. If we had kept the evaluate factory, we technically could have cached it on the compartment, and re-used it between evaluate calls with different __moduleShimLexicals__, but that kinda feels like we'd want another kind of 2 stage process where only the moduleLexicals change instead of the whole context object.
Finally, I really hope we get to a point where we can build the evaluate factory more dynamically if globalLexicals or moduleLexicals are missing / empty, removing layers of unnecessary with blocks siting between the user code and lookups on the globalObject.
| __moduleShimLexicals__ !== undefined && | ||
| __moduleShimLexicals__ !== null |
There was a problem hiding this comment.
This is really one of those cases where I prefer to write __moduleShimLexicals__ != null
| * @param {object} context.globalLexicals | ||
| * @param {object} context.globalObject | ||
| * @param {object} context.globalLexicals | ||
| * @param {object} context.moduleLexicals |
There was a problem hiding this comment.
wondering if we should keep the order the same as in the nested scopes
| const moduleLexicalOptimizer = buildOptimizer( | ||
| moduleLexicalConstants, | ||
| 'moduleLexicals', | ||
| ); |
There was a problem hiding this comment.
Can this ever actually happen? I mean this is probably the safest way, and doesn't cost much.
There was a problem hiding this comment.
Not in any correct arrangement. Any potential overlap would need to be censored by a transform.
|
In out-of-band group review, we elected to instead remove support for |
Fixes #912
The global lexicals are intended to provide a namespace that guest code cannot enumerate for arbitrary properties. By employing a transform for all evaluation in a compartment and also transforming all module code provided to a compartment's import hooks, a host program can introduce a name like
meterinto the global lexical scope of a program and also deny that program access to the lexical name through censorship of the original source. Previously, a crafted module could use a function in module lexical scope to capture the global lexicals object or the scope proxy. This change creates another intermediate layer in the safe evaluator that separates the global lexicals from the module lexicals, plugging this leak.Individually reviewable commits.