1566998 - WebRTC DNS queries bypass DoH mode 2

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

Attachment: screenshot of my wireshark trace from the test described

It appears that DNS queries from Firefox WebRTC bypass DNS-over-HTTPS.

Steps to reproduce:

• set TRR mode 2 in about:config
• open on a second device or browser a room on appear.in for example https://appear.in/firefox-doh
• start you preferred network sniffer
• join the same room with you TRR enabled Firefox
• once the call gets established stop you network sniffer and search for DNS queries

Result:
I see in wireshark DNS queries for turnserver.appearin.net

Expected result:
I should NOT see any DNS queries in clear on the wire as they should go over DoH.

This is where the WebRTC searches for the DNS service: https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/media/mtransport/nriceresolver.cpp#94

Comment 1

[Nils Ohlmeier [:drno]]

Testing in TRR mode 3 shows no DNS queries for turnserver.appearin.net on the wire. Instead I can see it on the about:networking DNS page as a TRR queried entry.

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: nsHostResolver log TRR mode 3

Log file from TRR mode 3

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: nsHostResolver log TRR mode 2

Log from running in TRR mode 2.

Strangely the logs suggests that turnserver.appearin.net gets resolved via TRR. And about:networking#dns claims the same. But tcpdump shows DNS queries in the clear on the wire at the same time.
Notably the only query in clear on the wire is for turnserver.appearin.net, which is the only DNS entry which gets resolved from the WebRTC code (other entries like signal.appear.in etc don't get resolved by WebRTC, but by DOM).

Comment 4

[Nils Ohlmeier [:drno]]

WebRTC uses Necko to resolve DNS, see description for a link where it looks up the DNS resolver service via XPcom. Thus I don't think this a WebRTC issue, but a Necko issue.

Comment 5

[Byron Campen [:bwc]]

So DoH mode 2 means that we're supposed to try DoH, and if that fails, fall back to DNS in the clear. I'm guessing that means that DoH is failing on turnserver.appearin.net for some reason?

Comment 6

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

Unsure why we haven't looked at the logs until now.
What seems to be happening is:
We do separate A and AAAA resolutions for turnserver.appearin.net.
TRR for A succeeds, but TRR for AAAA fails, so we fallback to native DNS. Native DNS for AAAA also fails (afaict from the log).
Not sure exactly why wireshark shows both A and AAAA requests. We shouldn't be leaking the A at all. Maybe it's a quirk of getaddrinfo.

Comment 7

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

Attachment: Bug 1566998 - Test r=#necko

Comment 8

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

Attachment: Bug 1566998 - Improve awaiting new TRRDNSListener by returning an object r=#necko

It's nicer to destructure the response using
let {inRecord, inStatus} = await new TRRDNSListener(...)
instead of
let [,inRecord,inStatus] = await new TRRDNSListener(...)

Depends on D129507

Comment 9

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

Attachment: Bug 1566998 - Make test_trr/test_odoh::test_CNAME not be dependent on A/AAAA order r=#necko

The order in which we send A/AAAA requests is unspecified.
This test assumed the A request is always first. If we change that logic,
then the variable ends up != 0 on the second request, so we don't get the
proper response anymore.

This patch changes the server handler so it returns the proper response
after decoding the request packet.

Depends on D130042

Comment 10

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

Attachment: Bug 1566998 - Split DispatchByType into different method r=#necko

Depends on D130043

Comment 11

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

Attachment: Bug 1566998 - Fix lint warning r=#necko

Depends on D130044

Comment 12

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

Attachment: Bug 1566998 - Fix test_dns_retry r=#necko

Because trr_test_setup() sets network.dns.native-is-localhost to true and
the test expects donotexist.example.com to fail to resolve, the test
seems to always fail.
We need to clear the pref after calling trr_test_setup.

Depends on D130045

Comment 13

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

Attachment: Bug 1566998 - A or AAAA TRR request should not fallback to Do53 if there is a record for the other family r=#necko

Normally DNS resolutions are AF_UNSPEC - meaning that both A and AAAA
responses are acceptable.
However, some consumers of the DNS API will query A or AAAA separately
(like the WebRTC code), and use the responses independently.
In that case, we want to ensure that for a host that only has a A record,
we don't fallback to Do53 for the AAAA request and leak the hostname to
the system resolver.

To achieve this we change the logic as such:
If the response from DoH was NXDOMAIN we then query the other family.
If the other response was OK, that means we should not fallback to Do53.
If the other family response was also NXDOMAIN, the host is probably not
available to the public internet and falling back is probably fine.

Depends on D130046

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d948fe16b8b0
Test r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/ecbfa0721168
Improve awaiting new TRRDNSListener by returning an object r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/f392ebf40d2a
Make test_trr/test_odoh::test_CNAME not be dependent on A/AAAA order r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/b674d5f15f60
Split DispatchByType into different method r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/24f76460c44b
Fix lint warning r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/dc39d8837aae
Fix test_dns_retry r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/1d0f667507f4
A or AAAA TRR request should not fallback to Do53 if there is a record for the other family r=nhnt11

Comment 15

[Atila Butkovits]

Backed out for causing build bustages at Logging.h.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e7fe2ba2732075fbe0cc00f4c519a3b78bb47547

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=fRailzgMQqCOoVZ3EkrYHg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=1d0f667507f43cf1767443f20ee77ee1dcf050d5

Failure log: https://treeherder.mozilla.org/logviewer?job_id=357535195&repo=autoland&lineNumber=71995

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22c9dea31b17
Test r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/6f9ef895fcd5
Improve awaiting new TRRDNSListener by returning an object r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/f7c65c2a3f10
Make test_trr/test_odoh::test_CNAME not be dependent on A/AAAA order r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/bb9308bbc58c
Split DispatchByType into different method r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/ecb0bd5abbe2
Fix lint warning r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/77a7ffeccacc
Fix test_dns_retry r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/20801f57aea1
A or AAAA TRR request should not fallback to Do53 if there is a record for the other family r=nhnt11

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/22c9dea31b17
https://hg.mozilla.org/mozilla-central/rev/6f9ef895fcd5
https://hg.mozilla.org/mozilla-central/rev/f7c65c2a3f10
https://hg.mozilla.org/mozilla-central/rev/bb9308bbc58c
https://hg.mozilla.org/mozilla-central/rev/ecb0bd5abbe2
https://hg.mozilla.org/mozilla-central/rev/77a7ffeccacc
https://hg.mozilla.org/mozilla-central/rev/20801f57aea1