1541580 - OOM during ProxyObject create leaves partially initialized object in GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ted Campbell [:tcampbell]]

In the comment at [1] we note that we are doing bad things but not to worry, unfortunately if ProxyObject::create fails to create a singleton group in [2], we create the object but return nullptr. This leaves the Proxy partially initialized and the GC calls uninitialized memory as the finalizer function.

This came up will updating the patches for Bug 1406146. I'll put together a smaller test cases if I can.

Might be good to sanity check the other GC-things have don't have these partial initializations too.

I think issue starts with Bug 1339411.

This can case can be hit by gecko SetNewDocument as well as es6 modules..

[1] https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/js/src/vm/ProxyObject.cpp#97-98
[2] https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/js/src/vm/ProxyObject.cpp#207

Comment 1

[Ted Campbell [:tcampbell]]

// Linux64 hg: a53585c83f44d09c2d7f29e07af5d426a7f61084
// --enable-debug --disable-optimize

let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
  return moduleRepo[specifier];
});

moduleRepo['a'] = parseModule(`export let x;`);
oomAtAllocation(5405);
parseModule(`import * as ns from "a";`).declarationInstantiation();

Here is an example of finalizer crash. I picked a the correct allocation count to fail so this is brittle, but concept holds. This bug also applies to browser WindowProxy (which don't work in shell).

Comment 2

[Jan de Mooij [:jandem]]

Good find. So before bug 1339411 we (hackishly) relied on proxy slots overlaying (and getting allocated as) NativeObject dynamic slots and it happened to prevent this...

Comment 3

[Ted Campbell [:tcampbell]]

Hmm.. trace functions also suffer same problem. https://crash-stats.mozilla.com/report/index/3f158ec7-2012-41c6-92af-f3be00190329

Comment 4

[Ted Campbell [:tcampbell]]

Studying this more, I'd say that a core problem here is that the setSingleton call should not be inside ProxyObject::create. The remaining create methods are infallible once the Cell is allocated and that seems a reasonable invariant.

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1541580 - Cleanup ProxyObject creation

Comment 6

[Ted Campbell [:tcampbell]]

There is also a non-sec bug in ModuleNamespaceObject where the trace/finalize functions may dereference an UndefinedValue() and trigger a deterministic crash. I'll fix that in an unrelated bug on central only.

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is probably straightforward for a knowledgeable reader to see that this patch changes OOM-during-init behavior. The affected code affects a small number of complex behaviors and requires an causing an OOM during precisely one step and is likely hard to reliably hit at all. On top of this, the heap corruption must be carefully controlled.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: FF54+
• If not all supported branches, which bug introduced the flaw?: Bug 1339411
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Change is straightforward but various style-reformats mean minor tweaks are needed.
• How likely is this patch to cause regressions; how much testing does it need?: Low-risk. This patch affects rare error-handling code only and simply ensures basic initialization happens one step sooner.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation

sec-approval+ for mozilla-central. Please nominate a beta and ESR60 patch as well.

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1541580 - [ESR60] Cleanup ProxyObject creation

Comment 10

[Ted Campbell [:tcampbell]]

Original patch applies to beta, and I've attached a rebase (just { } problems) for ESR60. I'll request uplifts once the first patch merges to central.

Comment 11

[Ted Campbell [:tcampbell]]

https://hg.mozilla.org/integration/autoland/rev/660471b4a2ada83f46bcb2f072f1c69a1d317724

Comment 12

[Ted Campbell [:tcampbell]]

A testcase will be landed in the future in Bug 1543092 since the testcase hits both the bug here and the bug there.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/660471b4a2ad

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1339411
• User impact if declined: This is a sec-high bug.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a minor change of a rare path. We are moving a simple initialization step one step earlier. Automated tests for the 'normal' case are passing since this merged. There is no automated testcase yet as this is a sec bug.
• String changes made/needed:

Comment 15

[Ted Campbell [:tcampbell]]

Comment on attachment 9056907 [details]
Bug 1541580 - [ESR60] Cleanup ProxyObject creation

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high.
• User impact if declined: Hard-to-reproduce, but exploitable memory corruption.
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a minor change of a rare path. We are moving a simple initialization step one step earlier. Automated tests for the 'normal' case are passing since this merged. There is no automated testcase yet as this is a sec bug.
• String or UUID changes made by this patch:

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation

Uplift approved for 67 beta, thanks.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/590d0706d113

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9056907 [details]
Bug 1541580 - [ESR60] Cleanup ProxyObject creation

Fix for sec-high crash, let's take it for ESR 60.7.

Comment 19

[Calixte Denizet (:calixte)]

There are still some crashes in 67b10 & b11.

Comment 20

[Ted Campbell [:tcampbell]]

The issue identified in this bug is certainly a security problem, but it is also not surprising if it wasn't the sole reason for these crashes (which could also represent other general memory corruption). I'll take a look at some of the remaining crashes and see if there are any actions there. Thanks for pointing it out.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/5acb3927d1ad

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Should we track the remaining crashes in a separate bug? Seems like tracking the fixes is going to get messy otherwise...

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Agreed. I need to mark this fixed for ESR - please file a followup bug for whatever work is left.

Comment 24

[Ted Campbell [:tcampbell]]

Agreed. Closing this as fixed.