OOM during ProxyObject create leaves partially initialized object in GC
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
People
(Reporter: tcampbell, Assigned: tcampbell)
References
(Regression)
Details
(Keywords: csectype-uninitialized, regression, sec-high, Whiteboard: [adv-main67+][adv-esr60.7+])
Crash Data
Attachments
(1 file, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
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
Assignee | ||
Comment 1•5 years ago
|
||
// 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•5 years ago
|
||
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...
Assignee | ||
Comment 3•5 years ago
|
||
Hmm.. trace functions also suffer same problem. https://crash-stats.mozilla.com/report/index/3f158ec7-2012-41c6-92af-f3be00190329
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 8•5 years ago
|
||
Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation
sec-approval+ for mozilla-central. Please nominate a beta and ESR60 patch as well.
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Comment 10•5 years ago
|
||
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.
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
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:
Assignee | ||
Comment 15•5 years ago
|
||
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•5 years ago
|
||
Comment on attachment 9055866 [details]
Bug 1541580 - Cleanup ProxyObject creation
Uplift approved for 67 beta, thanks.
Comment 17•5 years ago
|
||
uplift |
Comment 18•5 years ago
|
||
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•5 years ago
•
|
||
There are still some crashes in 67b10 & b11.
Assignee | ||
Comment 20•5 years ago
|
||
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•5 years ago
|
||
uplift |
Updated•5 years ago
|
Comment 22•5 years ago
|
||
Should we track the remaining crashes in a separate bug? Seems like tracking the fixes is going to get messy otherwise...
Comment 23•5 years ago
|
||
Agreed. I need to mark this fixed for ESR - please file a followup bug for whatever work is left.
Assignee | ||
Comment 24•5 years ago
|
||
Agreed. Closing this as fixed.
Updated•5 years ago
|
Updated•4 years ago
|
Updated•2 years ago
|
Description
•