1407751 - Crash with failed "@mozilla.org/net/osfileconstantsservice;1" and "@mozilla.org/places/colorAnalyzer;1" instances

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Oriol Brufau [:Oriol]]

Open browser console and enter this code:

  try { Cc["@mozilla.org/net/osfileconstantsservice;1"].createInstance(Ci.nsIXMLHttpRequest);}catch(err){}
  try { Cc["@mozilla.org/places/colorAnalyzer;1"].createInstance(Ci.nsIXMLHttpRequest);}catch(err){}

Firefox crashes.

https://crash-stats.mozilla.com/report/index/ebc7cc54-48df-4f6c-87d4-fd1800171011
https://crash-stats.mozilla.com/report/index/ce7ed10c-533f-4d36-a592-df3a90171011
https://crash-stats.mozilla.com/report/index/95aa8808-741b-4d30-ac9a-24f630171011
https://crash-stats.mozilla.com/report/index/a29e2fd3-8740-48d7-bae7-e81d90171011

Comment 1

[Nathan Froyd [:froydnj]]

baku, can you take a look at the worker crash (the first crash report)?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash.patch

The reason why we crash is because when |Cc["@mozilla.org/net/osfileconstantsservice;1"].createInstance| is called, the DTOR of osfileconstantsservice is executed. This nullifies gInitialized.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Andrea Marchesini [:baku] from comment #2)
> The reason why we crash is because when
> |Cc["@mozilla.org/net/osfileconstantsservice;1"].createInstance| is called,
> the DTOR of osfileconstantsservice is executed. This nullifies gInitialized.

Um.  Let me try to explain my understanding of that.  The sequence of steps is something like:

1) Create an osfileconstants object.
2) Try to cast it to nsIXMLHttpRequest, which fails.
3) Nobody is holding a reference to the object anymore.
4) Delete the osfileconstants object.
5) Run the dtor, which cleans up a bunch of stuff.
...time passes
6) A (chrome) worker gets started.
7) The worker attempts to initialize a bunch of stuff.
8) We try to initialize osfileconstants, but...fail because we thought we were init'd when we actually weren't.  And, bonus, we're touching memory that was already freed. :(

Is that correct?  My inclination here would be to null out gPaths in CleanupOSFileConstants, though that paints a slightly bigger bulls-eye on the problem.

Marking as sec-high since this is a UAF; this is so easy to trigger, though...

Comment 4

[Andrea Marchesini [:baku]]

Right. Note that this can be triggered basically only via Browser Console.
Wondering if we can block the browser console to execute chrome code.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8917843 [details] [diff] [review]
crash.patch

Review of attachment 8917843 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, let's go with this.  Thanks!

Comment 7

[Oriol Brufau [:Oriol]]

Since this bug was marked security-sensitive, you might want to protect similar bugs I have filed: bug 1407683, bug 1407735, bug 1407740, bug 1407977, bug 1408005 and bug 1408017.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8917843 [details] [diff] [review]
crash.patch

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

no. This can be triggered just via Browser Console.

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

no. Just a security check.

Which older supported branches are affected by this flaw?

all.

If not all supported branches, which bug introduced the flaw?

BrowserConsole I would say...

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

easy. low risk.

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

none.

Comment 9

[Oriol Brufau [:Oriol]]

Just a note, this affects all privileged code, not just the browser console. The web console in a privileged page like about:preferences or an add-on could also trigger this.

Comment 10

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

Comment on attachment 8917843 [details] [diff] [review]
crash.patch

sec-approval+ for trunk.
I'd like to see a Beta and ESR52 patch nominated ASAP as well.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Al Billings [:abillings] from comment #10)
> sec-approval+ for trunk.
> I'd like to see a Beta and ESR52 patch nominated ASAP as well.

Is sec-high the right categorization for this, even if it can only be triggered through the browser console?  I guess it's possible that our chrome JS could trigger this itself, given the right circumstances...

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f76e40db328

Please request Beta/ESR52 approval on this when you get a chance.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8917843 [details] [diff] [review]
crash.patch

Approval Request Comment
[Feature/Bug causing the regression]: BrowserConsole can trigger a crash following the description of the bug.
[User impact if declined]: BrowserConsole with chrome privilege is disabled by default, but it can be enabled and the user can make the browser to crash
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: manually yes.
[Needs manual test from QE? If yes, steps to reproduce]: follow the description of the bug. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: a simple null check.
[String changes made/needed]: none

[Approval Request Comment]
Fix Landed on Version: 58

Comment 14

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

(In reply to Nathan Froyd [:froydnj] from comment #11)

> Is sec-high the right categorization for this, even if it can only be
> triggered through the browser console?  I guess it's possible that our
> chrome JS could trigger this itself, given the right circumstances...

That's what I'm afraid of. Better safe than sorry here.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917843 [details] [diff] [review]
crash.patch

Sec-high, Beta57+, ESR52.5+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95e2671a811d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/592df6088926

Comment 18

[Ciprian Georgiu, Desktop QA]

I have managed to reproduce this crash (https://crash-stats.mozilla.com/report/index/97d8248d-f710-420a-be55-c1bef0171109) using an affected Nightly build from 2017-10-11.

I cannot reproduce this crash signature(s) with the STR from comment 0, on latest Nightly 58.0a1 (20171108110838), 57.0 RC (20171106194249) and esr 52.5.0 (20171107091003) under Windows 10 x64, macOS 10.13 and Windows 10 x64.

It seems that if Firefox is closed after the code is ran in the browser console the FF is crashing, but it has a different signature:
-  https://crash-stats.mozilla.com/report/index/7fc6a798-7106-459a-a80d-ce26e0171109
-  https://crash-stats.mozilla.com/report/index/bd5142e5-4c2f-4e79-9af2-fa4590171109
-  https://crash-stats.mozilla.com/report/index/bd54c4f1-75d0-48ca-84fb-bd7c80171109

I can reproduce this crash on all fixed version mentioned above. See the following screencast: https://imgur.com/a/w4yq6

Andrea, please let me know if I should file a new bug for this crashes.

Comment 19

[Oriol Brufau [:Oriol]]

I don't get a crash when I close FF, but some time after running the code I got

https://crash-stats.mozilla.com/report/index/5a4a6861-0c9d-4235-9264-9fe260171109

Comment 20

[Oriol Brufau [:Oriol]]

OK, this time I did get a crash when closing:
https://crash-stats.mozilla.com/report/index/758e1726-40ed-418a-a317-e7b270171109

Comment 21

[Andrea Marchesini [:baku]]

> Andrea, please let me know if I should file a new bug for this crashes.

Yes, please, file a separate bug.

Comment 22

[Ciprian Georgiu, Desktop QA]

Thanks, filed bug 1416174.
Closing this as verified fixed per comment 18.