Closed Bug 1325877 Opened 7 years ago Closed 7 years ago

Compartment mismatch in nsDOMConstructor::HasInstance

Categories

(Core :: DOM: Core & HTML, defect)

53 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 51+ verified
firefox50 --- wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified

People

(Reporter: Oriol, Assigned: Waldo)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Crash Data

Attachments

(2 files, 1 obsolete file)

1. Disable e10s
2. Install Firebug add-on
3. Right-click and inspect some element with firebug

Firefox crashes.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=84ba5ac2326637c96878318434d9fc580be92fb9&tochange=74717635e168385c1e979141d771a74536e1422a

Caused by bug 1325345 or bug 1324521.

Whose fault is this, Firefox's or Firebug's?
Flags: needinfo?(jdemooij)
In another PC I get this other crash signature
Crash Signature: [@ js::assertSameCompartment<T> ] → [@ js::assertSameCompartment<T> ] [@ js::CompartmentChecker::fail ]
Group: javascript-core-security
Component: Untriaged → JavaScript Engine
Flags: needinfo?(jdemooij)
Product: Firefox → Core
Hi Oriol, do you have a link to a crash report? I see some crashes on crash-stats but I want to be sure it's the same one you are seeing.

This is bug 1325345.
Flags: needinfo?(oriol-bugzilla)
That said, I'm pretty sure your report will be similar to this one: https://crash-stats.mozilla.com/report/index/fd78773b-7955-461d-9840-a32632161226

nsDOMConstructor::HasInstance is calling JS_GetPrototype without entering |proto|'s compartment. We should probably just add a JSAutoCompartment(cx, proto) there...
[Tracking Requested - why for this release]:
Summary: Opening Firebug crashes Firefox → Compartment mismatch in nsDOMConstructor::HasInstance(
Summary: Compartment mismatch in nsDOMConstructor::HasInstance( → Compartment mismatch in nsDOMConstructor::HasInstance
Attached patch Patch (obsolete) — Splinter Review
This should fix the compartment mismatch, but I've no idea if there's more to do here...
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8821988 - Flags: review?(bzbarsky)
Keywords: sec-critical
Attached patch Bazinga!Splinter Review
Nope.  JS_GetPrototype doesn't produce anything cross-compartment, so JSAC shouldn't be needed per-loop.  And in the loop, |proto| is compared to |dot_prototype| from a *different* compartment -- they need to be same-compartment to be directly compared.  So enter |dom_obj|'s compartment -- once.  But properly wrap |dot_prototype| into that entered compartment so the comparison actually works.
Attachment #8822055 - Flags: review?(bzbarsky)
Attachment #8821988 - Attachment is obsolete: true
Attachment #8821988 - Flags: review?(bzbarsky)
Assignee: jdemooij → jwalden+bmo
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> And in the loop, |proto| is compared to
> |dot_prototype| from a *different* compartment -- they need to be
> same-compartment to be directly compared.

Yeah, I was wondering about that. Thanks for stealing this.
Group: javascript-core-security → dom-core-security
Component: JavaScript Engine → DOM
Keywords: csectype-uaf
Comment on attachment 8822055 [details] [diff] [review]
Bazinga!

r=me
Attachment #8822055 - Flags: review?(bzbarsky) → review+
Oh, except this needs a useful commit message, of course.
Tracking 51/52/53 for this sec critical crash issue.
Comment on attachment 8822055 [details] [diff] [review]
Bazinga!

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

If the attacker knows how to attack compartment mismatches, easily.  If not, harder.  I don't know how prevalent the former knowledge is.

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?

Adding a JSAutoCompartment is blatant in its own way, but whether that blatancy's weaponizable depends on ^.  (And on whether any of this code is triggerable by web content, or triggerable if a page can provoke a user into it.)

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

The non-compartment-entering goes at least to ESR45.  It's possible that the mismatch only became a problem when bug 888969 landed, because before that JS_GetPrototype would have done stuff across compartment boundaries, but not anything a user could meaningfully control.  But I'm not entirely certain of that limitation, and this isn't invasive enough to be worth taking a risk on, IMO.

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

ESR45 code looks pretty similar, so backporting should be simple.

> How likely is this patch to cause regressions; how much testing
> does it need?

With a couple people who understand compartments having looked at it, I think it's relatively low-risk.

It *should* be fairly easy to cons up an automated testcase for this, and I'd really like to have one on hand to test with now (and to land after the usual delay).  However, I'm having inordinate trouble consing up a few lines of (ideally) xpcshell to invoke this function and the modified code.

If anyone can at least give an object that, if used as RHS of |instanceof|, would invoke this code, I'd be very much interested, as it'd make writing a testcase for this basically trivial.
Attachment #8822055 - Flags: sec-approval?
We usually rate bugs like this sec-high because they are likely difficult to turn into exploits.
Keywords: sec-criticalsec-high
Bazinga, indeed.

sec-approval+ for trunk. Let's get patches nominated for Aurora, Beta, and ESR45 so this can be fixed everywhere once it lands.
Attachment #8822055 - Flags: sec-approval? → sec-approval+
Turns out the patch has no backport issues at all -- guess the code wasn't even negligibly different as I thought it was from eyeballing.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: sec-high
Fix Landed on Version: 51 (when it lands, prefer landing in unison across branches generally)
Risk to taking this patch (and alternatives if risky): Firebug and possibly other extension crashes, maybe web code affected but not sure
String or UUID changes made by this patch: N/A

Approval Request Comment
[Feature/Bug causing the regression]: ancient, see comment 12
[User impact if declined]: sec-high
[Is this code covered by automated tests?]: no -- would like to, probably can, haven't managed to figure out out yet -- see comment 12
[Has the fix been verified in Nightly?]: not yet, hoping to land everywhere fairly simultaneously
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: see comment 12
[Why is the change risky/not risky?]: 
[String changes made/needed]: N/A
Attachment #8822555 - Flags: review+
Attachment #8822555 - Flags: approval-mozilla-esr45?
Attachment #8822555 - Flags: approval-mozilla-beta?
Attachment #8822555 - Flags: approval-mozilla-aurora?
Blocks: 851892
Comment on attachment 8822555 [details] [diff] [review]
With updated commit message

Fix for a sec-high crash, let's take this for the beta 11 build.
Attachment #8822555 - Flags: approval-mozilla-esr45?
Attachment #8822555 - Flags: approval-mozilla-esr45+
Attachment #8822555 - Flags: approval-mozilla-beta?
Attachment #8822555 - Flags: approval-mozilla-beta+
Attachment #8822555 - Flags: approval-mozilla-aurora?
Attachment #8822555 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/99f2e17cae94
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: dom-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Reproduced the bug on an affected build (53.0a1, 20161226030205) using Firebug (2.0.18) with e10s disabled, on Ubuntu 16.04 x64:

    bp-e1994cbe-c9b6-42d7-9537-152172170125


This is verified fixed on:

  - 45.7.0esr-build1 (20170118123525)
  - 51.0-build2 (20170118123726)
  - 52.0b1-build2 (20170124094647)
  - 53.0a2 (20170124223930)
  - 54.0a1 (20170125110119)

using Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12.2. No crashes encountered on any of the fixed builds.
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: