1771084 - Try to automatically seal Object/Array/Function constructors/prototypes in the shared system global

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jan de Mooij [:jandem]]

Let's land something simple that covers Object/Array/Function and then we can extend that later to other prototypes.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1771084 part 1 - Fix pre-existing clang-format issues. r?tcampbell!

This simplifies the next patch.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1771084 part 2 - Add Realm option to freeze builtins. r?tcampbell!,nika!

Depends on D147280

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1771084 part 3 - Freeze builtins for the shared system global. r?mccr8!,peterv!

Comment 4

[Ryan VanderMeulen [:RyanVM]]

We may still try to ship these changes in a dot release during the upcoming cycle.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to normal. However, the bug is flagged with the sec-high keyword.
:jandem, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to normal. However, the bug is marked as tracked for firefox102 (nightly) and tracked for firefox101 (beta).
:jandem, could you consider increasing the severity of this tracked bug?

For more information, please visit auto_nag documentation.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9278154 [details]
Bug 1771084 part 3 - Freeze builtins for the shared system global. r?mccr8!,peterv!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard, we landed a spot fix for the original issue and this is a structural fix.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?:
• Is Android affected?: Yes

Comment 8

[Peter Van der Beken [:peterv]]

This might cause an issue with the json5 library, which is used in https://searchfox.org/mozilla-central/source/devtools/shared/storage/utils.js. For example, it tries to redefine Function.prototype.toString in https://searchfox.org/mozilla-central/rev/f818cf7d7a46728a66b4dcfece73dc017da43f98/devtools/shared/storage/vendor/json5.js#146-165, which fails (for some reason I'm not seeing an exception). That's causing a failure in https://searchfox.org/mozilla-central/source/devtools/server/tests/xpcshell/test_extension_storage_actor.js for me.

Comment 9

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9278121 [details]
Bug 1771084 part 1 - Fix pre-existing clang-format issues. r?tcampbell!

We can land these patches when ready.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Peter Van der Beken [:peterv] from comment #8)

This might cause an issue with the json5 library, which is used in https://searchfox.org/mozilla-central/source/devtools/shared/storage/utils.js. For example, it tries to redefine Function.prototype.toString in https://searchfox.org/mozilla-central/rev/f818cf7d7a46728a66b4dcfece73dc017da43f98/devtools/shared/storage/vendor/json5.js#146-165, which fails (for some reason I'm not seeing an exception). That's causing a failure in https://searchfox.org/mozilla-central/source/devtools/server/tests/xpcshell/test_extension_storage_actor.js for me.

Hmm, this is quite unfortunate. IIRC devtools loads modules like this one within a system principal sandbox or a simple global when running the debugger attached to a worker (not sure if this module is imported there). We can probably dodge that issue by removing the special logic to enable this requirement by default on system principal sandboxes and worker globals for now, which would make this safer to uplift, but this is making it somewhat clear that we are going to have a lot of issues with vendored JS code which we use in system contexts when trying to expand this restriction.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #10)

but this is making it somewhat clear that we are going to have a lot of issues with vendored JS code which we use in system contexts when trying to expand this restriction.

I'm not sure we will. Or, if we do, I think it is probably a feature rather than a bug, at least for the shared JSM global. We definitely don't want to load libraries that are going to modify shared prototypes there. That's generally been considered a bad practice in most of the web JS community for a while now too. If there are places where we do want to allow it, I think I'd rather those globals explicitly opt into it. And we should probably require some kind of review process for cases where they do.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Per Slack discussion, these patches are riskier than we originally anticipated and it would be better to let them follow normal landing & uplift processes rather than targeting a dot release.

Comment 13

[Jan de Mooij [:jandem]]

I landed parts 1 and 2, but autoland is closed atm.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I think we should be relatively safe with the shared system global, but I know we have issues with devtools window globals due to redux (which mutates the Symbol constructor object), and the code overwriting Promise with a type from a shared global to prevent the promises from dying when the window is unloaded.

In the more-distant future it may be nice to make the shared system global completely immutable but that's tricky to do right now (we definitely add some properties lazily right now, and eagerly resolving all properties appears to cause other issues due to things like L10N becoming initialized too early).

