1982263 - Assertion failure: outermostScope().hasNonSyntacticScopeOnChain() == sc->hasNonSyntacticScope(), at /home/r00t/firefox/js/src/frontend/BytecodeEmitter.cpp:13200

Status: ASSIGNED

(Core :: JavaScript Engine, defect, P2)

Description

[neseesun]

Attachment: 5.js

component is spidermonkey=> interpreter
spidermonkey version: lattest

run with: ./js poc.js

Comment 1

[neseesun]

Attachment: stacktrace.txt

Comment 2

[Tooru Fujisawa [:arai]]

Thank you for reporting.

This is caused by the following logic not reflecting the enclosing scope of delazified function, because the delazified function's scope's doesn't have the information about the enclosing scope.

https://searchfox.org/mozilla-central/rev/ca035d76ad92757859375c2dc807c7a61055823b/js/src/frontend/CompilationStencil.h#191-195,204-206

bool hasOnChain(ScopeKind kind) const {
  return scope_.match([=](const Scope* ptr) { return ptr->hasOnChain(kind); },
                      [=](const ScopeStencilRef& ref) {
                        ScopeStencilRef it = ref;
                        while (true) {
...
                          if (!scope.hasEnclosing()) {
                            break;
                          }

This case should walk the scope by looking up the lazy function's enclosing stencil's script stencil, and then its enclosing scope.

Then, given that the concurrent delazification is not enabled by default, and also the web content script doesn't have direct access to APIs that allows compiling/evaluating scripts with random object as environment chain, I suppose the severity isn't high.

Comment 3

[Daniel Veditz [:dveditz]]

The code in comment 2 appears to have come from bug 1730881 in 2021, so everything we still support should be affected

We intend to enable delazification by default when it's ready so that doesn't factor into the security severity. The fact that it's controlled by us and not web content is a much stronger case for minimizing the severity. Does a web page have any control at all over that? I agree it does not look like it's high severity at all, but trying to figure out if this is potentially worse than "low".

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Veditz [:dveditz] from comment #3)

The code in comment 2 appears to have come from bug 1730881 in 2021, so everything we still support should be affected

This is partially true, this code has been here for a moment but the logic surrounding it changed to match a new data representation (Bug 1933685). Most of the scope chain logic and iteration had to be refactored a few months ago.

The previous code only went on by default as part of a failed experiment on nightly, which is no longer running.

We intend to enable delazification by default when it's ready so that doesn't factor into the security severity. The fact that it's controlled by us and not web content is a much stronger case for minimizing the severity. Does a web page have any control at all over that? I agree it does not look like it's high severity at all, but trying to figure out if this is potentially worse than "low".

So far it remains disabled by default, and this is tracked by Bug 1779292.

The web pages should have no control over the usage of concurrent delazification. Only the user via dom.script_loader.delazification.strategy preference or the testing code can turn this on.

The script, makes use of the options eagerDelazificationStrategy given to the evaluate JS Shell function, which is part of out testing infrastructure. Which explain why the reporter was able to find this issue despite not being enabled by default.

Comment 5

[Daniel Veditz [:dveditz]]

The web pages should have no control over the usage of concurrent delazification. Only the user via dom.script_loader.delazification.strategy preference or the testing code can turn this on.

Currently yes. At some point we will turn that one, and should this bug remain unfixed could a web page at that time reliably trigger the conditions? A web page couldn't use eagerDelazificationStrategy but can they do things to influence when our JS engine decides to use it? In practice it does not matter to our users since this is disabled, but it does matter to the reporter because it effects how we evaluate their bug bounty request.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Veditz [:dveditz] from comment #5)

Currently yes. At some point we will turn that one, and should this bug remain unfixed could a web page at that time reliably trigger the conditions? A web page couldn't use eagerDelazificationStrategy but can they do things to influence when our JS engine decides to use it? In practice it does not matter to our users since this is disabled, but it does matter to the reporter because it effects how we evaluate their bug bounty request.

If this code remains disabled, and this bug remains unfixed, the only way to jump into this code would be to corrupt the JS::TransitiveCompileOptions::eagerDelazificationStrategy_ flag before another script gets parsed in order to spawn a DelazifyTask after parsing the top-level.

There is no way, under normal condition, that this code can be entered at the moment. Only ParseEverythingEagerly and OnDemand are baked into the code as assignment, and the only path which can assign a different value is through the preference which defaults to parsing everything eagerly.

Comment 7

[Tooru Fujisawa [:arai]]

The inconsistency triggered by this bug affects how the global/dynamic name operations inside delazified functions works.
(I haven't yet checked all details, see the last paragraph)
This bug makes the delazification not aware of the enclosing non-syntactic scope, which means the global/dynamic name operations are emitted with the global name operations, which ignores bindings inside the environment chains.

The non-syntactic scope is used by the event handlers, and some other privileged scripts.
In terms of exploitability, only the event handlers are affected, and the env chain used there contains the HTML elements around the target element.
So, if this bug happens, the name operations there will result in using wrong bindings in the global environment, instead of using the elements properties,
but "using global variable" is already available to web content by writing globalThis.foo = ... etc.

Thus, this bug will result in wrong executions, which will make website not working, such as clicking a button does nothing,
but so far this characteristics is not exploitable by attackers.

Then, I'll see if there's any other code inside the compiler that relies on this flag (something that relies on the flag being consistent across compilation and does some optimization etc), to see if there's other way to exploit this.
I'll post the update by the end of next week.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: (secure)

hasOnChain implementation was not consistent with the way the
InputScope::enclosing was implemented and was walked over. This change isolate
the implementation of InputScope::enclosing and host it on the
ScopeStencilRef::enclosing function, which returns a Result function which
carries the evaluation of hasEnclosing by returning a special typed error for
cases where the returned value is not known at compile time.

Comment 9

[Daniel Veditz [:dveditz]]

Thus, this bug will result in wrong executions, which will make website not working, such as clicking a button does nothing,
but so far this characteristics is not exploitable by attackers.

Thank you! I'll wait for your update before un-hiding the bug just in case you find something

Comment 10

[Tooru Fujisawa [:arai]]

I don't find anything that can cause critical things, beyond the wrong JS execution.

Comment 11

[Daniel Veditz [:dveditz]]

This is not a bug that affects security so it is ineligible for the bug bounty