1325877 - Compartment mismatch in nsDOMConstructor::HasInstance

Status: VERIFIED FIXED

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

Description

[Oriol Brufau [:Oriol]]

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?

Comment 1

[Oriol Brufau [:Oriol]]

In another PC I get this other crash signature

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[Oriol Brufau [:Oriol]]

An example for each crash signature:

https://crash-stats.mozilla.com/report/index/552b4496-f03e-48e3-916e-71ce12161226
https://crash-stats.mozilla.com/report/index/a552836e-d6d9-47a6-be97-84d872161226

Comment 5

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

This should fix the compartment mismatch, but I've no idea if there's more to do here...

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Bazinga!

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.

Comment 8

[Jan de Mooij [:jandem]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8822055 [details] [diff] [review]
Bazinga!

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, except this needs a useful commit message, of course.

Comment 11

[Marcia Knous [:marcia]]

Tracking 51/52/53 for this sec critical crash issue.

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

We usually rate bugs like this sec-high because they are likely difficult to turn into exploits.

Comment 14

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

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.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: With updated commit message

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

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99f2e17cae94

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21d3da6e4f05
https://hg.mozilla.org/releases/mozilla-beta/rev/b40eb1f2db20
https://hg.mozilla.org/releases/mozilla-esr45/rev/3cff736e3bb6

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99f2e17cae94

Comment 20

[Andrei Vaida [:avaida]]

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.