Closed Bug 1384121 Opened 2 years ago Closed 2 years ago

C++ helper functions can trigger ObjectGroup sweeping and walk the stack on OOM

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main57+][post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

decoder found some hard to repro crashes that seem to be caused by C++ functions called directly from IC code triggering ObjectGroup sweeping.

OOM there will walk the stack which is not okay when we make a callWithABI call.
Summary: C++ helper functions can trigger ObjectGroup sweeping → C++ helper functions can trigger ObjectGroup sweeping and walk the stack on OOM
Tomorrow I'll look into adding a debug-only mechanism to make this stuff safer by complaining more loudly in debug builds. The goal is to get assertion failures on jit-tests with that in place.
Sounds kind of moderate due to how hard it is to trigger, but adjust as desired.
Attached patch WIP asserts (obsolete) — Splinter Review
This adds some DEBUG-only code so we can later add the following assert to ObjectGroup::maybeSweep:

  MOZ_ASSERT(!TlsContext.get()->inUnsafeCallWithABI);

With the patch + that assert we catch the bug reliably on jit-tests.

The patch requires annotating all callWithABI callees but that seems worth it.
Blocks: 1390861
Filed bug 1399471 with the patch to add the instrumentation to check for these issues.

The patch there won't catch *this* bug yet, but after that lands it's a matter of adding a few asserts. I'll do that as part of this bug.
Attached patch Part 1 - FixSplinter Review
This uses the DontCheckGeneration variants of various TI functions. It fixes all jit-test failures I get with the next patch that adds debug checks.
Attachment #8892447 - Attachment is obsolete: true
Attachment #8908145 - Flags: review?(bhackett1024)
This uses the machinery added in bug 1399471 to assert we don't walk the stack etc under callWithABI calls.

This also fixes some false positives in jit-tests: under Bailout and InvalidationBailout we can walk the stack (due to the bailoutData on JitActivation) so I whitelisted them.

InitBaselineFrameForOsr can walk the stack when the debugger is enabled and that's okay because we push an exit frame.
Attachment #8908150 - Flags: review?(nicolas.b.pierron)
Attachment #8908145 - Flags: review?(bhackett1024) → review+
Comment on attachment 8908145 [details] [diff] [review]
Part 1 - Fix

Requesting sec-approval for both patches. I think we should land both on 57 but we only have to backport the first one.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very difficult.

> 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?
55+.

> If not all supported branches, which bug introduced the flaw?
Bug 1341067 and more recent ones.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial.

> How likely is this patch to cause regressions; how much testing does it need?
Pretty unlikely.
Attachment #8908145 - Flags: sec-approval?
Oh this was marked sec-moderate. Well let's request sec-approval just in case :)
Duplicate of this bug: 1397411
Attachment #8908150 - Flags: review?(nicolas.b.pierron) → review+
This is too late for Firefox 56. We're shipping in a week and a half. As a sec-moderate, you can just check this into trunk though.
Attachment #8908145 - Flags: sec-approval?
Priority: -- → P1
Duplicate of this bug: 1390861
https://hg.mozilla.org/mozilla-central/rev/eab55565955d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main57+]
Flags: qe-verify-
Whiteboard: [adv-main57+] → [adv-main57+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.