Closed Bug 1408005 Opened 2 years ago Closed 2 years ago

Crash with failed "@mozilla.org/downloads/application-reputation-service;1" instance

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 57+ verified
firefox56 --- wontfix
firefox57 + verified
firefox58 + verified
firefox59 --- verified

People

(Reporter: Oriol, Assigned: mccr8)

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main57+][adv-esr52.5+][post-critsmash-triage])

Crash Data

Attachments

(2 files, 1 obsolete file)

Open browser console and enter this code:

  try { Cc["@mozilla.org/downloads/application-reputation-service;1"].createInstance(Ci.nsISpeechService); } catch(err){ }
  try { Cc["@mozilla.org/downloads/application-reputation-service;1"].createInstance(Ci.nsIApplicationReputationService); } catch(err){ }


Firefox crashes.

https://crash-stats.mozilla.com/report/index/62ae9282-adde-4df3-9ca8-416ca0171012
https://crash-stats.mozilla.com/report/index/308e6b6f-d6c6-44fd-b549-cc3cf0171012
https://crash-stats.mozilla.com/report/index/d425e682-dd1b-4fcf-8a01-362800171012
This is a use after free.
Group: core-security
Keywords: csectype-uaf
Attached file asan log.txt (obsolete) —
Attached file ASan log
Assignee: nobody → continuation
Attachment #8920313 - Attachment is obsolete: true
ApplicationReputationService::GetSingleton() allocates a new object, and returns an already addrefed pointer to the object. It also stores a non-owning pointer in gApplicationReputationService. If the caller to GetSingleton() destroys the object it gets, as is the case when we have a failure to QI in ApplicationReputationServiceConstructor(), then gApplicationReputationService is a dangling pointer, which gets returned to the next caller to GetSingleton().

I think this works in practice because GetSingleton() is only called from JS in a single place, and there's a singleton JS object that refers to it, so the XPCWN keeps alive the ApplicationReputationService object until shutdown. When the XPCWN goes away, it frees the object so there's no leak.

I think the fix is to make the dtor for ApplicationReputationService clear the global.
Group: core-security → toolkit-core-security
Component: XPCOM → Downloads API
Product: Core → Toolkit
I'm going to mark this sec-high, though I think that is conservative. As I said in comment 4, it appears that the way this data structure is used in Firefox currently that nothing bad is going to happen, but any additional uses of it (in addons or Firefox code) would cause a UAF. I looked in Addons MXR and I don't see any references to this downloads/application-reputation-service so it doesn't seem like anybody else is using it.

I ran the tests I found in toolkit/components/jsdownloads/ and toolkit/components/downloads and they passed.
Keywords: sec-high
Attachment #8920624 - Flags: review?(francois) → review+
Comment on attachment 8920624 [details] [diff] [review]
Clear gApplicationReputationService in the dtor.

Approval Request Comment
[Feature/Bug causing the regression]: old
[User impact if declined]: possible security problems, though it seems unlikely
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: I manually checked the test case.
[Needs manual test from QE? If yes, steps to reproduce]: Not really.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This should only really affect the behavior when things have gone terribly wrong already. Plus the patch is tiny.
[String changes made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily. I don't think it could be exploited without an addon doing something unusual.

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

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

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely. The patch is very simple and I doubt it affects behavior except when we'd be crashing otherwise.
Attachment #8920624 - Flags: sec-approval?
Attachment #8920624 - Flags: approval-mozilla-esr52?
Attachment #8920624 - Flags: approval-mozilla-beta?
(In reply to Andrew McCreight [:mccr8] from comment #7)
> [Has the fix been verified in Nightly?]: I manually checked the test case.

It's highly unlikely that your change broke anything, if you want to cover all the bases and make sure that the feature still works, you can go to https://testsafebrowsing.appspot.com/ and make sure that all of the downloads in "Desktop Download Warnings" get blocked successfully.

Make sure first that browser.safebrowsing.downloads.enabled = true since it defaults to false in non-official builds.
If we're late in the cycle, I think it is okay for this to miss 57. I'd still like to get it into ESR next cycle. I think it is unlikely that this can be exploited.
Comment on attachment 8920624 [details] [diff] [review]
Clear gApplicationReputationService in the dtor.

sec-approval+ for trunk. 

This really is a two line patch so I'd suggest taking it on Beta and ESR52.
Attachment #8920624 - Flags: sec-approval? → sec-approval+
Comment on attachment 8920624 [details] [diff] [review]
Clear gApplicationReputationService in the dtor.

Giving Beta and ESR52 approval because all release management are AFK and it is Friday afternoon. I figure in now is better than later and it is tiny.
Attachment #8920624 - Flags: approval-mozilla-esr52?
Attachment #8920624 - Flags: approval-mozilla-esr52+
Attachment #8920624 - Flags: approval-mozilla-beta?
Attachment #8920624 - Flags: approval-mozilla-beta+
Group: toolkit-core-security → core-security-release
Whiteboard: [adv-main57+][adv-esr52.5+]
Confirmed issue on Fx56.0.2.
Verified fixed on Fx57.0b14, Fx52.5.0esr.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]
I have managed to reproduce the issue described in comment 0 using Firefox 58.0a1 (BuildId:20171005100211).

This issue is no longer reproducible using Firefox 59.0a1 (BuildId:20171214220032), Firefox 58.0b11 (BuildId:20171211020921), Firefox 57.0.2 (BuildId:20171206182557) and Firefox 52.5.2 esr (BuildId:20171206101620) using Windows 10 64bit, macOS 10.13 and Ubuntu 16.04 64bit.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.