Closed Bug 1541580 Opened 1 year ago Closed 1 year ago

OOM during ProxyObject create leaves partially initialized object in GC

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ fixed
firefox66 --- wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

(Keywords: csectype-uninitialized, sec-high, Whiteboard: [adv-main67+][adv-esr60.7+])

Crash Data

Attachments

(1 file, 1 obsolete file)

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

// 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).

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...

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.

Attachment #9055866 - Attachment description: Bug 1541580 - Make ProxyObject creation match NativeObject creation → Bug 1541580 - Cleanup ProxyObject creation

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 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.
Attachment #9055866 - Flags: sec-approval?

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

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

Attachment #9055866 - Flags: sec-approval? → sec-approval+

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.

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

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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:
Attachment #9055866 - Flags: approval-mozilla-beta?

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:
Attachment #9056907 - Flags: approval-mozilla-esr60?

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

Uplift approved for 67 beta, thanks.

Attachment #9055866 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

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

Attachment #9056907 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

There are still some crashes in 67b10 & b11.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

Attachment #9056907 - Attachment is obsolete: true

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

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

Flags: needinfo?(tcampbell)

Agreed. Closing this as fixed.

Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Flags: needinfo?(tcampbell)
Resolution: --- → FIXED
Whiteboard: [adv-main67+][adv-esr60.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.