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

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect, P1)

Description

[[:philipp]]

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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Valentin, do you have cycles to look at this?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

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

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Hold a ref to mRequest in PACResolver::Notify

MozReview-Commit-ID: 1QeFlAiTCNt

Comment 4

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 5

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

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fb95c9979452fe25e42873f54cf56a80a0a0d8

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7fb95c99794

Comment 8

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/364fadf112db