(In reply to Jan de Mooij [:jandem] from comment #13)

I landed parts 1 and 2, but autoland is closed atm.

Landing a stripped down part 3 on top of that, which should only freeze the shared system global and system principal windows.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/b0f53121ccf21bc41fba4ea0b2e1d025edce3a44

Backed out for causing mochitest-remote failures:
https://hg.mozilla.org/integration/autoland/rev/b0f53121ccf21bc41fba4ea0b2e1d025edce3a44

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=b0f53121ccf21bc41fba4ea0b2e1d025edce3a44&selectedTaskRun=FziJy725SXmvFV82LXKBCQ.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=380494241&repo=autoland

[task 2022-06-07T17:26:44.422Z] 17:26:44     INFO - Entering test bound objectIdInvalidTypes
[task 2022-06-07T17:26:44.424Z] 17:26:44     INFO - Buffered messages finished
[task 2022-06-07T17:26:44.428Z] 17:26:44     INFO - TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/dom/browser_describeNode.js | Uncaught exception in test - at chrome://mochitests/content/browser/remote/cdp/test/browser/chrome-remote-interface.js:1 - TypeError: can't redefine non-configurable property "toString"

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Taking the NI, as the buggy patch was my part.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

My patch got backed out because of chrome-remote-interface.js attempting to redefine the "toString" property. Unfortunately the code is very difficult to read thanks to it being webpacked minimized JS which we probably imported from https://github.com/cyrus-and/chrome-remote-interface.

After formatting the minimized code a bit to make sense of it, it appears we're somewhere in this code:

  function(e, t, n) {
    var r = n(2),
      i = n(12),
      o = n(15),
      a = n(36)("src"),
      s = n(156),
      c = ("" + s).split("toString");
    (n(19).inspectSource = function(e) {
      return s.call(e);
    }),
      (e.exports = function(e, t, n, s) {
        var p = "function" == typeof n;
        p && (o(n, "name") || i(n, "name", t)),
          e[t] !== n &&
            (p && (o(n, a) || i(n, a, e[t] ? "" + e[t] : c.join(String(t)))),
            e === r
              ? (e[t] = n)
              : s
              ? e[t]
                ? (e[t] = n)
                : i(e, t, n)
              : (delete e[t], i(e, t, n)));
      })(Function.prototype, "toString", function() {
        return ("function" == typeof this && this[a]) || s.call(this);
      });
  },

which appears to be doing something to re-define the toString property on object.prototype for some reason? I'm really not sure why.

This brings up the fun problem of "oh right people can load random chrome JS into firefox through the marionette-like APIs", and it appears that some code (probably a dependency of chrome-remote-interface?) actually wants to mutate the function prototype. I'm not sure what our best option here is unfortunately.

There are 2 approaches I see going forward to get this landed right away:

1. We could restrict the globals even more to only impact the shared system global, meaning that prototypes are mutable on browser windows. This would have prevented the existing issues (and is the place where we want to have the strongest guarantees about the global not being mutated long-term, as it's shared!), but would mean we aren't protecting the system global.
2. Alternatively we could stop protecting the Function prototype, and continue to have the protection impact both the shared system global and chrome windows.

I'm inclined to keep the function prototype sealing but restrict it further to the shared system global for now. That has the biggest impact, we have the most control over it, and if anyone's mutating it it's already a bit of a bug (it's shared!) so we can be much more aggressive in restricting mutations on it.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

It was mentioned in Slack that we could potentially only disable this when marionette and/or rdp is enabled. This would add some complexity (currently these APIs only start after the first chrome windows are created, so extra logic would need to be added to nsAppStartup duplicating the relevant checks so we can do them before we run any JS), and I worry it would also cause us potential issues, as it would disable these restrictions when we run the majority of automated tests.

Doing some form of dynamic disabling based on how the browser was started is definitely a potential approach to working around the issues with marionette and rdp executing system JS, but is complex enough to do well that I don't think we can reasonably tackle it for this bug.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please note that chrome-remote-interface.js is not loaded by Marionette but the Browser-chrome mochitest harness. It's used as CDP client so that we can write the integration tests by using JavaScript. Also RDP (which DevTools uses) is not involved here. It's really only for our partial CDP implementation.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ef1c94e04f7
https://hg.mozilla.org/mozilla-central/rev/9bff6c2a2b59

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 9278154 [details]
Bug 1771084 part 3 - Freeze builtins for the shared system global. r?mccr8!,peterv!

Beta/Release Uplift Approval Request

• User impact if declined: Security improvement as follow-up to Pwn2Own.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There's a lot of chrome JS code so it's hard to know for sure it doesn't break anything, however this is a more minimal version than what we had initially.
• String changes made/needed:
• Is Android affected?: Yes

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9278154 [details]
Bug 1771084 part 3 - Freeze builtins for the shared system global. r?mccr8!,peterv!

Approved for 102 beta 7, thanks.

Comment 23

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/82e3067ad2e7
https://hg.mozilla.org/releases/mozilla-beta/rev/6e4639dc3614

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Looks like this grafts cleanly to ESR91 whenever we feel it's baked long enough to request uplift.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 9278122 [details]
Bug 1771084 part 2 - Add Realm option to freeze builtins. r?tcampbell!,nika!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Security issues
• Fix Landed on Version: 102
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There is some risk of breaking chrome JS code that's different from what we have in 102+.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9278122 [details]
Bug 1771084 part 2 - Add Realm option to freeze builtins. r?tcampbell!,nika!

I had Rob from the TB team do a bit of smoketesting with these patches and things seem to work OK. Approved for 91.11esr.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/72bca7a337e4
https://hg.mozilla.org/releases/mozilla-esr91/rev/befef01949f2

Comment 28

[Tom Ritter [:tjr] (OOTO until mid-August)]

We landed Parts "2" and "3" but I could not find a Part 1. If someone was cherry-picking patches, is there a part 1 for this bug they would need?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Part 1 (https://phabricator.services.mozilla.com/D147280) was abandoned.