Crash in nsCOMPtr<T>::nsCOMPtr<T> | nsHostResolver::DetachCallback

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
critical
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: philipp, Assigned: valentin)

Tracking

(5 keywords)

59 Branch
mozilla61
Unspecified
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 wontfix, firefox60+ fixed, firefox61+ fixed)

Details

(Whiteboard: [necko-triaged][adv-main60+][post-critsmash-triage], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is
report bp-af95af75-2515-42fb-838b-c403a0180401.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsCOMPtr<nsIMutationObserver>::nsCOMPtr<nsIMutationObserver> mfbt/RefPtr.h:112
1 xul.dll nsHostResolver::DetachCallback netwerk/dns/nsHostResolver.cpp:1007
2 xul.dll nsDNSAsyncRequest::Cancel netwerk/dns/nsDNSService2.cpp:398
3 xul.dll mozilla::net::PACResolver::Notify netwerk/base/ProxyAutoConfig.cpp:311
4 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:704
5 xul.dll nsTimerEvent::Run xpcom/threads/TimerThread.cpp:290
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1096
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
8 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:814
9 xul.dll mozilla::net::WaitForThreadShutdown::Run netwerk/base/nsPACMan.cpp:160

=============================================================

this signature is newly showing up on windows since firefox 59 - probably related to bug 1424834 and just a signature change.
it's a uaf in some instances though...
Valentin, do you have cycles to look at this?
Flags: needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Priority: -- → P1
Whiteboard: [necko-triaged]
It seems that either PACResolver or nsDNSAsyncRequest is being released on another thread, while we call Cancel on the main thread.
I think the solution here is to hold a ref to mRequest in PACResolver::Notify()
MozReview-Commit-ID: 1QeFlAiTCNt
Attachment #8966717 - Flags: review?(daniel)
Attachment #8966717 - Flags: review?(daniel) → review+
Comment on attachment 8966717 [details] [diff] [review]
Hold a ref to mRequest in PACResolver::Notify

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Difficult. The race causing this issue would be quite hard for an attacker to trigger.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Kind of. The problem is obvious if you look at the code.

Which older supported branches are affected by this flaw?
Firefox 59 according to comment 0

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

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

How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions.
Attachment #8966717 - Flags: sec-approval?
Clearing sec-approval as it doesn't need it since we made this a sec-moderate. That said, I'd like to see a Beta patch nominated so we can have this fixed for ESR60 as well.
Flags: needinfo?(valentin.gosu)
Attachment #8966717 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/f7fb95c99794
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8966717 [details] [diff] [review]
Hold a ref to mRequest in PACResolver::Notify

Approval Request Comment
[Feature/Bug causing the regression]:
According to comment 0 the first crashes started in Firefox 59

[User impact if declined]:
Crashes may occur, most likely during shutdown.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Just landed on Nightly. Waiting for confirmation that the crashes have stopped.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
The patch just adds an extra AddRef.

[String changes made/needed]:
None.
Flags: needinfo?(valentin.gosu)
Attachment #8966717 - Flags: approval-mozilla-beta?
Attachment #8966717 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+]
Flags: qe-verify-
Whiteboard: [necko-triaged][adv-main60+] → [necko-triaged][adv-main60+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.