1513519 - Crash in nsHostResolver::ResolveHost

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

This bug was filed from the Socorro interface and is
report bp-544e2419-3744-408a-8d94-b46b10181212.
=============================================================

Top 10 frames of crashing thread:

0 libxul.so nsHostResolver::ResolveHost mfbt/RefPtr.h:44
1 libxul.so nsDNSService::AsyncResolveInternal netwerk/dns/nsDNSService2.cpp:849
2 libxul.so nsDNSService::AsyncResolveNative netwerk/dns/nsDNSService2.cpp:913
3 libxul.so nsDNSPrefetch::Prefetch netwerk/base/nsDNSPrefetch.cpp:65
4 libxul.so mozilla::net::nsHttpChannel::MaybeStartDNSPrefetch netwerk/protocol/http/nsHttpChannel.cpp:6405
5 libxul.so mozilla::net::nsHttpChannel::BeginConnectActual netwerk/protocol/http/nsHttpChannel.cpp:6428
6 libxul.so mozilla::net::nsHttpChannel::BeginConnect netwerk/protocol/http/nsHttpChannel.cpp:6377
7 libxul.so mozilla::net::nsHttpChannel::OnProxyAvailable netwerk/protocol/http/nsHttpChannel.cpp:6671
8 libxul.so mozilla::net::nsAsyncResolveRequest::DoCallback netwerk/base/nsProtocolProxyService.cpp:396
9 libxul.so mozilla::net::nsAsyncResolveRequest::Run netwerk/base/nsProtocolProxyService.cpp:262

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

Comment 1

[Daniel Veditz [:dveditz]]

There's an older crash with this signature, but the use-after-frees seem to have started with 64.x. Mostly on Android, but there's a couple windows UAFs too.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Dragana, do you think you'll have cycles to look at this during this cycle still?

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)

Dragana, do you think you'll have cycles to look at this during this cycle still?

I was on PTO and back today.

I looked into this today a bit and I will continue looking... I may have a theory why this is happening.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't see anything interesting in the reports that have logs on Android. The stack is kinda weird, though.

According to the reports, the crash is occuring in RefPtr::AddRef(), so we are likely creating a RefPtr with a garbage referent, but frame 1 points to the line[0] where nsDNSService::AsyncResolveInternal calls nsHostResolver::ResolveHost. We aren't creating a RefPtr here, but we would've for the new nsDNSAsyncRequest(...) just above for the mResolver member. Optimization side effect? Busted stack trace? Also, we're passing the value for mResolver from an existing RefPtr, so it doesn't make sense that it could suddenly become garbage.

[0] https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/netwerk/dns/nsDNSService2.cpp#849

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Is this still a P1 for this release cycle?

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Emma Humphries, Bugmaster ☕️🎸🧞‍♀️✨ (she/her) [:emceeaich] (UTC-8) needinfo? me from comment #6)

Is this still a P1 for this release cycle?

I would like to fix it but currently I do not know what the next step should be.

I will keep it P1.

Comment 8

[Liz Henry (:lizzard)]

Dragana, are you able to take another look? The crash volume went up quite a lot on 66.0.2 for Fennec.
Petru, cc-ing you into the bug in case you may be able to investigate.

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Sorry but all this seems native code for which I don't anything to contribute atm.
Feel free to ping me if there's anything to be done in the Android app.

Comment 10

[Liz Henry (:lizzard)]

Actually - at this point, this isn't likely to be fixed for 66. But we could still take a patch for 68 or potentially, 67.

Comment 11

[Pascal Chevrel:pascalc]

The volume of crashes on 67 beta is low and we don't currently have a patch in review orwhile we are one week away from RC week. Marking as fix-optional at this point for 67.

Comment 12

[Michal Novotny [:michal]]

The cause seems to be the same as in bug 1549398. Looks like nsDNSService::mResolver is invalid, but I have no idea why...

Comment 13

[Liz Henry (:lizzard)]

Too late for a fix in 68 as we are a week away from release.

Comment 14

[Dragana Damjanovic [:dragana]]

We do not know how to fix this at the moment.

Comment 15

[Liz Henry (:lizzard)]

OK. I'll mark this stalled for now. Dan just double checking that's ok with you.

Comment 16

[Honza Bambas (:mayhemer)]

Christian, making you aware. This is likely a race in our DNS code we can't figure out.

Comment 17

[Julien Cristau [:jcristau]]

setting status for esr68 since these are mostly fennec crashes.

Comment 18

[David Lawrence [:dkl]]

Removing employee no longer with company from CC list of private bugs.

Comment 19

[Valentin Gosu [:valentin]]

Attachment: Bug 1513519 - Always hold lock when accessing mResolver r=#necko

Comment 20

[Valentin Gosu [:valentin]]

Comment on attachment 9206332 [details]
Bug 1513519 - Always hold lock when accessing mResolver r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Racy code is rather obvious. Unclear how easy this would be to exploit.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• 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?: Low risk of regressions - only locks mutex for sensitive operations

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 9206332 [details]
Bug 1513519 - Always hold lock when accessing mResolver r=#necko

sec-approval=dveditz

Comment 22

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

Always hold lock when accessing mResolver r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/a9c0999063b5358a196fa958563a9c43b3ebf6eb
https://hg.mozilla.org/mozilla-central/rev/a9c0999063b5

Comment 23

[Julien Cristau [:jcristau]]

Please request beta/esr uplift when you get a chance.

Comment 24

[Valentin Gosu [:valentin]]

Comment on attachment 9206332 [details]
Bug 1513519 - Always hold lock when accessing mResolver r=#necko

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high
• User impact if declined: nsDNSService::mResolver may be accessed from multiple threads without holding a lock leading to a use after free.
• Fix Landed on Version: 88
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch makes no big logic changes. We just hold the lock when accessing the RefPtr in a few extra places in the code.
• String or UUID changes made by this patch:

Beta/Release Uplift Approval Request

• User impact if declined: nsDNSService::mResolver may be accessed from multiple threads without holding a lock leading to a use after free.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch makes no big logic changes. We just hold the lock when accessing the RefPtr in a few extra places in the code.
• String changes made/needed:

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 9206332 [details]
Bug 1513519 - Always hold lock when accessing mResolver r=#necko

approved for 87.0b8

Comment 26

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fdf58e1aeece

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9206332 [details]
Bug 1513519 - Always hold lock when accessing mResolver r=#necko

Approved for 78.9esr.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/8d99c9d47f3c

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.